[PATCH] D40989: [WebAssembly] De-dup indirect function table
Sam Clegg via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 8 10:28:18 PST 2017
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
More information about the llvm-commits
mailing list