Skip to content

bpo-33955: Support USE_STACKCHECK on macOS #8046

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

bpo-33955: Support USE_STACKCHECK on macOS #8046

wants to merge 7 commits into from

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Jul 2, 2018 •
edited by bedevere-bot
Loading

@corona10 corona10 changed the title bpo-33955: Support USE_STACKCHECK on Mac OS bpo-33955: [WIP] Support USE_STACKCHECK on Mac OS Jul 2, 2018
ronaldoussoren
ronaldoussoren requested changes Jul 2, 2018
View reviewed changes
Copy link
Contributor

@ronaldoussoren ronaldoussoren left a comment

Choose a reason for hiding this comment

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

The basic patch looks ok. Please add a test that demonstrates that the new code works. For example by setting the recursion limit to a too high value and check that this doesn't crash the interpreter (there may be a test for this that's only enabled for windows at the moment).

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@corona10
Copy link
Member Author

corona10 commented Jul 2, 2018

@ronaldoussoren

Thank you for a quick review.
I will add a new test soon!
Thanks!

@corona10 corona10 changed the title bpo-33955: [WIP] Support USE_STACKCHECK on Mac OS bpo-33955: Support USE_STACKCHECK on Mac OS Jul 2, 2018
@corona10 corona10 changed the title bpo-33955: Support USE_STACKCHECK on Mac OS bpo-33955: Support USE_STACKCHECK on macOS Jul 2, 2018
@corona10 corona10 changed the title bpo-33955: Support USE_STACKCHECK on macOS bpo-33955: [WIP] Support USE_STACKCHECK on macOS Jul 2, 2018
@corona10 corona10 force-pushed the bpo-33955 branch 3 times, most recently from 148001b to 4be8d33 Compare July 2, 2018 18:54
@corona10
Copy link
Member Author

corona10 commented Jul 2, 2018

@ronaldoussoren

https://github.com/python/cpython/blob/master/Lib/test/crashers/recursive_call.py

As far as I know, a unit test for USE_STACKCHECK is only /Lib/test/crashers/recursive_call.py.
Although I added this test on test_sys.py both Windows and macOS is failed on the CI except my local.
Is there any reference that I didn't catch?

@corona10 corona10 changed the title bpo-33955: [WIP] Support USE_STACKCHECK on macOS bpo-33955: Support USE_STACKCHECK on macOS Jul 2, 2018
@corona10 corona10 changed the title bpo-33955: Support USE_STACKCHECK on macOS bpo-33955: [WIP] Support USE_STACKCHECK on macOS Jul 2, 2018
@corona10
Copy link
Member Author

corona10 commented Jul 2, 2018

@ronaldoussoren
Hmm maybe, I can add a new test on test_sys.py.
I will try it soon.

I added a logic for estimating a new stack space by using the stack space that was just needed.

@corona10 corona10 force-pushed the bpo-33955 branch 2 times, most recently from b5e933f to 138eba1 Compare July 5, 2018 03:50
@corona10 corona10 changed the title bpo-33955: [WIP] Support USE_STACKCHECK on macOS bpo-33955: Support USE_STACKCHECK on macOS Jul 5, 2018
@corona10 corona10 force-pushed the bpo-33955 branch 5 times, most recently from ada28bf to 181fa6e Compare July 5, 2018 05:05
vstinner
vstinner reviewed Dec 17, 2019
View reviewed changes
@corona10 corona10 changed the title bpo-33955: Support USE_STACKCHECK on macOS [WIP] bpo-33955: Support USE_STACKCHECK on macOS Dec 17, 2019
@corona10 corona10 changed the title [WIP] bpo-33955: Support USE_STACKCHECK on macOS bpo-33955: Support USE_STACKCHECK on macOS Dec 17, 2019
@corona10 corona10 force-pushed the bpo-33955 branch 2 times, most recently from d1919f5 to 131dfee Compare December 17, 2019 15:52
@corona10
Copy link
Member Author

corona10 commented Dec 17, 2019 •
edited
Loading

@vstinner cc @ronaldoussoren

Thank you for the kind review and thank you for the guide about how to use ThreadState.

  • Change the store area about stack size tracing into ThreadState for TLS purpose, not ThreadState->interp.
  • There is an API that can change the stack size, but it will apply to the newly created thread. Should we care about it? threading.stack_size
  • So I locally run the script with threading and it works well.

@corona10 corona10 requested a review from vstinner December 17, 2019 16:23
@corona10
Copy link
Member Author

corona10 commented Dec 17, 2019 •
edited
Loading

Sorry, I found an issue with this PR.
I will ping you after this issue is resolved.

@corona10
Copy link
Member Author

@vstinner
Sorry for the verbose message.
The issue was not related to this PR.
After make clean it works well on my local machine.
Please take a look when you have time.

Thanks for understanding.

vstinner
vstinner reviewed Jan 7, 2020
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Honestly, I'm not super excited by the implementation. I'm not sure that it's correct. But I added a few comments.

Python/pythonrun.c
{
PyThreadState *tstate = _PyThreadState_GET();
Init_Stack_Status(tstate);
const uintptr_t end = (uintptr_t)pthread_get_stackaddr_np(pthread_self());
Copy link
Member

Choose a reason for hiding this comment

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

Do you know if it's implemented in pure userspace, or if it's a system call?

@corona10
Copy link
Member Author

corona10 commented Jan 13, 2020 •
edited
Loading

@vstinner I applied your review and some of the issues are under investigation.

List

  • Do you know if it's implemented in pure userspace, or if it's a system call?
  • Is this function available on GCC and Clang? -> clang okay, GCC is needed check
  • Honestly, I'm not super excited by the implementation. I'm not sure that it's correct. ->
    I wonder to know there is a good way to running some of the scripts that can be run by this patch.

@csabella
Copy link
Contributor

@corona10, is this still waiting changes or should it be moved back to code review? Thanks!

@corona10
Copy link
Member Author

@vstinner @ronaldoussoren Let's reopen this PR when we need this feature ;)

@corona10 corona10 closed this Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner requested changes

@ronaldoussoren ronaldoussoren Awaiting requested review from ronaldoussoren

Assignees
No one assigned
Labels
awaiting change review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@corona10 @bedevere-bot @csabella @vstinner @ronaldoussoren @the-knights-who-say-ni