[PATCH] D65783: [WebAssembly] Initialize memory in start function

[Thomas Lively via Phabricator via llvm-commits]

tlively marked 6 inline comments as done.
tlively added a comment.

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?



================
Comment at: lld/test/wasm/data-segment-merging.ll:92
+; PASSIVE-MERGE-NEXT:      - Index:           2
+; PASSIVE-MERGE-NEXT:        Name:            __wasm_init_tls
 
----------------
aheejin wrote:
> So `__wasm_init_tls` is also called from the start function in passive segments as in `__wasm_init_memory`, right?
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.


================
Comment at: lld/test/wasm/data-segments.ll:5
 
 ; atomics => error
 ; RUN: not wasm-ld -no-gc-sections --no-entry --shared-memory --max-memory=131072 %t.atomics.o -o %t.atomics.wasm 2>&1 | FileCheck %s --check-prefix ERROR
----------------
aheejin wrote:
> 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.
Good idea, will do.


================
Comment at: lld/test/wasm/shared-memory.yaml:3
 
-# RUN: not wasm-ld --no-entry --shared-memory --active-segments %t1.o -o - 2>&1 | FileCheck %s --check-prefix SHARED-NO-MAX
+# RUN: not wasm-ld --no-entry --shared-memory %t1.o -o - 2>&1| FileCheck %s --check-prefix SHARED-NO-MAX
 
----------------
aheejin wrote:
> Real nit: why remove the whitespace before `|`?
Hmm, I don't think that was intentional. Thanks!


================
Comment at: lld/wasm/Driver.cpp:469
                                                             true};
+  assert(!config->relocatable);
+  WasmSym::callCtors = symtab->addSyntheticFunction(
----------------
aheejin wrote:
> Are we guaranteed not to be relocatable here?
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.


================
Comment at: lld/wasm/Driver.cpp:516
 
   if (config->sharedMemory && !config->shared) {
+    // Passive segments are used to avoid memory being reinitialized on each
----------------
aheejin wrote:
> What happens when `config->sharedMemory && config->shared`?
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.


================
Comment at: lld/wasm/MarkLive.cpp:88
+  if (config->sharedMemory)
+    enqueue(WasmSym::initMemory);
+
----------------
aheejin wrote:
> `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?
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65783/new/

https://reviews.llvm.org/D65783