[PATCH] D85062: [WebAssembly] GC constructor functions in otherwise unused archive objects

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: lld/test/wasm/Inputs/ctor-ctor.s:13
+	.type	ctor, at function
+ctor:
+	.functype	ctor () -> ()
----------------
Can we call this something less generic like `myctor` or `test_ctor` so its not confused with maybe keyword or magic symbol?


================
Comment at: lld/test/wasm/Inputs/ctor-lib.s:5
+	.type	usethis, at function
+usethis:
+	.functype	usethis () -> ()
----------------
What do you think about renaming `usethis -> lib_func` and `bar -> unused_lib_func`?

I think you can also from the `.hidden` lines in all these tests to keep them short.


================
Comment at: lld/test/wasm/Inputs/ctor-start.s:3
+	.globl	_start
+	.type	_start, at function
+_start:
----------------
I think you can drop the `.section` and `.type` lines for functions which i think makes the tests more readable.


================
Comment at: lld/test/wasm/Inputs/ctor-start.s:6
+	.functype	_start () -> ()
+	call	usethis
+	end_function
----------------
Extra spaces there and in other `.s` files?


================
Comment at: lld/test/wasm/ctor-gc-setup.test:10
+; RUN: llvm-ar rcs %t.ctor.a %t.ctor.o
+; RUN: llvm-ar rcs %t.lib.a %t.lib.o
+; RUN: wasm-ld %t.setup.o %t.lib.a %t.ctor.a -o %t.wasm
----------------
Why not put both these in the same archive file?  Seems like it would at least make the test a little more concise.


================
Comment at: lld/wasm/InputFiles.h:63
 
+  void markLive() { live = true; }
+  bool isLive() const { return live; }
----------------
How about a comment along the lines of.  "An InputFile is considered live if any of the symbols defined by it are live"?


================
Comment at: lld/wasm/MarkLive.cpp:62
+  InputFile *file = sym->getFile();
+  bool needInitFunctions = file && !file->isLive() && sym->isDefined();
+
----------------
So here we are detecting the first time a given object is marked as live and then marking its init functions at that time.

The guess this is inherently iterative since queuing the init functions creates more marking work?   


================
Comment at: lld/wasm/MarkLive.cpp:72
+    ObjFile *obj = dyn_cast<ObjFile>(file);
+    assert(obj);
+    enqueueInitFunctions(obj);
----------------
isn't `cast<>` an equivalent way of saying these two lines?

(in which case this if body can probably just be one line)


================
Comment at: lld/wasm/MarkLive.cpp:113
+  // function.  However, this function does not contain relocations so we
+  // have to manually mark the ctors as live.
+  for (const ObjFile *obj : symtab->objectFiles)
----------------
Maybe move the above three lines of comment to the declaration of enqueueInitFunctions since it seems like a general comment not specific to this callsite.


================
Comment at: lld/wasm/Writer.cpp:1088
       // comdat exclusions can cause init functions be discarded.
-      if (sym->isDiscarded())
+      if (sym->isDiscarded() || !sym->isLive())
         continue;
----------------
I think `!sym->isLive()` is enough since all discarded symbols will be non-live.

oh. wait.. I think I forgot about `--no-gc-sections` in which case everything is live but stuff can still get discarded.  


================
Comment at: lld/wasm/Writer.cpp:1090
         continue;
       assert(sym->isLive());
       if (sym->signature->Params.size() != 0)
----------------
I guess you can drop this assert now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D85062/new/

https://reviews.llvm.org/D85062