[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)
----------------
sbc100 wrote:
> 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.
> 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.

So this would become:

```
// No point in importing a table if we don't have any references to one.
if (config->importTable && existing)
```

And `--relocatable` could set `config->importTable` internally?


================
Comment at: lld/wasm/Driver.cpp:837
       return createUndefinedIndirectFunctionTable(functionTableName);
   } else if ((existing && existing->isLive()) || config->exportTable) {
     // A defined table is required.  Either because the user request an exported
----------------
Can an error when `--export-table` is passed in `--relocatable` mode so we know we never get here.
Perhaps even an assert on line 838 too.    In fact, perhaps we can land a separate change that no table
flags should be passed in `--relocatable` mode?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96770