Page MenuHomePhabricator

[WebAssembly] Initialize memory in start function
ClosedPublic

Authored by tlively on Aug 5 2019, 5:46 PM.

Details

Summary
  • __wasm_init_memory is now the WebAssembly start function instead of being called from __wasm_call_ctors or called directly by the runtime.
  • Adds a new synthetic data symbol __wasm_init_memory_flag that is atomically incremented from zero to one by the thread responsible for initializing memory.
  • All threads now unconditionally perform data.drop on all passive segments.
  • Removes --passive-segments and --active-segments flags and controls segment type based on --shared-memory instead. The deleted flags were only present to ameliorate the upgrade path in Emscripten.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Aug 5 2019, 5:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2019, 5:46 PM

I don't have the full context/history of this feature, but it mostly looks LGTM to me.

  • The tool-convention linking doc says we don't generate the start section for now. I guess we should update that too.
  • I'm not sure what the exact condition for the passive segment generation. We should have config->sharedMemory for sure, but in some place we have !config->relocatable and others !config->shared but not both.
  • How did we tell which one is the main thread before this patch?
lld/test/wasm/data-segment-merging.ll
92 ↗(On Diff #213504)

So __wasm_init_tls is also called from the start function in passive segments as in __wasm_init_memory, right?

lld/test/wasm/data-segments.ll
5 ↗(On Diff #213504)

Now that tests in this file don't have --active-segments and --passive-segments anymore and are instead controlled by --shared-memory, it might be helpful to add whether we have shared memory or not in the comments, like

; atomics, shared memory => error
; atomics, bulk memory, shared memory => passive segments
...

The same for other comments below.
lld/test/wasm/shared-memory.yaml
3 ↗(On Diff #213504)

Real nit: why remove the whitespace before |?

lld/wasm/Driver.cpp
469 ↗(On Diff #213504)

Are we guaranteed not to be relocatable here?

516 ↗(On Diff #213504)

What happens when config->sharedMemory && config->shared?

lld/wasm/MarkLive.cpp
88 ↗(On Diff #213504)

WasmSym::initMemory was created like

if (config->sharedMemory && !config->shared) {
  WasmSym::initMemory = symtab->addSyntheticFunction(
      "__wasm_init_memory", WASM_SYMBOL_VISIBILITY_HIDDEN,
      make<SyntheticFunction>(nullSignature, "__wasm_init_memory"));
 ...
}

Don't we need the condition !config->shared here too?

lld/wasm/Writer.cpp
375 ↗(On Diff #213504)

Nit: It looks we have using namespace llvm above, so we don't need llvm::

400 ↗(On Diff #213504)

This may not be related to this CL, but just wondering, what happens we explicitly enable a feature using --feature= option that's in the disallowed set in one of the objects? Do we error out?

709 ↗(On Diff #213504)
  • Before this change, how did we distinguish the main thread? (I don't know the full history of this feature, sorry)
  • Does this new code mean whichever thread that first runs this code becomes the main thread?
tlively marked 6 inline comments as done.Aug 14 2019, 9:59 AM

Thanks for you comments! I am also planning to update the memory initialization scheme to use wait and notify so that it works with racing modules instead of requiring the runtime to ensure that no module is called into before the main module is finished initializing. In a web context the runtime will have to ensure the main thread wins this race because it is not allowed to wait, but this is still strictly more general than the current scheme. Does that sound good to you?

lld/test/wasm/data-segment-merging.ll
92 ↗(On Diff #213504)

No, __wasm_init_tls requires a runtime-allocated TLS buffer as an argument, so it has to be called from runtime code rather than a start function.

lld/test/wasm/data-segments.ll
5 ↗(On Diff #213504)

Good idea, will do.

lld/test/wasm/shared-memory.yaml
3 ↗(On Diff #213504)

Hmm, I don't think that was intentional. Thanks!

lld/wasm/Driver.cpp
469 ↗(On Diff #213504)

Yes, this function is only called when !config->relocatable. This may have been refactored in the meantime, though. I'll have to rebase and merge.

516 ↗(On Diff #213504)

Only the main module (which I understand is never config->shared?) needs to initialize memory. It would be bad if __wasm_init_memory were run more than once per memory. So shared modules just don't have these symbols.

lld/wasm/MarkLive.cpp
88 ↗(On Diff #213504)

Hmm, I guess it works regardless because enqueue silently disregards null arguments. But you're right that the condition should be there for consistency. Will update.

Talked offline. Yeah that would be good.

tlively planned changes to this revision.Sep 3 2019, 11:40 AM
tlively updated this revision to Diff 218569.Sep 3 2019, 5:24 PM
  • Switch to more robust scheme with cmpxchg/wait/notify
tlively updated this revision to Diff 218574.Sep 3 2019, 6:19 PM
tlively marked 7 inline comments as done.
  • Address review comments
lld/wasm/Writer.cpp
400 ↗(On Diff #213504)

Yes, that would be an error unless --no-check-features is also passed. --feature= is just a more explicit way of passing features and is not meant to be any more privileged than the target features sections in determining what is correct and safe.

709 ↗(On Diff #213504)

Before this change we relied on the runtime to manually invoke __wasm_init_memory or __wasm_call_ctors (which called __wasm_init_memory) on the main thread. With this change, the code is more robust because it doesn't require the runtime to call anything or even ensure that there is only one thread running during memory initialization.

Whichever thread runs first initializes the memory, but is not otherwise marked as the main thread or privileged in any way. If you instantiate a module on a number of threads at once it should not be possible to determine which one initialized the memory.

sbc100 added a comment.Sep 3 2019, 6:26 PM

Can you verify that you can pass all the "wasm2" tests in emscripten with this change?

aheejin accepted this revision.Sep 3 2019, 10:18 PM

LGTM % nits if tests pass.

lld/wasm/Writer.cpp
711 ↗(On Diff #218574)

I can't find this block instruction in the code. Am I missing something?

720 ↗(On Diff #218574)

Nit: How about $__init_memory_flag for consistency?

This revision is now accepted and ready to land.Sep 3 2019, 10:18 PM

Can you verify that you can pass all the "wasm2" tests in emscripten with this change?

Yes, although this CL only affects shared-memory builds, so wasm2 tests are not affected. I have also verified that the pthreads tests in the browser test suite also pass with this change.

tlively marked 2 inline comments as done.Sep 4 2019, 11:39 AM
tlively added inline comments.
lld/wasm/Writer.cpp
711 ↗(On Diff #218574)

I wrote this wat using Binaryen's text format, which requires a block here that is not present in the binary. I will rewrite the wat using the standard text format, which will be less confusing.

720 ↗(On Diff #218574)

Sounds good.

This revision was automatically updated to reflect the committed changes.