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

threads: Retrieve default stack size from __heap_base/__data_end #350

Merged
merged 3 commits into from
Dec 19, 2022

Conversation

loganek
Copy link
Contributor

@loganek loganek commented Dec 2, 2022

When compiling with -z stack-size flag, only the main thread's stack size is set to the specified value and other threads use musl's default value. That's inconsistent with LLD's -Wl,-stack_size.

I think we can make it similar to musl's behavior, where thread's stack size can be set via PT_GNU_STACK program header (via -Wl,-z,stack-size flag).

Configuring stack size through pthread_attr_t still works as expected and overrides the defaults (pthread_create.c).

@loganek loganek force-pushed the loganek/default-stack-size branch from 336f6fe to 79a4360 Compare December 2, 2022 17:26
@loganek loganek force-pushed the loganek/default-stack-size branch from df8ec15 to 60aba3b Compare December 5, 2022 11:48
@loganek loganek force-pushed the loganek/default-stack-size branch from 60aba3b to 7a7005a Compare December 6, 2022 09:45
Copy link
Collaborator

@abrown abrown left a comment

Choose a reason for hiding this comment

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

This makes sense to me! I'll let @sbc100 be the final approver since he had some comments already.

Copy link
Contributor

@yamt yamt left a comment

Choose a reason for hiding this comment

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

lgtm

@loganek
Copy link
Contributor Author

loganek commented Dec 11, 2022

I think it's ready for merge @sbc100 @abrown ?

@abrown
Copy link
Collaborator

abrown commented Dec 16, 2022

I was about to merge this but now it has a merge conflict; @loganek, can you resolve that?

Verified

This commit was signed with the committer’s verified signature.
wpoynter Will Poynter
When compiling with `-z stack-size` flag, only the main thread's stack
size is set to the specified value and other threads use musl's default value.
That's inconsistent with LLD's `-Wl,-stack_size`.

I think we can make it similar to MUSL's behavior, where thread's stack
size can be set via `PT_GNU_STACK` program header (via `-Wl,-z,stack-size`
flag).

Configuring stack size through `pthread_attr_t` still work as expected and
overrides the defaults ([pthread_create.c](https://github.com/WebAssembly/wasi-libc/blob/be1ffd6a9eba1704085987482557c2a32724227f/libc-top-half/musl/src/thread/pthread_create.c#L362))
default settings.

Verified

This commit was signed with the committer’s verified signature.
wpoynter Will Poynter
… they're defined
… of the memory

I.e. when `--stack-first` flag is provided to the linker
@loganek loganek force-pushed the loganek/default-stack-size branch from 7a7005a to f768d5a Compare December 18, 2022 23:32
@loganek
Copy link
Contributor Author

loganek commented Dec 18, 2022

@abrown should be ready for merge now

@loganek loganek changed the title Retrieve default stack size from __heap_base/__data_end threads: Retrieve default stack size from __heap_base/__data_end Dec 18, 2022
@sbc100 sbc100 merged commit 957c711 into WebAssembly:main Dec 19, 2022
john-sharratt pushed a commit to john-sharratt/wasix-libc that referenced this pull request Mar 6, 2023
…Assembly#350)

When compiling with `-z stack-size` flag, only the main thread's stack
size is set to the specified value and other threads use musl's default value.
That's inconsistent with LLD's `-Wl,-stack_size`.

I think we can make it similar to MUSL's behavior, where thread's stack
size can be set via `PT_GNU_STACK` program header (via `-Wl,-z,stack-size`
flag).

Configuring stack size through `pthread_attr_t` still work as expected and
overrides the defaults ([pthread_create.c](https://github.com/WebAssembly/wasi-libc/blob/be1ffd6a9eba1704085987482557c2a32724227f/libc-top-half/musl/src/thread/pthread_create.c#L362))
default settings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants