Skip to content

bpo-29884: faulthandler: Restore the old sigaltstack during teardown #777

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
Merged
vstinner merged 1 commit into from
Mar 23, 2017
Merged

bpo-29884: faulthandler: Restore the old sigaltstack during teardown #777

vstinner merged 1 commit into from
Mar 23, 2017

Conversation

@tich
Copy link
Contributor

@tich tich commented Mar 23, 2017

No description provided.

@mention-bot
Copy link

mention-bot commented Mar 23, 2017

@tich, thanks for your PR! By analyzing the history of the files in this pull request, we identified @birkenfeld, @serhiy-storchaka and @benjaminp to be potential reviewers.

vstinner
vstinner requested changes Mar 23, 2017
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.

The fix itself LGTM, but please also:

  • Add yourself to Misc/ACKS
  • Mention the fix in Misc/NEWS, in the Library section

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Mar 23, 2017

And tests.

@vstinner
Copy link
Member

vstinner commented Mar 23, 2017

And tests.

IMHO it's overkill to test this bugfix. It's a corner case which requires to write a program which embeds Python and uses sigaltstack().

@tich
Copy link
Contributor Author

tich commented Mar 23, 2017

Added myself to ACKS and listed the fix in NEWS. Thanks!

serhiy-storchaka
serhiy-storchaka reviewed Mar 23, 2017
View reviewed changes
Misc/NEWS Outdated
Copy link
Member

@serhiy-storchaka serhiy-storchaka Mar 23, 2017

Choose a reason for hiding this comment

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

Move it to the start of the Library section.

Copy link
Contributor Author

@tich tich Mar 23, 2017

Choose a reason for hiding this comment

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

Done

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Mar 23, 2017

IMHO it's overkill to test this bugfix. It's a corner case which requires to write a program which embeds Python and uses sigaltstack().

Okay, then tests are not required.

@vstinner vstinner merged commit 20fbf8a into python:master Mar 23, 2017
@vstinner
Copy link
Member

vstinner commented Mar 23, 2017

Thanks! I merged your fix. It would be nice to backport the fix Python 3.5 & 3.6 (using git cherry-pick).

tich added a commit to tich/cpython that referenced this pull request Mar 24, 2017
@tich
faulthandler: Restore the old sigaltstack during teardown (python#777)
e3b1a5a 
(cherry picked from commit 20fbf8a)
tich added a commit to tich/cpython that referenced this pull request Mar 24, 2017
@tich
faulthandler: Restore the old sigaltstack during teardown (python#777)
20d474a 
(cherry picked from commit 20fbf8a)
Mariatta pushed a commit that referenced this pull request Mar 24, 2017
@tich @Mariatta
faulthandler: Restore the old sigaltstack during teardown (GH-777) (G…
90eafdb 
…H-797)

(cherry picked from commit 20fbf8a)
Mariatta pushed a commit that referenced this pull request Mar 24, 2017
@tich @Mariatta
faulthandler: Restore the old sigaltstack during teardown (GH-777) (G…
6a45811 
…H-796)

(cherry picked from commit 20fbf8a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

@vstinner vstinner vstinner approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tich @mention-bot @serhiy-storchaka @vstinner @Mariatta @the-knights-who-say-ni