Page MenuHomePhabricator

[WebAssembly] Use passive segments when memory is shared
Changes PlannedPublic

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

Details

Summary

And adds corresponding memory.init and data.drop instructions to
initialize the passive segments in __wasm_call_ctors. Using
passive segments prevents segments from automatically being
reinitialized every time a new WebAssembly instance is created,
e.g. when creating a thread on a new worker.

memory.init, data.drop, and passive segments are a feature of
bulk memory, but the linker emits them when the memory is shared
whether or not bulk memory is enabled because doing otherwise
does not correctly implement source language semantics. This
means that additional postprocessing to separate the segments
into a mem file is necessary when building for Chrome 74 or any
other target that implements atomics but not bulk memory. It is
our hope that in the future there will be no such targets, in
which case we can make atomics imply bulk memory.

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
32

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
32

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
32

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
32

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
32

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
72

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

lld/wasm/OutputSections.cpp
141

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
1405

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
72

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
141

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

lld/wasm/Writer.cpp
1405

Sounds good. Done.

sbc100 added inline comments.Apr 16 2019, 10:49 AM
lld/wasm/Driver.cpp
446–448

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
85

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
85

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.