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

[Erik Österlund]

Hi Martin,

The new webrev looks good.

Note though the following though... it looks like the AArch64 code 
doesn't do appropriate fencing if the field is volatile.
The normal JNI accessor goes through thread transitions causing the 
following semantics:
fence()
load
fence()
Which is more than enough for a volatile field load. However, with JNI 
fast get field... it is insufficient.

Thanks,
/Erik

On 2019-07-23 12:29, Doerr, Martin wrote:
> 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
>