[PATCH] D96001: [WebAssembly][lld] Preassign table number 0 to indirect function table for MVP inputs

[Andy Wingo via Phabricator via llvm-commits]

wingo marked 3 inline comments as done.
wingo added inline comments.


================
Comment at: lld/wasm/Symbols.h:382
+  void preassignTableNumberZero();
+  bool hasPreassignedTableNumberZero() const;
+
----------------
sbc100 wrote:
> I feel a little bad for pushing back more on the this change, but it still feels a little more invasive than it could be.
> 
> Rather than adding these two methods to the table class what do you think about this appoach:
> 
> - Add a global config variable that gets set if we have MVP objects in the link.   We could call this  something like`bool legacyFunctionTable = false`.   Or alternatively "forceFunctioTableToBeZero"?  Or "functionTableMustBeZero"?   I kind of like the idea of a bool here because it clearly indicates that are two different modes.
> - We already track the global symbol `indirectFunctionTable`.
> - Then, we when we comes to assigning indexes we do something like this:
> 
> ```
>    if (condig->legacyFunctionTable) {
>         placeIndirectFunctionTableFirst()   <-- here we could report errors if the sorting is impossible
>    }
> ```
> 
> This sorting and error reporting could be done in the `TableSection::assignIndexes()`.
> 
> Hopefully this should avoid adding the preassignTableNumberZero/hasPreassignedTableNumberZero methods and should avoid extra logic to too much extra logic `addTable` method.  
> 
> WDYT?
> 
> (I guess it would also need to happen in `ImportSection::assignIndexes`...)
> 
> These are mostly cosmetic changes, that actual behaviour LGTM.
> 
> 
This can work.  It's not quite as minimal as this because tables can also get assigned numbers via the import section.  I'll give it a go and see.


================
Comment at: lld/wasm/SyntheticSections.cpp:250
+            "table imports.  Suggest recompiling all inputs with "
+            "-mattr=+reference-types.");
+    }
----------------
sbc100 wrote:
> Can we be more succinct here?    If you feel its is helpful/necessary to convey all this information than perhaps this verbosity is unavoidable.  
> 
> Would something shorter such as "object file was not built -mattr=+reference-types enabled.  This is incompatible with the use tables imports in `<other-object-name>`" be enough?
We can certainly be more succinct but I don't know how to be as informative.  As the comment notes, we don't have a good way to know where the "conflicting" import comes from AFAIU.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96001