This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add option to emit passive segments
ClosedPublic

Authored by tlively on Mar 13 2019, 7:01 PM.

Details

Summary

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.

Event Timeline

tlively created this revision.Mar 13 2019, 7:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2019, 7:01 PM

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?

tlively marked an inline comment as done.Mar 14 2019, 1:16 PM
tlively added inline comments.
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?

sbc100 added inline comments.Mar 14 2019, 2:01 PM
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?)

tlively marked an inline comment as done.Mar 14 2019, 2:47 PM
tlively added inline comments.
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=....

tlively updated this revision to Diff 191623.Mar 20 2019, 7:26 PM
  • Add --shared-memory to avoid new errors
tlively updated this revision to Diff 191624.Mar 20 2019, 7:28 PM
  • ATOMICS -> SHARED in test
tlively marked 2 inline comments as done.Mar 20 2019, 7:28 PM
tlively added inline comments.
lld/test/wasm/data-segment-merging.ll
12

Updated for the new error behavior and changed from ATOMICS to SHARED as originally suggested.

tlively added a subscriber: azakai.Mar 28 2019, 5:35 PM

@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.

tlively updated this revision to Diff 194987.Apr 12 2019, 5:27 PM
  • Rebase on top of D60637 and add data.drop
tlively edited the summary of this revision. (Show Details)Apr 12 2019, 5:32 PM
tlively edited the summary of this revision. (Show Details)Apr 12 2019, 5:34 PM
tlively added a reviewer: dschuff.
sbc100 added inline comments.Apr 15 2019, 1:58 PM
lld/test/wasm/data-segment-merging.ll
24

Can we switch to MERGE-SHARED-NEXT here and below?

lld/wasm/OutputSections.cpp
139

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
1295

I'm tempted to make this a new function of its own that is than called as constructor 0. What do you think?

tlively updated this revision to Diff 195277.Apr 15 2019, 5:18 PM
tlively marked 3 inline comments as done.
tlively edited the summary of this revision. (Show Details)
  • address comments
tlively added inline comments.Apr 15 2019, 7:01 PM
lld/test/wasm/data-segment-merging.ll
24

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
139

Makes sense, but I think that can be a separate PR.

lld/wasm/Writer.cpp
1295

Sounds good. Done.

sbc100 added inline comments.Apr 16 2019, 10:49 AM
lld/wasm/Driver.cpp
447 ↗(On Diff #195277)

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
81 ↗(On Diff #195277)

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.

tlively updated this revision to Diff 196146.Apr 22 2019, 3:42 PM
tlively marked 2 inline comments as done.
  • Address comments
tlively planned changes to this revision.Apr 24 2019, 11:18 AM

This CL should not be merged until Chrome 75 releases to stable on June 4.

lld/wasm/MarkLive.cpp
81 ↗(On Diff #195277)

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.

tlively retitled this revision from [WebAssembly] Use passive segments when memory is shared to [WebAssembly] Add option to emit passive segments.Jun 28 2019, 4:43 PM
tlively edited the summary of this revision. (Show Details)
tlively edited the summary of this revision. (Show Details)
tlively updated this revision to Diff 207176.Jun 28 2019, 4:57 PM

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.

tlively updated this revision to Diff 207178.Jun 28 2019, 5:05 PM
  • Remove extraneous braces

@sbc100 it's been a long time but there is nothing blocking this change any more, so PTAL!

sbc100 accepted this revision.Jul 2 2019, 5:47 PM

Nice!

lld/wasm/Driver.cpp
479 ↗(On Diff #207178)

The last line of this comment doesn't seem to be full sentence and is inaccurate so perhaps remove that line while you are here?

lld/wasm/Writer.cpp
1278

Take a StringRef rather than const string&?

This revision is now accepted and ready to land.Jul 2 2019, 5:47 PM
This revision was automatically updated to reflect the committed changes.
tlively marked 2 inline comments as done.