Skip to content
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

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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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