RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
martin.doerr at sap.com
Tue Jul 23 10:29:11 UTC 2019
Hi David and Erik,
thank you for reviewing and for your very valuable feedback.
> 1) In the x86_64 assembly, you can combine the movl; testl; into a single
> test instruction with one memory operand to the counter, and one
> immediate zero.
Thanks for the hint. I'm using cmp32 in my new webrev.
> 2) If libjvm.so maps in far away, then the movl taking an ExternalAddress,
> will actually scratch rscratch1, which is r10.
Good catch! I've exchanged registers and added assert_different_registers.
> I was secretly hoping to never have to touch fast JNI getfield again,
> because it is so shady, and the odd cases are very hard to test, making it so
> easy to mess up. The ForceUnreachable JVM flag might be useful in checking
> if a solution works also when rscratch1 gets clobbered when referencing JVM
> symbols that are now "far away".
I've also changed the test to run with -XX:+ForceUnreachable and -XX:+SafepointALot to hit more corner cases.
But as you explained, the test would normally not notice the destroyed counter and just execute the slow path.
> The subtle issue of referencing JVM symbols that can be far away,
> suddenly clobbering r10, has bitten us many times. Perhaps it should be
> made more explicit somehow.
It would be possible to explicitly kill r10 in all such assembler instructions in the dbg build, but that'd come with an overhead.
> But that's a separate issue.
> Also, I noticed that the counter that we are checking if it has changed, is a
> 32 bit signed integer. They can actually wrap around, which is undefined
> behaviour at best, and will make these tests fail in the worst case. When we
> don't want counters to overflow, we use 64 bit integers.
We could also make it unsigned to get defined behavior, but that's out of scope here.
More information about the hotspot-runtime-dev