[GR-62966] Prevent absent hashcode computation split across safepoint checks.

PullRequest: graal/20298

Author: peter-hofer

substratevm/src/com.oracle.svm.core.genscavenge/src/com/oracle/svm/core/genscavenge/ObjectHeaderImpl.java

@@ -197,19 +197,14 @@ void setIdentityHashInField(Object o) {
     @Uninterruptible(reason = "Prevent a GC interfering with the object's identity hash state.", callerMustBe = true)
     @Override
     public void setIdentityHashFromAddress(Pointer ptr, Word currentHeader) {
-        if (GraalDirectives.inIntrinsic()) {
-            ReplacementsUtil.staticAssert(isIdentityHashFieldOptional(), "use only when hashcode fields are optional");
-        } else {
-            assert isIdentityHashFieldOptional() : "use only when hashcode fields are optional";
-            assert !hasIdentityHashFromAddress(currentHeader) : "must not already have a hashcode";
-        }
+        assert isIdentityHashFieldOptional() : "use only when hashcode fields are optional";
+        assert !hasIdentityHashFromAddress(currentHeader) : "must not already have a hashcode";
 
         UnsignedWord fromAddressState = IDHASH_STATE_FROM_ADDRESS.shiftLeft(IDHASH_STATE_SHIFT);
         UnsignedWord newHeader = currentHeader.and(IDHASH_STATE_BITS.not()).or(fromAddressState);
         writeHeaderToObject(ptr.toObjectNonNull(), newHeader);
-        if (!GraalDirectives.inIntrinsic()) {
-            assert hasIdentityHashFromAddress(readHeaderFromObject(ptr));
-        }
+
+        assert hasIdentityHashFromAddress(readHeaderFromPointer(ptr));
     }
 
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/identityhashcode/IdentityHashCodeSupport.java

@@ -33,6 +33,7 @@
 import com.oracle.svm.core.config.ConfigurationValues;
 import com.oracle.svm.core.config.ObjectLayout;
 import com.oracle.svm.core.heap.Heap;
+import com.oracle.svm.core.heap.ObjectHeader;
 import com.oracle.svm.core.hub.LayoutEncoding;
 import com.oracle.svm.core.snippets.SubstrateForeignCallTarget;
 import com.oracle.svm.core.threadlocal.FastThreadLocalFactory;
@@ -85,6 +86,34 @@ public static int generateIdentityHashCode(Object obj) {
         return newHashCode;
     }
 
+    @SubstrateForeignCallTarget(stubCallingConvention = false)
+    @Uninterruptible(reason = "Prevent a GC interfering with the object's identity hash state.")
+    public static int computeAbsentIdentityHashCode(Object obj) {
+        /*
+         * This code must not be inlined into the snippet because it could be used in an
+         * interruptible method: the individual reads and writes of the object header and hash salt
+         * could be independently spread across a safepoint check where a GC could happen, during
+         * which addresses, header or salt can change. This could cause inconsistent hash values,
+         * corrupted object headers (when modified by GC), or memory corruption and crashes (when
+         * writing the object header to the object's previous location after is has been moved).
+         */
+        ObjectHeader oh = Heap.getHeap().getObjectHeader();
+        Word objPtr = Word.objectToUntrackedPointer(obj);
+        Word header = ObjectHeader.readHeaderFromPointer(objPtr);
+        if (oh.hasOptionalIdentityHashField(header)) {
+            /*
+             * Between the snippet and execution of this method, another thread could have set the
+             * header bit and a GC could have triggered and added the field.
+             */
+            int offset = LayoutEncoding.getIdentityHashOffset(obj);
+            return ObjectAccess.readInt(obj, offset, IdentityHashCodeSupport.IDENTITY_HASHCODE_LOCATION);
+        }
+        if (!oh.hasIdentityHashFromAddress(header)) {
+            oh.setIdentityHashFromAddress(objPtr, header);
+        }
+        return computeHashCodeFromAddress(obj);
+    }
+
     @Uninterruptible(reason = "Prevent a GC interfering with the object's identity hash state.")
     public static int computeHashCodeFromAddress(Object obj) {
         Word address = Word.objectToUntrackedPointer(obj);

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/identityhashcode/SubstrateIdentityHashCodeFeature.java

@@ -24,15 +24,22 @@
  */
 package com.oracle.svm.core.identityhashcode;
 
+import com.oracle.svm.core.config.ConfigurationValues;
+import com.oracle.svm.core.config.ObjectLayout;
+import com.oracle.svm.core.feature.AutomaticallyRegisteredFeature;
 import com.oracle.svm.core.feature.InternalFeature;
 import com.oracle.svm.core.graal.meta.SubstrateForeignCallsProvider;
-import com.oracle.svm.core.feature.AutomaticallyRegisteredFeature;
 
 @AutomaticallyRegisteredFeature
 final class SubstrateIdentityHashCodeFeature implements InternalFeature {
 
     @Override
     public void registerForeignCalls(SubstrateForeignCallsProvider foreignCalls) {
-        foreignCalls.register(SubstrateIdentityHashCodeSnippets.GENERATE_IDENTITY_HASH_CODE);
+        ObjectLayout ol = ConfigurationValues.getObjectLayout();
+        if (ol.isIdentityHashFieldOptional()) {
+            foreignCalls.register(SubstrateIdentityHashCodeSnippets.COMPUTE_ABSENT_IDENTITY_HASH_CODE);
+        } else {
+            foreignCalls.register(SubstrateIdentityHashCodeSnippets.GENERATE_IDENTITY_HASH_CODE);
+        }
     }
 }

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/identityhashcode/SubstrateIdentityHashCodeSnippets.java

@@ -26,7 +26,6 @@
 
 import static jdk.graal.compiler.core.common.spi.ForeignCallDescriptor.CallSideEffect.NO_SIDE_EFFECT;
 import static jdk.graal.compiler.nodes.extended.BranchProbabilityNode.LIKELY_PROBABILITY;
-import static jdk.graal.compiler.nodes.extended.BranchProbabilityNode.NOT_FREQUENT_PROBABILITY;
 import static jdk.graal.compiler.nodes.extended.BranchProbabilityNode.SLOW_PATH_PROBABILITY;
 import static jdk.graal.compiler.nodes.extended.BranchProbabilityNode.probability;
 
@@ -53,6 +52,9 @@ final class SubstrateIdentityHashCodeSnippets extends IdentityHashCodeSnippets {
     static final SubstrateForeignCallDescriptor GENERATE_IDENTITY_HASH_CODE = SnippetRuntime.findForeignCall(
                     IdentityHashCodeSupport.class, "generateIdentityHashCode", NO_SIDE_EFFECT, IdentityHashCodeSupport.IDENTITY_HASHCODE_LOCATION);
 
+    static final SubstrateForeignCallDescriptor COMPUTE_ABSENT_IDENTITY_HASH_CODE = SnippetRuntime.findForeignCall(
+                    IdentityHashCodeSupport.class, "computeAbsentIdentityHashCode", NO_SIDE_EFFECT);
+
     static Templates createTemplates(OptionValues options, Providers providers) {
         return new Templates(new SubstrateIdentityHashCodeSnippets(), options, providers, IdentityHashCodeSupport.IDENTITY_HASHCODE_LOCATION);
     }
@@ -63,29 +65,24 @@ protected int computeIdentityHashCode(Object obj) {
         if (ol.isIdentityHashFieldOptional()) {
             int identityHashCode;
             ObjectHeader oh = Heap.getHeap().getObjectHeader();
-            Word objPtr = Word.objectToUntrackedPointer(obj);
-            Word header = ObjectHeader.readHeaderFromPointer(objPtr);
+            Word header = ObjectHeader.readHeaderFromObject(obj);
             if (probability(LIKELY_PROBABILITY, oh.hasOptionalIdentityHashField(header))) {
                 int offset = LayoutEncoding.getIdentityHashOffset(obj);
                 identityHashCode = ObjectAccess.readInt(obj, offset, IdentityHashCodeSupport.IDENTITY_HASHCODE_LOCATION);
             } else {
-                identityHashCode = IdentityHashCodeSupport.computeHashCodeFromAddress(obj);
-                if (probability(NOT_FREQUENT_PROBABILITY, !oh.hasIdentityHashFromAddress(header))) {
-                    // This write leads to frame state issues that break scheduling if done earlier
-                    oh.setIdentityHashFromAddress(objPtr, header);
-                }
+                identityHashCode = foreignCall(COMPUTE_ABSENT_IDENTITY_HASH_CODE, obj);
             }
             return identityHashCode;
         }
 
         int offset = LayoutEncoding.getIdentityHashOffset(obj);
         int identityHashCode = ObjectAccess.readInt(obj, offset, IdentityHashCodeSupport.IDENTITY_HASHCODE_LOCATION);
         if (probability(SLOW_PATH_PROBABILITY, identityHashCode == 0)) {
-            identityHashCode = generateIdentityHashCode(GENERATE_IDENTITY_HASH_CODE, obj);
+            identityHashCode = foreignCall(GENERATE_IDENTITY_HASH_CODE, obj);
         }
         return identityHashCode;
     }
 
     @NodeIntrinsic(ForeignCallNode.class)
-    private static native int generateIdentityHashCode(@ConstantNodeParameter ForeignCallDescriptor descriptor, Object obj);
+    private static native int foreignCall(@ConstantNodeParameter ForeignCallDescriptor descriptor, Object obj);
 }