Skip to content

Commit e1cb906

Browse files
jbower-fbfacebook-github-bot
jbower-fb
authored and
facebook-github-bot
committed
Support 3.14+ revised protocol for stack args when calling functions
Summary: This reflects the changes in python/cpython#107788. The changes to the relevant `LOAD_` instructions are mostly covered by updating `LoadMethodResult` to have a non-default constructor in 3.14+ which normalizes the content. Reviewed By: alexmalyshev Differential Revision: D80667929 fbshipit-source-id: 2d62f1850056792695faf8d9ceea61a0b4b31253
1 parent dab3381 commit e1cb906

6 files changed

Lines changed: 202 additions & 38 deletions

File tree

‎cinderx/Common/util.h‎

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,39 @@ using auto_jit_string_t = std::unique_ptr<jit_string_t, jit_string_deleter>;
7474

7575
const char* ss_get_string(const auto_jit_string_t& ss);
7676

77-
// Loading a method means getting back a callable and possibly the object
78-
// instance to use as the first argument.
77+
// Loading a method returns up to 2 items, for one of three possible outcomes:
78+
// * A callable plus an object instance (self).
79+
// * A bound method.
80+
// * Error.
81+
//
82+
// Prior to 3.14 in CPython, the first element returned indicated if we had a
83+
// bound method through being nullptr. However, we wanted to use nullptr to
84+
// trigger a deopt for the error case so instead the JIT used Py_None and
85+
// handled this in the runtime.
86+
//
87+
// After 3.14 things are simpler and we always have a callable as the first
88+
// element, so free to use nullptr on error to trigger a deopt.
7989
struct LoadMethodResult {
80-
PyObject* func;
81-
PyObject* inst;
90+
LoadMethodResult(PyObject* none_or_callable, PyObject* inst_or_callable) {
91+
if constexpr (PY_VERSION_HEX >= 0x030E0000) {
92+
if (none_or_callable == nullptr) {
93+
JIT_CHECK(
94+
inst_or_callable == nullptr, "Error, both args should be nullptr");
95+
callable = self_or_null = nullptr;
96+
} else if (none_or_callable == Py_None) {
97+
callable = inst_or_callable;
98+
self_or_null = nullptr;
99+
} else {
100+
callable = none_or_callable;
101+
self_or_null = inst_or_callable;
102+
}
103+
} else {
104+
callable = none_or_callable;
105+
self_or_null = inst_or_callable;
106+
}
107+
}
108+
PyObject* callable;
109+
PyObject* self_or_null;
82110
};
83111

84112
// Per-function entry point function to resume a JIT generator. Arguments are:

‎cinderx/Jit/deopt.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ static void reifyLocalsplus(
134134
*localsplus = Ci_STACK_NULL;
135135
} else {
136136
PyObject* obj = mem.readOwned(*value).release();
137-
*localsplus = Ci_STACK_STEAL(obj);
137+
*localsplus = obj == nullptr ? Ci_STACK_NULL : Ci_STACK_STEAL(obj);
138138
}
139139
localsplus++;
140140
}

‎cinderx/Jit/hir/builder.cpp‎

Lines changed: 40 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2118,7 +2118,8 @@ void HIRBuilder::emitCallEx(
21182118
CallFlags flags) {
21192119
Register* dst = temps_.AllocateStack();
21202120
OperandStack& stack = tc.frame.stack;
2121-
bool has_kwargs = bc_instr.oparg() & 0x1;
2121+
// In 3.14+ we always have kwargs on the stack but it may be null.
2122+
bool has_kwargs = (PY_VERSION_HEX >= 0x030E0000) || bc_instr.oparg() & 0x1;
21222123
Register* kwargs = nullptr;
21232124
if (has_kwargs) {
21242125
kwargs = stack.pop();
@@ -2129,17 +2130,18 @@ void HIRBuilder::emitCallEx(
21292130
kwargs = nullp;
21302131
}
21312132
Register* pargs = stack.pop();
2132-
Register* func = stack.pop();
2133-
#if PY_VERSION_HEX >= 0x030D0000
2134-
JIT_ABORT("Unused stack slot in CALL_FUNCTION_EX needs reviewing");
2135-
#endif
2136-
#if PY_VERSION_HEX >= 0x030C0000
2137-
// This isn't obviously explained by code, docs, or comments but
2138-
// CALL_FUNCTION_EX in 3.12 has an "unused" value on the stack.
2139-
// I guess this had something to do with simplifying the bytecode
2140-
// compiler? This has already changed in upstream main.
2141-
stack.pop();
2142-
#endif
2133+
Register* func;
2134+
// CALL_FUNCTION_EX has an unused value on the stack, starting with 3.12.
2135+
// In 3.14 this swapped location.
2136+
if constexpr (PY_VERSION_HEX >= 0x030E0000) {
2137+
stack.pop();
2138+
func = stack.pop();
2139+
} else if constexpr (PY_VERSION_HEX >= 0x030C0000) {
2140+
func = stack.pop();
2141+
stack.pop();
2142+
} else {
2143+
func = stack.pop();
2144+
}
21432145
tc.emit<CallEx>(dst, func, pargs, kwargs, flags, tc.frame);
21442146
stack.push(dst);
21452147
}
@@ -3431,7 +3433,7 @@ void HIRBuilder::emitLoadGlobal(
34313433
int name_idx = loadGlobalIndex(bc_instr.oparg());
34323434
Register* result = temps_.AllocateStack();
34333435

3434-
if constexpr (PY_VERSION_HEX >= 0x030B0000) {
3436+
if constexpr (PY_VERSION_HEX >= 0x030B0000 && PY_VERSION_HEX < 0x030E0000) {
34353437
if (bc_instr.oparg() & 1) {
34363438
emitPushNull(tc);
34373439
}
@@ -3458,6 +3460,12 @@ void HIRBuilder::emitLoadGlobal(
34583460
}
34593461

34603462
tc.frame.stack.push(result);
3463+
3464+
if constexpr (PY_VERSION_HEX >= 0x030E0000) {
3465+
if (bc_instr.oparg() & 1) {
3466+
emitPushNull(tc);
3467+
}
3468+
}
34613469
}
34623470

34633471
void HIRBuilder::emitMakeFunction(
@@ -4623,8 +4631,25 @@ void HIRBuilder::emitDictMerge(
46234631
TranslationContext& tc,
46244632
const BytecodeInstruction& bc_instr) {
46254633
auto& stack = tc.frame.stack;
4626-
Register* dict = stack.top(bc_instr.oparg());
4627-
Register* func = stack.top(bc_instr.oparg() + 2);
4634+
Register *dict, *func;
4635+
if constexpr (PY_VERSION_HEX < 0x030E0000) {
4636+
dict = stack.top(bc_instr.oparg());
4637+
func = stack.top(bc_instr.oparg() + 2);
4638+
} else {
4639+
// According to bytecodes.c, at this point on the stack we have:
4640+
// update (top of the stack)
4641+
// [unused if oparg is 0]
4642+
// dict
4643+
// unused
4644+
// unused
4645+
// callable
4646+
// Looking at codegen.c for 3.14, oparg is only ever 1 so the optional
4647+
// "unused" slot is never present. So the 1 and 4 offsets skip to "dict" and
4648+
// "callable" respectively.
4649+
JIT_CHECK(bc_instr.oparg() == 1, "oparg must be 1");
4650+
dict = stack.top(1);
4651+
func = stack.top(4);
4652+
}
46284653
Register* update = stack.pop();
46294654
Register* out = temps_.AllocateStack();
46304655
tc.emit<DictMerge>(out, dict, update, func, tc.frame);

‎cinderx/Jit/jit_rt.cpp‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -967,16 +967,24 @@ PyObject* JITRT_Call(
967967
(nargsf & PY_VECTORCALL_ARGUMENTS_OFFSET),
968968
"JITRT_Call must always be called as a vectorcall");
969969

970-
// Trying to call a function rather than a method on an object. Shift the
971-
// arguments over by one.
972-
//
973-
// In theory this is supposed to expect nullptr on the stack, but our HIR
974-
// implementation of LOAD_ATTR/LOAD_METHOD uses Py_None. Check for nullptr
975-
// just in case.
976-
if (callable == nullptr || Py_IsNone(callable)) {
977-
callable = args[0];
978-
args += 1;
979-
nargsf -= 1;
970+
if constexpr (PY_VERSION_HEX >= 0x030E0000) {
971+
// Calling a bound method leaves us with an unused first arg.
972+
if (args[0] == nullptr) {
973+
args += 1;
974+
nargsf -= 1;
975+
}
976+
} else {
977+
// Trying to call a function rather than a method on an object. Shift the
978+
// arguments over by one.
979+
//
980+
// In theory this is supposed to expect nullptr on the stack, but our HIR
981+
// implementation of LOAD_ATTR/LOAD_METHOD uses Py_None. Check for nullptr
982+
// just in case.
983+
if (callable == nullptr || Py_IsNone(callable)) {
984+
callable = args[0];
985+
args += 1;
986+
nargsf -= 1;
987+
}
980988
}
981989

982990
PyThreadState* tstate = _PyThreadState_GET();

‎cinderx/PythonLib/test_cinderx/test_python314_bytecodes.py‎

Lines changed: 103 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,10 @@
77
import cinderx.test_support as cinder_support
88

99

10+
def one():
11+
return 1
12+
13+
1014
@unittest.skipUnless(sys.version_info >= (3, 14), "Python 3.14+ only")
1115
class Python314Bytecodes(unittest.TestCase):
1216
def _assertBytecodeContains(self, func, expected_opcode):
@@ -43,6 +47,105 @@ def x():
4347
self.assertEqual(x(), 1)
4448
self._assertBytecodeContains(x, "BINARY_OP")
4549

50+
def test_LOAD_ATTR_CALL_unbound(self):
51+
class C:
52+
def m(self, v):
53+
return 1 + v
54+
55+
@cinder_support.fail_if_deopt
56+
@cinder_support.failUnlessJITCompiled
57+
def x(o):
58+
return o.m(2)
59+
60+
self.assertEqual(x(C()), 3)
61+
self._assertBytecodeContains(x, "LOAD_ATTR")
62+
self._assertBytecodeContains(x, "CALL")
63+
64+
def test_LOAD_ATTR_CALL_bound(self):
65+
class C:
66+
def m(self, v):
67+
return 1 + v
68+
69+
@cinder_support.fail_if_deopt
70+
@cinder_support.failUnlessJITCompiled
71+
def x(o):
72+
return o.m(2)
73+
74+
self.assertEqual(x(C()), 3)
75+
self._assertBytecodeContains(x, "LOAD_ATTR")
76+
self._assertBytecodeContains(x, "CALL")
77+
78+
def test_LOAD_ATTR_CALL_bound_via_attr(self):
79+
class C:
80+
def m(self, v):
81+
return 1 + v
82+
83+
def __getattr__(self, name):
84+
return self.m # Returns bound method
85+
86+
@cinder_support.fail_if_deopt
87+
@cinder_support.failUnlessJITCompiled
88+
def x(o):
89+
return o.m_attr(2)
90+
91+
self.assertEqual(x(C()), 3)
92+
self._assertBytecodeContains(x, "LOAD_ATTR")
93+
self._assertBytecodeContains(x, "CALL")
94+
95+
def test_LOAD_ATTR_CALL_err(self):
96+
class C:
97+
def m(self, v):
98+
return 1 + v
99+
100+
@cinder_support.fail_if_deopt
101+
@cinder_support.failUnlessJITCompiled
102+
def x(o):
103+
return o.err(2)
104+
105+
with self.assertRaises(AttributeError):
106+
x(C())
107+
108+
self._assertBytecodeContains(x, "LOAD_ATTR")
109+
self._assertBytecodeContains(x, "CALL")
110+
111+
def test_DICT_MERGE(self):
112+
def y(i=1, j=2):
113+
return i * j
114+
115+
@cinder_support.fail_if_deopt
116+
@cinder_support.failUnlessJITCompiled
117+
def x(k):
118+
return y(i=2, **k)
119+
120+
self.assertEqual(x({"j": 3}), 6)
121+
self._assertBytecodeContains(x, "DICT_MERGE")
122+
123+
def test_LOAD_SUPER_ATTR_CALL_bound(self):
124+
class A:
125+
attr = lambda _: 1 # noqa: E731
126+
127+
class B(A):
128+
pass
129+
130+
@cinder_support.fail_if_deopt
131+
@cinder_support.failUnlessJITCompiled
132+
def m(self):
133+
return super().attr()
134+
135+
self.assertEqual(B().m(), 1)
136+
self._assertBytecodeContains(B.m, "LOAD_SUPER_ATTR")
137+
self._assertBytecodeContains(B.m, "CALL")
138+
139+
def test_LOAD_GLOBAL_CALL(self):
140+
@cinder_support.fail_if_deopt
141+
@cinder_support.failUnlessJITCompiled
142+
def f():
143+
return one()
144+
145+
self.assertEqual(f(), 1)
146+
self._assertBytecodeContains(f, "LOAD_GLOBAL")
147+
self._assertBytecodeContains(f, "CALL")
148+
46149

47150
if __name__ == "__main__":
48151
unittest.main()

‎cinderx/RuntimeTests/inline_cache_test.cpp‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -47,24 +47,24 @@ regular_meth = RequestContext.regular_meth
4747
auto py_class_meth = Ref<>::steal(PyUnicode_FromString("class_meth"));
4848
jit::LoadTypeMethodCache cache;
4949
auto res = cache.lookup(klass, py_class_meth);
50-
ASSERT_EQ(res.inst, klass)
50+
ASSERT_EQ(res.self_or_null, klass)
5151
<< "Expected instance to be equal to class from cache look up";
5252
PyObject* class_meth = PyDict_GetItemString(locals, "class_meth");
53-
ASSERT_EQ(PyObject_RichCompareBool(res.func, class_meth, Py_EQ), 1)
53+
ASSERT_EQ(PyObject_RichCompareBool(res.callable, class_meth, Py_EQ), 1)
5454
<< "Expected method " << class_meth << " to be equal from cache lookup";
55-
ASSERT_EQ(cache.value(), res.func)
55+
ASSERT_EQ(cache.value(), res.callable)
5656
<< "Expected method " << py_class_meth << " to be cached";
5757

5858
for (auto& meth : {"static_meth", "regular_meth"}) {
5959
auto name = Ref<>::steal(PyUnicode_FromString(meth));
6060
jit::LoadTypeMethodCache cache;
6161
auto res = cache.lookup(klass, name);
62-
ASSERT_EQ(res.func, Py_None)
62+
ASSERT_EQ(res.callable, Py_None)
6363
<< "Expected first part of cache result to be Py_None";
6464
PyObject* py_meth = PyDict_GetItemString(locals, meth);
65-
ASSERT_EQ(PyObject_RichCompareBool(res.inst, py_meth, Py_EQ), 1)
65+
ASSERT_EQ(PyObject_RichCompareBool(res.self_or_null, py_meth, Py_EQ), 1)
6666
<< "Expected method " << meth << " to be equal from cache lookup";
67-
ASSERT_EQ(cache.value(), res.inst)
67+
ASSERT_EQ(cache.value(), res.self_or_null)
6868
<< "Expected method " << meth << " to be cached";
6969
}
7070
}
@@ -93,9 +93,9 @@ module_meth = functools._unwrap_partial
9393

9494
jit::LoadModuleMethodCache cache;
9595
auto res = cache.lookup(functools_mod, name);
96-
ASSERT_EQ(PyObject_RichCompareBool(res.inst, module_meth, Py_EQ), 1)
96+
ASSERT_EQ(PyObject_RichCompareBool(res.self_or_null, module_meth, Py_EQ), 1)
9797
<< "Expected method " << name << " to be cached";
98-
ASSERT_EQ(Py_None, res.func)
98+
ASSERT_EQ(Py_None, res.callable)
9999
<< "Expected Py_None to be returned from cache lookup";
100100

101101
ASSERT_EQ(PyObject_RichCompareBool(cache.value(), module_meth, Py_EQ), 1)

0 commit comments

Comments
 (0)