CALL_LIST_APPEND specialization can make no progress

[Fidget-Spinner]

Bug report

Bug description:

CALL_LIST_APPEND requires self to be a PyList_CheckExact in bytecodes.c. However, the specialize.c doesn't check that. This means there's a possible discrepancy between what we specialize for and what we deopt.

The fix is to check that self is a list. It should be as simple as

diff --git a/Python/specialize.c b/Python/specialize.c
index a1c5dedd615..5b44432b8b7 100644
--- a/Python/specialize.c
+++ b/Python/specialize.c
@@ -2041,8 +2041,12 @@ specialize_method_descriptor(PyMethodDescrObject *descr, _Py_CODEUNIT *instr,
 bool pop = (next.op.code == POP_TOP);
 int oparg = instr->op.arg;
 if ((PyObject *)descr == list_append && oparg == 1 && pop){
- specialize(instr, CALL_LIST_APPEND);
- return 0;
+ PyThreadState *tstate = PyThreadState_GET();
+ PyObject *self = PyStackRef_AsPyObjectBorrow(tstate->current_frame->stackpointer[-2]);
+ if (PyList_CheckExact(self)){
+ specialize(instr, CALL_LIST_APPEND);
+ return 0;
+ }
 }
 specialize(instr, CALL_METHOD_DESCRIPTOR_O);
 return 0;

The test case should be in test_opcache.py, and test something like

class MyList(list): pass
x = MyList()
for _ in range(spec_counter):
 x.append(1)

and ensure that the append is not specialized to CALL_LIST_APPEND.

@efimov-mikhail can I let you take up this issue please? I'm pretty sure 3.14 is affected too.

CPython versions tested on:

CPython main branch, 3.15, 3.14

Operating systems tested on:

No response

Linked PRs

• gh-141367: Use CALL_LIST_APPEND instruction only for lists, not for list subclasses #141398
• gh-141367: Use actual SPECIALIZATION_THRESHOLD value in specialization related test #141417

[efimov-mikhail]

@Fidget-Spinner , of course, I will take this, thanks.

[efimov-mikhail]

I'm pretty sure 3.14 is affected too.

Actually, it isn't affected.

You can see your own comment on PR with this change:
#134240 (comment)

So, basically, we should fix specialization on main branch only.
I'll create a PR soon.

[Fidget-Spinner]

Great observation, thanks!

[efimov-mikhail]

@Fidget-Spinner , I've created a PR with your proposed solution, CI is green.
I've used _PyThreadState_GET instead of PyThreadState_GET because it's sligtly faster, IIUC.
And make test for both list and MyList append specialization.

[markshannon]

Rather than probing the stack tstate->current_frame->stackpointer[-2], we should pass the arguments to _Py_Specialize_Call.
We also need to add an additional guard in _CALL_LIST_APPEND that callable == interp->callable_cache.list_append.

[efimov-mikhail]

Rather than probing the stack tstate->current_frame->stackpointer[-2], we should pass the arguments to _Py_Specialize_Call.

Ok, I'll investigate this.

We also need to add an additional guard in _CALL_LIST_APPEND that callable == interp->callable_cache.list_append.

Do we really need this since _GUARD_CALLABLE_LIST_APPEND is present?

Maybe we should remove checking DEOPT_IF(!PyList_CheckExact(self_o)) in _GUARD_CALLABLE_LIST_APPEND because _GUARD_NOS_LIST is already used in CALL_LIST_APPEND macro?

[markshannon]

Do we really need this since _GUARD_CALLABLE_LIST_APPEND is present?

No we don't. I was just looking at the uop, not the whole instruction.

Maybe we should remove checking DEOPT_IF(!PyList_CheckExact(self_o)) in _GUARD_CALLABLE_LIST_APPEND because _GUARD_NOS_LIST is already used in CALL_LIST_APPEND macro?

Yes, that makes sense.

[efimov-mikhail]

While working on this issue I've found this PR:
https://github.com/python/cpython/pull/132018/files#diff-af5c4ee62cca27529efe0febc3babfd5745cc1ae48a280a58605a8c85ee38942R335

Maybe we should change this test a little to use actual SPECIALIZATION_THRESHOLD instead of constant 3?

[Fidget-Spinner]

Maybe not for this PR, but if you open another PR to fix it I'd be open to merging.