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.

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

Raise DEFAULT_MIN_STACK_SIZE to at least 64KiB #126059

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

workingjubilee
Copy link
Member

Prevent copy-paste errors from producing new starved-for-resources threaded platforms by raising DEFAULT_MIN_STACK_SIZE from 4096 bytes to at least 64KiB.

Two platforms "affected" by this have no actual threads:

  • UEFI
  • "unsupported"

Platforms that this actually affects:

  • wasm32-wasi with "atomics" enabled
  • wasm32-wasi-p1-threads

Two exceptions:

  • SGX: a "secure code execution" platform, stays at 4096B
  • TEEOS: also a "secure code execution" platform, stays at 8192B

I believe either of these may have sufficiently "interesting" semantics around threads, or significant external library support. Either would mean making any choices here for them is suspect.

Fixes #126027 which is a bug report about DEFAULT_MIN_STACK_SIZE being too low on wasi.

Prevent copy-paste errors from producing new starved-for-resources
threaded platforms by raising `DEFAULT_MIN_STACK_SIZE` from 4096 bytes
to at least 64KiB.

Two platforms "affected" by this have no actual threads:
- UEFI
- "unsupported"

Platforms that this actually affects:
- wasm32-wasi with "atomics" enabled
- wasm32-wasi-p1-threads

Two exceptions:
- SGX: a "secure code execution" platform, stays at 4096B
- TEEOS: also a "secure code execution" platform, stays at 8192B

I believe either of these may have sufficiently "interesting" semantics
around threads, or significant external library support. Either would
mean making any choices here for them is suspect.
@rustbot
Copy link
Collaborator

rustbot commented Jun 6, 2024

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-wasi Operating system: Wasi, Webassembly System Interface O-wasm Target: WASM (WebAssembly), http://webassembly.org/ S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 6, 2024
@workingjubilee
Copy link
Member Author

workingjubilee commented Jun 6, 2024

OpenBSD 4.6 was a long while ago, so 128KiB might make more sense (the Alpine minimum, also used in this repo by Xous and ITRON).

@ChrisDenton
Copy link
Member

This sounds great to me and I don't see a need to keep such a small default but cc @alexcrichton in case I'm missing something?

@alexcrichton
Copy link
Member

Definitely no reason for WASI to be so small. I've no recollection myself of the provenance of the current value and digging through the history it seems like it's just a bunch of historical accidents that led to this. Stacks in wasi should definitely be ok to be larger than 4k, and 4k is indeed overly small. Thanks for the PR to raise it @workingjubilee!

@ChrisDenton
Copy link
Member

Great! Then let's do this...

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 6, 2024

📌 Commit 8781074 has been approved by ChrisDenton

It is now in the queue for this repository.

@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 Jun 6, 2024
fmease added a commit to fmease/rust that referenced this pull request Jun 6, 2024
…r, r=ChrisDenton

Raise `DEFAULT_MIN_STACK_SIZE` to at least 64KiB

Prevent copy-paste errors from producing new starved-for-resources threaded platforms by raising `DEFAULT_MIN_STACK_SIZE` from 4096 bytes to at least 64KiB.

Two platforms "affected" by this have no actual threads:
- UEFI
- "unsupported"

Platforms that this actually affects:
- wasm32-wasi with "atomics" enabled
- wasm32-wasi-p1-threads

Two exceptions:
- SGX: a "secure code execution" platform, stays at 4096B
- TEEOS: also a "secure code execution" platform, stays at 8192B

I believe either of these may have sufficiently "interesting" semantics around threads, or significant external library support. Either would mean making any choices here for them is suspect.

Fixes rust-lang#126027 which is a bug report about `DEFAULT_MIN_STACK_SIZE` being too low on wasi.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 6, 2024
…r, r=ChrisDenton

Raise `DEFAULT_MIN_STACK_SIZE` to at least 64KiB

Prevent copy-paste errors from producing new starved-for-resources threaded platforms by raising `DEFAULT_MIN_STACK_SIZE` from 4096 bytes to at least 64KiB.

Two platforms "affected" by this have no actual threads:
- UEFI
- "unsupported"

Platforms that this actually affects:
- wasm32-wasi with "atomics" enabled
- wasm32-wasi-p1-threads

Two exceptions:
- SGX: a "secure code execution" platform, stays at 4096B
- TEEOS: also a "secure code execution" platform, stays at 8192B

I believe either of these may have sufficiently "interesting" semantics around threads, or significant external library support. Either would mean making any choices here for them is suspect.

Fixes rust-lang#126027 which is a bug report about `DEFAULT_MIN_STACK_SIZE` being too low on wasi.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 6, 2024
…kingjubilee

Rollup of 12 pull requests

Successful merges:

 - rust-lang#125220 (Repair several `riscv64gc-unknown-linux-gnu` codegen tests)
 - rust-lang#126033 (CI: fix publishing of toolstate history)
 - rust-lang#126034 (Clarify our tier 1 Windows Server support)
 - rust-lang#126035 (Some minor query system cleanups)
 - rust-lang#126051 (Clarify an `x fmt` error.)
 - rust-lang#126059 (Raise `DEFAULT_MIN_STACK_SIZE` to at least 64KiB)
 - rust-lang#126064 (Migrate `run-make/manual-crate-name` to `rmake.rs`)
 - rust-lang#126072 (compiletest: Allow multiple `//@ run-flags:` headers)
 - rust-lang#126073 (Port `tests/run-make-fulldeps/obtain-borrowck` to ui-fulldeps)
 - rust-lang#126081 (Do not use relative paths to Rust source root in run-make tests)
 - rust-lang#126086 (use windows compatible executable name for libcxx-version)
 - rust-lang#126096 ([RFC-2011] Allow `core_intrinsics` when activated)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 68c57de into rust-lang:master Jun 7, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 7, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 7, 2024
Rollup merge of rust-lang#126059 - workingjubilee:stack-nothing-higher, r=ChrisDenton

Raise `DEFAULT_MIN_STACK_SIZE` to at least 64KiB

Prevent copy-paste errors from producing new starved-for-resources threaded platforms by raising `DEFAULT_MIN_STACK_SIZE` from 4096 bytes to at least 64KiB.

Two platforms "affected" by this have no actual threads:
- UEFI
- "unsupported"

Platforms that this actually affects:
- wasm32-wasi with "atomics" enabled
- wasm32-wasi-p1-threads

Two exceptions:
- SGX: a "secure code execution" platform, stays at 4096B
- TEEOS: also a "secure code execution" platform, stays at 8192B

I believe either of these may have sufficiently "interesting" semantics around threads, or significant external library support. Either would mean making any choices here for them is suspect.

Fixes rust-lang#126027 which is a bug report about `DEFAULT_MIN_STACK_SIZE` being too low on wasi.
@workingjubilee workingjubilee deleted the stack-nothing-higher branch June 7, 2024 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-wasi Operating system: Wasi, Webassembly System Interface O-wasm Target: WASM (WebAssembly), http://webassembly.org/ S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent DEFAULT_MIN_STACK_SIZE Across Platforms Causes Unexpected Issues
5 participants