[PATCH] D43264: [WebAssembly] Add explicit symbol table

[Nicholas Wilson via Phabricator via llvm-commits]

ncw accepted this revision.
ncw added a comment.

It looks OK still. It's familiar, in that I tried a similar refactor (making InputGlobal not inherit InputChunk), and I can see you've faced the same problems.

I think it works, just some comments about consistency below.



================
Comment at: wasm/MarkLive.cpp:45-47
     InputChunk *Chunk = Sym->getChunk();
     if (!Chunk || Chunk->Live)
       return;
----------------
You're not marking globals as Live, so how will they avoid being GC'd? You need something like this to be added I think:

```
if (DefinedGlobal *G = dyn_cast<DefinedGlobal>(Sym)) {
  G->Global->Live = true;
  return;
}
InputChunk *Chunk = ...
```

It's probably just slipped under the radar because the stack-pointer is a synthetic global, and there aren't any tests yet for non-synthetic ones.

I have some tests I'm waiting to commit for more global support, I guess we could fix GC for defined globals then.


================
Comment at: wasm/SymbolTable.cpp:147-148
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
-  assert(WasInserted);
   return replaceSymbol<DefinedData>(S, Name, Flags);
 }
----------------
It's odd that this is the one case without an `assert(WasInserted)` - I've forgotten, is there a reason we can't assert here like we do in addSyntheticFunction/Global?


================
Comment at: wasm/Symbols.cpp:49-51
   if (auto *F = dyn_cast<DefinedFunction>(this))
     if (F->Function)
       return F->Function->hasOutputIndex();
----------------
Missing specialisation for DefinedGlobal here; hasOutputIndex should definitely match the implementation for getOutputIndex below!


================
Comment at: wasm/Symbols.cpp:166-173
+const WasmGlobalType &GlobalSymbol::getGlobalType() const {
+  if (auto *G = dyn_cast<DefinedGlobal>(this))
+    if (G->Global) return G->Global->getType();
+
+  DEBUG(dbgs() << "getGlobalType: " << getName() << "\n");
+  assert(GlobalType != nullptr);
+  return *GlobalType;
----------------
Shouldn't this method go away too now? We got rid of it on master for FunctionSymbol, so GlobalSymbol should match that unless there's a good reason to use the old scheme still.


================
Comment at: wasm/Writer.cpp:144
   std::string CtorFunctionBody;
+  std::unique_ptr<InputGlobal> StackPointerInputGlobal;
 
----------------
Should this `unique_ptr<InputGlobal>` still be here? There are now two of these, one in the Linker and one here. (Personally, I think it makes sense to own the stack-pointer here and get rid of the InputGlobal in Linker, so that CtorFunction and StackPointer behave in the same way; I can't immediately think of a reason why they shouldn't...)


================
Comment at: wasm/Writer.cpp:721-723
+      InputChunk *Chunk = Sym->getChunk();
+      if (Chunk && !Chunk->Live)
+        continue;
----------------
Also needs a special branch like in MarkLive:

```
if (DefinedGlobal* G = dyn_cast<DefinedGlobal>(Sym))
  if (G->Global && !G->Global->Live)
    continue;
}
InputChunk *Chunk = Sym->getChunk();
if (Chunk && !Chunk->Live)
  continue;
```

I know it's tedious... but if you're going to have the Live flag on InputGlobal (which I agree we do want) then we have to check it separately everywhere that we currently check the InputChunk::Live flag. That's the nuisancesome consequence of splitting InputGlobal off from InputChunk, but we can stomach that cost for now.


================
Comment at: wasm/Writer.cpp:804-809
+          // Mark target global as live
+          GlobalSymbol *Sym = File->getGlobalSymbol(Reloc.Index);
+          if (auto *G = dyn_cast<DefinedGlobal>(Sym)) {
+            DEBUG(dbgs() << "marking global live: " << Sym->getName() << "\n");
+            G->Global->Live = true;
+          }
----------------
?? Shouldn't this be done in MarkLive; why should globals be different to functions in this regard?


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43264