[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:
> > > 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).


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44311