[lworld] RFR: object methods in VM for lworld value type

[David Simms]

Sorry for the delayed reply, inline...


On 27/04/18 23:55, Karen Kinnear wrote:
> Mr Simms,
>
> Yay! Thank you so much for doing the vm changes for Object methods for -Xint.
>
> Question on functionality:
> 1) Cloneable - this is different than we had originally discussed - so I should bring this to an EG discussion -
> hopefully for when you are there

Sure thing. So there is a debate, choices would be that values...

  * May be Cloneable, but may not override clone method
  * Implicitly Cloneable, but may not override clone method (personally
    like this one)
  * Disallow Cloneable (current patch)

Are there further options ? Can discuss later.

>
> Hard to catch all our sync fast paths - many thanks. Looks like the fast way to do this - hopefully
> below radar. I don’t know all the JIT paths, but sounds like that is a follow-on for our JIT folks.

Yeap.

>
> 1.templateTable_x86.cpp
> What happens if you skip verification and generate bytecodes with a monitor exit but no monitorenter
> for a value type? Does this already fall through to IMSE?

The mismatched monitor block start / stack pointer check should cover 
it, but I did add a test for regression to 
"ObjectMethods.checkMonitorExit(Object)", using raw bytecode generated 
with "MethodHandleBuilder" and running without verification.

Also added extra check for value type in 
"TemplateInterpreterGenerator::lock_method()" for cases where folks have 
illegally generated "synchronized" methods on value type receiver and 
run with verification disabled.


>
> 2. synchronizer.cpp
> Do you need checks for:
> jni_enter, jni_exit, reenter, complete_exit
> otherwise they seem to go into revoke_and_rebias and then inflate, never checking for exceptions

So after going threw those code paths again, I added more asserts and 
moved the check and throw exception into synchronizer.cpp (from jvm.cpp)

Added start of JNI test, covers monitor ops for now, "ValueWithJni"

http://cr.openjdk.java.net/~dsimms/valhalla/object_methods/webrev1/test/hotspot/jtreg/runtime/valhalla/valuetypes/ValueWithJni.java.html 


>
> 3. jvm.cpp
> So JVM_Clone doesn’t need to throw CloneNotSupported because we ensure that this is not
> a cloneable in classfileparser, and j.l.Object catches?
> Is it worth adding an assertion here?

Yes, good idea, done updated webrev...
>
> thanks,
> Karen
>

Updated webrev: 
http://cr.openjdk.java.net/~dsimms/valhalla/object_methods/webrev1/

Can wait until clone discussion on Wednesday before pushing

Thanks for looking into this Karen !
Cheers
/D