Reapply "Optimize OpenJDK VarHandle operation methods" #19566
Conversation
This commit fixes two problems in the determination of the address to
use when unsafe fast path transforms an access to a static field via
VarHandle with OpenJDK VarHandles. The changes should have no meaningful
effect until unsafe fast path's support for OpenJDK VarHandles is
reinstated in a following commit.
First, for getAnd{Set,Add}{Int,Long} intrinsics accessing static fields,
the address should be
object.vmRef.ramStatics + (offset & ~J9_SUN_FIELD_OFFSET_MASK)
but with OpenJDK VarHandles it would instead be (object + offset), as
though for an instance field. So the access would produce an incorrect
result and almost certainly corrupt the java/lang/Class instance or
something near it in the heap.
Unsafe fast path already knows how to generate code that will use the
right address for these static field VarHandle accesses, but it didn't
apply to OpenJDK VarHandles because it was looking for a specific
recognized method value that will not occur. Now instead it detects
static accesses using isUnsafeCallerAccessingStaticField().
Second, transformed get and put operations accessing static fields with
OpenJDK VarHandles were using the wrong mask to clear the low bits of
the offset, specifically 1 instead of J9_SUN_FIELD_OFFSET_MASK (3). It
worked in Java 11 because FieldVarHandle uses lookupField(), which (for
static fields) sets J9_SUN_STATIC_FIELD_OFFSET_TAG (1) but never sets
J9_SUN_FINAL_FIELD_OFFSET_TAG (2). In Java 17+, a VarHandle representing
a static final field does have J9_SUN_FINAL_FIELD_OFFSET_TAG set in its
offset field, and clearing only the lowest bit will produce an offset
that is off by 2.
|
@0xdaryl could you please review? |
|
When it comes time for PR testing, pls ensure sanity.openjdk is run, if it hasn't already been done internally. There was quite a bit of fallout from the previous bad PR. It took jenkins machines offline after filling the disk, and delayed testing for the 0.44 release. |
|
Oh, sorry, I forgot to mention that. I did run JDK17 sanity.openjdk as part of my own testing, and it was good. I also ran a 5x grinder of jdk_lang_VarHandleTest_j9_[01], which passed. To make sure the grinder was meaningful, I made a build with the revert only, i.e. without my additional fixes, and used it to run another grinder the same way. That one failed all over the place and showed JIT and VM crashes similar to those in #19532 |
|
Jenkins test sanity.functional,sanity.openjdk all jdk21 |
|
@jdmpapin : please sift through the test failures. There are multiple JIT optimizer crashes in UseDef. Examples: https://openj9-jenkins.osuosl.org/job/Test_openjdk21_j9_sanity.openjdk_s390x_linux_Personal_testList_1/25/console |
|
Actually, at least some of those failures have been seen elsewhere, so likely not related to this PR: |
|
I've looked at all of the failures so far, and they're all #19661 Only the windows tests are still running at the moment. I expect to see the same crashes in windows sanity.openjdk |
|
Can you rebase so we pick up the fix for 19661 and I can re-start testing? I will only do the platform/OS combos that failed before. |
|
PR builds use So the fix will already be included. I fetched it myself to double check: I might as well restart them now Jenkins test sanity.openjdk alinux64,amac,aix,xlinux,xmac,zlinux jdk21 |
|
Both Windows test jobs were aborted Jenkins test sanity.functional,sanity.openjdk win jdk21 |
|
I'm guessing this has caused #19693 |
This PR reverts #19535, effectively reapplying #19234.
But before that, it fixes two problems in the determination of the address to use when unsafe fast path transforms an access to a static field via
VarHandlewith OpenJDKVarHandles. Applying these fixes first allows them to be kept separate from the revert commit without reintroducing the bugs between commits.First, for
getAnd{Set,Add}{Int,Long}intrinsics accessing static fields, the address should bebut with OpenJDK
VarHandles it would instead be(object + offset), as though for an instance field. So the access would produce an incorrect result and almost certainly corrupt thejava/lang/Classinstance or something near it in the heap.Unsafe fast path already knows how to generate code that will use the right address for these static field
VarHandleaccesses, but it didn't apply to OpenJDKVarHandles because it was looking for a specific recognized method value that will not occur. Now instead it detects static accesses usingisUnsafeCallerAccessingStaticField().Second, transformed get and put operations accessing static fields with OpenJDK
VarHandles were using the wrong mask to clear the low bits of the offset, specifically 1 instead ofJ9_SUN_FIELD_OFFSET_MASK(3). It worked in Java 11 becauseFieldVarHandleuseslookupField(), which (for static fields) setsJ9_SUN_STATIC_FIELD_OFFSET_TAG(1) but never setsJ9_SUN_FINAL_FIELD_OFFSET_TAG(2). In Java 17+, aVarHandlerepresenting a static final field does haveJ9_SUN_FINAL_FIELD_OFFSET_TAGset in its offset field, and clearing only the lowest bit will produce an offset that is off by 2.Fixes #19532