[PATCH] D42511: [WebAssembly] Add support for --gc-sections

[Sam Clegg via Phabricator via llvm-commits]

sbc100 marked 4 inline comments as done.
sbc100 added inline comments.


================
Comment at: wasm/Driver.cpp:229-230
 
+static void forEachSuccessor(InputChunk &Chunk,
+                             std::function<void(InputChunk *)> Fn) {
+  DEBUG(dbgs() << "forEachSuccessor: " << Chunk.getName() << "\n");
----------------
ncw wrote:
> Is it better LLVM style to make this a template on the Fn type, to avoid forcing the lambda to be wrapped in a std::function? (I think std::function creates some extra virtual wrapper objects.)
I basically copied this directly from the ELF linker, but I'm open to changing it.

@ruiu what do you think about this?

I was thinking we might as well make this a local lamba within `markLive()` that always calls `Enqueue`?  Right now is needlessly generic.  Then we could just call it `EnqueueSuccessors()`


================
Comment at: wasm/Driver.cpp:344-345
   Config->Entry = getEntry(Args, Args.hasArg(OPT_relocatable) ? "" : "_start");
+  errorHandler().FatalWarnings =
+      Args.hasFlag(OPT_fatal_warnings, OPT_no_fatal_warnings, false);
   Config->ImportMemory = Args.hasArg(OPT_import_memory);
----------------
ncw wrote:
> This patch doesn't seem to add any warnings? I don't see any harm to this - but if it's set, do we now need to remember to add `if (errorCount()) return` after any places where we emit warnings? I can't think of any I've noticed many/any warnings Wasm-LLD though.
Sorry this should be a separate change..


================
Comment at: wasm/Driver.cpp:415-419
+    if (auto *Obj = dyn_cast<ObjFile>(F)) {
+      for (InputChunk *C : Obj->Functions)
+        InputChunks.push_back(C);
+      for (InputChunk *C : Obj->Segments)
+        InputChunks.push_back(C);
----------------
ncw wrote:
> I see above that you've made it so that `-r` can't be used with GC.
> 
> But, for avoidable of future bugs, it might wise to do an `InputChunks.push_back(CtorFunction)` where we create, and ditto in future for any other "synthetic" input chunks. Otherwise, it's too tempting to assume that InputChunks contains all the input. Or put another way, now that you have InputChunks as a global variable we may as well make it more generally useful by putting the CtorFunction in it as well, even though GC isn't currently used with the CtorFunction.
Actually I think I might just remove InputChunks, this change doesn't justify its existence.  I was copying it from the ELF linker which makes more use of it.


================
Comment at: wasm/InputChunks.h:61
   std::vector<OutputRelocation> OutRelocations;
+  const ObjFile *File;
+
----------------
ncw wrote:
> This should probably be a `const ObjFile *const` pointer if it's now public.
lld doesn't seem to be reaching for that level of const correctness so far (at least I dont' see any `* const` pointers yet


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42511