[PATCH] D49517: [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass

[Derek Schuff via Phabricator via llvm-commits]

dschuff accepted this revision.
dschuff added a comment.

I like this approach too, assuming it's sufficient to handle the CMake (and autoconf?) ugliness. I am a bit concerned that this pass will become a large and difficult-to-maintain pile of heuristics with different purposes, so I think we should be as clear as possible about what use cases we are trying to handle.
Other than the comment nit, the code looks fine too.



================
Comment at: lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp:113
+// at runtime).  Such programs are deep into undefined behaviour territory,
+// but failing at runtime is preferable to the alternative which would be fail
+// at compiler time or to generate an invalid wasm module.
----------------
I actually disagree that failing at runtime is preferable, I would much rather fail as early as possible :)
I wouldn't say (as you do on line 117) that C in general expects to be able to compile and link programs with incorrect signatures; that's UB in C.

Having said that of course, some programmers may expect this because C has no name mangling and you can usually get away with it, and in particular CMake expects this and that's an important enough use case that we are willing to do this anyway. So we should just come out and say that in the comment :)

I do agree though that generating an invalid wasm module is a bad failure mode. Delaying the error until link time does seem preferable to that.


Repository:
  rL LLVM

https://reviews.llvm.org/D49517