Skip to content

gh-157468: Validate correct builtins used under JIT - #157766

Open
johng wants to merge 4 commits into
python:mainfrom
johng:gh-builtit-bug
Open

johng wants to merge 4 commits into
python:mainfrom
johng:gh-builtit-bug

Conversation

@johng

@johng johng commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

This adds the two checks at tracing and at JIT runtime that the builtin dict is the original interpreter's builtins

- Test 1: verifying the traceguard is correctly activating

- Test 2: verifying the runtime guard is activating

@cocolato cocolato left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't force push PRs. We‘ll squash merge commits at the end. And some comments:

Comment thread Python/optimizer_analysis.c Outdated
Comment on lines +181 to +213
@@ -199,6 +200,17 @@ convert_global_to_const(_PyUOpInstruction *inst, PyObject *obj)
if (res == NULL) {
return NULL;
}
return res;
}

/* Rewrite INST in place into a load of the constant it fetches from OBJ. */
static PyObject *
convert_global_to_const(_PyUOpInstruction *inst, PyObject *obj)
{
PyObject *res = lookup_global_const(inst, obj);
if (res == NULL) {
return NULL;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a new helper here. We can keep the original convert_global_to_const(), check for two free slots before calling it, then emit the guard and converted instruction with ADD_OP.

Comment thread Python/optimizer_bytecodes.c Outdated
Comment on lines 2530 to 2544
if (ctx->frame->globals_checked_version != 0 && ctx->frame->globals_watched) {
cnst = convert_global_to_const(this_instr, builtins);
cnst = lookup_global_const(this_instr, builtins);
if (cnst != NULL) {
/* Emitting two uops in place of one: make sure they fit. */
if (uop_buffer_remaining_space(&ctx->out_buffer) < 2) {
cnst = NULL;
}
else {
ADD_OP(_GUARD_BUILTINS_IS_CANONICAL, 0, 0);
ADD_OP(_Py_IsImmortal(cnst) ? _LOAD_CONST_INLINE_BORROW
: _LOAD_CONST_INLINE,
0, (uintptr_t)cnst);
}
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would remove the nested capacity branch and the assignment back to NULL:

Suggested change
if (ctx->frame->globals_checked_version != 0 && ctx->frame->globals_watched) {
cnst = convert_global_to_const(this_instr, builtins);
cnst = lookup_global_const(this_instr, builtins);
if (cnst != NULL) {
/* Emitting two uops in place of one: make sure they fit. */
if (uop_buffer_remaining_space(&ctx->out_buffer) < 2) {
cnst = NULL;
}
else {
ADD_OP(_GUARD_BUILTINS_IS_CANONICAL, 0, 0);
ADD_OP(_Py_IsImmortal(cnst) ? _LOAD_CONST_INLINE_BORROW
: _LOAD_CONST_INLINE,
0, (uintptr_t)cnst);
}
}
}
if (ctx->frame->globals_checked_version != 0 &&
ctx->frame->globals_watched &&
uop_buffer_remaining_space(&ctx->out_buffer) >= 2)
{
cnst = convert_global_to_const(this_instr, builtins);
if (cnst != NULL) {
ADD_OP(_GUARD_BUILTINS_IS_CANONICAL, 0, 0);
ADD_OP(this_instr->opcode, 0, this_instr->operand0);
}
}

@johng
johng requested a review from cocolato September 20, 2026 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants