[PATCH] D40989: [WebAssembly] De-dup indirect function table

[Nicholas Wilson via Phabricator via llvm-commits]

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

Fantastic! You've saved me some work, thanks. It's on my todo list for today to start splitting up the WIP Comdat reviews into smaller chunks. Plus, you've tidied it up along the way.

Could there be a test for a weakly-exported symbol, defined in two translation units, and which has its address taken in both? Something like this:

  === file 1
  define weak void @weakFn() {
  entry:
    ret void
  }
  
  define i32 @exportFn1() {
  entry:
    ret i32 ptrtoint (void ()* @weakFn to i32)
  }
  
  === file2
  
  define weak void @weakFn() {
  entry:
    ret void
  }
  
  define i32 @exportFn2() {
  entry:
    ret i32 ptrtoint (void ()* @weakFn to i32)
  }
  
  === Expected
    exportFn1() == exportFn2()



================
Comment at: wasm/InputFiles.cpp:204
+    if (Segment.Offset.Value.Int32 != 0)
+      fatal(getName() + ": unsupported segment offset");
+    TableSymbols.reserve(Segment.Functions.size());
----------------
In my patch, I'd added support for multiple element segments. It was probably a bit overkill, but the more Wasm syntax we can support, the smaller the linking conventions can be. Doesn't really matter, this is fine.

You're checking the segment offset is zero, but for completeness do you need to check `Segment.Offset.Opcode == WASM_OPCODE_I32_CONST` before reading from `Segment.Offset.Value.Int32`?

(Finally, a couple of typos: "with all symbols //that// are called indirectly", "contains more than //one// element segment".)


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D40989