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

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

In D96001#2541348 <https://reviews.llvm.org/D96001#2541348>, @wingo wrote:

> Looking over this changeset, I see that we need some tests.  But also there is a fair amount of code duplication for the "precoloring" logic.  I can make it be a part of TableSymbol::setTableNumber and InputTable::setTableNumber; it would seem that that much duplication is necessary.  But then the errors would probably be less helpful because there's less context at that point.  Thoughts?

IMO having helpful error messages is well worth some code duplication. Code duplication won't cause users to file the same bug every other week about their links failing, but unhelpful error messages will!



================
Comment at: lld/wasm/Driver.cpp:805
+  if (existing) {
+    if (TableSymbol *ts = dyn_cast<TableSymbol>(existing)) {
+      if (ts->hasTableNumber())
----------------
Can this `dyn_cast` ever fail? If not, it would be better to use `cast` here or even make the parameter a `TableSymbol`.


================
Comment at: lld/wasm/InputFiles.cpp:346
+    // it's new.
+    if (auto *ts = dyn_cast<TableSymbol>(s)) {
+      if (ts->hasTableNumber()) {
----------------
Same question about this `dyn_cast`.


================
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 "
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96001