[GR-69306] Fix rare race conditions.

PullRequest: graal/22033

Author: christianhaeubl

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/SystemPropertiesSupport.java

@@ -25,7 +25,6 @@
 package com.oracle.svm.core.jdk;
 
 import static jdk.graal.compiler.core.common.LibGraalSupport.LIBGRAAL_SETTING_PROPERTY_PREFIX;
-import static jdk.graal.compiler.nodes.extended.MembarNode.FenceKind.STORE_STORE;
 
 import java.util.ArrayList;
 import java.util.Collections;
@@ -45,14 +44,12 @@
 
 import com.oracle.svm.core.FutureDefaultsOptions;
 import com.oracle.svm.core.SubstrateOptions;
-import com.oracle.svm.core.SubstrateUtil;
 import com.oracle.svm.core.VM;
 import com.oracle.svm.core.c.locale.LocaleSupport;
 import com.oracle.svm.core.config.ConfigurationValues;
 import com.oracle.svm.core.util.VMError;
 
 import jdk.graal.compiler.api.replacements.Fold;
-import jdk.graal.compiler.nodes.extended.MembarNode;
 
 /**
  * This class maintains the system properties at run time.
@@ -294,8 +291,8 @@ private synchronized void initializeAllProperties() {
         }
 
         /*
-         * No memory barrier is needed because the loop above already emits one STORE_STORE barrier
-         * per initialized system property.
+         * No memory barrier is needed here (same reasoning as for
+         * LazySystemProperty.markAsInitialized()).
          */
         allPropertiesInitialized = true;
     }
@@ -318,13 +315,7 @@ private void ensurePropertyInitialized(String key) {
         }
 
         LazySystemProperty property = lazySystemProperties.get(key);
-        if (property != null) {
-            ensureInitialized(property);
-        }
-    }
-
-    private void ensureInitialized(LazySystemProperty property) {
-        if (!property.isInitialized()) {
+        if (property != null && !property.isInitialized()) {
             initializeProperty(property);
         }
     }
@@ -385,11 +376,12 @@ public String computeValue() {
             return supplier.get();
         }
 
+        /**
+         * No memory barrier is needed here because the involved maps ({@link #initialProperties}
+         * and {@link #currentProperties}) already use acquire/release semantics when a value is
+         * added or accessed.
+         */
         public void markAsInitialized() {
-            if (!SubstrateUtil.HOSTED) {
-                /* Ensure that other threads see consistent values once 'initialized' is true. */
-                MembarNode.memoryBarrier(STORE_STORE);
-            }
             initialized = true;
         }
     }

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jdk/Target_jdk_internal_misc_VM.java

@@ -37,8 +37,6 @@
 import com.oracle.svm.core.annotate.TargetClass;
 import com.oracle.svm.core.snippets.KnownIntrinsics;
 
-import jdk.internal.misc.Unsafe;
-
 @TargetClass(className = "jdk.internal.misc.VM")
 public final class Target_jdk_internal_misc_VM {
     /** Ensure that we do not leak the full set of properties from the image generator. */
@@ -72,11 +70,14 @@ public static ClassLoader latestUserDefinedLoader0() {
 }
 
 final class DirectMemoryAccessors {
-    /*
-     * Not volatile to avoid a memory barrier when reading the values. Instead, an explicit barrier
-     * is inserted when writing the values.
+    /**
+     * This field needs to be volatile to ensure that reads emit a LOAD-LOAD barrier. Without this
+     * barrier, subsequent reads could be reordered before the read of {@link #initialized},
+     * allowing threads to observe an uninitialized value for {@link #directMemory}. We could
+     * directly emit a LOAD-LOAD barrier instead, but it doesn't make any difference in terms of the
+     * used instructions on any of the relevant CPU architectures.
      */
-    private static boolean initialized;
+    private static volatile boolean initialized;
     private static long directMemory;
 
     static long getDirectMemory() {
@@ -102,27 +103,16 @@ private static long tryInitialize() {
             newDirectMemory = Runtime.getRuntime().maxMemory();
         }
 
-        /*
-         * The initialization is not synchronized, so multiple threads can race. Usually this will
-         * lead to the same value, unless the runtime options are modified concurrently - which is
-         * possible but not a case we care about.
-         */
         directMemory = newDirectMemory;
-
-        /* Ensure values are published to other threads before marking fields as initialized. */
-        Unsafe.getUnsafe().storeFence();
+        /* STORE_STORE barrier is executed as part of the volatile write. */
         initialized = true;
-
         return newDirectMemory;
     }
 }
 
 final class PageAlignDirectMemoryAccessors {
-    /*
-     * Not volatile to avoid a memory barrier when reading the values. Instead, an explicit barrier
-     * is inserted when writing the values.
-     */
-    private static boolean initialized;
+    /** See {@link DirectMemoryAccessors#initialized} on why this needs to be volatile. */
+    private static volatile boolean initialized;
     private static boolean pageAlignDirectMemory;
 
     static Boolean getPageAlignDirectMemory() {
@@ -134,9 +124,7 @@ static Boolean getPageAlignDirectMemory() {
 
     private static void initialize() {
         pageAlignDirectMemory = Boolean.getBoolean("sun.nio.PageAlignDirectMemory");
-
-        /* Ensure values are published to other threads before marking fields as initialized. */
-        Unsafe.getUnsafe().storeFence();
+        /* STORE_STORE barrier is executed as part of the volatile write. */
         initialized = true;
     }
 }