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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Thanks.  Looks like a good approach.  Good that we already had support for synthetic functions.

Lets see if there is any feedback on https://github.com/WebAssembly/tool-conventions/issues/46 before we land this.



================
Comment at: test/wasm/undefined-weak-call.ll:88
+; CHECK-NEXT:       - Index:           1
+; CHECK-NEXT:         Name:            __undefined
+; CHECK-NEXT:       - Index:           2
----------------
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).


================
Comment at: test/wasm/undefined-weak-getglobal.test:7
+
+# CHECK: {{.*}}.o: undefined weak global symbol: weakGlobal
+
----------------
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?


================
Comment at: wasm/MarkLive.cpp:84
+          cast<FunctionSymbol>(Sym)->hasTableIndex())
+        continue;
+      Enqueue(Sym);
----------------
What would be the harm in marking them as live here?


================
Comment at: wasm/SymbolTable.h:52
+  ArrayRef<InputFunction *> getWeakUndefinedStubs() const
+  { return WeakUndefinedStubs; }
+
----------------
This formatting looks off (Can you run `git clang-format origin/master`?)


================
Comment at: wasm/Writer.cpp:738
   }
+  for (InputFunction *Func : Symtab->getWeakUndefinedStubs())
+    AddDefinedFunction(Func);
----------------
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?


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44028