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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

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?



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


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


================
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
 
----------------
Real nit: why remove the whitespace before `|`?


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


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


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


================
Comment at: lld/wasm/Writer.cpp:375
   StringMap<std::string> disallowed;
+  llvm::SmallSet<std::string, 8> &allowed = out.targetFeaturesSec->features;
   bool tlsUsed = false;
----------------
Nit: It looks we have `using namespace llvm` above, so we don't need `llvm::`


================
Comment at: lld/wasm/Writer.cpp:400
         break;
       case WASM_FEATURE_PREFIX_DISALLOWED:
         disallowed.insert({feature.Name, fileName});
----------------
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?


================
Comment at: lld/wasm/Writer.cpp:709
+    writeU8(os, WASM_OPCODE_IF, "IF");
+    writeU8(os, WASM_TYPE_NORESULT, "blocktype");
+
----------------
- 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65783