[PATCH] D90428: [WebAssembly] Improved LLD error messages in case of mixed wasm32/wasm64 object files

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 5 inline comments as done.
aardappel added inline comments.


================
Comment at: lld/wasm/InputFiles.cpp:51
+    fatal(toString(file) +
+          ": wasm32 object file can\'t be linked in wasm64 mode");
+  }
----------------
sbc100 wrote:
> aardappel wrote:
> > sbc100 wrote:
> > > No need to escape single quote is there?
> > > 
> > > Can we get tests for these new error cases?
> > You want a test that builds two different .o files and tries to link them? Doesn't that sound a bit overkill for this? Do all LLD errors have tests? Certainly the ones we already had that I am replacing didn't.
> You thats exactly what I mean.  If the existing error message didn't have tests they should have done. 
> 
> It should be trivial to just use and empty `.s` file as the test,  since all you need to do is compile it two ways and try to link it two ways to get each error.
> 
> Or you can use a `.test` that references and existing `.s` such as `lld/test/wasm/Inputs/start.s`
well, it can't be empty since it


================
Comment at: lld/wasm/InputFiles.cpp:600
   }
-  bool is64 = t.getArch() == Triple::wasm64;
-  if (config->is64.hasValue() && *config->is64 != is64) {
-    error(toString(this) + ": machine type for all bitcode files must match");
-    return;
-  }
-  config->is64 = is64;
+  checkArch(t.getArch(), this);
   std::vector<bool> keptComdats;
----------------
sbc100 wrote:
> Passing `this` at each callside makes me think this should be a method, no?
I considered that, but then I made it similar to the already existing `toString`, as an implementation detail local to this file seemed cleaner to me. Also pulls a new include into the header. But as you wish.


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

https://reviews.llvm.org/D90428