[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/InputGlobal.h:55
+  return (toString(G->File) + ":(" + G->getName() + ")").str();
+}
+
----------------
sbc100 wrote:
> ncw wrote:
> > sbc100 wrote:
> > > ncw wrote:
> > > > sbc100 wrote:
> > > > > Non of this is needed if you don't include globals in the output of --print-gc-section.
> > > > True... but it's not a lot of code, and why would we support and test `--print-gc-section` for functions but not globals? I added it because otherwise the existing tests would look lopsided. It seems odd if the test has an assertion that does: "check that a function and a global were GC'd and the logging indicates that the function but not the global was GC'd".
> > > There a couple of reasons I don't think this is needed/wanted.  Firstly, I don't consider wasm globals to be sections.  Secondly that building of types and globals section I chose not be contolled by the --gc-sections flag.  Rather its always generated precisely, based on the inputs.  In the future maybe we could revisit this, and maybe we want to completely remove --gc-section/--print-gc-sections from the wasm port, since we probably always want to enable them.   However I think this incremental change muddies the water.
> > > 
> > > Perhaps once we have more use for wasm globals we can revisit?   At least then we won't need to include the yaml test input.
> > Hmm. It's linked to whether you're happy to move the GC of globals into MarkLive (as Rui suggested, and which would tidy up Writer).
> > 
> > I can do it - and keep globals as they are.. it's just going to mean that yet another block of MarkLive is going to get copy-and-pasted into Writer in D44313 (import GC). By the time we have chunk/global GC and import GC and print-gc-sections duplicated between MarkLive and Writer it's just getting progressively uglier keep two implementations going.
> > 
> > I realise you don't see it as a section... I just want to check you're happy with the implications. Because in order to keep the existing behaviour of doing global GC regardless of whether gc-sections is on, that implies //not// moving global GC to MarkLive in D44313 (which you hadn't objected to in that review).
> I don't think I'd want to move this handling to markLive, at least no unless we completely remove the --no-gc-sections option and have markLive always called.  Given that we are already processing relocations twice, once in markLive and once here, then I think it makes sense to only handle functions and data segments in markLive.  I don't see any real urgency to unify these two.
> 
> Is see this code not so much as "mark live", more as "find and build the import table".   Just like we use this same loop to build the indirect function table.
In my defence - the sole purpose of the code in question is to set the Live bit on InputGlobal. (And markLive is there to set the Live bit on InputGlobal).

The urgency/immediate reason was simply that the Live/Used bit on a symbol is the same for globals as for functions. If we continue to handle InputGlobal::Live in Writer, we'd be in the awkward situation of doing GC for global imports in a different place to the InputGlobals.

What I'll do is rebase these GC patches onto master, and see how it looks just with each change in isolation rather than as a sequence of cumulative changes.

Thanks for your patience!


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44311