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

[Nicholas Wilson via Phabricator via llvm-commits]

ncw accepted this revision.
ncw added a comment.
This revision is now accepted and ready to land.

Overall great, just some comments/questions for potential polishing.

One big thing missing - should imports be GC'ed? I would have thought so. We shouldn't import unused symbols, if the import is only used in a function that's discarded/non-live.



================
Comment at: wasm/Driver.cpp:229-230
 
+static void forEachSuccessor(InputChunk &Chunk,
+                             std::function<void(InputChunk *)> Fn) {
+  DEBUG(dbgs() << "forEachSuccessor: " << Chunk.getName() << "\n");
----------------
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.)


================
Comment at: wasm/Driver.cpp:306
+  while (!Q.empty())
+    forEachSuccessor(*Q.pop_back_val(), Enqueue);
+
----------------
Enqueue and MarkSymbol are pretty similar, do we need two nearly-identical helpers? is it marginally clearer to pass MarkSymbol into forEachSuccessor, or to get rid of MarkSymbol and simply call Enqueue directly on the Symbol's chunk in the two places where it's used? Could call "Enqueue" instead "MarkChunk".

The logic looks good, it's satisfyingly short and correct. Nice :)


================
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);
----------------
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.


================
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);
----------------
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.


================
Comment at: wasm/InputChunks.h:61
   std::vector<OutputRelocation> OutRelocations;
+  const ObjFile *File;
+
----------------
This should probably be a `const ObjFile *const` pointer if it's now public.


================
Comment at: wasm/Writer.cpp:649-650
         continue;
+      if (!Sym->getChunk()->Live)
+        continue;
       if (Sym->isGlobal())
----------------
You've said that the intention is that exported symbols should not be GC'ed.

(Aside: I think that's good, that's exactly what we want - the public interface of the Wasm module can't be GC'ed, and it's the job of the programmer to use `fvisibility=hidden` or attributes to declare their "public" symbols to the linker.)

So if exported symbols are not GC'ed, how can it be that Live is false here? It would be clearer to `assert(Sym->getChunk()->Live)` just underneath the isHidden check (a few lines down).


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D42511