Require names when using STACK_OVERFLOW_CHECK=2#26878
Require names when using STACK_OVERFLOW_CHECK=2#26878sbc100 wants to merge 1 commit intoemscripten-core:mainfrom
STACK_OVERFLOW_CHECK=2#26878Conversation
| args.append('--check-stack-overflow') | ||
| # The check-stack pass in binaryen needs to be able to locate `__stack_pointer` by name. | ||
| modify_wasm = True | ||
| need_name_section = True |
There was a problem hiding this comment.
Is this stripped out from the final binary, if it doesn't need to be there? We have tests for that in general but I'm not sure if for this specific mode. I'm just worried about silently starting to emit the name section in this situation.
There was a problem hiding this comment.
I verified locally that this is removed later. This need_name_section does not apply to the later binaries passes that run. For example when we do optimization with wasm-opt this does not apply.
Its a local-variable and does not effect the presence of absence of -g in the later optimization phases.
There was a problem hiding this comment.
Actually, we have an even better guarantee than that.
Since we set need_name_section but we don't do args.append('-g'), when wasm-emscripten-finalize will strip the name section by default.
I confirmed that that name section is stripped both before and after this change when running:
$ ./emcc ~/test/hello.c -sSTACK_OVERFLOW_CHECK=2
There was a problem hiding this comment.
Before this change the stripping was happening in the llvm-objcopy phase. After this change the stripping happens during wasm-emscripten-finalize, which is what we want.
There was a problem hiding this comment.
Sounds good. Maybe add a test for it?
There was a problem hiding this comment.
It can just be another line in the existing names section test in other.
There was a problem hiding this comment.
Are you think of a particular test? I tried to find one but didn't have much look. It looks like only test_emsymbolizer_functions and test_separate_dwarf use the .has_name_section method.
2 participants
This change is needed so that WebAssembly/binaryen#8679 can roll in.
The combination will then fix #24964