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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: wasm/MarkLive.cpp:45-47
     InputChunk *Chunk = Sym->getChunk();
     if (!Chunk || Chunk->Live)
       return;
----------------
ncw wrote:
> 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.
Globals (like Types) are always generated precisely regardless of --gc-sections, and so are not handled here.


================
Comment at: wasm/SymbolTable.cpp:147-148
   bool WasInserted;
   std::tie(S, WasInserted) = insert(Name);
-  assert(WasInserted);
   return replaceSymbol<DefinedData>(S, Name, Flags);
 }
----------------
ncw wrote:
> 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?
Oops, that must have been merge conflict.  Restored.


================
Comment at: wasm/Writer.cpp:721-723
+      InputChunk *Chunk = Sym->getChunk();
+      if (Chunk && !Chunk->Live)
+        continue;
----------------
ncw wrote:
> 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.
I think we will want an isLive() method on Symbol, like the COFF linker has.  I will add that before landing this change.


================
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;
+          }
----------------
ncw wrote:
> ?? Shouldn't this be done in MarkLive; why should globals be different to functions in this regard?
When I added GC I decided to make it only apply to functions and data (chunks).  For synthetic sections like the Type section, and now the Global section, I decided to always be precise and write only the types/globals that are needed.  This means that you effectivly get type and global GC regardless of the --gc-sections flag.  The code above marks the types are live already, this just extends it to handle globals


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D43264