Repository navigation
JIT: stale recorded_values leak when specialized op deopts #148571
Description
Activity
cpython/Include/internal/pycore_optimizer.h
Lines 95 to 103 in 52a7f1b
I think we can add arecorded_opcodehere to check the invariant.@NekoAsakura we actually check for deopt in the trace translator, and should already be protecting against this. You can add the clearing of recorded values to this branch https://github.com/python/cpython/blob/main/Python/optimizer.c#L665
The fact this is allowed through means the deopt detection is not working?
I've confirmed via instrumentation that the leak exists; let me look into why this deopt detection isn't being triggered.
re-specialization is fine (we need that to support polymorphism)
A possible fix is to ensure that all recording ops are same throughout a family (we can verify this in the cases generator)
Sorry I'm not quite following. Are you saying we'd have to migrate every
LOAD_ATTR_*to_RECORD_TOSand rework the downstream guards to derive the type themselves? Wouldn't that bump up the maintenance overhead a fair bit, and mean that every future specialisation wanting a different record shape triggers another family-wide migration?I think we can add a
recorded_opcodehere to check the invariant.My original idea is set it at record and check
if tracer->prev_state.recorded_opcode != opcodein translate.I'm concerned that adding
recorded_opcodewill only help us exits correctly and won't enable re-specialization.Reacted by Ken Jin and Neko AsakuraSorry I'm not quite following. Are you saying we'd have to migrate every
LOAD_ATTR_*to_RECORD_TOSand rework the downstream guards to derive the type themselves? Wouldn't that bump up the maintenance overhead a fair bit, and mean that every future specialisation wanting a different record shape triggers another family-wide migration?Yes but have a stronger invariant: We record the exact same way for every op in a specialization family. We need this to handle respecialization correctly.
Eg, just specify base LOAD_ATTR as RECORD_TOS, and then that automatically means everything in family(LOAD_ATTR) has that recording op. We then call recording_func[_PyOpcode_Deopt[opcode]].
This is required to safely handle the respecialization to another op, and also ensure we decref properly. If we ensure uniformity across the whole family and make it specializaion agnostic, it will handle specialization.
Reacted by Hai Zhu and Neko AsakuraI'm concerned that adding
recorded_opcodewill only help us exits correctly and won't enable re-specialization.I think
DISPATCH_SAME_OPARGis the only site that mutates the byte at the current pc while tracing, so we can slot a small function in there. Bail early if the new opcode matches what we already recorded, otherwise drop the stale snapshot and rebuild it for the new opcode via_PyOpcode_RecordFunctionsagainst the current stack. That keeps the trace going with re-specialisation.Lines 216 to 221 in c7e9919
We likely can't slot a function in DISPATCH_SAME_OPARG, as that will slow down the normal (non-tracing interpreter).
Oh I think
IS_JIT_TRACING()is cheap enough, and we'd only call that function when tracing.Hmm, when I wrote the initial tracing frontend, Mark was quite adamant about not having
IS_JIT_TRACING()checks over the normal interpreter code. At the time I didn't really appreciate it but I now think he's right. As far as possible, the normal interpreter's behaviour should not be modified for tracing. We want to check for less state and reason about things easier.Reacted by Neko AsakuraFair point. I'll wait until you've had enough time to talk it through.
As @Fidget-Spinner says, this is one of the cases (like stack effect) where we must ensure that all instructions in a family act the same.
All family members will need to pre-record (inTRACE_RECORD) the same set of values, and then process them differently when adding uops to the trace.We must not record values in the trace when we only need the type, so we would need to extract the actual value we need when storing into the trace.
In the example above, we would record the TOS value for all members of the
LOAD_ATTRfamily, and then process that differently for each uop.
The_RECORD_TOSinLOAD_ATTR_CLASSwe would simply copy the value into the uop.
The_RECORD_TOS_TYPEinLOAD_ATTR_SLOT, we would need to extract the class, incref the class and decref the value.This is going to be a bit slower (an extra incref and decref in many cases), but should be robust.
So I'm thinking: add
_RECORD_TOSto baseLOAD_ATTRand override allLOAD_ATTR_*entries ingenerate_recorder_tablesto use the base's recording. Then in translation, if a uop expects a type, convert it:PyObject *type = (PyObject *)Py_TYPE(recorded_value); Py_INCREF(type); Py_DECREF(recorded_value); recorded_value = type;Lines 949 to 950 in 0fcf2b7
Does that sound right to you?Affected:
TO_BOOL,BINARY_OP,SEND,STORE_ATTR,LOAD_SUPER_ATTR,LOAD_ATTR,FOR_ITER,CALL,CALL_KW,CALL_FUNCTION_EX.import re text = open("Python/bytecodes.c").read() families = {n: [x.strip() for x in body.split(',') if x.strip()] for n, body in re.findall(r'family\((\w+),[^)]+\)\s*=\s*\{([^}]+)\}', text)} macro_recs = {n: tuple(re.findall(r'_RECORD_\w+', body)) for n, body in re.findall(r'macro\((\w+)\)\s*=\s*([^;]+);', text)} fmt = lambda r: ", ".join(r) or "(none)" for fam, members in families.items(): shapes = {macro_recs.get(n, ()) for n in [fam] + members} if len(shapes) < 2: continue base = macro_recs.get(fam, ()) print(f"\n=== {fam} ({len(shapes)} shapes) ===\n base: {fmt(base)}") for mem in members: rec = macro_recs.get(mem, ()) print(f" {mem}: {fmt(rec)}{' <--' * (rec != base)}")So I'm thinking: add
_RECORD_TOSto baseLOAD_ATTRand override allLOAD_ATTR_*entries ingenerate_recorder_tablesto use the base's recording. Then in translation, if a uop expects a type, convert it:PyObject *type = (PyObject *)Py_TYPE(recorded_value); Py_INCREF(type); Py_DECREF(recorded_value); recorded_value = type;Lines 949 to 950 in 0fcf2b7
else if (_PyUop_Flags[uop] & HAS_RECORDS_VALUE_FLAG) {
PyObject *recorded_value = tracer->prev_state.recorded_values[record_idx];Does that sound right to you?
That makes sense to me. Would you want to work on this?
I think you need a dispatch table like this:
record_trace_transforms[MAX_UOP_ID] = { [_RECORD_TOS_TYPE] = record_trace_transform_to_type, [_RECORD_NOS_GEN_FUNC] = record_trace_transform_gen_func, };Because for the
FOR_ITERfamily we need_RECORD_NOS_GEN_FUNC,
and for theSENDfamily, we might need to add_RECORD_3OS.
Then you can convert them when storing them into the tracetransform = record_trace_transforms[uop]; if (transform != NULL && recorded_value != NULL) { recorded_value = transform(recorded_value); }- added a commit that references this issue
on Apr 28, 2026
Bug report
Bug description:
TRACE_RECORDfillsrecorded_valuesbefore dispatching the instruction bodycpython/Python/bytecodes.c
Lines 6366 to 6372 in 52a7f1b
At this point,
recorded_values[0..record_entry->count-1]hold the values recorded for the specialised opcode that is about to run. ForLOAD_ATTR_CLASS_WITH_METACLASS_CHECKin #148394, the record entry is_RECORD_TOS, sorecorded_values[0]holds the raw TOS (PyObject *itself). Unlike_RECORD_TOS_TYPEstoresPy_TYPE(tos), which is always aPyTypeObject *.EXIT_IFwhen its guard fails. ThenEXIT_IF/DEOPT_IFcompile down to a plaingotothroughJUMP_TO_PREDICTED, with no cleanup in the emitted code:cpython/Tools/cases_generator/generators_common.py
Line 190 in 52a7f1b
cpython/Python/ceval_macros.h
Line 136 in 52a7f1b
Nothing on this path touches
tracer->prev_state.recorded_values[]._SPECIALIZE_LOAD_ATTRcalls the adaptive specializer at the same pc.Call chain:
_SPECIALIZE_LOAD_ATTR→
_Py_Specialize_LoadAttr→
specialize(instr, LOAD_ATTR_SLOT)→
set_opcode(instr, LOAD_ATTR_SLOT)→
instr->op.code = opcode;Once
_Py_Specialize_LoadAttrreturns, the pc that previously heldLOAD_ATTR_CLASS_WITH_METACLASS_CHECK(181) now holds, for example,LOAD_ATTR_SLOT(191). Control then falls through toDISPATCH_SAME_OPARG(), which re-reads the opcode byte and dispatches the new specialisation at the same pc:cpython/Python/ceval_macros.h
Lines 216 to 221 in 52a7f1b
Because that dispatch uses
DISPATCH_GOTO_NON_TRACING, the new specialisation runs without re-enteringTRACE_RECORD.TRACE_RECORDtranslates against a stale picture.It reads
prev_state.instr->op.code.prev_state.instris still the pointer set in step 1; only the byte at that address has changed.Inside translate, each uop in the macro expansion of the current opcode byte (e.g. 191 =
LOAD_ATTR_SLOT) that carriesHAS_RECORDS_VALUE_FLAGis handedrecorded_values[record_idx]as itsoperand0. There is no check that the recording actually came from the opcode currently being translated:cpython/Python/optimizer.c
Lines 949 to 954 in 52a7f1b
LOAD_ATTR_SLOTbegins with_RECORD_TOS_TYPE, which expects aPyTypeObject:cpython/Python/bytecodes.c
Lines 2883 to 2890 in 52a7f1b
But the value still sitting in
recorded_values[0]is whatever_RECORD_TOSstored for the old 181 — Boom.The only existing cleanup in
_PyJit_FinalizeTracingruns when tracing ends — far too late for a deopt that occurs mid-trace:cpython/Python/optimizer.c
Lines 1126 to 1129 in 52a7f1b
CPython versions tested on:
CPython main branch
Operating systems tested on:
Linux
Linked PRs