[PATCH] D96770: [lld][WebAssembly] Fix resolveIndirectFunctionTable for relocatable output

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: lld/wasm/Driver.cpp:832
 
-  if (config->importTable) {
+  if (config->importTable || (config->relocatable && existing)) {
     if (existing)
----------------
wingo wrote:
> wingo wrote:
> > sbc100 wrote:
> > > Would it be better to ensure that `importTable` is always set when `config->relocatable`.   Anything else would be nonsensical I think.
> > > 
> > > We should also error out if `-r` is used with any of the table control options (`--export-table`, `--import-table`, `--growable-table` all make no sense with `--relocatable`.
> > > 
> > > Also why the extra `existing` here?   In `--relocatable` mode we never want to end up calling createDefinedIndirectFunctionTable so we need to make sure that this branch of the `if` I think, no?
> > I can make this change, I just thought you wanted a precise mapping between command-line args and the importTable config flag.
> > 
> > The extra "existing" is because both `importTable` and `exportTable` force the table to be present, even if not needed.  That's not what we want with `relocatable`.
> Sorry, following up here -- can't just set `importTable` when `relocatable` because that could cause the function table to be created when not needed.  Advice welcome; as is I don't see an obvious change to make.
I think if fine for `--import-table` to mean "import table if you need a table at all". 

Export table is different because the embedded may want access to the exported table.    For the import, there no point in imported a table one does not user.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96770/new/

https://reviews.llvm.org/D96770