Fix boot issue: ensure that __eq__ is never called during bootstrapping

steve-s · steve-s · commit c51cb85a83fa · 2023-11-30T20:32:41.000+01:00
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/ErrnoModuleBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/ErrnoModuleBuiltins.java
@@ -48,8 +48,10 @@
 import com.oracle.graal.python.builtins.CoreFunctions;
 import com.oracle.graal.python.builtins.Python3Core;
 import com.oracle.graal.python.builtins.PythonBuiltins;
+import com.oracle.graal.python.builtins.objects.common.EconomicMapStorage;
 import com.oracle.graal.python.builtins.objects.dict.PDict;
 import com.oracle.graal.python.builtins.objects.exception.OSErrorEnum;
+import com.oracle.graal.python.lib.PyObjectHashNode;
 import com.oracle.graal.python.nodes.function.PythonBuiltinBaseNode;
 import com.oracle.truffle.api.dsl.NodeFactory;
 import com.oracle.truffle.api.strings.TruffleString;
@@ -64,19 +66,21 @@ protected List<? extends NodeFactory<? extends PythonBuiltinBaseNode>> getNodeFa
     @Override
     public void initialize(Python3Core core) {
         super.initialize(core);
-        PDict errorCode = core.factory().createDict();
+        OSErrorEnum[] enumValues = OSErrorEnum.values();
+        EconomicMapStorage storage = EconomicMapStorage.create(enumValues.length + 1);
 
-        for (OSErrorEnum value : OSErrorEnum.values()) {
+        for (OSErrorEnum value : enumValues) {
             // if more OSError have the same number -> the last one wins
-            addConstant(value.getNumber(), toTruffleStringUncached(value.name()), errorCode);
+            addConstant(value.getNumber(), toTruffleStringUncached(value.name()), storage);
         }
 
         // publish the dictionary with mapping code -> string name
+        PDict errorCode = core.factory().createDict(storage);
         addBuiltinConstant("errorcode", errorCode);
     }
 
-    private void addConstant(int number, TruffleString name, PDict dict) {
+    private void addConstant(int number, TruffleString name, EconomicMapStorage storage) {
         addBuiltinConstant(name, number);
-        dict.setItem(number, name);
+        storage.putUncachedWithJavaEq(number, PyObjectHashNode.hash(number), name);
     }
 }
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/SysModuleBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/SysModuleBuiltins.java
@@ -673,8 +673,7 @@ public void postInitialize0(Python3Core core) {
     private static PFrozenSet createStdLibModulesSet(PythonObjectFactory factory) {
         EconomicMapStorage storage = EconomicMapStorage.create(STDLIB_MODULE_NAMES.length);
         for (String s : STDLIB_MODULE_NAMES) {
-            TruffleString ts = toTruffleStringUncached(s);
-            storage.putUncached(ts, PNone.NONE);
+            storage.putUncachedWithJavaEq(s, PNone.NONE);
         }
         return factory.createFrozenSet(storage);
     }
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/hashlib/HashlibModuleBuiltins.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/hashlib/HashlibModuleBuiltins.java
@@ -49,7 +49,6 @@
 import java.security.NoSuchAlgorithmException;
 import java.security.Security;
 import java.util.ArrayList;
-import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -153,11 +152,11 @@ protected List<? extends NodeFactory<? extends PythonBuiltinBaseNode>> getNodeFa
 
     @Override
     public void initialize(Python3Core core) {
-        LinkedHashMap<String, Object> algos = new LinkedHashMap<>();
+        EconomicMapStorage algos = EconomicMapStorage.create(DIGEST_ALGORITHMS.length);
         for (var digest : DIGEST_ALGORITHMS) {
-            algos.put(digest, PNone.NONE);
+            algos.putUncachedWithJavaEq(digest, PNone.NONE);
         }
-        addBuiltinConstant("openssl_md_meth_names", core.factory().createFrozenSet(EconomicMapStorage.create(algos)));
+        addBuiltinConstant("openssl_md_meth_names", core.factory().createFrozenSet(algos));
 
         EconomicMapStorage storage = EconomicMapStorage.create();
         addBuiltinConstant(CONSTRUCTORS, core.factory().createMappingproxy(core.factory().createDict(storage)));
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/EconomicMapStorage.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/EconomicMapStorage.java
@@ -41,6 +41,7 @@
 package com.oracle.graal.python.builtins.objects.common;
 
 import static com.oracle.graal.python.util.PythonUtils.TS_ENCODING;
+import static com.oracle.graal.python.util.PythonUtils.toTruffleStringUncached;
 
 import java.util.LinkedHashMap;
 import java.util.Map.Entry;
@@ -153,6 +154,17 @@ private void putUncached(Object key, Object value) {
         ObjectHashMap.PutNode.putUncached(this.map, key, PyObjectHashNode.executeUncached(key), value);
     }
 
+    // Solves boot-order problem, do not use in normal code or during startup when __eq__ of
+    // builtins may not be properly set-up
+    public void putUncachedWithJavaEq(Object key, long keyHash, Object value) {
+        ObjectHashMap.PutNode.putUncachedWithJavaEq(this.map, key, keyHash, value);
+    }
+
+    public void putUncachedWithJavaEq(String key, Object value) {
+        TruffleString ts = toTruffleStringUncached(key);
+        putUncachedWithJavaEq(ts, PyObjectHashNode.hash(ts, HashCodeNode.getUncached()), value);
+    }
+
     private static void putAllUncached(LinkedHashMap<String, Object> map, EconomicMapStorage result) {
         CompilerAsserts.neverPartOfCompilation();
         for (Entry<String, Object> entry : map.entrySet()) {
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/ObjectHashMap.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/ObjectHashMap.java
@@ -44,9 +44,14 @@
 
 import java.util.Arrays;
 
+import com.oracle.graal.python.builtins.PythonBuiltinClassType;
+import com.oracle.graal.python.builtins.objects.cext.PythonNativeClass;
+import com.oracle.graal.python.builtins.objects.type.PythonManagedClass;
+import com.oracle.graal.python.builtins.objects.type.SpecialMethodSlot;
 import com.oracle.graal.python.lib.PyObjectRichCompareBool;
 import com.oracle.graal.python.lib.PyObjectRichCompareBool.EqNode;
 import com.oracle.graal.python.util.PythonUtils;
+import com.oracle.truffle.api.CompilerAsserts;
 import com.oracle.truffle.api.CompilerDirectives;
 import com.oracle.truffle.api.CompilerDirectives.TruffleBoundary;
 import com.oracle.truffle.api.HostCompilerDirectives.InliningCutoff;
@@ -60,6 +65,7 @@
 import com.oracle.truffle.api.nodes.Node;
 import com.oracle.truffle.api.profiles.InlinedBranchProfile;
 import com.oracle.truffle.api.profiles.InlinedCountingConditionProfile;
+import com.oracle.truffle.api.strings.TruffleString;
 
 /**
  * Generic dictionary/set backing storage implementation.
@@ -347,6 +353,8 @@ public static Object doGetWithRestart(Frame frame, Node inliningTarget, ObjectHa
                         @Cached InlinedCountingConditionProfile collisionFoundNoValue,
                         @Cached InlinedCountingConditionProfile collisionFoundEqKey,
                         @Cached PyObjectRichCompareBool.EqNode eqNode) {
+            // must not call generic __eq__ before this
+            assert map.size == 0 || SpecialMethodSlot.areBuiltinSlotsInitialized();
             while (true) {
                 try {
                     return doGet(frame, map, key, keyHash, inliningTarget, foundNullKey, foundSameHashKey,
@@ -449,6 +457,14 @@ public static void putUncached(ObjectHashMap map, Object key, long keyHash, Obje
 
         abstract void execute(Frame frame, Node inliningTarget, ObjectHashMap map, Object key, long keyHash, Object value);
 
+        static void putUncachedWithJavaEq(ObjectHashMap map, Object key, long keyHash, Object value) {
+            assert isJavaEqualsAllowed(key) : key;
+            doPutWithRestart(null, null, map, key, keyHash, value,
+                            InlinedBranchProfile.getUncached(), InlinedCountingConditionProfile.getUncached(), InlinedCountingConditionProfile.getUncached(),
+                            InlinedCountingConditionProfile.getUncached(), InlinedCountingConditionProfile.getUncached(), InlinedBranchProfile.getUncached(), InlinedBranchProfile.getUncached(),
+                            null);
+        }
+
         // "public" for testing...
         @Specialization
         public static void doPutWithRestart(Frame frame, Node inliningTarget, ObjectHashMap map, Object key, long keyHash, Object value,
@@ -460,6 +476,7 @@ public static void doPutWithRestart(Frame frame, Node inliningTarget, ObjectHash
                         @Cached InlinedBranchProfile rehash1Profile,
                         @Cached InlinedBranchProfile rehash2Profile,
                         @Cached PyObjectRichCompareBool.EqNode eqNode) {
+            assert map.size == 0 || (SpecialMethodSlot.areBuiltinSlotsInitialized() || eqNode == null);
             while (true) {
                 try {
                     doPut(frame, map, key, keyHash, value, inliningTarget, foundNullKey, foundEqKey,
@@ -728,6 +745,10 @@ private boolean keysEqual(int[] originalIndices, Frame frame, Node inliningTarge
         if (originalKey == key) {
             return true;
         }
+        if (CompilerDirectives.inInterpreter() && eqNode == null) {
+            // this is hack, see putUncachedWithJavaEq
+            return javaEquals(originalKey, key);
+        }
         boolean result = eqNode.compare(frame, inliningTarget, originalKey, key);
         if (getKey(index) != originalKey || indices != originalIndices) {
             // Either someone overridden the slot we are just examining, or rehasing reallocated the
@@ -746,6 +767,18 @@ private boolean keysEqual(int[] originalIndices, Frame frame, Node inliningTarge
         return result;
     }
 
+    private static boolean javaEquals(Object a, Object b) {
+        CompilerAsserts.neverPartOfCompilation();
+        assert isJavaEqualsAllowed(a) : a;
+        assert isJavaEqualsAllowed(b) : b;
+        return a.equals(b);
+    }
+
+    private static boolean isJavaEqualsAllowed(Object o) {
+        return o instanceof PythonManagedClass || o instanceof PythonBuiltinClassType || //
+                        o instanceof PythonNativeClass || o instanceof Number || o instanceof TruffleString;
+    }
+
     /**
      * Called when we need space for new entry. It determines the new size from the number of slots
      * occupied by real values (i.e., does not count dummy entries), so the new size may be actually
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/SpecialMethodSlot.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/SpecialMethodSlot.java
@@ -444,6 +444,10 @@ private static void initializeBuiltinClassSpecialMethodSlots(Python3Core core, P
     private static final Object builtinSlotsInitializationLock = new Object();
     private static volatile boolean builtinSlotsInitialized;
 
+    public static boolean areBuiltinSlotsInitialized() {
+        return builtinSlotsInitialized;
+    }
+
     /**
      * Initialized builtin type according to its respective builtin class. Only context independent
      * values are pushed from the class to the type, because types are shared across contexts. This
diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/TypeNodes.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/TypeNodes.java
@@ -750,9 +750,19 @@ protected static void unsafeAddSubclass(Object base, Object subclass) {
             long hash = ObjectBuiltins.HashNode.hash(subclass);
             PDict dict = executeUncached(base);
             HashingStorage storage = dict.getDictStorage();
-            HashingStorageSetItemWithHash setItem = HashingStorageSetItemWithHashNodeGen.getUncached();
-            storage = setItem.execute(null, null, storage, subclass, hash, subclass);
-            dict.setDictStorage(storage);
+            // Booting order problem: special method slot for object.__eq__ is not initialized yet
+            // In the unlikely event of hash collision __eq__ would fail
+            if (HashingStorageLen.executeUncached(storage) == 0) {
+                // This should not call __eq__
+                HashingStorageSetItemWithHash setItem = HashingStorageSetItemWithHashNodeGen.getUncached();
+                storage = setItem.execute(null, null, storage, subclass, hash, subclass);
+                dict.setDictStorage(storage);
+            } else {
+                // the storage must be EconomicMap, because keys should not be Strings so there is
+                // no other option left
+                EconomicMapStorage mapStorage = (EconomicMapStorage) storage;
+                mapStorage.putUncachedWithJavaEq(subclass, hash, subclass);
+            }
         }
 
         protected static void unsafeRemoveSubclass(Object base, Object subclass) {
@@ -829,10 +839,12 @@ abstract static class EachSubclassAdd extends HashingStorageForEachCallback<Pyth
             static PythonAbstractClassList doIt(Frame frame, Node inliningTarget, HashingStorage storage, HashingStorageIterator it, PythonAbstractClassList subclasses,
                             @Cached HashingStorageIteratorKey itKey,
                             @Cached HashingStorageIteratorKeyHash itKeyHash,
+                            @Cached HashingStorageIteratorValue itValue,
                             @Cached HashingStorageGetItemWithHash getItemNode) {
                 long hash = itKeyHash.execute(inliningTarget, storage, it);
                 Object key = itKey.execute(inliningTarget, storage, it);
-                subclasses.add(PythonAbstractClass.cast(getItemNode.execute(frame, inliningTarget, storage, key, hash)));
+                Object value = itValue.execute(inliningTarget, storage, it);
+                subclasses.add(PythonAbstractClass.cast(value));
                 return subclasses;
             }
         }

Original file line number	Diff line number	Diff line change
`@@ -673,8 +673,7 @@ public void postInitialize0(Python3Core core) {`
`673`	`673`	`private static PFrozenSet createStdLibModulesSet(PythonObjectFactory factory) {`
`674`	`674`	`EconomicMapStorage storage = EconomicMapStorage.create(STDLIB_MODULE_NAMES.length);`
`675`	`675`	`for (String s : STDLIB_MODULE_NAMES) {`
`676`		`- TruffleString ts = toTruffleStringUncached(s);`
`677`		`- storage.putUncached(ts, PNone.NONE);`
	`676`	`+ storage.putUncachedWithJavaEq(s, PNone.NONE);`
`678`	`677`	`}`
`679`	`678`	`return factory.createFrozenSet(storage);`
`680`	`679`	`}`