[PATCH] D44028: [WebAssembly] Handle weak undefined functions with a synthetic stub

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: wasm/MarkLive.cpp:78-85
+      Symbol *Sym = C->File->getSymbol(Reloc.Index);
+      // Don't mark functions live if we're taking the address of a function
+      // that won't actually go in the function table (has index zero).  This
+      // is the case for some synthetic functions.
+      if ((Reloc.Type == R_WEBASSEMBLY_TABLE_INDEX_SLEB ||
+           Reloc.Type == R_WEBASSEMBLY_TABLE_INDEX_I32) &&
+          cast<FunctionSymbol>(Sym)->hasNullTableIndex())
----------------
ruiu wrote:
> ncw wrote:
> > sbc100 wrote:
> > > ruiu wrote:
> > > > Adding a special rule for a weak symbol seems a bit odd to me because they are orthogonal in concept. What if you create functions after you garbage-collect symbols? Then you can remove this logic, can't you?
> > > I also find this part of the change a little bit strange.   And I think the hasNullTableIndex() method helps to describe what is going on here.  However, inlineing hasNullTableIndex() along with a good comment is OK too.
> > It's not a super-special rule: it's generic in the sense that it would apply to any symbol that had the (weird) property of needing to compare equal to the null pointer. It's not checking for weakness, it's checking something that's actually relevant to the relocation we're considering here. I agree it's not ideal, but I think it's OK (there's already a decent comment above).
> I'm little confused -- why do we need this in the first place? Even if a weak symbol is resolved to function table index zero, you can still call it, and if you call it, it should abort with UB, no?
This is an GC optimization, for the case where such as function is address taken but never called,  in that case we want to GC the dummy function (which is why we don't mark it live here).   Normally one cannot determine that address taken functions are never called.. but in this case we can.

I you think it makes sense,  I'd be OK with forgoing this optimization for the sake of clarity, since its hard to imagine  a case where it would make a big difference to codesize.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D44028