[PATCH] D44311: [WebAssembly] Implement -print-gc-sections to test global GC

[Nicholas Wilson via Phabricator via llvm-commits]

ncw added inline comments.


================
Comment at: wasm/Writer.cpp:790-791
+      } else if (Reloc.Type == R_WEBASSEMBLY_GLOBAL_INDEX_LEB &&
+                 Config->GcSections) {
         // Mark target global as live
         GlobalSymbol *Sym = File->getGlobalSymbol(Reloc.Index);
----------------
sbc100 wrote:
> ruiu wrote:
> > I believe you added `Config->GcSections` because if GC is off, all globals are alive by default and therefore we don't need to set that bit at all. Is this correct? If so, please add a comment.
> I don't really consider what we are going here as GC of sections.  Its more like we are building the sythentic type and global section, and we always do it precisely based on what is needed.   There is no option to disable this, as there is no need to a more conservative approach AFAICT.
> 
> Perhaps it we rename Global->Live to Global->Used it will make the distinction more clear?
> 
> So I don't agree with this part of the change.
Globals are an externally-visible piece of linked output. The browser can tell whether globals were GC'ed or not, so it's very different to say building the TYPE section. That's purely a filesize compression, using the TypeIsUsed map. It's not GC as such, since the contents of the TYPE section are not visible as part of the module's "interface" (no way of telling if extra types were included or not).

But for globals, they really are usable/callable objects just like functions are. Hence `--no-gc-sections` ought to prevent them being stripped. They are part of the objects that the user passed to the linker, and requested to be included in the final output!

(To be honest, I slightly struggle to see whether `--no-gc-sections` has any good use-cases, in fact. Are there situations where you'd want it - maybe to include some functions in the indirect table, that are never called from the Wasm code but should be callable externally via an indirect call?)


================
Comment at: wasm/Writer.cpp:790-791
+      } else if (Reloc.Type == R_WEBASSEMBLY_GLOBAL_INDEX_LEB &&
+                 Config->GcSections) {
         // Mark target global as live
         GlobalSymbol *Sym = File->getGlobalSymbol(Reloc.Index);
----------------
ncw wrote:
> sbc100 wrote:
> > ruiu wrote:
> > > I believe you added `Config->GcSections` because if GC is off, all globals are alive by default and therefore we don't need to set that bit at all. Is this correct? If so, please add a comment.
> > I don't really consider what we are going here as GC of sections.  Its more like we are building the sythentic type and global section, and we always do it precisely based on what is needed.   There is no option to disable this, as there is no need to a more conservative approach AFAICT.
> > 
> > Perhaps it we rename Global->Live to Global->Used it will make the distinction more clear?
> > 
> > So I don't agree with this part of the change.
> Globals are an externally-visible piece of linked output. The browser can tell whether globals were GC'ed or not, so it's very different to say building the TYPE section. That's purely a filesize compression, using the TypeIsUsed map. It's not GC as such, since the contents of the TYPE section are not visible as part of the module's "interface" (no way of telling if extra types were included or not).
> 
> But for globals, they really are usable/callable objects just like functions are. Hence `--no-gc-sections` ought to prevent them being stripped. They are part of the objects that the user passed to the linker, and requested to be included in the final output!
> 
> (To be honest, I slightly struggle to see whether `--no-gc-sections` has any good use-cases, in fact. Are there situations where you'd want it - maybe to include some functions in the indirect table, that are never called from the Wasm code but should be callable externally via an indirect call?)
> I believe you added Config->GcSections because if GC is off, all globals are alive by default and therefore we don't need to set that bit at all. Is this correct? If so, please add a comment.

I added it here, so that globals behave the same as functions. This exact same check is made in MarkLive, so adding it here is simply removing a existing difference, rather than introducing something "weird". We currently don't have any tests at all for GC of globals, so when I added them to `tests/wasm/gc-sections.ll` the test started to look very odd. In a test file that exercises `--gc-sections` and `--no-gc-sections` for functions and data symbols, it would strange to assert that the same options don't have any effect on globals!


================
Comment at: wasm/Writer.cpp:807
 
+  if (Config->PrintGcSections) {
+    for (const ObjFile *Obj : Symtab->ObjectFiles) {
----------------
ruiu wrote:
> It seems a bit too late to do this kind of stuff, since this is a writer. Is there any way to detect liveness of globals earlier than it is now, move code to MarkLive, and also move this code to MarkLive?
Yes, it can certainly be done in MarkLive, and I feel it really ought to be done in MarkLive. Sam was resistant to that though, so this change preserves the status quo.

In D44313 (which builds on this patch) I've moved all the GC into MarkLive, where I feel it belongs.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44311