Skip to content

Commit fa1f052

Browse files
committed
Fix incorrect usage of @CachedLibrary("foreignObject") InteropLibrary objectLibrary in GetForeignObjectClassNode
* Specifically `objectLibrary.isIdentical(metaClass, cachedMetaClass, metaClassLibrary)` was incorrect as it used the objectLibrary for the metaClass, and the metaClassLibrary for the cachedMetaClass (both incorrect). * Avoid passing InteropLibrary instances behind TruffleBoundary as it is error prone and suboptimal for footprint. * Rename to foreignObject, metaObject and Library suffix for consistency in the whole file.
1 parent 2a31d99 commit fa1f052

File tree

1 file changed

+31
-30
lines changed

1 file changed

+31
-30
lines changed

graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/object/GetRegisteredClassNode.java

Lines changed: 31 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -94,37 +94,38 @@ static Object noRegisteredClass(Object foreignObject,
9494

9595
// Always return ForeignObject for meta objects (classes), because custom behavior should only
9696
// be added to instances.
97-
@Specialization(guards = "interopLibrary.isMetaObject(foreignObject) || !interopLibrary.hasMetaObject(foreignObject)")
97+
@Specialization(guards = "objectLibrary.isMetaObject(foreignObject) || !objectLibrary.hasMetaObject(foreignObject)")
9898
static Object getClassLookup(Object foreignObject,
99-
@SuppressWarnings("unused") @CachedLibrary(limit = "getCallSiteInlineCacheMaxDepth()") @Exclusive InteropLibrary interopLibrary,
99+
@SuppressWarnings("unused") @CachedLibrary(limit = "getCallSiteInlineCacheMaxDepth()") @Exclusive InteropLibrary objectLibrary,
100100
@Shared @Cached GetForeignObjectClassNode getForeignObjectClassNode) {
101101
return getForeignObjectClassNode.execute(foreignObject);
102102
}
103103

104104
@Specialization(guards = {"isSingleContext()",
105105
"!objectLibrary.isMetaObject(foreignObject)",
106106
"objectLibrary.hasMetaObject(foreignObject)",
107-
"objectLibrary.isIdentical(metaClass, cachedMetaClass, metaClassLibrary)"}, limit = "getCallSiteInlineCacheMaxDepth()", assumptions = "getContext().interopTypeRegistryCacheValidAssumption.getAssumption()")
107+
"metaObjectLibrary.isIdentical(metaObject, cachedMetaObject, cachedMetaObjectLibrary)"}, limit = "getCallSiteInlineCacheMaxDepth()", assumptions = "getContext().interopTypeRegistryCacheValidAssumption.getAssumption()")
108108
static Object getCachedClassLookup(@SuppressWarnings("unused") Object foreignObject,
109109
@SuppressWarnings("unused") @CachedLibrary("foreignObject") InteropLibrary objectLibrary,
110-
@SuppressWarnings("unused") @Bind("getMetaObject(objectLibrary, foreignObject)") Object metaClass,
111-
@SuppressWarnings("unused") @Cached(value = "metaClass") Object cachedMetaClass,
112-
@SuppressWarnings("unused") @CachedLibrary("cachedMetaClass") InteropLibrary metaClassLibrary,
113-
@Cached(value = "lookupUncached($node, foreignObject, cachedMetaClass, metaClassLibrary)") Object pythonClass) {
110+
@SuppressWarnings("unused") @Bind("getMetaObject(objectLibrary, foreignObject)") Object metaObject,
111+
@SuppressWarnings("unused") @CachedLibrary("metaObject") InteropLibrary metaObjectLibrary,
112+
@SuppressWarnings("unused") @Cached("metaObject") Object cachedMetaObject,
113+
@SuppressWarnings("unused") @CachedLibrary("cachedMetaObject") InteropLibrary cachedMetaObjectLibrary,
114+
@Cached(value = "lookupUncached($node, foreignObject, cachedMetaObject)") Object pythonClass) {
114115
return pythonClass;
115116
}
116117

117118
// used to catch the exception
118-
static Object getMetaObject(InteropLibrary interopLibrary, Object foreignObject) {
119+
static Object getMetaObject(InteropLibrary objectLibrary, Object foreignObject) {
119120
try {
120-
return interopLibrary.getMetaObject(foreignObject);
121+
return objectLibrary.getMetaObject(foreignObject);
121122
} catch (UnsupportedMessageException e) {
122123
throw CompilerDirectives.shouldNotReachHere(e);
123124
}
124125
}
125126

126-
static Object lookupUncached(Node inliningTarget, Object foreignObject, Object metaObject, InteropLibrary metaClassLibrary) {
127-
return lookup(inliningTarget, InlinedConditionProfile.getUncached(), foreignObject, metaObject, metaClassLibrary, ObjectHashMapFactory.GetNodeGen.getUncached());
127+
static Object lookupUncached(Node inliningTarget, Object foreignObject, Object metaObject) {
128+
return lookup(inliningTarget, InlinedConditionProfile.getUncached(), foreignObject, metaObject, InteropLibrary.getUncached(), ObjectHashMapFactory.GetNodeGen.getUncached());
128129
}
129130

130131
@Specialization(replaces = "getCachedClassLookup", guards = {"!objectLibrary.isMetaObject(foreignObject)", "objectLibrary.hasMetaObject(foreignObject)"})
@@ -140,8 +141,8 @@ static Object getFullLookupNode(Object foreignObject,
140141
static Object lookup(Node inliningTarget,
141142
InlinedConditionProfile builtClassFoundProfile,
142143
Object foreignObject,
143-
Object foreignMetaObject,
144-
InteropLibrary interopLibrary,
144+
Object metaObject,
145+
InteropLibrary metaObjectLibrary,
145146
GetNode getNode) {
146147
try {
147148
// lookup generated classes
@@ -151,27 +152,27 @@ static Object lookup(Node inliningTarget,
151152
var possiblePythonClass = getNode.execute(null,
152153
inliningTarget,
153154
PythonContext.get(inliningTarget).interopGeneratedClassCache,
154-
foreignMetaObject,
155-
interopLibrary.identityHashCode(foreignMetaObject));
155+
metaObject,
156+
metaObjectLibrary.identityHashCode(metaObject));
156157

157158
return builtClassFoundProfile.profile(inliningTarget, possiblePythonClass != null)
158159
? possiblePythonClass
159-
: lookupWithInheritance(inliningTarget, foreignObject, foreignMetaObject, interopLibrary);
160+
: lookupWithInheritance(inliningTarget, foreignObject, metaObject);
160161
} catch (UnsupportedMessageException e) {
161162
throw CompilerDirectives.shouldNotReachHere(e);
162163
}
163164
}
164165

165166
private static PythonClass buildClassAndRegister(Object foreignObject,
166-
Object foreignMetaObject,
167+
Object metaObject,
167168
Object[] bases,
168169
Node inliningTarget,
169-
PutNode putNode,
170-
InteropLibrary interopLibrary) throws UnsupportedMessageException {
170+
PutNode putNode) throws UnsupportedMessageException {
171+
InteropLibrary interopLibrary = InteropLibrary.getUncached();
171172
var context = PythonContext.get(inliningTarget);
172173
String languageName;
173174
try {
174-
languageName = context.getEnv().getLanguageInfo(interopLibrary.getLanguage(foreignMetaObject)).getName();
175+
languageName = context.getEnv().getLanguageInfo(interopLibrary.getLanguage(metaObject)).getName();
175176
} catch (UnsupportedMessageException e) {
176177
// some objects might not expose a language. Use "Foreign" as default
177178
languageName = "Foreign";
@@ -180,7 +181,7 @@ private static PythonClass buildClassAndRegister(Object foreignObject,
180181
var className = PythonUtils.toTruffleStringUncached(String.format(
181182
"%s_%s_generated",
182183
languageName,
183-
interopLibrary.getMetaQualifiedName(foreignMetaObject)));
184+
interopLibrary.getMetaQualifiedName(metaObject)));
184185

185186
// use length + 1 to make space for the foreign class
186187
var basesWithForeign = Arrays.copyOf(bases, bases.length + 1, PythonAbstractClass[].class);
@@ -193,19 +194,20 @@ private static PythonClass buildClassAndRegister(Object foreignObject,
193194
// Catch the error to additionally print the collected classes and specify the error
194195
// occurred during class creation
195196
throw PRaiseNode.getUncached().raiseWithCause(PythonBuiltinClassType.TypeError, e, ErrorMessages.INTEROP_CLASS_CREATION_NOT_POSSIBLE,
196-
interopLibrary.getMetaQualifiedName(foreignMetaObject),
197+
interopLibrary.getMetaQualifiedName(metaObject),
197198
Arrays.toString(basesWithForeign));
198199
}
199200
var module = context.lookupBuiltinModule(BuiltinNames.T_POLYGLOT);
200201
// set module attribute for nicer debug print, but don't add it to the module to prevent
201202
// access to the class
202203
pythonClass.setAttribute(SpecialAttributeNames.T___MODULE__, module.getAttribute(SpecialAttributeNames.T___NAME__));
203-
putNode.put(null, inliningTarget, context.interopGeneratedClassCache, foreignMetaObject, InteropLibrary.getUncached().identityHashCode(foreignMetaObject), pythonClass);
204+
putNode.put(null, inliningTarget, context.interopGeneratedClassCache, metaObject, InteropLibrary.getUncached().identityHashCode(metaObject), pythonClass);
204205
return pythonClass;
205206
}
206207

207208
@TruffleBoundary
208-
private static Object lookupWithInheritance(Node inliningTarget, Object foreignObject, Object foreignMetaObject, InteropLibrary interopLibrary) {
209+
private static Object lookupWithInheritance(Node inliningTarget, Object foreignObject, Object metaObject) {
210+
InteropLibrary interopLibrary = InteropLibrary.getUncached();
209211
// Try the full and expensive lookup
210212
try {
211213
var registry = PythonContext.get(inliningTarget).interopTypeRegistry;
@@ -216,23 +218,22 @@ private static Object lookupWithInheritance(Node inliningTarget, Object foreignO
216218
var possiblePythonClasses = (Object[]) getNode.execute(null,
217219
inliningTarget,
218220
registry,
219-
foreignMetaObject,
220-
interopLibrary.identityHashCode(foreignMetaObject));
221+
metaObject,
222+
interopLibrary.identityHashCode(metaObject));
221223
if (possiblePythonClasses != null) {
222224
Collections.addAll(foundClasses, possiblePythonClasses);
223225
}
224226

225227
// Now search also for parent classes
226-
searchAllParentClassesBfs(new Object[]{foreignMetaObject}, foundClasses, registry, getNode, inliningTarget);
228+
searchAllParentClassesBfs(new Object[]{metaObject}, foundClasses, registry, getNode, inliningTarget);
227229

228230
return foundClasses.isEmpty()
229231
? GetForeignObjectClassNode.getUncached().execute(foreignObject)
230232
: buildClassAndRegister(foreignObject,
231-
foreignMetaObject,
233+
metaObject,
232234
foundClasses.toArray(),
233235
inliningTarget,
234-
ObjectHashMapFactory.PutNodeGen.getUncached(),
235-
interopLibrary);
236+
ObjectHashMapFactory.PutNodeGen.getUncached());
236237
} catch (UnsupportedMessageException | InvalidArrayIndexException e) {
237238
throw CompilerDirectives.shouldNotReachHere(e);
238239
}

0 commit comments

Comments
 (0)