[GR-38038] Fix potential race conditions.

PullRequest: graal/11587

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

@@ -128,6 +128,7 @@ AlignedHeader produceAlignedChunk() {
     }
 
     void freeExcessAlignedChunks() {
+        assert VMOperation.isGCInProgress();
         consumeAlignedChunks(WordFactory.nullPointer(), false);
     }
 
@@ -136,6 +137,7 @@ void freeExcessAlignedChunks() {
      * system. This method may only be called after the chunks were already removed from the spaces.
      */
     void consumeAlignedChunks(AlignedHeader firstChunk, boolean keepAll) {
+        assert VMOperation.isGCInProgress();
         assert firstChunk.isNull() || HeapChunk.getPrevious(firstChunk).isNull() : "prev must be null";
 
         UnsignedWord maxChunksToKeep = WordFactory.zero();
@@ -173,6 +175,7 @@ void consumeAlignedChunks(AlignedHeader firstChunk, boolean keepAll) {
     }
 
     private static void cleanAlignedChunk(AlignedHeader alignedChunk) {
+        assert VMOperation.isGCInProgress();
         AlignedHeapChunk.reset(alignedChunk);
         if (HeapParameters.getZapConsumedHeapChunks()) {
             zap(alignedChunk, HeapParameters.getConsumedHeapChunkZapWord());
@@ -192,6 +195,7 @@ private static void cleanAlignedChunk(AlignedHeader alignedChunk) {
      * list.
      */
     private void pushUnusedAlignedChunk(AlignedHeader chunk) {
+        assert VMOperation.isGCInProgress();
         if (SubstrateOptions.MultiThreaded.getValue()) {
             VMThreads.guaranteeOwnsThreadMutex("Should hold the lock when pushing to the global list.");
         }
@@ -243,7 +247,7 @@ private AlignedHeader popUnusedAlignedChunkUninterruptibly() {
     }
 
     private void freeUnusedAlignedChunksAtSafepoint(UnsignedWord count) {
-        VMOperation.guaranteeInProgressAtSafepoint("Removing non-atomically from the unused chunk list.");
+        assert VMOperation.isGCInProgress();
         if (count.equal(0)) {
             return;
         }
@@ -291,6 +295,7 @@ public static boolean areUnalignedChunksZeroed() {
      * to a free list.
      */
     static void consumeUnalignedChunks(UnalignedHeader firstChunk) {
+        assert VMOperation.isGCInProgress();
         freeUnalignedChunkList(firstChunk);
     }
 
@@ -322,6 +327,7 @@ Log report(Log log, boolean traceHeapChunks) {
     }
 
     boolean walkHeapChunks(MemoryWalker.Visitor visitor) {
+        assert VMOperation.isInProgressAtSafepoint();
         boolean continueVisiting = true;
         MemoryWalker.HeapChunkAccess<AlignedHeapChunk.AlignedHeader> access = AlignedHeapChunk.getMemoryWalkerAccess();
         for (AlignedHeapChunk.AlignedHeader aChunk = unusedAlignedChunks.get(); continueVisiting && aChunk.isNonNull(); aChunk = HeapChunk.getNext(aChunk)) {

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/code/AbstractRuntimeCodeInstaller.java

@@ -24,6 +24,7 @@
  */
 package com.oracle.svm.core.code;
 
+import com.oracle.svm.core.util.VMError;
 import org.graalvm.nativeimage.ImageSingletons;
 import org.graalvm.nativeimage.c.function.CodePointer;
 import org.graalvm.word.Pointer;
@@ -101,6 +102,7 @@ protected void operate() {
 
                 CodeInfoTable.getRuntimeCodeCache().addMethod(codeInfo);
                 platformHelper().performCodeSynchronization(codeInfo);
+                VMError.guarantee(CodeInfoAccess.getState(codeInfo) == CodeInfo.STATE_CODE_CONSTANTS_LIVE && installedCode.isValid(), "The code can't be invalidated before the VM operation finishes");
             } catch (Throwable e) {
                 error = e;
             }

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/code/CodeInfoAccess.java

@@ -50,8 +50,10 @@
  * object. This helper class is necessary to ensure that {@link CodeInfo} objects are used
  * correctly, as they are garbage collected even though they live in unmanaged memory. For that
  * purpose, every {@link CodeInfo} object has a tether object. The garbage collector can free a
- * {@link CodeInfo} object if its tether object is unreachable at a safepoint, that is, in any
- * method that is not annotated with {@link Uninterruptible}.
+ * {@link CodeInfo} object if its tether object is unreachable at a safepoint, that is, in
+ * <b>ANY</b> method that is <b>NOT</b> annotated with {@link Uninterruptible}. Even a blocking VM
+ * operation that needs a safepoint won't guarantee that the {@link CodeInfo} object is kept alive
+ * because GCs can be triggered within VM operations as well.
  * <p>
  * For better type-safety (i.e., to indicate if the tether of a {@link CodeInfo} object was already
  * acquired), we distinguish between {@link UntetheredCodeInfo} and {@link CodeInfo}.

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/code/CodeInfoTable.java

@@ -188,13 +188,21 @@ public static void invalidateInstalledCode(SubstrateInstalledCode installedCode)
         vmOp.enqueue();
     }
 
-    /**
-     * This invalidation is done at a safepoint and we acquire the tether of the {@link CodeInfo}
-     * object. Therefore, it is guaranteed that there is no conflict with the {@link CodeInfo}
-     * invalidation/freeing that the GC does because the tether is still reachable.
-     */
     @Uninterruptible(reason = "Must prevent the GC from freeing the CodeInfo object.")
-    private static void invalidateInstalledCodeAtSafepoint(CodePointer codePointer) {
+    private static void invalidateInstalledCodeAtSafepoint(SubstrateInstalledCode installedCode, CodePointer codePointer) {
+        /*
+         * Don't try to invalidate the code if it was already invalidated earlier. It is essential
+         * that we do this check in uninterruptible code because the GC can invalidate code as well.
+         */
+        if (!installedCode.isAlive()) {
+            return;
+        }
+
+        /*
+         * This invalidation is done at a safepoint and we acquire the tether of the {@link
+         * CodeInfo} object. Therefore, it is guaranteed that there is no conflict with the {@link
+         * CodeInfo} invalidation/freeing that the GC does because the tether is still reachable.
+         */
         UntetheredCodeInfo untetheredInfo = getRuntimeCodeCache().lookupCodeInfo(codePointer);
         Object tether = CodeInfoAccess.acquireTether(untetheredInfo);
         try {
@@ -259,9 +267,8 @@ private static class InvalidateInstalledCodeOperation extends JavaVMOperation {
         @Override
         protected void operate() {
             counters().invalidateInstalledCodeCount.inc();
-            if (installedCode.isAlive()) { // could be invalid (non-entrant), but executing
-                invalidateInstalledCodeAtSafepoint(WordFactory.pointer(installedCode.getAddress()));
-            }
+            CodePointer codePointer = WordFactory.pointer(installedCode.getAddress());
+            invalidateInstalledCodeAtSafepoint(installedCode, codePointer);
         }
     }
 }

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/code/RuntimeCodeCache.java

@@ -30,7 +30,6 @@
 import org.graalvm.compiler.options.Option;
 import org.graalvm.compiler.options.OptionType;
 import org.graalvm.nativeimage.CurrentIsolate;
-import org.graalvm.nativeimage.ImageSingletons;
 import org.graalvm.nativeimage.IsolateThread;
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.Platforms;
@@ -39,7 +38,6 @@
 import org.graalvm.word.UnsignedWord;
 import org.graalvm.word.WordFactory;
 
-import com.oracle.svm.core.MemoryWalker;
 import com.oracle.svm.core.SubstrateOptions;
 import com.oracle.svm.core.annotate.NeverInline;
 import com.oracle.svm.core.annotate.RestrictHeapAccess;
@@ -152,12 +150,14 @@ private static int binarySearch(NonmovableArray<UntetheredCodeInfo> a, int fromI
     }
 
     public void addMethod(CodeInfo info) {
-        assert VMOperation.isInProgressAtSafepoint() : "Modifying code tables that are used by the GC";
+        assert VMOperation.isInProgressAtSafepoint();
         InstalledCodeObserverSupport.activateObservers(RuntimeCodeInfoAccess.getCodeObserverHandles(info));
-        addMethodOperation(info);
+        addMethod0(info);
+        RuntimeCodeInfoHistory.singleton().logAdd(info);
     }
 
-    private void addMethodOperation(CodeInfo info) {
+    @Uninterruptible(reason = "Modifying code tables that are used by the GC")
+    private void addMethod0(CodeInfo info) {
         addMethodCount.inc();
         assert verifyTable();
         if (codeInfos.isNull() || numCodeInfos >= NonmovableArrays.lengthOf(codeInfos)) {
@@ -173,10 +173,10 @@ private void addMethodOperation(CodeInfo info) {
         numCodeInfos++;
         NonmovableArrays.setWord(codeInfos, insertionPoint, info);
 
-        RuntimeCodeInfoHistory.singleton().logAdd(info);
         assert verifyTable();
     }
 
+    @Uninterruptible(reason = "Modifying code tables that are used by the GC")
     private void enlargeTable() {
         int newTableSize = numCodeInfos * 2;
         if (newTableSize < INITIAL_TABLE_SIZE) {
@@ -191,6 +191,7 @@ private void enlargeTable() {
     }
 
     protected void invalidateMethod(CodeInfo info) {
+        assert VMOperation.isInProgressAtSafepoint();
         prepareInvalidation(info);
 
         /*
@@ -203,14 +204,13 @@ protected void invalidateMethod(CodeInfo info) {
     }
 
     protected void invalidateNonStackMethod(CodeInfo info) {
-        assert VMOperation.isGCInProgress() : "must only be called by the GC";
+        assert VMOperation.isGCInProgress() : "may only be called by the GC";
         prepareInvalidation(info);
         assert codeNotOnStackVerifier.verify(info);
         finishInvalidation(info, false);
     }
 
     private void prepareInvalidation(CodeInfo info) {
-        VMOperation.guaranteeInProgressAtSafepoint("Modifying code tables that are used by the GC");
         invalidateMethodCount.inc();
         assert verifyTable();
 
@@ -227,6 +227,13 @@ private void prepareInvalidation(CodeInfo info) {
     }
 
     private void finishInvalidation(CodeInfo info, boolean notifyGC) {
+        InstalledCodeObserverSupport.removeObservers(RuntimeCodeInfoAccess.getCodeObserverHandles(info));
+        finishInvalidation0(info, notifyGC);
+        RuntimeCodeInfoHistory.singleton().logInvalidate(info);
+    }
+
+    @Uninterruptible(reason = "Modifying code tables that are used by the GC")
+    private void finishInvalidation0(CodeInfo info, boolean notifyGC) {
         /*
          * Now it is guaranteed that the InstalledCode is not on the stack and cannot be invoked
          * anymore, so we can free the code and all metadata.
@@ -239,8 +246,7 @@ private void finishInvalidation(CodeInfo info, boolean notifyGC) {
         numCodeInfos--;
         NonmovableArrays.setWord(codeInfos, numCodeInfos, WordFactory.nullPointer());
 
-        RuntimeCodeInfoAccess.partialReleaseAfterInvalidate(info, notifyGC);
-        RuntimeCodeInfoHistory.singleton().logInvalidate(info);
+        RuntimeCodeInfoAccess.freePartially(info, notifyGC);
         assert verifyTable();
     }
 
@@ -268,34 +274,6 @@ private boolean verifyTable() {
         return true;
     }
 
-    public boolean walkRuntimeMethods(MemoryWalker.Visitor visitor) {
-        VMOperation.guaranteeInProgress("Modifying code tables that are used by the GC");
-        boolean continueVisiting = true;
-        for (int i = 0; (continueVisiting && (i < numCodeInfos)); i += 1) {
-            continueVisiting = walkRuntimeMethod(visitor, i);
-        }
-        return continueVisiting;
-    }
-
-    @Uninterruptible(reason = "Must prevent the GC from freeing the CodeInfo object.")
-    private boolean walkRuntimeMethod(MemoryWalker.Visitor visitor, int i) {
-        boolean continueVisiting;
-        UntetheredCodeInfo untetheredInfo = NonmovableArrays.getWord(codeInfos, i);
-        Object tether = CodeInfoAccess.acquireTether(untetheredInfo);
-        try {
-            CodeInfo codeInfo = CodeInfoAccess.convert(untetheredInfo, tether);
-            continueVisiting = visitRuntimeMethod0(visitor, codeInfo);
-        } finally {
-            CodeInfoAccess.releaseTether(untetheredInfo, tether);
-        }
-        return continueVisiting;
-    }
-
-    @Uninterruptible(reason = "Pass the now protected CodeInfo to interruptible code.", calleeMustBe = false)
-    private static boolean visitRuntimeMethod0(MemoryWalker.Visitor visitor, CodeInfo codeInfo) {
-        return visitor.visitCode(codeInfo, ImageSingletons.lookup(CodeInfoMemoryWalker.class));
-    }
-
     private static final class CodeNotOnStackVerifier extends StackFrameVisitor {
         private CodeInfo codeInfoToCheck;
 

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/code/RuntimeCodeInfoAccess.java

@@ -63,6 +63,7 @@ public final class RuntimeCodeInfoAccess {
     private RuntimeCodeInfoAccess() {
     }
 
+    @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     public static SubstrateInstalledCode getInstalledCode(CodeInfo info) {
         return CodeInfoAccess.getObjectField(info, CodeInfoImpl.INSTALLEDCODE_OBJFIELD);
     }
@@ -211,13 +212,8 @@ public static UnsignedWord getSizeOfCodeInfo() {
         return SizeOf.unsigned(CodeInfoImpl.class);
     }
 
-    static void partialReleaseAfterInvalidate(CodeInfo info, boolean notifyGC) {
-        InstalledCodeObserverSupport.removeObservers(RuntimeCodeInfoAccess.getCodeObserverHandles(info));
-        freePartially(info, notifyGC);
-    }
-
     @Uninterruptible(reason = "Prevent the GC from running - otherwise, it could accidentally visit the freed memory.")
-    private static void freePartially(CodeInfo info, boolean notifyGC) {
+    static void freePartially(CodeInfo info, boolean notifyGC) {
         CodeInfoImpl impl = cast(info);
         assert CodeInfoAccess.isAliveState(impl.getState()) || impl.getState() == CodeInfo.STATE_READY_FOR_INVALIDATION : "unexpected state (probably already released)";
         if (notifyGC) {

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/code/RuntimeCodeInfoHistory.java

@@ -24,6 +24,7 @@
  */
 package com.oracle.svm.core.code;
 
+import com.oracle.svm.core.annotate.Uninterruptible;
 import com.oracle.svm.core.code.CodeInfoAccess.HasInstalledCode;
 import com.oracle.svm.core.deopt.SubstrateInstalledCode;
 import com.oracle.svm.core.thread.Safepoint;
@@ -72,16 +73,20 @@ private void logOperation(String kind, CodeInfo info) {
         assert VMOperation.isInProgressAtSafepoint();
 
         traceCodeCache(kind, info, true);
-        recentOperations.next().setValues(kind, info, CodeInfoAccess.getState(info), CodeInfoAccess.getName(info), CodeInfoAccess.getCodeStart(info), CodeInfoAccess.getCodeEnd(info),
+        logOperation0(kind, info, CodeInfoAccess.getName(info));
+    }
+
+    @Uninterruptible(reason = "Prevent the GC from logging any invalidations as this could causes races.")
+    private void logOperation0(String kind, CodeInfo info, String name) {
+        recentOperations.next().setValues(kind, info, CodeInfoAccess.getState(info), name, CodeInfoAccess.getCodeStart(info), CodeInfoAccess.getCodeEnd(info),
                         RuntimeCodeInfoAccess.getInstalledCode(info));
     }
 
     public void logFree(CodeInfo info) {
         assert VMOperation.isInProgressAtSafepoint() || VMThreads.isTearingDown();
 
         traceCodeCache("Freed", info, false);
-        recentOperations.next().setValues("Freed", info, CodeInfoAccess.getState(info), null, CodeInfoAccess.getCodeStart(info), CodeInfoAccess.getCodeEnd(info),
-                        RuntimeCodeInfoAccess.getInstalledCode(info));
+        logOperation0("Freed", info, null);
     }
 
     private static void traceCodeCache(String kind, CodeInfo info, boolean allowJavaHeapAccess) {
@@ -128,6 +133,7 @@ private static class CodeCacheLogEntry {
         CodeCacheLogEntry() {
         }
 
+        @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
         public void setValues(String kind, CodeInfo codeInfo, int codeInfoState, String codeName, CodePointer codeStart, CodePointer codeEnd, SubstrateInstalledCode installedCode) {
             assert VMOperation.isInProgressAtSafepoint();
             assert Heap.getHeap().isInImageHeap(kind);

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/code/UntetheredCodeInfoAccess.java

@@ -32,7 +32,7 @@
 import com.oracle.svm.core.deopt.SubstrateInstalledCode;
 
 /**
- * Provides access to {@link UntetheredCodeInfo} objects. All methods in here should only be called
+ * Provides access to {@link UntetheredCodeInfo} objects. All methods in here may only be called
  * from uninterruptible code as the GC could free the {@link UntetheredCodeInfo} otherwise.
  */
 public final class UntetheredCodeInfoAccess {

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/deopt/SubstrateInstalledCode.java

@@ -24,6 +24,7 @@
  */
 package com.oracle.svm.core.deopt;
 
+import com.oracle.svm.core.annotate.Uninterruptible;
 import org.graalvm.compiler.core.common.CompilationIdentifier;
 
 import com.oracle.svm.core.code.CodeInfo;
@@ -49,9 +50,11 @@ public interface SubstrateInstalledCode {
     String getName();
 
     /** The entry point address of this code if {@linkplain #isValid valid}, or 0 otherwise. */
+    @Uninterruptible(reason = "Called from uninterruptible code", mayBeInlined = true)
     long getEntryPoint();
 
     /** The address of this code if {@linkplain #isAlive alive}, or 0 otherwise. */
+    @Uninterruptible(reason = "Called from uninterruptible code", mayBeInlined = true)
     long getAddress();
 
     /**
@@ -90,6 +93,7 @@ public interface SubstrateInstalledCode {
     void invalidate();
 
     /** Whether the code represented by this object exists and could have live invocations. */
+    @Uninterruptible(reason = "Called from uninterruptible code", mayBeInlined = true)
     boolean isAlive();
 
     /**