RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
david.holmes at oracle.com
Wed Jul 24 06:34:22 UTC 2019
No further comments from me. I'm obviously not knowledgeable enough to
review any of the assembler changes.
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.
>> 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:
> Best regards,
More information about the hotspot-runtime-dev