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

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

This looks great, thanks @wingo! My only comments are about comments and error message contents. It would also be good to add tests for more of the error messages, especially the one about "Cannot preassign table number 0 to indirect function table..." because I expect that to be a common user-facing issue.



================
Comment at: lld/wasm/InputFiles.cpp:325
+// function table used by call_indirect and which is the address space for
+// function points.  If this table is present, it is always an import.  If we
+// have a file with a table import but no table symbols, it is an MVP object
----------------



================
Comment at: lld/wasm/InputFiles.cpp:350-351
+          Twine(tableSymbolCount) +
+          " symbol(s) instead.  Recompile this input with "
+          "-mattr=+reference-types.");
+    return;
----------------
I would remove the last sentence here and in the errors below. `-mattr=+reference-types` is an llc flag but not a clang flag, so it is probably not what the user wants. Also, clang should never produce objects that produce this error no matter what flags are passed to it.


================
Comment at: lld/wasm/Symbols.cpp:391
+  if (hasTableNumber()) {
+    assert(getTableNumber() == 0);
+    return true;
----------------
It would be good to add a comment here along the lines of "zero is the only table number that can be preassigned." Otherwise from just the name of the function, it looks strange that other numbers aren't handled gracefully.


================
Comment at: lld/wasm/SyntheticSections.cpp:237-238
+    // referenced by an MVP input and preassigned to table number 0 will no
+    // longer work as the imported tables get the lowest table numbers (unless
+    // for some reason the indirect function table is itself imported).
+    assert(table->getTableNumber() == 0);
----------------
The indirect function table is imported in Emscripten's dynamic linking scheme, so this isn't necessarily unexpected.


================
Comment at: lld/wasm/SyntheticSections.cpp:248-250
+            "object files compiled with -mattr=+reference-types declared "
+            "table imports.  Suggest recompiling all inputs with "
+            "-mattr=+reference-types.");
----------------
`-mreference-types` is the clang flags for enabling this feature.

Or another option would be to be tool- and flag-agnostic by saying "feature 'reference-types'" instead of naming particular flags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96001