Skip to content

GH-137573: Check C stack depth before stack allocating JIT optimizer struct - #137676

Closed
markshannon wants to merge 4 commits into
python:mainfrom
faster-cpython:check-stack-in-jit
Closed

GH-137573: Check C stack depth before stack allocating JIT optimizer struct#137676
markshannon wants to merge 4 commits into
python:mainfrom
faster-cpython:check-stack-in-jit

Conversation

@markshannon

@markshannonmarkshannon commented Aug 12, 2025•
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

@bedevere-appbedevere-appBot mentioned this pull request Aug 12, 2025
Closed
AA-Turner
AA-Turner reviewed Aug 12, 2025
View reviewed changes
Comment threadLib/test/test_call.py Outdated
@Fidget-Spinner

Fidget-Spinner commented Aug 12, 2025

Copy link
Copy Markdown
Member

Just so I'm clear: you plan to move this to the thread state later but only on main right? So that we don't change the struct layout in an rc for 3.14?

markshannonand others added 2 commits August 13, 2025 11:44
@markshannon@AA-Turner
Update Lib/test/test_call.py
0a889df
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
@markshannon
Account for trace buffer as well
be99bd4
@Fidget-Spinner

Fidget-Spinner commented Aug 13, 2025

Copy link
Copy Markdown
Member

Can you please add a Py_NO_INLINE to the _PyOptimizer_Optimize function too? I don't think this will fix it as if it's inlined, the alloca might be hoisted out to the main interpreter loop by the compiler.

@markshannon

markshannon commented Aug 13, 2025

Copy link
Copy Markdown
MemberAuthor

Can you please add a Py_NO_INLINE to the _PyOptimizer_Optimize function too? I don't think this will fix it as if it's inlined, the alloca might be hoisted out to the main interpreter loop by the compiler.

Yes, but not in this PR as it seems unrelated

@markshannon

markshannon commented Aug 13, 2025

Copy link
Copy Markdown
MemberAuthor

Just so I'm clear: you plan to move this to the thread state later but only on main right? So that we don't change the struct layout in an rc for 3.14?

Yes, that's the plan.

@markshannonmarkshannon added the needs backport to 3.14 bugs and security fixes label Aug 13, 2025
Fidget-Spinner
Fidget-Spinner reviewed Aug 14, 2025
View reviewed changes
Comment threadPython/optimizer_analysis.c
OPT_STAT_INC(optimizer_attempts);
/* Make sure we have enough C stack space for the optimizer */
int margin = 1 + sizeof(JitOptContext)/_PyOS_STACK_MARGIN_BYTES;
if (_Py_ReachedRecursionLimitWithMargin(_PyThreadState_GET(), margin)) {

@Fidget-SpinnerFidget-SpinnerAug 14, 2025

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.

To work properly, the optimize_uops function or this function must be marked Py_NO_INLINE as well.

Otherwise, the compiler can just inline the functions and hoist the alloca above the check, making the check useless.

@markshannon

markshannon commented Sep 18, 2025

Copy link
Copy Markdown
MemberAuthor

We've already started moving the buffers to the heap, so this won't be needed.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AA-TurnerAA-TurnerAA-Turner left review comments
@Fidget-SpinnerFidget-SpinnerFidget-Spinner left review comments
@tomasr8tomasr8Awaiting requested review from tomasr8tomasr8 is a code owner

Assignees

@Fidget-SpinnerFidget-Spinner

Labels

awaiting core reviewneeds backport to 3.14bugs and security fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@markshannon@Fidget-Spinner@AA-Turner