Skip to content

Conversation

@vstinner
Copy link
Member

@vstinner vstinner commented Oct 6, 2025

Add PyUnstable_ThreadState_SetStack() and
PyUnstable_ThreadState_ResetStack() functions to set the stack base address and stack size of a Python thread state.


📚 Documentation preview 📚: https://cpython-previews--139668.org.readthedocs.build/

@encukou
Copy link
Member

encukou commented Oct 6, 2025

This generally matches what I came up with :)

The start argument might be confusing, as the place usage grows towards.
Why not use bottom & top as in the internals?

@vstinner
Copy link
Member Author

vstinner commented Oct 6, 2025

This generally matches what I came up with :)

Good!

The start argument might be confusing, as the place usage grows towards. Why not use bottom & top as in the internals?

APIs like make_fcontext() or sigaltstack() use void *stack_start_address and size_t stack_size.

pthread_attr_setstack() and pthread_attr_getstack() also use void *stack_start_address and size_t stack_size.

@vstinner
Copy link
Member Author

vstinner commented Oct 6, 2025

cc @markshannon @zooba

@encukou
Copy link
Member

encukou commented Oct 7, 2025

On systems like musl that just set top to the current stack pointer, PyUnstable_ThreadState_ResetStack has two issues:

  • calling it “often” will essentially prevent stack protection
  • returning from a function that called it will get you past the top of the stack, which should be fine but isn't really tested.

Would it be better to save the results of the initial _Py_InitializeRecursionLimits on the thread state, and have PyUnstable_ThreadState_ResetStack return to that?

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

The API looks good, but it can fail in a couple of ways.

@bedevere-app
Copy link

bedevere-app bot commented Oct 7, 2025

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@vstinner
Copy link
Member Author

vstinner commented Oct 7, 2025

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Oct 7, 2025

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from markshannon October 7, 2025 10:27
@markshannon
Copy link
Member

markshannon commented Oct 7, 2025

Can we please merge #139294 first before we add stuff on top of it?

@vstinner
Copy link
Member Author

vstinner commented Oct 7, 2025

Can we please merge #139294 first before we add stuff on top of it?

Yes. I approved your PR.

Add PyUnstable_ThreadState_SetStack() and
PyUnstable_ThreadState_ResetStack() functions to set the stack base
address and stack size of a Python thread state.
@vstinner
Copy link
Member Author

vstinner commented Oct 7, 2025

Can we please merge #139294 first before we add stuff on top of it?

PR gh-139294 has been merged. I rebased my PR on top of it.

I also modified my PR to use (base, top) internally instead of (start_address, size).

@vstinner
Copy link
Member Author

vstinner commented Oct 7, 2025

Oh, TSan CI now fails with:

0:03:59 load avg: 5.34 [43/76/1] test_capi worker non-zero exit code (Exit code -6 (SIGABRT)) -- running (2): ...

python: Python/ceval.c:506: void tstate_set_stack(PyThreadState *, uintptr_t, uintptr_t):
Assertion `ts->c_stack_soft_limit < ts->c_stack_top' failed.

Fatal Python error: Aborted

@vstinner
Copy link
Member Author

vstinner commented Oct 7, 2025

Oh, TSan CI now fails

Fixed.

@vstinner
Copy link
Member Author

@markshannon: Please review the updated PR.

#ifndef NDEBUG
// Sanity checks
_PyThreadStateImpl *ts = (_PyThreadStateImpl *)tstate;
assert(ts->c_stack_hard_limit <= ts->c_stack_soft_limit);
Copy link
Member

Choose a reason for hiding this comment

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

Be aware that this is going to conflict with #140028

@markshannon
Copy link
Member

Looks good, but should we wait for #140028?

@vstinner
Copy link
Member Author

I created capi-workgroup/decisions#81 for the C API Working Group.

@vstinner
Copy link
Member Author

Looks good, but should we wait for #140028?

I can wait until #140028 is merged. I will wait for C API Working Group anyway.

@encukou
Copy link
Member

encukou commented Oct 15, 2025

It might be better to merge this first, then update #140028 and ask @stefanor to test on HPPA.

Co-authored-by: Petr Viktorin <[email protected]>
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.

3 participants