[PATCH] D96001: [WebAssembly][lld] Precolor table numbers for MVP inputs

[Andy Wingo via Phabricator via llvm-commits]

wingo added a comment.

Thanks very much for the review!

The "precoloring" name is a bit sloppy -- comes from register allocation FWIW, where you solve the graph coloring problem for the interference graph of register uses.  Some nodes require that a register be in a specific place, e.g. `%rdi` for argument 0 on x86; imposing that constraint is called precoloring.  So for tables, we also have a kind of precoloring constraint.  But, probably best to just used the term "preassign".



================
Comment at: lld/wasm/Driver.cpp:805
+  if (existing) {
+    if (TableSymbol *ts = dyn_cast<TableSymbol>(existing)) {
+      if (ts->hasTableNumber())
----------------
tlively wrote:
> Can this `dyn_cast` ever fail? If not, it would be better to use `cast` here or even make the parameter a `TableSymbol`.
I was thinking that it could fail if the existing symbol isn't a table.  In that case we would have already issued an error, but control flow could still fall through here.  But now I see that we make an early-return in that case below; will fix.


================
Comment at: lld/wasm/InputFiles.cpp:346
+    // it's new.
+    if (auto *ts = dyn_cast<TableSymbol>(s)) {
+      if (ts->hasTableNumber()) {
----------------
tlively wrote:
> Same question about this `dyn_cast`.
Unfortunately it is possible that e.g. `SymbolTable::createUndefinedTable` returns a non-table symbol, if there was an existing symtab entry.  We would have issued an error at that point though.


================
Comment at: lld/wasm/InputFiles.cpp:349
+        if (ts->getTableNumber() != tableNumber)
+          error(toString(this) + ": incompatible precoloring");
+      } else {
----------------
sbc100 wrote:
> I'm not sure this is a very usefull user-facing error.    Can it ever actually happen?   Can we improve it or perhaps make it an assert?
This is a sloppy error and I need to reword it; tx for catching.


================
Comment at: lld/wasm/InputFiles.cpp:351
+      } else {
+        ts->setTableNumber(tableNumber);
+      }
----------------
sbc100 wrote:
> Can we simplify the code here by asserting that the table number is always zero and that there is only one table.
> 
> In fact, perhaps this should be called `synthesizeTableSymbol` (non-plural) and just return after for first call to `addTable`?
Humm, good question!  Old linkers would not propagate "additional" tables from input to output.  Also, old compilers wouldn't define a table locally -- the indirect function table would only be imported.  So maybe it would be good to avoid adding a new form of acceptable input, in which object files have many table symbols.

If we tighten this restriction, we can issue an error for inputs that
 - have no table symbols. and:
   - import any table not named __indirect_function_table, or
   - import more than one table, or
   - define any table

WDYT?


================
Comment at: lld/wasm/SymbolTable.cpp:213
+                                uint32_t newNumber) {
+  if (TableSymbol *ts = dyn_cast<TableSymbol>(existing)) {
+    if (!ts->hasTableNumber()) {
----------------
sbc100 wrote:
> I think this can be an unconditionally `cast<>` can't it?
AFAICS, sadly, no :(  Otherwise we could simplify `checkTableType` as well.


================
Comment at: lld/wasm/SymbolTable.cpp:219
+            "\n>>> assigned nonrelocatable table number " +
+            Twine(ts->getTableNumber()) + " in " + toString(ts->getFile()) +
+            "\n>>> assigned nonrelocatable table number " + Twine(newNumber) +
----------------
sbc100 wrote:
> Similarly, since there is exactly one nonrelocatable table number (zero)... can we simplify this code based on that fact?
Sure.  I can add a `isPreassignedToTableNumberZero` method or so.  Surely with a better name.


================
Comment at: lld/wasm/SyntheticSections.cpp:118-119
+    if (t->getTableNumber() != tableNumber)
+      error("failed to assign table number " + Twine(tableNumber) +
+            " for table " + Twine(t->getTableNumber()) +
+            "; try recompiling all inputs "
----------------
tlively wrote:
> Would it be possible to include the name of an object file that contains the offending non-relocatable table? I've anecdotally found that including file names in similar error messages about unlinkable MVP and atomics objects has dramatically decreased the number of bugs that get filed about it.
Sure.


================
Comment at: lld/wasm/SyntheticSections.cpp:121
+            "; try recompiling all inputs "
+            "with -mattr=+reference-types");
+  } else {
----------------
sbc100 wrote:
> Can this error actually happy?    Wouldn't it be a bug in lld if it did?   It seems like in order for this to happen there would be a bug in the pre-coloring logic.
I was thinking that it could happen if you had an MVP input that defined a table locally (not imported), but a reftypes input imported a table.  But prompted by your questions I think I see that MVP inputs wouldn't ever define tables, so this error can't happen.


================
Comment at: lld/wasm/Writer.cpp:597
+      continue;
+    if (auto *ts = dyn_cast<TableSymbol>(sym)) {
+      if (!ts->hasTableNumber()) {
----------------
tlively wrote:
> It would be good to have a short comment here as well about why some tables need to be deferred.
Sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96001