[PATCH] D44313: [WebAssembly] Implement GC for imports

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added inline comments.


================
Comment at: wasm/Symbols.cpp:85
+  }
+  if (InputChunk *C = getChunk()) {
+    C->Live = true;
----------------
sbc100 wrote:
> I think getChunk can now be removed and inlines into `isLive` and `getLive`, it think it will be more readable that wasy.
I've inlined it if that's neater for you (and converted to switch after inlining), but it'll still be used in MarkLive


================
Comment at: wasm/Symbols.h:78-79
 
+  // Marks the symbol as Live, so that it will be included in the final image.
+  void markLive();
+
----------------
sbc100 wrote:
> ncw wrote:
> > sbc100 wrote:
> > > ruiu wrote:
> > > > sbc100 wrote:
> > > > > ruiu wrote:
> > > > > > I don't think we should add a notion of "liveness" to symbols. It is easy to be confused, but "liveness" is for chunks, data, globals, etc, and symbols themselves are not subject of GC. I understand that since each wasm global have only one symbol associated with it, that distinction is ambiguous, but I still want to maintain that distinction to make things easy to understand.
> > > > > But for undefined symbols we need to know which ones to write to the relocatable output.  For undefined symbols there is no chunk.
> > > > > 
> > > > > We already have in in ELF/Symbols.h: `unsigned Used : 1;`
> > > > > 
> > > > > If you prefer we can give it the same name here for consistency.
> > > > > 
> > > > > I'm not sure there is any difference between `Used` and `Live` though, so it might be worth renaming the one in ELF?
> > > > If symbol is referenced by someone else, we have to do something in some cases. E.g. we have to put it to the dynamic symbol table if it is being used. But that's different from garbage collection.
> > > This is a little like the dynamic symbol table.  Our imports a the moral equivalent right now of dynamic undefined symbols.    We want to populate it only with those symbols that we use.    This change as an incremental improvement makes sense to me.
> > > I understand that since each wasm global have only one symbol associated with it, that distinction is ambiguous
> > 
> > On a side note - A Wasm global is just like any other symbol. There can be many Symbols for the InputGlobal just as there can be many Symbols aliasing the same InputFunction.
> > 
> > I agree that we don't want to GC symbols. I've added a Live bit to the Symbol, not in order to GC the symbol, but in order to GC the function/global object which is imported. The Symbol itself is not GC'ed, but in the case of undefined Symbols, there isn't any existing object to hold the "Live" flag. It has to go somewhere on the Symbol, although it could be renamed to "Used" or heavily commented to clarify.
> > 
> > Regardless of whether a symbol is used/live, it's always included in the symbol table - no pruning is being done there.
> I don't think that last statement is true is it?
> 
> If an undefined symbols it not used (or not live if you prefer), it should not appear in the symbol table, and it should not appear in the imports of the final binary.
Currently, symbol table is only written out when there's no GC, and the full contents of LLD's Symtab are written out. But you're right it might not be true in future if we were ever to write out a symtab from LLD for non-relocatable output.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44313