Skip to content

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

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

johng wants to merge 10 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 thread Python/optimizer_bytecodes.c Outdated
@johng
johng requested a review from cocolato September 20, 2026 12:20

@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.

Thanks for doing this, there are also a few comments

Comment thread Lib/test/test_capi/test_opt.py Outdated
Comment thread Lib/test/test_capi/test_opt.py Outdated
Co-authored-by: Hai Zhu <haiizhu@outlook.com>
@johng
johng requested a review from cocolato September 21, 2026 07:13

@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.

LGTM, and let's wait a core dev to review this

@markshannon markshannon 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.

We should be able to remove repeated checks in much the same way as do with ctx->frame->globals_checked_version by adding a ctx->frame->builtins_checked flag.

Comment thread Python/optimizer_bytecodes.c Outdated
if (ctx->frame->globals_checked_version != 0 && ctx->frame->globals_watched) {
if (ctx->frame->globals_checked_version != 0 &&
ctx->frame->globals_watched &&
uop_buffer_remaining_space(&ctx->out_buffer) >= 2)

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.

We always allow enough headroom for small changes like this. No need to check here.

/* convert_global_to_const already chose the right
* _LOAD_CONST_INLINE[_BORROW] opcode; reuse it. */
ADD_OP(_GUARD_BUILTINS_IS_CANONICAL, 0, 0);
ADD_OP(this_instr->opcode, 0, (uintptr_t)cnst);

@markshannon markshannon Sep 21, 2026

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.

Why are you adding the same instruction twice?
My mistake. convert_global_to_const converts the input inplace and relies on its being copied. We should probably update its interface, but that's for a different PR.

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.

The two ADD_OP calls emit different instructions:

// Rewrite this_instr as a constant load; nothing is emitted yet.
cnst = convert_global_to_const(this_instr, builtins);

if (cnst != NULL) {
    // Emit the builtins identity guard.
    ADD_OP(_GUARD_BUILTINS_IS_CANONICAL, 0, 0);

    // Emit the constant load prepared above.
    ADD_OP(this_instr->opcode, 0, this_instr->operand0);
}

@johng

johng commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

I reverted the last commit against my fork and the same test failed so don't think it's related (and didn't seem to be anyway)

1 test failed:
test.test_gdb.test_jit

Edit: Saying that main appears to be in a good state although confused about the failure here

@johng

johng commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

The failing test is now passing after latest merge to main @cocolato @markshannon I did some analysis and looks to be related to the code layout generated and whether a veneer is generated. I raised a separate issue above tracking it.

Comment thread Python/optimizer_bytecodes.c Outdated
Comment thread Python/optimizer_bytecodes.c Outdated
@methane

methane commented Sep 23, 2026

Copy link
Copy Markdown
Member

Isn't keys->dk_version = 0; needed in clone_combined_dict_keys()? @markshannon

johng and others added 2 commits September 23, 2026 07:18
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.

4 participants