[PATCH] D44028: [WebAssembly] Handle weak undefined globals and functions

[Nicholas Wilson via Phabricator via llvm-commits]

ncw marked 2 inline comments as done.
ncw added inline comments.


================
Comment at: test/wasm/undefined-weak-call.ll:88
+; CHECK-NEXT:       - Index:           1
+; CHECK-NEXT:         Name:            __undefined
+; CHECK-NEXT:       - Index:           2
----------------
sbc100 wrote:
> Is it OK that all these will have the same name?
> 
> Perhaps its worth adding a test for two function with the same sig?   And two functions with different sigs?  (Either in this test file or separate).
Yes it should be OK. I'll check in a test though, you're right it's necessary.


================
Comment at: test/wasm/undefined-weak-call.test:2
+# RUN: yaml2obj %s > %t.o
+# RUN: not lld -flavor wasm --no-entry %t.o -o %t.wasm 2>&1 | FileCheck %s
+
----------------
ruiu wrote:
> Please use wasm-ld. `lld` as a command is essentially deprecated.
Already done in one case, I'll squash the other too. Thanks!

I've got rid of one of the yaml2obj uses, I'll get rid of the other based on Sam's improvement to push the check back into WasmObjectFile.


================
Comment at: test/wasm/undefined-weak-getglobal.test:7
+
+# CHECK: {{.*}}.o: undefined weak global symbol: weakGlobal
+
----------------
sbc100 wrote:
> Shall we make this even stronger and say that WEAK undefined symbols are not allowed for global at all, then we can put this check in the object file reader and we would need this check or this test in lld?
> 
> There is no point in having WEAK undefined symbols if they can't actually undefined at link time is there?  Then it might as well be strong?
D'oh, of course, I somehow rationalised it to myself but yes, there's no point allowing the Weak flag on undefined globals at all, if it's only legal to link when the symbol is actually defined.

Then we'll all be happy because this test case can go, and we won't need to use yaml2obj, etc.


================
Comment at: wasm/MarkLive.cpp:84
+          cast<FunctionSymbol>(Sym)->hasTableIndex())
+        continue;
+      Enqueue(Sym);
----------------
sbc100 wrote:
> What would be the harm in marking them as live here?
If they were marked live, they'd be included in the final binary even if never called. Just harmless bloat. Some of our existing tests do this - only ever call a weak undefined indirectly via its address. Since the address evaluates to zero, the stub is as dead as can be and seems a reasonable candidate for GC.


================
Comment at: wasm/Writer.cpp:738
   }
+  for (InputFunction *Func : Symtab->getWeakUndefinedStubs())
+    AddDefinedFunction(Func);
----------------
sbc100 wrote:
> Is it worth making this `getSyntheticFunctions`, and then we can add the __wasm_call_ctors function to this list and it will get added via `AddDefinedFunction` too?
With some wider refactoring... there's a chicken-and-egg problem though.

Building the body (InputFunction) for CallCtors happens at the very end, since it doesn't have relocations and so needs all the function indexes to have been previously assigned. Hence we can't have the InputFunction for CallCtors available here in Writer::assignIndexes, where the indexes are set for the first time!

CallCtors is naturally a "late defined" synthetic (again assuming it's generated without relocs), and the weak stubs are naturally "early defined" (because to me it makes most sense to build them just before calling reportRemainingUndefines, since they do similar things).

If you want CallCtors to be processed via Writer::assignIndexes, then we'd have to generate the CallCtors body much earlier and actually generate relocs for it. That might actually be the better option - but it's for another commit.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44028