RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime

[Doerr, Martin]

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.
Agreed.

> 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.

New webrev:
http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.01/

Best regards,
Martin