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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 marked an inline comment as done.
sbc100 added a comment.

Good call regarding that test, I added it here in a separate change so we can see how this change effects it:
https://reviews.llvm.org/D41024



================
Comment at: wasm/InputFiles.cpp:204
+    if (Segment.Offset.Value.Int32 != 0)
+      fatal(getName() + ": unsupported segment offset");
+    TableSymbols.reserve(Segment.Functions.size());
----------------
ncw wrote:
> 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".)
I'd rather limit support for multiple segments or non-zero-based segments because:
a) it simpler
b) clang has no use for it yet so always writes a single segment
c) its hard to produce test inputs because of (b).
d) we can always remove restrictions later


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D40989