[PATCH] D90948: [WebAssembly] call_indirect issues table number relocs
Thomas Lively via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 3 11:52:52 PST 2021
tlively added a comment.
In D90948#2539419 <https://reviews.llvm.org/D90948#2539419>, @sbc100 wrote:
> In D90948#2538707 <https://reviews.llvm.org/D90948#2538707>, @wingo wrote:
>
>> OK one nit I just realized. It would be nice to allow linking MVP and reftypes inputs. However this will not be possible if the reftypes input imports a table. This is because table numbers are assigned first to imports, then to module-local definitions. The MVP input would have `call_indirect` against table 0, expecting it to be the indirect function table, but instead it is an import from the reftypes module. I guess the linker just emits an error in that case.
>>
>> It would be possible to have a command-line tool to migrate an MVP object file to a reftypes object file, parsing function code and reconstructing relocs. I guess the linker itself isn't the right place for that though.
>
> Yup that does sounds like a showstopper for linking mixed inputs. Should we have the linker error out in only cases were there are imported tables? Or should error out on any kind of mixed inputs? I guess it depends how useful linking mixed object files is, and how often folks will be using table imports.
>
> I think maybe we should start out by making mixed inputs a hard error. We could try to relax it later if there is demand. Does that sounds reasonable @tlively ?
I would prefer to error out only when a reftypes object imports a table. This seems similar to what we do when linking MVP and atomics-enabled objects, i.e. allow the link when we know it to be safe and possible. That's certainly been useful in practice, so I expect that allowing MVP+reftypes links will be useful as well as reftypes picks up adoption. Being permissive and having good error messages from the start will save us a lot of support emails and Emscripten issues down the line :) Of course this relaxation doesn't have to be in the initial patch, but I don't think we should wait to implement it until users complain.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90948/new/
https://reviews.llvm.org/D90948
More information about the llvm-commits
mailing list