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

[David Holmes]

Hi Martin,

No further comments from me. I'm obviously not knowledgeable enough to 
review any of the assembler changes.

Thanks,
David

On 23/07/2019 8:29 pm, 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
>