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

Wasm targets should append lld --stack-first #4496

Closed
fengb opened this issue Feb 18, 2020 · 3 comments · Fixed by #5501
Closed

Wasm targets should append lld --stack-first #4496

fengb opened this issue Feb 18, 2020 · 3 comments · Fixed by #5501
Labels
accepted This proposal is planned. arch-wasm 32-bit and 64-bit WebAssembly contributor friendly This issue is limited in scope and/or knowledge of Zig internals. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone
0.7.0

Comments

@fengb
Copy link
Contributor

fengb commented Feb 18, 2020 •
edited
Loading

By default, LLVM adopts this memory model:

|   Globals   |   <-- Stack   |   Heap -->  |++

This means when the stack runs out of space, it will start trampling over global memory before causing a stack overflow.

Appending --stack-first will flip where the stack and the globals are located, so stack overflow will error out immediately instead of corrupting globals.

Rust has made a similar decision: rustwasm/team#81 (comment)
@andrewrk andrewrk added arch-wasm 32-bit and 64-bit WebAssembly proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. labels Feb 18, 2020
@andrewrk andrewrk added this to the 0.7.0 milestone Feb 18, 2020
@andrewrk andrewrk added accepted This proposal is planned. contributor friendly This issue is limited in scope and/or knowledge of Zig internals. labels Feb 18, 2020
@zigazeljko
Copy link
Contributor

So the only change is adding lj->args.append("--stack-first"); to construct_linker_job_wasm?

If so, I can prepare a pull request.

@fengb
Copy link
Contributor Author

fengb commented Jun 1, 2020

Yep that should be it.

@zigazeljko zigazeljko mentioned this issue Jun 1, 2020
Merged
jedisct1 added a commit to jedisct1/libsodium that referenced this issue Jul 28, 2020
@jedisct1
WASI: Put the stack before globals
7e9095b 
See ziglang/zig#4496
jedisct1 added a commit to jedisct1/libsodium that referenced this issue Jul 28, 2020
@jedisct1
WASI: Put the stack before globals
d37afd6 
See ziglang/zig#4496
@ryuukk ryuukk mentioned this issue Dec 7, 2021
Merged
@ryuukk
Copy link
Contributor

ryuukk commented Dec 7, 2021

thanks, you saved me after 2 days of headache

@zetanumbers zetanumbers mentioned this issue Dec 28, 2021
Open
This was referenced Jan 11, 2022
Closed
Closed
@sid-008 sid-008 mentioned this issue Jun 26, 2024
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
accepted This proposal is planned. arch-wasm 32-bit and 64-bit WebAssembly contributor friendly This issue is limited in scope and/or knowledge of Zig internals. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Milestone
0.7.0
Development

Successfully merging a pull request may close this issue.

Append lld --stack-first on wasm targets
4 participants
@andrewrk @fengb @zigazeljko @ryuukk