Skip to content

Commit cbaf315

Browse files
steve-ssteve-s
committed
Fix issues uncovered by using the special method slots more
1 parent 4d3022a commit cbaf315

File tree

12 files changed

+165
-148
lines changed

12 files changed

+165
-148
lines changed

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/PythonLanguage.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -862,7 +862,7 @@ public void registerBuiltinDescriptorCallTarget(BuiltinMethodDescriptor descript
862862
@TruffleBoundary
863863
public RootCallTarget getDescriptorCallTarget(BuiltinMethodDescriptor descriptor) {
864864
RootCallTarget callTarget = descriptorCallTargets.get(descriptor);
865-
assert callTarget != null : "Missing call target for builtin slot descriptor " + descriptor.getFactory();
865+
assert callTarget != null : "Missing call target for builtin slot descriptor " + descriptor;
866866
return callTarget;
867867
}
868868

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/function/BuiltinMethodDescriptor.java

Lines changed: 70 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -40,17 +40,12 @@
4040
*/
4141
package com.oracle.graal.python.builtins.objects.function;
4242

43-
import static com.oracle.graal.python.nodes.SpecialMethodNames.__GETATTRIBUTE__;
44-
4543
import java.util.Objects;
4644
import java.util.concurrent.ConcurrentHashMap;
4745

4846
import com.oracle.graal.python.builtins.Builtin;
4947
import com.oracle.graal.python.builtins.PythonBuiltinClassType;
50-
import com.oracle.graal.python.builtins.objects.module.ModuleBuiltinsFactory;
51-
import com.oracle.graal.python.builtins.objects.object.ObjectBuiltinsFactory;
5248
import com.oracle.graal.python.builtins.objects.type.PythonBuiltinClass;
53-
import com.oracle.graal.python.builtins.objects.type.TypeBuiltinsFactory;
5449
import com.oracle.graal.python.nodes.function.PythonBuiltinBaseNode;
5550
import com.oracle.graal.python.nodes.function.builtins.PythonBinaryBuiltinNode;
5651
import com.oracle.graal.python.nodes.function.builtins.PythonTernaryBuiltinNode;
@@ -79,14 +74,18 @@ public abstract class BuiltinMethodDescriptor {
7974
*/
8075
private static final ConcurrentHashMap<BuiltinMethodDescriptor, BuiltinMethodDescriptor> CACHE = new ConcurrentHashMap<>();
8176

82-
public static final BuiltinMethodDescriptor OBJ_GET_ATTRIBUTE = get(__GETATTRIBUTE__, ObjectBuiltinsFactory.GetAttributeNodeFactory.getInstance(), PythonBuiltinClassType.PythonObject);
83-
public static final BuiltinMethodDescriptor MODULE_GET_ATTRIBUTE = get(__GETATTRIBUTE__, ModuleBuiltinsFactory.ModuleGetattritbuteNodeFactory.getInstance(), PythonBuiltinClassType.PythonModule);
84-
public static final BuiltinMethodDescriptor TYPE_GET_ATTRIBUTE = get(__GETATTRIBUTE__, TypeBuiltinsFactory.GetattributeNodeFactory.getInstance(), PythonBuiltinClassType.PythonClass);
85-
77+
/**
78+
* First caller of this method within given {@code PythonLanguage} instance should add a cache
79+
* entry for this builtin's call target.
80+
*/
8681
public static BuiltinMethodDescriptor get(PBuiltinFunction function) {
8782
CompilerAsserts.neverPartOfCompilation();
8883
NodeFactory<? extends PythonBuiltinBaseNode> factory = function.getBuiltinNodeFactory();
89-
if (factory == null || needsFrame(factory)) {
84+
if (factory == null) {
85+
return null;
86+
}
87+
Builtin builtinAnnotation = findBuiltinAnnotation(function.getName(), factory);
88+
if (builtinAnnotation.needsFrame()) {
9089
return null;
9190
}
9291

@@ -103,18 +102,24 @@ public static BuiltinMethodDescriptor get(PBuiltinFunction function) {
103102
return get(function.getName(), factory, type);
104103
}
105104

106-
private static BuiltinMethodDescriptor get(String name, NodeFactory<? extends PythonBuiltinBaseNode> factory, PythonBuiltinClassType type) {
105+
static BuiltinMethodDescriptor get(String name, NodeFactory<? extends PythonBuiltinBaseNode> factory, PythonBuiltinClassType type) {
106+
Builtin builtinAnnotation = findBuiltinAnnotation(name, factory);
107+
assert !builtinAnnotation.needsFrame();
108+
return get(name, factory, type, builtinAnnotation);
109+
}
110+
111+
private static BuiltinMethodDescriptor get(String name, NodeFactory<? extends PythonBuiltinBaseNode> factory, PythonBuiltinClassType type, Builtin builtinAnnotation) {
107112
CompilerAsserts.neverPartOfCompilation();
108113
Class<? extends PythonBuiltinBaseNode> nodeClass = factory.getNodeClass();
109114
BuiltinMethodDescriptor result = null;
110115
if (PythonUnaryBuiltinNode.class.isAssignableFrom(nodeClass)) {
111-
result = new UnaryBuiltinDescriptor(name, factory, type);
116+
result = new UnaryBuiltinDescriptor(name, factory, type, builtinAnnotation);
112117
assert result.getBuiltinAnnotation().minNumOfPositionalArgs() <= 1 : name;
113118
} else if (PythonBinaryBuiltinNode.class.isAssignableFrom(nodeClass)) {
114-
result = new BinaryBuiltinDescriptor(name, factory, type);
119+
result = new BinaryBuiltinDescriptor(name, factory, type, builtinAnnotation);
115120
assert result.getBuiltinAnnotation().minNumOfPositionalArgs() <= 2 : name;
116121
} else if (PythonTernaryBuiltinNode.class.isAssignableFrom(nodeClass)) {
117-
result = new TernaryBuiltinDescriptor(name, factory, type);
122+
result = new TernaryBuiltinDescriptor(name, factory, type, builtinAnnotation);
118123
assert result.getBuiltinAnnotation().minNumOfPositionalArgs() <= 3 : name;
119124
}
120125
if (result != null) {
@@ -127,31 +132,53 @@ public static boolean isInstance(Object obj) {
127132
return obj instanceof BuiltinMethodDescriptor;
128133
}
129134

130-
private static boolean needsFrame(NodeFactory<? extends PythonBuiltinBaseNode> factory) {
135+
private static Builtin findBuiltinAnnotation(String name, NodeFactory<? extends PythonBuiltinBaseNode> factory) {
131136
for (Builtin builtin : factory.getNodeClass().getAnnotationsByType(Builtin.class)) {
132-
if (builtin.needsFrame()) {
133-
return true;
137+
if (builtin.name().equals(name)) {
138+
return builtin;
134139
}
135140
}
136-
return false;
141+
throw new IllegalStateException(String.format(
142+
"Cannot find corresponding builtin annotation on class %s for builtin '%s'",
143+
factory.getNodeClass().getSimpleName(), name));
137144
}
138145

139146
private final NodeFactory<? extends PythonBuiltinBaseNode> factory;
140147
private final PythonBuiltinClassType type;
141-
// Name allows us to differentiate between builtins shared for reversible operations, such as
142-
// int.__mul__ and int.__rmul__, which have the same node factory
148+
// The builtin annotation allows us to differentiate between builtins shared for reversible
149+
// operations, such as int.__mul__ and int.__rmul__, which have the same node factory
150+
private final Builtin builtinAnnotation;
151+
// Name and isReverseOperation are shortcuts for builtinAnnotation.name()/reverseOperation()
143152
private final String name;
153+
private final boolean isReverseOperation;
144154

145-
private BuiltinMethodDescriptor(String name, NodeFactory<? extends PythonBuiltinBaseNode> factory, PythonBuiltinClassType type) {
155+
private BuiltinMethodDescriptor(String name, NodeFactory<? extends PythonBuiltinBaseNode> factory, PythonBuiltinClassType type, Builtin builtinAnnotation) {
156+
assert name.equals(builtinAnnotation.name());
146157
this.name = name;
147158
this.factory = factory;
148159
this.type = type;
160+
this.builtinAnnotation = builtinAnnotation;
161+
this.isReverseOperation = builtinAnnotation.reverseOperation();
149162
}
150163

151-
public final NodeFactory<? extends PythonBuiltinBaseNode> getFactory() {
164+
protected final NodeFactory<? extends PythonBuiltinBaseNode> getFactory() {
152165
return factory;
153166
}
154167

168+
public final <T extends NodeFactory<? extends PythonBuiltinBaseNode>> boolean isSameFactory(Class<T> builtinNodeFactoryClass) {
169+
// The assertion is possibly not strictly necessary, but this situation should get an
170+
// attention: it can be dangerous to rely only on factory identity for reverse operations,
171+
// because the factory cannot be used to create a functional node, we may also need to swap
172+
// the arguments.
173+
assert !getBuiltinAnnotation().reverseOperation() : this;
174+
return builtinNodeFactoryClass.isInstance(getFactory());
175+
}
176+
177+
public final boolean isDescriptorOf(PBuiltinFunction fun) {
178+
assert fun.getEnclosingType() instanceof PythonBuiltinClassType;
179+
return fun.getName().equals(name) && fun.getBuiltinNodeFactory() == getFactory() && fun.getEnclosingType() == type;
180+
}
181+
155182
public final PythonBuiltinClassType getEnclosingType() {
156183
return type;
157184
}
@@ -160,34 +187,45 @@ public final String getName() {
160187
return name;
161188
}
162189

190+
public final boolean isReverseOperation() {
191+
return isReverseOperation;
192+
}
193+
163194
public final Builtin getBuiltinAnnotation() {
164-
return factory.getNodeClass().getAnnotationsByType(Builtin.class)[0];
195+
return builtinAnnotation;
165196
}
166197

167-
@SuppressWarnings("StringEquality")
168198
@Override
169199
public final boolean equals(Object o) {
200+
CompilerAsserts.neverPartOfCompilation();
170201
if (this == o) {
171202
return true;
172203
}
173204
if (o == null || getClass() != o.getClass()) {
174205
return false;
175206
}
176207
BuiltinMethodDescriptor that = (BuiltinMethodDescriptor) o;
177-
return factory == that.factory && type == that.type && name == that.name;
208+
return factory == that.factory && type == that.type && name.equals(that.name);
178209
}
179210

180211
@Override
181212
public final int hashCode() {
182-
return Objects.hash(factory, type, System.identityHashCode(name));
213+
CompilerAsserts.neverPartOfCompilation();
214+
return Objects.hash(factory, type, name);
215+
}
216+
217+
@Override
218+
public String toString() {
219+
CompilerAsserts.neverPartOfCompilation();
220+
return getClass().getSimpleName() + "{" + type + "." + name + '}';
183221
}
184222

185223
// Note: manually written subclass for each builtin works better with Truffle DSL than one
186224
// generic class that would parametrize the 'factory' field
187225

188226
public static final class UnaryBuiltinDescriptor extends BuiltinMethodDescriptor {
189-
public UnaryBuiltinDescriptor(String name, NodeFactory<? extends PythonBuiltinBaseNode> factory, PythonBuiltinClassType type) {
190-
super(name, factory, type);
227+
public UnaryBuiltinDescriptor(String name, NodeFactory<? extends PythonBuiltinBaseNode> factory, PythonBuiltinClassType type, Builtin builtinAnnotation) {
228+
super(name, factory, type, builtinAnnotation);
191229
}
192230

193231
public PythonUnaryBuiltinNode createNode() {
@@ -196,8 +234,8 @@ public PythonUnaryBuiltinNode createNode() {
196234
}
197235

198236
public static final class BinaryBuiltinDescriptor extends BuiltinMethodDescriptor {
199-
public BinaryBuiltinDescriptor(String name, NodeFactory<? extends PythonBuiltinBaseNode> factory, PythonBuiltinClassType type) {
200-
super(name, factory, type);
237+
public BinaryBuiltinDescriptor(String name, NodeFactory<? extends PythonBuiltinBaseNode> factory, PythonBuiltinClassType type, Builtin builtinAnnotation) {
238+
super(name, factory, type, builtinAnnotation);
201239
}
202240

203241
public PythonBinaryBuiltinNode createNode() {
@@ -206,8 +244,8 @@ public PythonBinaryBuiltinNode createNode() {
206244
}
207245

208246
public static final class TernaryBuiltinDescriptor extends BuiltinMethodDescriptor {
209-
public TernaryBuiltinDescriptor(String name, NodeFactory<? extends PythonBuiltinBaseNode> factory, PythonBuiltinClassType type) {
210-
super(name, factory, type);
247+
public TernaryBuiltinDescriptor(String name, NodeFactory<? extends PythonBuiltinBaseNode> factory, PythonBuiltinClassType type, Builtin builtinAnnotation) {
248+
super(name, factory, type, builtinAnnotation);
211249
}
212250

213251
public PythonTernaryBuiltinNode createNode() {

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/object/ObjectBuiltins.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,7 @@ public static <T extends NodeFactory<? extends PythonBuiltinBaseNode>> boolean o
260260
NodeFactory<? extends PythonBuiltinBaseNode> factory = factoryProfile.profile(((PBuiltinFunction) method).getBuiltinNodeFactory());
261261
return !builtinNodeFactoryClass.isInstance(factory);
262262
} else if (method instanceof BuiltinMethodDescriptor) {
263-
return !builtinNodeFactoryClass.isInstance(((BuiltinMethodDescriptor) method).getFactory());
263+
return !((BuiltinMethodDescriptor) method).isSameFactory(builtinNodeFactoryClass);
264264
}
265265
return true;
266266
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/type/SpecialMethodSlot.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -875,7 +875,7 @@ public static boolean checkSlotOverrides(Python3Core core) {
875875
continue;
876876
}
877877
if (typeValue instanceof BuiltinMethodDescriptor && klassValue instanceof PBuiltinFunction &&
878-
((BuiltinMethodDescriptor) typeValue).getFactory() == ((PBuiltinFunction) klassValue).getBuiltinNodeFactory()) {
878+
((BuiltinMethodDescriptor) typeValue).isDescriptorOf((PBuiltinFunction) klassValue)) {
879879
// BuiltinMethodDescriptor and matching PBuiltinFunction: OK
880880
continue;
881881
}
@@ -943,7 +943,7 @@ public static boolean validateSlots(Object klassIn) {
943943
Object expected = slot.getValue(type);
944944
if (expected instanceof BuiltinMethodDescriptor) {
945945
assert actual instanceof PBuiltinFunction;
946-
assert ((PBuiltinFunction) actual).getBuiltinNodeFactory() == ((BuiltinMethodDescriptor) expected).getFactory();
946+
assert ((BuiltinMethodDescriptor) expected).isDescriptorOf((PBuiltinFunction) actual);
947947
} else if (expected != null) {
948948
assert PythonLanguage.canCache(expected);
949949
assert actual == expected;

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/lib/PyObjectGetMethod.java

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@
4545
import com.oracle.graal.python.builtins.PythonBuiltinClassType;
4646
import com.oracle.graal.python.builtins.objects.PNone;
4747
import com.oracle.graal.python.builtins.objects.PythonAbstractObject;
48-
import com.oracle.graal.python.builtins.objects.function.BuiltinMethodDescriptor;
48+
import com.oracle.graal.python.builtins.objects.function.BuiltinMethodDescriptors;
4949
import com.oracle.graal.python.builtins.objects.type.PythonManagedClass;
5050
import com.oracle.graal.python.builtins.objects.type.SpecialMethodSlot;
5151
import com.oracle.graal.python.nodes.ErrorMessages;
@@ -88,7 +88,7 @@ protected static boolean isObjectGetAttribute(Object lazyClass) {
8888
} else if (lazyClass instanceof PythonManagedClass) {
8989
slotValue = SpecialMethodSlot.GetAttribute.getValue((PythonManagedClass) lazyClass);
9090
}
91-
return slotValue == BuiltinMethodDescriptor.OBJ_GET_ATTRIBUTE;
91+
return slotValue == BuiltinMethodDescriptors.OBJ_GET_ATTRIBUTE;
9292
}
9393

9494
@Specialization(guards = "!isObjectGetAttribute(lazyClass)", limit = "1")
@@ -100,22 +100,22 @@ static Object getGenericAttr(Frame frame, Object receiver, String name,
100100
}
101101

102102
@Specialization(guards = {"isObjectGetAttribute(lazyClass)", "name == cachedName"}, limit = "1")
103-
Object getFixedAttr(VirtualFrame frame, Object receiver, @SuppressWarnings("unused") String name,
104-
@SuppressWarnings("unused") @Shared("getClassNode") @Cached GetClassNode getClass,
105-
@Bind("getClass.execute(receiver)") Object lazyClass,
106-
@SuppressWarnings("unused") @Cached("name") String cachedName,
107-
@Cached("create(name)") LookupAttributeInMRONode lookupNode,
108-
@Shared("getDescrClass") @Cached GetClassNode getDescrClass,
109-
@Shared("lookupGet") @Cached(parameters = "Get") LookupCallableSlotInMRONode lookupGet,
110-
@Shared("lookupSet") @Cached(parameters = "Set") LookupCallableSlotInMRONode lookupSet,
111-
@Shared("callGet") @Cached CallTernaryMethodNode callGet,
112-
@Shared("readAttr") @Cached ReadAttributeFromObjectNode readAttr,
113-
@Shared("raiseNode") @Cached PRaiseNode raiseNode,
114-
@Cached BranchProfile hasDescr,
115-
@Cached BranchProfile returnDataDescr,
116-
@Cached BranchProfile returnAttr,
117-
@Cached BranchProfile returnUnboundMethod,
118-
@Cached BranchProfile returnBoundDescr) {
103+
static Object getFixedAttr(VirtualFrame frame, Object receiver, @SuppressWarnings("unused") String name,
104+
@SuppressWarnings("unused") @Shared("getClassNode") @Cached GetClassNode getClass,
105+
@Bind("getClass.execute(receiver)") Object lazyClass,
106+
@SuppressWarnings("unused") @Cached("name") String cachedName,
107+
@Cached("create(name)") LookupAttributeInMRONode lookupNode,
108+
@Shared("getDescrClass") @Cached GetClassNode getDescrClass,
109+
@Shared("lookupGet") @Cached(parameters = "Get") LookupCallableSlotInMRONode lookupGet,
110+
@Shared("lookupSet") @Cached(parameters = "Set") LookupCallableSlotInMRONode lookupSet,
111+
@Shared("callGet") @Cached CallTernaryMethodNode callGet,
112+
@Shared("readAttr") @Cached ReadAttributeFromObjectNode readAttr,
113+
@Shared("raiseNode") @Cached PRaiseNode raiseNode,
114+
@Cached BranchProfile hasDescr,
115+
@Cached BranchProfile returnDataDescr,
116+
@Cached BranchProfile returnAttr,
117+
@Cached BranchProfile returnUnboundMethod,
118+
@Cached BranchProfile returnBoundDescr) {
119119
boolean methodFound = false;
120120
Object descr = lookupNode.execute(lazyClass);
121121
Object getMethod = PNone.NO_VALUE;

0 commit comments

Comments
 (0)