Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Enable stack-overflow detection on musl for non-main threads #75703

Merged
merged 1 commit into from
Aug 20, 2020
Merged

Enable stack-overflow detection on musl for non-main threads #75703

merged 1 commit into from
Aug 20, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Aug 19, 2020 •
edited
Loading

No description provided.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 19, 2020
@mati865
Copy link
Contributor

mati865 commented Aug 19, 2020

I assume you have tested it, right?
Previously it made some of the tests crash.

@malbarbo
Copy link
Contributor

So, this fixes #31506?

@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 19, 2020

@malbarbo if you consider the issue #31506 to be about presence of the guard
pages, then as far as I know, they were always there. On the other hand if you
consider it to be about having a message indicating that stack overflow did
happen, then this PR produces such a message for threads other than main.

@mati865 I tested it with musl 1.2.0. CI uses 1.1.24 so we will have another
data point. Do you have links to previous attempts? I wonder if position
of the guard has been adjusted, since the fallback one is not correct for musl.

@mati865
Copy link
Contributor

mati865 commented Aug 19, 2020

@tmiasko I tested it locally and don't have the logs anymore.

@cuviper
Copy link
Member

cuviper commented Aug 19, 2020

Requires musl 1.1.19 or later. Earlier version didn't report guard size in pthread_getattr_np.

What did we get before, zero? garbage? If it was zero, then I guess the failure mode here is harmless, just computing an empty guard range, no better/worse than the None we have already.

@tmiasko tmiasko force-pushed the stack-overflow-musl branch from 24f6207 to 6a80b13 Compare August 19, 2020 18:35
@tmiasko
Copy link
Contributor Author

tmiasko commented Aug 19, 2020

It returned zero, but we panic if guard is missing. I adjusted code to use the page size as a fall-back in that specific case.

@cuviper
Copy link
Member

cuviper commented Aug 19, 2020

OK, sounds good.

@bors r+

@bors
Copy link
Contributor

bors commented Aug 19, 2020

📌 Commit 6a80b13 has been approved by cuviper

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 19, 2020
@cuviper
Copy link
Member

cuviper commented Aug 20, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 20, 2020

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Aug 20, 2020

📌 Commit 6a80b13 has been approved by cuviper

@cuviper
Copy link
Member

cuviper commented Aug 20, 2020

@bors rollup

@cuviper cuviper mentioned this pull request Aug 20, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 20, 2020
@bors
Auto merge of rust-lang#75747 - cuviper:rollup-icke90l, r=cuviper
3323691 
Rollup of 8 pull requests

Successful merges:

 - rust-lang#75672 (Move to intra-doc links for task.rs and vec.rs)
 - rust-lang#75702 (Clean up E0759 explanation)
 - rust-lang#75703 (Enable stack-overflow detection on musl for non-main threads)
 - rust-lang#75710 (Fix bad printing of const-eval queries)
 - rust-lang#75716 (Upgrade Emscripten on CI to 1.39.20 )
 - rust-lang#75731 (Suppress ty::Float in MIR comments of ty::Const)
 - rust-lang#75733 (Remove duplicated alloc vec bench push_all_move)
 - rust-lang#75743 (Rename rustc_lexer::TokenKind::Not to Bang)

Failed merges:

r? @ghost
@bors bors merged commit 7ac126e into rust-lang:master Aug 20, 2020
@tmiasko tmiasko deleted the stack-overflow-musl branch August 20, 2020 20:31
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees

@cuviper cuviper

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.47.0
Development

Successfully merging this pull request may close these issues.
6 participants
@tmiasko @mati865 @malbarbo @cuviper @bors @rust-highfive