Ensure class variables have at least acquire-release semantics

* Encapsulate accesses so all accesses use the proper memory barriers.
* See https://preshing.com/20120913/acquire-and-release-semantics/
  and https://preshing.com/20130823/the-synchronizes-with-relation/
  for good articles about this, and why those barriers should be enough.

Author: eregon

src/main/java/org/truffleruby/core/module/ModuleFields.java

@@ -197,11 +197,9 @@ public void initCopy(RubyModule from) {
         }
 
         for (Object key : fromFields.classVariables.getShape().getKeys()) {
-            final Object value = DynamicObjectLibrary.getUncached().getOrDefault(fromFields.classVariables, key, null);
+            final Object value = fromFields.classVariables.read((String) key, DynamicObjectLibrary.getUncached());
             if (value != null) { // do not copy if it was removed concurrently
-                synchronized (this.classVariables) {
-                    DynamicObjectLibrary.getUncached().put(this.classVariables, key, value);
-                }
+                this.classVariables.put((String) key, value, DynamicObjectLibrary.getUncached());
             }
         }
 
@@ -862,7 +860,7 @@ public void getAdjacentObjects(Set<Object> adjacent) {
         }
 
         for (Object key : classVariables.getShape().getKeys()) {
-            final Object value = DynamicObjectLibrary.getUncached().getOrDefault(classVariables, key, null);
+            final Object value = classVariables.read((String) key, DynamicObjectLibrary.getUncached());
             if (value != null) {
                 ObjectGraph.addProperty(adjacent, value);
             }

src/main/java/org/truffleruby/core/module/ModuleOperations.java

@@ -628,9 +628,7 @@ public static void setClassVariable(RubyLanguage language, RubyContext context,
         if (!trySetClassVariable(module, name, value)) {
             synchronized (context.getClassVariableDefinitionLock()) {
                 if (!trySetClassVariable(module, name, value)) {
-                    synchronized (moduleFields.getClassVariables()) {
-                        DynamicObjectLibrary.getUncached().put(moduleFields.getClassVariables(), name, value);
-                    }
+                    moduleFields.getClassVariables().put(name, value, DynamicObjectLibrary.getUncached());
                 }
             }
         }
@@ -639,30 +637,18 @@ public static void setClassVariable(RubyLanguage language, RubyContext context,
     private static boolean trySetClassVariable(RubyModule topModule, String name, Object value) {
         return classVariableLookup(
                 topModule,
-                module -> {
-                    synchronized (module.fields.getClassVariables()) {
-                        return DynamicObjectLibrary.getUncached().putIfPresent(
-                                module.fields.getClassVariables(),
-                                name,
-                                value) ? module : null;
-                    }
-                }) != null;
+                module -> module.fields.getClassVariables().putIfPresent(
+                        name,
+                        value,
+                        DynamicObjectLibrary.getUncached()) ? module : null) != null;
     }
 
     @TruffleBoundary
     public static Object removeClassVariable(ModuleFields fields, RubyContext context, Node currentNode, String name) {
         fields.checkFrozen(context, currentNode);
 
         final ClassVariableStorage classVariables = fields.getClassVariables();
-
-        final Object found;
-        synchronized (classVariables) {
-            found = DynamicObjectLibrary.getUncached().getOrDefault(classVariables, name, null);
-            if (found != null) {
-                DynamicObjectLibrary.getUncached().removeKey(classVariables, name);
-            }
-        }
-
+        final Object found = classVariables.remove(name, DynamicObjectLibrary.getUncached());
         if (found == null) {
             throw new RaiseException(
                     context,

src/main/java/org/truffleruby/language/objects/classvariables/ClassVariableStorage.java

@@ -10,7 +10,9 @@
 package org.truffleruby.language.objects.classvariables;
 
 import com.oracle.truffle.api.object.DynamicObject;
+import com.oracle.truffle.api.object.DynamicObjectLibrary;
 import org.truffleruby.RubyLanguage;
+import org.truffleruby.extra.ffi.Pointer;
 
 public class ClassVariableStorage extends DynamicObject {
 
@@ -27,4 +29,40 @@ public ClassVariableStorage(RubyLanguage language) {
         super(language.classVariableShape);
     }
 
+    public Object read(String name, DynamicObjectLibrary objectLibrary) {
+        final Object value = objectLibrary.getOrDefault(this, name, null);
+        if (objectLibrary.isShared(this)) {
+            // This extra fence is to ensure acquire-release semantics for class variables, so the read above behaves
+            // like a load-acquire. There is a corresponding store-release barrier for class variables writes.
+            Pointer.UNSAFE.loadFence(); // load-acquire
+        }
+        return value;
+    }
+
+    public void put(String name, Object value, DynamicObjectLibrary objectLibrary) {
+        Pointer.UNSAFE.storeFence(); // store-release
+        synchronized (this) {
+            objectLibrary.put(this, name, value);
+        }
+    }
+
+    public boolean putIfPresent(String name, Object value, DynamicObjectLibrary objectLibrary) {
+        Pointer.UNSAFE.storeFence(); // store-release
+        synchronized (this) {
+            return objectLibrary.putIfPresent(this, name, value);
+        }
+    }
+
+    public Object remove(String name, DynamicObjectLibrary objectLibrary) {
+        final Object prev;
+        synchronized (this) {
+            prev = read(name, objectLibrary);
+            if (prev != null) {
+                Pointer.UNSAFE.storeFence(); // store-release
+                objectLibrary.removeKey(this, name);
+            }
+        }
+        return prev;
+    }
+
 }

src/main/java/org/truffleruby/language/objects/classvariables/LookupClassVariableNode.java

@@ -31,12 +31,12 @@ protected Object lookupClassVariable(RubyModule module, String name,
             @Cached LookupClassVariableStorageNode lookupClassVariableStorageNode,
             @Cached ConditionProfile noStorageProfile,
             @CachedLibrary(limit = "getDynamicObjectCacheLimit()") DynamicObjectLibrary objectLibrary) {
-        final ClassVariableStorage objectForClassVariables = lookupClassVariableStorageNode.execute(module, name);
+        final ClassVariableStorage classVariables = lookupClassVariableStorageNode.execute(module, name);
 
-        if (noStorageProfile.profile(objectForClassVariables == null)) {
+        if (noStorageProfile.profile(classVariables == null)) {
             return null;
         } else {
-            return objectLibrary.getOrDefault(objectForClassVariables, name, null);
+            return classVariables.read(name, objectLibrary);
         }
     }
 

src/main/java/org/truffleruby/language/objects/classvariables/SetClassVariableNode.java

@@ -17,7 +17,6 @@
 import com.oracle.truffle.api.profiles.BranchProfile;
 import org.truffleruby.core.module.ModuleOperations;
 import org.truffleruby.core.module.RubyModule;
-import org.truffleruby.extra.ffi.Pointer;
 import org.truffleruby.language.RubyContextNode;
 import org.truffleruby.language.objects.shared.WriteBarrierNode;
 
@@ -49,13 +48,8 @@ protected Object setClassVariableShared(RubyModule module, String name, Object v
             @Cached BranchProfile slowPath) {
         // See WriteObjectFieldNode
         writeBarrierNode.executeWriteBarrier(value);
-        Pointer.UNSAFE.storeFence();
-
-        final boolean set;
-        synchronized (classVariableStorage) {
-            set = objectLibrary.putIfPresent(classVariableStorage, name, value);
-        }
 
+        final boolean set = classVariableStorage.putIfPresent(name, value, objectLibrary);
         if (!set) {
             slowPath.enter();
             ModuleOperations.setClassVariable(getLanguage(), getContext(), module, name, value, this);