Adds --passive-segments and --active-segments flags to control
what kind of segments are emitted. For now the default is always
to emit active segments so this is not a breaking change, but in
the future the default will be changed to passive segments when
shared memory is requested and active segments otherwise. When
passive segments are emitted, corresponding memory.init and
data.drop instructions are emitted in a __wasm_init_memory
function that is automatically called at the beginning of
__wasm_call_ctors.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 34099 Build 34098: arc lint + arc unit
Event Timeline
Comment Actions
Mostly lgtm
| lld/test/wasm/data-segment-merging.ll | ||
|---|---|---|
| 12 | The code looks like passive initiazlition is driver by the "--shared-memory" flag rather than atomics. Wouldn't it make more sense to use "SHARED" over "ATOMICS" in this test? | |
| lld/test/wasm/data-segment-merging.ll | ||
|---|---|---|
| 12 | D59284 makes the "atomics" feature imply the use of shared memory, and in this test the flag that causes shared memory to be used is -mattr=+atomics, so I think the current naming accurately describes what is happening. I expect the common way to get shared memory is by using the atomics feature, so perhaps we can even deprecate the --shared-memory flag, WDYT? | |
| lld/test/wasm/data-segment-merging.ll | ||
|---|---|---|
| 12 | I'm not sure. Part of me likes the explicit requirment/ability to pass this kind of thing as a linker flag. I think shared vs non-shared memory is significant enough that I'm not sure it should be driven my object file input. e.g. As a developer, I don't necessarily want the accidental inclusion of the bad object file to change the type of output object. It would be like infering -shared if I pass objects build with -PIC. I think I'd rather get a linker error (object files use atomics, did you mean to pass --shared-memory?) | |
| lld/test/wasm/data-segment-merging.ll | ||
|---|---|---|
| 12 | That's a reasonable argument. This CL introduces a similar error when atomics are used but a max memory size is not supplied. But I think requiring an explicit --shared-memory flag when the linker can infer that shared memory is necessary is an unnecessary speed bump for users. Inferring shared memory is consistent with our intent in the clang flags that using the atomics feature implies multithreading. The more robust mechanism for guarding against bad object files is explicitly specifying features with --features=.... | |
| lld/test/wasm/data-segment-merging.ll | ||
|---|---|---|
| 12 | Updated for the new error behavior and changed from ATOMICS to SHARED as originally suggested. | |
Comment Actions
@dschuff, @sbc100, @azakai we should figure out what our story is for pthreads and bulk-memory in the LLVM backend. This CL assumes that passive segments will always be available when creating multithreaded binaries, but obviously that's not true for Chrome 74. I was thinking that would be fine because using the asm2wasm path with mem files will suffice for anyone wanting to target chrome 74 with pthreads. But will we want to support mem files going forward? If so, I need to add a linker option to control that.
| lld/test/wasm/data-segment-merging.ll | ||
|---|---|---|
| 47 | Can we switch to MERGE-SHARED-NEXT here and below? | |
| lld/wasm/OutputSections.cpp | ||
| 137 | Perhaps we should make this into a fatal error? And we could perhaps catch it earlier. Doesn't need to happen in this CL though I guess, | |
| lld/wasm/Writer.cpp | ||
| 720 | I'm tempted to make this a new function of its own that is than called as constructor 0. What do you think? | |
| lld/test/wasm/data-segment-merging.ll | ||
|---|---|---|
| 47 | We're deliberately skipping SectionOffset lines in the output because they're not that very interesting. I could include them and switch to MERGE-SHARED-NEXT if you want, though. I've also de-duplicated a lot of the checking here. | |
| lld/wasm/OutputSections.cpp | ||
| 137 | Makes sense, but I think that can be a separate PR. | |
| lld/wasm/Writer.cpp | ||
| 720 | Sounds good. Done. | |
| lld/wasm/Driver.cpp | ||
|---|---|---|
| 477–479 | It might not be the second so perhaps how about: "which is called from __wasm_call_ctors before the user-level constructors". | |
| lld/wasm/MarkLive.cpp | ||
| 88 | The coding style for lld is to not include braces for single line blocks. We need to think a little more about this is going to work for non-PIC code. Up until now its been possible to write non-PIC static executable that runs the ctors itself via some arbitrary function. e.g. int mystartfunc() {
__wasm_call_ctors();
return main()
}This part of the markLive is designed to keep __wasm_call_ctors alive even if the user never references it (since its to be exported in the PIC case). So.. in the non-PIC case, even for shared-memory, I think we should let the normal GC process take its course. If they user forgets to reference __wasm_call_ctors, then there is not way it can ever be called, so we might as well let it be GC'd. But the dependency setup here is getting a little complicated. Really what we want to say is something like: "if CallCtors is alive then ApplyRelocs need to be too" and the same for InitMemory. Perhaps we should add Dependencies() to SyntheticFunction? For now I'm ok with the conservative approach of just keeping InitMemory alive as long as SharedMemory is present. | |
Comment Actions
This CL should not be merged until Chrome 75 releases to stable on June 4.
| lld/wasm/MarkLive.cpp | ||
|---|---|---|
| 88 | I looked into adding Dependencies, but because markLive happens before anything in Writer.cpp there didn't seem to be a good way to set that up. I simplified the logic in MarkLive.cpp though, now that I understand what it is doing more. | |
Comment Actions
Change to use explicit flags instead of depending on shared
memory. This means this change is a nop in existing
toolchains. Emscripten can be taught to use the new flags explicitly
without requiring a manual double roll.
After emscripten is updated to use the flags explicitly the default
behavior can be changed to depend on --shared-memory.
Comment Actions
@sbc100 it's been a long time but there is nothing blocking this change any more, so PTAL!
The code looks like passive initiazlition is driver by the "--shared-memory" flag rather than atomics. Wouldn't it make more sense to use "SHARED" over "ATOMICS" in this test?