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

wasm: Add new default lld switches to protect from stackoverflow overwriting global memory #3882

Merged
merged 4 commits into from
Dec 8, 2021

Conversation

ryuukk
Copy link
Contributor

@ryuukk ryuukk commented Dec 7, 2021

I was stuck on trying to fix a bug on my code, whenever i was manipulating around large stucts, it'd change global data

I first thought of a memory corruption somewhere, but it turns out it was a stackoverflow issue!

Increasing the default stack (from 64kb) to 1mb fixed the issue... temporarily! as i was playing with more data, i hit the issue again.. so i looked around and found this PR on the zig repo: ziglang/zig#4496

Tada! now instead of overwritting my global memory, it throws a runtime error! that's better! i can now debug the issue properly!

Let's save people days of headache and make these options the default!

Zig/Rust also bumped the default stack size to 1mb (emscripten has a stack size of 5mb)

@ryuukk ryuukk changed the title Add new default ldc switches to protect from stackoverlow overwriting global memory Add new default ldc switches to protect from stackoverflow overwriting global memory Dec 7, 2021
@PetarKirov
Copy link
Contributor

It's failing with:

FAIL: LDC :: baremetal/wasm2.d (17 of 308)
******************** TEST 'LDC :: baremetal/wasm2.d' FAILED ********************
Script:
--
: 'RUN: at line 6';   /var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/build/bin/ldc2 -mtriple=wasm32-unknown-unknown-wasm -betterC -w /private/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/cirrus-ci-build/tests/baremetal/wasm2.d -of=/private/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/build/tests/baremetal/Output/wasm2.d.tmp.wasm
: 'RUN: at line 7';   /var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/build/bin/ldc2 -mtriple=wasm32-unknown-unknown-wasm -betterC -w -fvisibility=hidden /private/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/cirrus-ci-build/tests/baremetal/wasm2.d -of=/private/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/build/tests/baremetal/Output/wasm2.d.tmp_hidden.wasm
: 'RUN: at line 10';   grep myExportedFoo /private/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/build/tests/baremetal/Output/wasm2.d.tmp.wasm
: 'RUN: at line 11';   grep myExportedFoo /private/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/build/tests/baremetal/Output/wasm2.d.tmp_hidden.wasm
--
Exit Code: 1
Command Output (stdout):
--
$ ":" "RUN: at line 6"
$ "/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/build/bin/ldc2" "-mtriple=wasm32-unknown-unknown-wasm" "-betterC" "-w" "/private/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/cirrus-ci-build/tests/baremetal/wasm2.d" "-of=/private/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/build/tests/baremetal/Output/wasm2.d.tmp.wasm"
# command stderr:
ldc2: Unknown command line argument '-z'.  Try: '/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/build/bin/ldc2 --help'
ldc2: Did you mean '-D'?
ldc2: Unknown command line argument '--stack-first'.  Try: '/var/folders/wh/z5_y2cv53sg24tzvtw_f_y1m0000gn/T/build/bin/ldc2 --help'
ldc2: Did you mean '--help-list'?
error: command failed with exit status: 1

So most like you have to use a different syntax to set these options for WASM.

Also, I think it would be cleaner if you set the options in the config file here:

ldc/ldc2.conf.in

Lines 37 to 40 in 5a28329

"^wasm(32|64)-":
{
switches = [
"-defaultlib=",@WASM_DEFAULT_LDC_SWITCHES@

@ryuukk
Copy link
Contributor Author

ryuukk commented Dec 7, 2021

Also, I think it would be cleaner if you set the options in the config file here:

That was my initial thought, but i wasn't sure, i used these options in my dub file:

"lflags-ldc": [
    "-z","stack-size=1048576",
    "--stack-first",

    "-allow-undefined"
],

I'm not sure how to translate to use directly with LDC

@Geod24
Copy link
Contributor

Geod24 commented Dec 7, 2021

dub -v will give you the command line DUB is using. From a quick look, those are lflags, so you might want to use -L.

@JohanEngelen
Copy link
Member

JohanEngelen commented Dec 7, 2021

Also, I think it would be cleaner if you set the options in the config file here:

ldc/ldc2.conf.in

Lines 37 to 40 in 5a28329

"^wasm(32|64)-":
{
switches = [
"-defaultlib=",@WASM_DEFAULT_LDC_SWITCHES@

Indeed. The proposed change in the CMakeFile will only help if LDC is built with LLD built-in. The case where an external LLD is used for linking, would still need manual adding of these flags. If you add the flags to ldc/ldc2.conf.in directly, then that is solved.

Edit: or you add the flags unconditionally to the variable in CMakeFile. Perhaps better, because there are multiple ldc2.conf files that need adjustment (will be automatic when you change the CMakeFile variable).

@kinke
Copy link
Member

kinke commented Dec 7, 2021

or you add the flags unconditionally to the variable in CMakeFile. Perhaps better

Yep, exactly. [And prefixed with -L as mentioned by Mathias.]

@ryuukk
Copy link
Contributor Author

ryuukk commented Dec 8, 2021

it needs approval to trigger builds

@kinke
Copy link
Member

kinke commented Dec 8, 2021

Yeah that's annoying, and IIRC, requires a new approval after each push. - Wrt. making LDC specify these flags without integrated LLD too, please just move the new lines right before the if(LDC_WITH_LLD) block (without empty lines to make clear this contiguous CMake part is all about setting up WASM_DEFAULT_LDC_SWITCHES).

@ryuukk
Copy link
Contributor Author

ryuukk commented Dec 8, 2021

done!

@kinke
Copy link
Member

kinke commented Dec 8, 2021

without empty lines

Sorry about being so pedantic, but the CMake scripts are messy enough already. ;)

@ryuukk
Copy link
Contributor Author

ryuukk commented Dec 8, 2021

Sorry about that! done!

@kinke kinke changed the title Add new default ldc switches to protect from stackoverflow overwriting global memory wasm: Add new default lld switches to protect from stackoverflow overwriting global memory Dec 8, 2021
@kinke kinke enabled auto-merge (squash) December 8, 2021 19:37
@kinke kinke disabled auto-merge December 8, 2021 21:49
@kinke kinke merged commit 370dc7e into ldc-developers:master Dec 8, 2021
@kinke
Copy link
Member

kinke commented Dec 8, 2021

Thanks!

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.

5 participants