Skip to content

Commit 7279f9c

Browse files
msimacekmsimacek
committed
[GR-48745] Fix slice accessors not promoting borrowed results
PullRequest: graalpython/3092
2 parents 3fab7cc + 6b5010e commit 7279f9c

File tree

5 files changed

+74
-20
lines changed

5 files changed

+74
-20
lines changed

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextBuiltinRegistry.java

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ private PythonCextBuiltinRegistry() {
348348
public static final CApiBuiltinExecutable PyTruffle_Trace_Type = new CApiBuiltinExecutable("PyTruffle_Trace_Type", CApiCallPath.Ignored, ArgDescriptor.Int, new ArgDescriptor[]{ArgDescriptor.Pointer, ArgDescriptor.Pointer}, 287);
349349
public static final CApiBuiltinExecutable PyTruffle_TriggerGC = new CApiBuiltinExecutable("PyTruffle_TriggerGC", CApiCallPath.Ignored, ArgDescriptor.Void, new ArgDescriptor[]{ArgDescriptor.SIZE_T}, 288);
350350
public static final CApiBuiltinExecutable PyTruffle_True = new CApiBuiltinExecutable("PyTruffle_True", CApiCallPath.Ignored, ArgDescriptor.PyObjectTransfer, new ArgDescriptor[]{}, 289);
351-
public static final CApiBuiltinExecutable PyTruffle_Type = new CApiBuiltinExecutable("PyTruffle_Type", CApiCallPath.Ignored, ArgDescriptor.PyTypeObject, new ArgDescriptor[]{ArgDescriptor.ConstCharPtrAsTruffleString}, 290);
351+
public static final CApiBuiltinExecutable PyTruffle_Type = new CApiBuiltinExecutable("PyTruffle_Type", CApiCallPath.Ignored, ArgDescriptor.PyTypeObjectTransfer, new ArgDescriptor[]{ArgDescriptor.ConstCharPtrAsTruffleString}, 290);
352352
public static final CApiBuiltinExecutable PyTruffle_Type_Modified = new CApiBuiltinExecutable("PyTruffle_Type_Modified", CApiCallPath.Ignored, ArgDescriptor.Int, new ArgDescriptor[]{ArgDescriptor.PyTypeObject, ArgDescriptor.ConstCharPtrAsTruffleString, ArgDescriptor.PyObject}, 291);
353353
public static final CApiBuiltinExecutable PyTruffle_Unicode_AsUTF8AndSize_CharPtr = new CApiBuiltinExecutable("PyTruffle_Unicode_AsUTF8AndSize_CharPtr", CApiCallPath.Direct, ArgDescriptor.ConstCharPtr, new ArgDescriptor[]{ArgDescriptor.PyObject}, 292);
354354
public static final CApiBuiltinExecutable PyTruffle_Unicode_AsUTF8AndSize_Size = new CApiBuiltinExecutable("PyTruffle_Unicode_AsUTF8AndSize_Size", CApiCallPath.Direct, ArgDescriptor.Py_ssize_t, new ArgDescriptor[]{ArgDescriptor.PyObject}, 293);
@@ -410,10 +410,10 @@ private PythonCextBuiltinRegistry() {
410410
public static final CApiBuiltinExecutable Py_get_PyCFunctionObject_m_self = new CApiBuiltinExecutable("Py_get_PyCFunctionObject_m_self", CApiCallPath.Ignored, ArgDescriptor.PyObjectBorrowed, new ArgDescriptor[]{ArgDescriptor.PyCFunctionObject}, 349);
411411
public static final CApiBuiltinExecutable Py_get_PyCFunctionObject_m_weakreflist = new CApiBuiltinExecutable("Py_get_PyCFunctionObject_m_weakreflist", CApiCallPath.Ignored, ArgDescriptor.PyObjectBorrowed, new ArgDescriptor[]{ArgDescriptor.PyCFunctionObject}, 350);
412412
public static final CApiBuiltinExecutable Py_get_PyCFunctionObject_vectorcall = new CApiBuiltinExecutable("Py_get_PyCFunctionObject_vectorcall", CApiCallPath.Ignored, ArgDescriptor.vectorcallfunc, new ArgDescriptor[]{ArgDescriptor.PyCFunctionObject}, 351);
413-
public static final CApiBuiltinExecutable Py_get_PyCMethodObject_mm_class = new CApiBuiltinExecutable("Py_get_PyCMethodObject_mm_class", CApiCallPath.Ignored, ArgDescriptor.PyTypeObject, new ArgDescriptor[]{ArgDescriptor.PyCMethodObject}, 352);
413+
public static final CApiBuiltinExecutable Py_get_PyCMethodObject_mm_class = new CApiBuiltinExecutable("Py_get_PyCMethodObject_mm_class", CApiCallPath.Ignored, ArgDescriptor.PyTypeObjectBorrowed, new ArgDescriptor[]{ArgDescriptor.PyCMethodObject}, 352);
414414
public static final CApiBuiltinExecutable Py_get_PyCompactUnicodeObject_wstr_length = new CApiBuiltinExecutable("Py_get_PyCompactUnicodeObject_wstr_length", CApiCallPath.Ignored, ArgDescriptor.Py_ssize_t, new ArgDescriptor[]{ArgDescriptor.PyCompactUnicodeObject}, 353);
415415
public static final CApiBuiltinExecutable Py_get_PyDescrObject_d_name = new CApiBuiltinExecutable("Py_get_PyDescrObject_d_name", CApiCallPath.Ignored, ArgDescriptor.PyObjectBorrowed, new ArgDescriptor[]{ArgDescriptor.PyDescrObject}, 354);
416-
public static final CApiBuiltinExecutable Py_get_PyDescrObject_d_type = new CApiBuiltinExecutable("Py_get_PyDescrObject_d_type", CApiCallPath.Ignored, ArgDescriptor.PyTypeObject, new ArgDescriptor[]{ArgDescriptor.PyDescrObject}, 355);
416+
public static final CApiBuiltinExecutable Py_get_PyDescrObject_d_type = new CApiBuiltinExecutable("Py_get_PyDescrObject_d_type", CApiCallPath.Ignored, ArgDescriptor.PyTypeObjectBorrowed, new ArgDescriptor[]{ArgDescriptor.PyDescrObject}, 355);
417417
public static final CApiBuiltinExecutable Py_get_PyFrameObject_f_lineno = new CApiBuiltinExecutable("Py_get_PyFrameObject_f_lineno", CApiCallPath.Ignored, ArgDescriptor.Int, new ArgDescriptor[]{ArgDescriptor.PyFrameObject}, 356);
418418
public static final CApiBuiltinExecutable Py_get_PyGetSetDef_closure = new CApiBuiltinExecutable("Py_get_PyGetSetDef_closure", CApiCallPath.Ignored, ArgDescriptor.Pointer, new ArgDescriptor[]{ArgDescriptor.PyGetSetDef}, 357);
419419
public static final CApiBuiltinExecutable Py_get_PyGetSetDef_doc = new CApiBuiltinExecutable("Py_get_PyGetSetDef_doc", CApiCallPath.Ignored, ArgDescriptor.ConstCharPtrAsTruffleString, new ArgDescriptor[]{ArgDescriptor.PyGetSetDef}, 358);
@@ -437,11 +437,11 @@ private PythonCextBuiltinRegistry() {
437437
public static final CApiBuiltinExecutable Py_get_PyModuleObject_md_dict = new CApiBuiltinExecutable("Py_get_PyModuleObject_md_dict", CApiCallPath.Ignored, ArgDescriptor.PyObjectBorrowed, new ArgDescriptor[]{ArgDescriptor.PyModuleObject}, 376);
438438
public static final CApiBuiltinExecutable Py_get_PyModuleObject_md_state = new CApiBuiltinExecutable("Py_get_PyModuleObject_md_state", CApiCallPath.Ignored, ArgDescriptor.Pointer, new ArgDescriptor[]{ArgDescriptor.PyModuleObject}, 377);
439439
public static final CApiBuiltinExecutable Py_get_PyObject_ob_refcnt = new CApiBuiltinExecutable("Py_get_PyObject_ob_refcnt", CApiCallPath.Ignored, ArgDescriptor.Py_ssize_t, new ArgDescriptor[]{ArgDescriptor.PyObjectWrapper}, 378);
440-
public static final CApiBuiltinExecutable Py_get_PyObject_ob_type = new CApiBuiltinExecutable("Py_get_PyObject_ob_type", CApiCallPath.Ignored, ArgDescriptor.PyTypeObject, new ArgDescriptor[]{ArgDescriptor.PyObject}, 379);
440+
public static final CApiBuiltinExecutable Py_get_PyObject_ob_type = new CApiBuiltinExecutable("Py_get_PyObject_ob_type", CApiCallPath.Ignored, ArgDescriptor.PyTypeObjectBorrowed, new ArgDescriptor[]{ArgDescriptor.PyObject}, 379);
441441
public static final CApiBuiltinExecutable Py_get_PySetObject_used = new CApiBuiltinExecutable("Py_get_PySetObject_used", CApiCallPath.Ignored, ArgDescriptor.Py_ssize_t, new ArgDescriptor[]{ArgDescriptor.PySetObject}, 380);
442-
public static final CApiBuiltinExecutable Py_get_PySliceObject_start = new CApiBuiltinExecutable("Py_get_PySliceObject_start", CApiCallPath.Ignored, ArgDescriptor.PyObject, new ArgDescriptor[]{ArgDescriptor.PySliceObject}, 381);
443-
public static final CApiBuiltinExecutable Py_get_PySliceObject_step = new CApiBuiltinExecutable("Py_get_PySliceObject_step", CApiCallPath.Ignored, ArgDescriptor.PyObject, new ArgDescriptor[]{ArgDescriptor.PySliceObject}, 382);
444-
public static final CApiBuiltinExecutable Py_get_PySliceObject_stop = new CApiBuiltinExecutable("Py_get_PySliceObject_stop", CApiCallPath.Ignored, ArgDescriptor.PyObject, new ArgDescriptor[]{ArgDescriptor.PySliceObject}, 383);
442+
public static final CApiBuiltinExecutable Py_get_PySliceObject_start = new CApiBuiltinExecutable("Py_get_PySliceObject_start", CApiCallPath.Ignored, ArgDescriptor.PyObjectBorrowed, new ArgDescriptor[]{ArgDescriptor.PySliceObject}, 381);
443+
public static final CApiBuiltinExecutable Py_get_PySliceObject_step = new CApiBuiltinExecutable("Py_get_PySliceObject_step", CApiCallPath.Ignored, ArgDescriptor.PyObjectBorrowed, new ArgDescriptor[]{ArgDescriptor.PySliceObject}, 382);
444+
public static final CApiBuiltinExecutable Py_get_PySliceObject_stop = new CApiBuiltinExecutable("Py_get_PySliceObject_stop", CApiCallPath.Ignored, ArgDescriptor.PyObjectBorrowed, new ArgDescriptor[]{ArgDescriptor.PySliceObject}, 383);
445445
public static final CApiBuiltinExecutable Py_get_PyTupleObject_ob_item = new CApiBuiltinExecutable("Py_get_PyTupleObject_ob_item", CApiCallPath.Ignored, ArgDescriptor.PyObjectPtr, new ArgDescriptor[]{ArgDescriptor.PyTupleObject}, 384);
446446
public static final CApiBuiltinExecutable Py_get_PyUnicodeObject_data = new CApiBuiltinExecutable("Py_get_PyUnicodeObject_data", CApiCallPath.Ignored, ArgDescriptor.Pointer, new ArgDescriptor[]{ArgDescriptor.PyUnicodeObject}, 385);
447447
public static final CApiBuiltinExecutable Py_get_PyVarObject_ob_size = new CApiBuiltinExecutable("Py_get_PyVarObject_ob_size", CApiCallPath.Ignored, ArgDescriptor.Py_ssize_t, new ArgDescriptor[]{ArgDescriptor.PyVarObject}, 386);

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextBuiltins.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@
5656
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyObjectTransfer;
5757
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyThreadState;
5858
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyTypeObject;
59+
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyTypeObjectTransfer;
5960
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.Py_ssize_t;
6061
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.SIZE_T;
6162
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.UINTPTR_T;
@@ -918,7 +919,7 @@ static TruffleString encoding() {
918919
}
919920
}
920921

921-
@CApiBuiltin(ret = PyTypeObject, args = {ConstCharPtrAsTruffleString}, call = Ignored)
922+
@CApiBuiltin(ret = PyTypeObjectTransfer, args = {ConstCharPtrAsTruffleString}, call = Ignored)
922923
abstract static class PyTruffle_Type extends CApiUnaryBuiltinNode {
923924

924925
private static final TruffleString[] LOOKUP_MODULES = new TruffleString[]{

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/modules/cext/PythonCextSlotBuiltins.java

Lines changed: 52 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@
6969
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PySliceObject;
7070
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyTupleObject;
7171
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyTypeObject;
72+
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyTypeObjectBorrowed;
7273
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyUnicodeObject;
7374
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.PyVarObject;
7475
import static com.oracle.graal.python.builtins.objects.cext.capi.transitions.ArgDescriptor.Py_ssize_t;
@@ -144,7 +145,9 @@
144145
import com.oracle.graal.python.lib.PyObjectLookupAttr;
145146
import com.oracle.graal.python.nodes.PGuards;
146147
import com.oracle.graal.python.nodes.SpecialAttributeNames;
148+
import com.oracle.graal.python.nodes.attributes.ReadAttributeFromDynamicObjectNode;
147149
import com.oracle.graal.python.nodes.attributes.ReadAttributeFromObjectNode;
150+
import com.oracle.graal.python.nodes.attributes.WriteAttributeToDynamicObjectNode;
148151
import com.oracle.graal.python.nodes.attributes.WriteAttributeToObjectNode;
149152
import com.oracle.graal.python.nodes.object.GetClassNode;
150153
import com.oracle.graal.python.nodes.object.GetDictIfExistsNode;
@@ -162,10 +165,13 @@
162165
import com.oracle.truffle.api.dsl.Cached;
163166
import com.oracle.truffle.api.dsl.Cached.Exclusive;
164167
import com.oracle.truffle.api.dsl.Fallback;
168+
import com.oracle.truffle.api.dsl.GenerateCached;
169+
import com.oracle.truffle.api.dsl.GenerateInline;
165170
import com.oracle.truffle.api.dsl.Specialization;
166171
import com.oracle.truffle.api.library.CachedLibrary;
167172
import com.oracle.truffle.api.nodes.Node;
168173
import com.oracle.truffle.api.object.DynamicObjectLibrary;
174+
import com.oracle.truffle.api.object.HiddenKey;
169175
import com.oracle.truffle.api.profiles.InlinedConditionProfile;
170176
import com.oracle.truffle.api.strings.TruffleString;
171177

@@ -271,7 +277,7 @@ static Object get(Object object,
271277
}
272278
}
273279

274-
@CApiBuiltin(ret = PyTypeObject, args = {PyCMethodObject}, call = Ignored)
280+
@CApiBuiltin(ret = PyTypeObjectBorrowed, args = {PyCMethodObject}, call = Ignored)
275281
abstract static class Py_get_PyCMethodObject_mm_class extends CApiUnaryBuiltinNode {
276282
@Specialization
277283
static Object get(PBuiltinMethod object) {
@@ -388,7 +394,7 @@ static Object get(GetSetDescriptor object) {
388394
}
389395
}
390396

391-
@CApiBuiltin(ret = PyTypeObject, args = {PyDescrObject}, call = Ignored)
397+
@CApiBuiltin(ret = PyTypeObjectBorrowed, args = {PyDescrObject}, call = Ignored)
392398
abstract static class Py_get_PyDescrObject_d_type extends CApiUnaryBuiltinNode {
393399

394400
@Specialization
@@ -682,7 +688,7 @@ static Object get(PythonAbstractObjectNativeWrapper wrapper) {
682688
}
683689
}
684690

685-
@CApiBuiltin(ret = PyTypeObject, args = {PyObject}, call = Ignored)
691+
@CApiBuiltin(ret = PyTypeObjectBorrowed, args = {PyObject}, call = Ignored)
686692
abstract static class Py_get_PyObject_ob_type extends CApiUnaryBuiltinNode {
687693

688694
@Specialization
@@ -711,27 +717,61 @@ static long get(PBaseSet object,
711717
}
712718
}
713719

714-
@CApiBuiltin(ret = PyObject, args = {PySliceObject}, call = Ignored)
720+
@GenerateInline
721+
@GenerateCached(false)
722+
abstract static class GetSliceField extends Node {
723+
abstract Object execute(Node inliningTarget, PSlice object, HiddenKey key, Object value);
724+
725+
@Specialization
726+
static Object get(Node inliningTarget, PSlice object, HiddenKey key, Object value,
727+
@Cached(inline = false) ReadAttributeFromDynamicObjectNode read,
728+
@Cached(inline = false) WriteAttributeToDynamicObjectNode write,
729+
@Cached PythonCextBuiltins.PromoteBorrowedValue promote) {
730+
Object promotedValue = read.execute(object, key);
731+
if (promotedValue == PNone.NO_VALUE) {
732+
promotedValue = promote.execute(inliningTarget, value);
733+
if (promotedValue == null) {
734+
return value;
735+
}
736+
write.execute(object, key, promotedValue);
737+
}
738+
return promotedValue;
739+
}
740+
}
741+
742+
@CApiBuiltin(ret = PyObjectBorrowed, args = {PySliceObject}, call = Ignored)
715743
abstract static class Py_get_PySliceObject_start extends CApiUnaryBuiltinNode {
744+
private static final HiddenKey START_KEY = new HiddenKey("promoted_start");
745+
716746
@Specialization
717-
static Object doStart(PSlice object) {
718-
return object.getStart();
747+
static Object doStart(PSlice object,
748+
@Bind("this") Node inliningTarget,
749+
@Cached GetSliceField getSliceField) {
750+
return getSliceField.execute(inliningTarget, object, START_KEY, object.getStart());
719751
}
720752
}
721753

722-
@CApiBuiltin(ret = PyObject, args = {PySliceObject}, call = Ignored)
754+
@CApiBuiltin(ret = PyObjectBorrowed, args = {PySliceObject}, call = Ignored)
723755
abstract static class Py_get_PySliceObject_step extends CApiUnaryBuiltinNode {
756+
private static final HiddenKey STEP_KEY = new HiddenKey("promoted_step");
757+
724758
@Specialization
725-
static Object doStep(PSlice object) {
726-
return object.getStep();
759+
static Object doStep(PSlice object,
760+
@Bind("this") Node inliningTarget,
761+
@Cached GetSliceField getSliceField) {
762+
return getSliceField.execute(inliningTarget, object, STEP_KEY, object.getStep());
727763
}
728764
}
729765

730-
@CApiBuiltin(ret = PyObject, args = {PySliceObject}, call = Ignored)
766+
@CApiBuiltin(ret = PyObjectBorrowed, args = {PySliceObject}, call = Ignored)
731767
abstract static class Py_get_PySliceObject_stop extends CApiUnaryBuiltinNode {
768+
private static final HiddenKey STOP_KEY = new HiddenKey("promoted_stop");
769+
732770
@Specialization
733-
static Object doStop(PSlice object) {
734-
return object.getStop();
771+
static Object doStop(PSlice object,
772+
@Bind("this") Node inliningTarget,
773+
@Cached GetSliceField getSliceField) {
774+
return getSliceField.execute(inliningTarget, object, STOP_KEY, object.getStop());
735775
}
736776
}
737777

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/CApiFunction.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1472,6 +1472,10 @@ private static void addCApiBuiltins(List<CApiBuiltinDesc> result, Class<?> conta
14721472
if (!annotation.name().isEmpty()) {
14731473
name = annotation.name();
14741474
}
1475+
if (!annotation.ret().isValidReturnType()) {
1476+
throw new IllegalArgumentException(
1477+
String.format("Invalid @CApiBuiltin %s: %s is not an allowed return type, use PyObjectTransfer or PyObjectBorrow variants", name, annotation.ret()));
1478+
}
14751479
Class<?> gen;
14761480
try {
14771481
gen = Class.forName(container.getName() + "Factory$" + clazz.getSimpleName() + "NodeGen");

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/cext/capi/transitions/ArgDescriptor.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ public enum ArgDescriptor {
120120
PyObjectWrapper(ArgBehavior.PyObjectWrapper, "PyObject*"),
121121
PyObjectAsTruffleString(ArgBehavior.PyObjectAsTruffleString, "PyObject*"),
122122
PyTypeObject(ArgBehavior.PyObject, "PyTypeObject*"),
123+
PyTypeObjectBorrowed(ArgBehavior.PyObjectBorrowed, "PyTypeObject*"),
123124
PyTypeObjectTransfer(ArgBehavior.PyObject, "PyTypeObject*", true),
124125
PyListObject(ArgBehavior.PyObject, "PyListObject*"),
125126
PyTupleObject(ArgBehavior.PyObject, "PyTupleObject*"),
@@ -430,6 +431,14 @@ public boolean isPyObject() {
430431
return behavior == ArgBehavior.PyObject || behavior == ArgBehavior.PyObjectBorrowed;
431432
}
432433

434+
public boolean isValidReturnType() {
435+
/*
436+
* We don't want to allow "bare" PyObject and force ourselves to decide between
437+
* PyObjectTransfer and PyObjectBorrow
438+
*/
439+
return behavior != ArgBehavior.PyObject || transfer;
440+
}
441+
433442
public boolean isCharPtr() {
434443
return this == CharPtrAsTruffleString || this == CHAR_PTR || this == ConstCharPtr || this == ConstCharPtrAsTruffleString;
435444
}

0 commit comments

Comments
 (0)