GH-115709: Invalidate executors when a local variable is changed via frame.f_locals#118639
Uh oh!
There was an error while loading. Please reload this page.
Conversation
markshannon changed the title markshannon changed the title 
gvanrossum left a comment
gvanrossum left a comment There was a problem hiding this comment.
While this isn't incorrect I'm not super happy with the inconsistency of sometimes #ifdefing out the call and other times relying on the dummy version existing. But this works, so do what's expedient to make it into beta 1.
| 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.
| inti=framelocalsproxy_getkeyindex(frame, key, false); | ||
| if (i >= 0){ | ||
| _PyLocals_Kindkind=_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, intis_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.
brandtbucher commented May 6, 2024
Is this the same failure that we're seeing on GH-118661? |
gvanrossum left a comment
gvanrossum left a comment There was a problem hiding this comment.
LGTM. Will merge this ASAP since CI is currently broken and this fixes it.
…d via frame.f_locals (python#118639) Also fix unrelated assert in debug Tier2/JIT builds.
3 participants
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