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

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added a comment.

Thanks for the comments, I'll deal with those nits.

Would it be a good compromise to rename "Symbol::Live" to "Symbol::ImportLive"? That would make it clear that it's the //imported function object// that's being GC'd, not the Symbol itself. Even more strictly, I could move the Live bit to the UndefinedGlobal/UndefinedFunction/UndefinedData classes (although that's more mess code-wise it might be seen as clearer?).



================
Comment at: wasm/Symbols.cpp:89
+  }
+  Live = true;
 }
----------------
sbc100 wrote:
> sbc100 wrote:
> > Maybe assert `isUndefined` here?
> Alternatively we might want to set this unconditionally at the top of the function?
> 
> In which case isLive becomes just `return Live`?
> 
I'd have to check if that's possible, or it would break aliasing. Liveness isn't really a property of the Symbol, it conceptually is a property of the "thing" that the symbol points to. It's just that for undefined symbols there is no "thing". To me, it's cleaner to avoid getting/setting the Live bit on the Symbol except in the undefined case where we have to.


================
Comment at: wasm/Symbols.h:78-79
 
+  // Marks the symbol as Live, so that it will be included in the final image.
+  void markLive();
+
----------------
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.
> 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.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44313