do not cache attributes in case they are stored in a dict with a key that could have potential sideeffects on mro

Author: tomasstupka

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/EconomicMapStorage.java

@@ -188,6 +188,7 @@ static Object getItemGeneric(EconomicMapStorage self, Object key, ThreadState st
         }
     }
 
+    @ExportMessage
     protected static boolean hasSideEffect(EconomicMapStorage self) {
         return self.map.hasSideEffect();
     }
@@ -351,7 +352,7 @@ static HashingStorage generic(EconomicMapStorage self, HashingStorage other,
         }
     }
 
-    protected static boolean hasSideEffect(Object o, LookupInheritedAttributeNode.Dynamic lookup) {
+    private static boolean hasDELSideEffect(Object o, LookupInheritedAttributeNode.Dynamic lookup) {
         return o instanceof PythonObject && lookup.execute(o, __DEL__) != PNone.NO_VALUE;
     }
 
@@ -377,10 +378,10 @@ static HashingStorage delItemWithStateWithSideEffect(EconomicMapStorage self, Ob
                         @Exclusive @Cached("createBinaryProfile()") ConditionProfile gotState) {
             DictKey newKey = new DictKey(key, self.getHashWithState(key, lib, state, gotState));
             Object value = self.map.removeKey(newKey, lib, otherlib, gotState, state);
-            if (hasSideEffect(key, lookup)) {
+            if (hasDELSideEffect(key, lookup)) {
                 callNode.executeObject(lookup.execute(key, __DEL__), key);
             }
-            if (hasSideEffect(value, lookup)) {
+            if (hasDELSideEffect(value, lookup)) {
                 callNode.executeObject(lookup.execute(value, __DEL__), value);
             }
             return self;
@@ -409,8 +410,8 @@ static HashingStorage clearWithSideEffect(EconomicMapStorage self,
             while (advance(cursor)) {
                 Object key = getKey(cursor);
                 Object value = getValue(cursor);
-                entries[i++] = hasSideEffect(key, lookup) ? key : null;
-                entries[i++] = hasSideEffect(value, lookup) ? value : null;
+                entries[i++] = hasDELSideEffect(key, lookup) ? key : null;
+                entries[i++] = hasDELSideEffect(value, lookup) ? value : null;
             }
             self.map.clear();
             for (Object o : entries) {

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/common/HashingStorageLibrary.java

@@ -293,6 +293,16 @@ public final <T> T forEach(HashingStorage self, ForEachNode<T> node, T arg) {
      */
     public abstract HashingStorage copy(HashingStorage self);
 
+    /**
+     * Determines if the storage has elements with a potential side effect on access.
+     * 
+     * @return {@code true} if the storage has elements with a potential side effect, otherwise
+     *         {@code false}.
+     */
+    public boolean hasSideEffect(HashingStorage self) {
+        return false;
+    }
+
     /**
      * @return {@code true} if the key-value pairs are equal between these storages.
      */

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/attributes/LookupAttributeInMRONode.java

@@ -43,6 +43,12 @@
 import com.oracle.graal.python.PythonLanguage;
 import com.oracle.graal.python.builtins.PythonBuiltinClassType;
 import com.oracle.graal.python.builtins.objects.PNone;
+import com.oracle.graal.python.builtins.objects.cext.PythonAbstractNativeObject;
+import com.oracle.graal.python.builtins.objects.cext.capi.CExtNodes;
+import com.oracle.graal.python.builtins.objects.cext.capi.NativeMember;
+import com.oracle.graal.python.builtins.objects.common.HashingCollectionNodes.GetDictStorageNode;
+import com.oracle.graal.python.builtins.objects.common.HashingStorageLibrary;
+import com.oracle.graal.python.builtins.objects.dict.PDict;
 import com.oracle.graal.python.builtins.objects.object.PythonObjectLibrary;
 import com.oracle.graal.python.builtins.objects.type.PythonClass;
 import com.oracle.graal.python.builtins.objects.type.TypeNodes;
@@ -64,7 +70,6 @@
 import com.oracle.truffle.api.dsl.ImportStatic;
 import com.oracle.truffle.api.dsl.ReportPolymorphism;
 import com.oracle.truffle.api.dsl.Specialization;
-import com.oracle.truffle.api.library.CachedLibrary;
 import com.oracle.truffle.api.nodes.ExplodeLoop;
 
 @ImportStatic(PythonOptions.class)
@@ -114,6 +119,8 @@ public static LookupAttributeInMRONode.Dynamic getUncached() {
     @Child private TypeNodes.IsSameTypeNode isSameTypeNode = IsSameTypeNodeGen.create();
     @Child private GetMroStorageNode getMroNode;
 
+    protected static final int MAX_DICT_TYPES = ReadAttributeFromObjectNode.MAX_DICT_TYPES;
+
     protected PythonCore getCore() {
         if (contextRef == null) {
             CompilerDirectives.transferToInterpreterAndInvalidate();
@@ -204,6 +211,22 @@ static final class AttributeAssumptionPair {
 
     protected AttributeAssumptionPair findAttrAndAssumptionInMRO(Object klass) {
         CompilerAsserts.neverPartOfCompilation();
+        // - avoid cases when attributes are stored in a dict containing elements
+        // with a potential MRO sideeffect on access.
+        // Also note that attr should not be read more than once.
+        // - assuming that keys/elements can't be added or replaced in a class dict.
+        // (PythonMangedClass returns MappingProxy, which is read-only). Native classes could
+        // possibly do so, but for now leaving it as it is.
+        PDict dict;
+        if (klass instanceof PythonAbstractNativeObject) {
+            Object nativedict = CExtNodes.GetTypeMemberNode.getUncached().execute(klass, NativeMember.TP_DICT);
+            dict = nativedict == PNone.NO_VALUE ? null : (PDict) nativedict;
+        } else {
+            dict = PythonObjectLibrary.getUncached().getDict(klass);
+        }
+        if (dict != null && HashingStorageLibrary.getUncached().hasSideEffect(GetDictStorageNode.getUncached().execute(dict))) {
+            return null;
+        }
         MroSequenceStorage mro = getMro(klass);
         Assumption attrAssumption = mro.createAttributeInMROFinalAssumption(key);
         for (int i = 0; i < mro.length(); i++) {
@@ -223,16 +246,11 @@ protected AttributeAssumptionPair findAttrAndAssumptionInMRO(Object klass) {
         return new AttributeAssumptionPair(attrAssumption, PNone.NO_VALUE);
     }
 
-    /**
-     * NOTE: Guard for case when attributes stored in a dict. Calling __eq__ or __hash__ on such
-     * could have mro related sideeffects.
-     */
-    @Specialization(guards = {"!lib.hasDict(klass)", "isSameType(cachedKlass, klass)", "cachedAttrInMROInfo != null"}, //
+    @Specialization(guards = {"isSameType(cachedKlass, klass)", "cachedAttrInMROInfo != null"}, //
                     limit = "getAttributeAccessInlineCacheMaxDepth()", //
                     assumptions = {"cachedAttrInMROInfo.assumption", "singleContextAssumption()"})
     protected static Object lookupConstantMROCached(@SuppressWarnings("unused") Object klass,
                     @Cached("klass") @SuppressWarnings("unused") Object cachedKlass,
-                    @CachedLibrary("klass") @SuppressWarnings("unused") PythonObjectLibrary lib,
                     @Cached("findAttrAndAssumptionInMRO(cachedKlass)") AttributeAssumptionPair cachedAttrInMROInfo) {
         return cachedAttrInMROInfo.value;
     }

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/attributes/ReadAttributeFromObjectNode.java

@@ -95,7 +95,7 @@ public static ReadAttributeFromObjectNode getUncachedForceType() {
 
     public abstract Object execute(Object object, Object key);
 
-    protected static final int MAX_DICT_TYPES = 2;
+    static final int MAX_DICT_TYPES = 2;
 
     // read from the DynamicObject store
     @Specialization(guards = {