GH-115709: Invalidate executors when a local variable is changed via frame.f_locals #118639
Conversation
markshannon
changed the title
markshannon
changed the title
| if (_PyOpcode_Caches[_PyOpcode_Deopt[opcode]]) { | ||
| PAUSE_ADAPTIVE_COUNTER(this_instr[1].counter); | ||
| } |
There was a problem hiding this comment.
How does it connect to the purpose of the PR? Or is it just a drive-by bugfix?
There was a problem hiding this comment.
It's unconnected.
This PR uncovered the bug, so I'll need to fix it before merging this. Might as well do it here.
| @@ -148,8 +148,9 @@ framelocalsproxy_setitem(PyObject *self, PyObject *key, PyObject *value) | |||
| if (PyUnicode_CheckExact(key)) { | |||
| int i = framelocalsproxy_getkeyindex(frame, key, false); | |||
| if (i >= 0) { | |||
| _PyLocals_Kind kind = _PyLocals_GetKind(co->co_localspluskinds, i); | |||
| _Py_Executors_InvalidateDependency(PyInterpreterState_Get(), co, 1); | |||
There was a problem hiding this comment.
The way I solved the _Py_TIER2 dependency elsewhere is to put this call itself inside #ifdef _Py_TIER2. Then you don't need the dummy version you added to optimizer.c.
Python/optimizer.c
Outdated
| #else | ||
|
|
||
| void | ||
| _Py_Executors_InvalidateDependency(PyInterpreterState *interp, void *obj, int is_invalidation) |
There was a problem hiding this comment.
I'm not super keen on having a dummy version here -- and if we do this we should probably make a few more dummy versions and remove the occasional #if _Py_TIER2 sprinkled around elsewhere.
There was a problem hiding this comment.
Would you prefer this in optimizer.h
#else
#define _Py_Executors_InvalidateDependency(A, B, C) ((void)0)
#endif _Py_TIER2and leave optimizer.c alone?
There was a problem hiding this comment.
That's better. Though if this is what Brandt asks about maybe we should split it and put that bugfix in beta1 even if the f_locals bugfix goes to beta2.
|
Is this the same failure that we're seeing on GH-118661? |
…d via frame.f_locals (python#118639) Also fix unrelated assert in debug Tier2/JIT builds.
Skipping news as this is fixing a hypothetical bug, rather than a reported bug.
The new test causes a C assertion failure in a debug build of main.
frame.f_localsin a debugger or similar. #115709