Merge pull request #145 from bulasevich/GR-63266-2

[Backport] [Oracle GraalVM] [GR-63266] Backport to 23.1: Prevent absent hashcode computation split across safepoint checks.

Author: zakkak

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

@@ -194,18 +194,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(!hasFixedIdentityHashField(), "must always access field");
-        } else {
-            VMError.guarantee(!hasFixedIdentityHashField());
-            assert !hasIdentityHashFromAddress(currentHeader);
-        }
+        VMError.guarantee(!hasFixedIdentityHashField());
+        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

@@ -40,6 +40,8 @@
 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;
 import com.oracle.svm.core.threadlocal.FastThreadLocalObject;
@@ -83,6 +85,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.getOptionalIdentityHashOffset(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.hasFixedIdentityHashField()) {
+            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

@@ -25,7 +25,6 @@
 package com.oracle.svm.core.identityhashcode;
 
 import static org.graalvm.compiler.nodes.extended.BranchProbabilityNode.LIKELY_PROBABILITY;
-import static org.graalvm.compiler.nodes.extended.BranchProbabilityNode.NOT_FREQUENT_PROBABILITY;
 import static org.graalvm.compiler.nodes.extended.BranchProbabilityNode.SLOW_PATH_PROBABILITY;
 import static org.graalvm.compiler.nodes.extended.BranchProbabilityNode.probability;
 
@@ -52,6 +51,9 @@ final class SubstrateIdentityHashCodeSnippets extends IdentityHashCodeSnippets {
     static final SubstrateForeignCallDescriptor GENERATE_IDENTITY_HASH_CODE = SnippetRuntime.findForeignCall(
                     IdentityHashCodeSupport.class, "generateIdentityHashCode", true, IdentityHashCodeSupport.IDENTITY_HASHCODE_LOCATION);
 
+    static final SubstrateForeignCallDescriptor COMPUTE_ABSENT_IDENTITY_HASH_CODE = SnippetRuntime.findForeignCall(
+                    IdentityHashCodeSupport.class, "computeAbsentIdentityHashCode", true);
+
     static Templates createTemplates(OptionValues options, Providers providers) {
         return new Templates(new SubstrateIdentityHashCodeSnippets(), options, providers, IdentityHashCodeSupport.IDENTITY_HASHCODE_LOCATION);
     }
@@ -64,26 +66,21 @@ protected int computeIdentityHashCode(Object obj) {
             int offset = ol.getFixedIdentityHashOffset();
             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;
         }
         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.getOptionalIdentityHashOffset(obj);
             identityHashCode = ObjectAccess.readInt(obj, offset, IdentityHashCodeSupport.IDENTITY_HASHCODE_LOCATION);
         } else {
-            identityHashCode = IdentityHashCodeSupport.computeHashCodeFromAddress(obj);
-            if (probability(NOT_FREQUENT_PROBABILITY, !oh.hasIdentityHashFromAddress(header))) {
-                // Note 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;
     }
 
     @NodeIntrinsic(ForeignCallNode.class)
-    private static native int generateIdentityHashCode(@ConstantNodeParameter ForeignCallDescriptor descriptor, Object obj);
+    private static native int foreignCall(@ConstantNodeParameter ForeignCallDescriptor descriptor, Object obj);
 }