Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 20966 Build 20966: arc lint + arc unit
Event Timeline
| lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp | ||
|---|---|---|
| 208 | Doesn't seem to fail in this case. Probably worth leaving the debug print though, just changing the wording to be more accurate. | |
| 210 | Maybe debug-print something about how this branch successfully created a wrapper? We should probably print something along all the possible execution paths for this function. | |
| 211 | Should annotate the false values with the argument names, like /*SrcIsSigned = */ false | |
| test/CodeGen/WebAssembly/function-bitcasts-varargs.ll | ||
| 23 | This is probably less fragile test-wise, as well as being easier to read later. | |
| test/CodeGen/WebAssembly/unsupported-function-bitcasts.ll | ||
| 25 | Should add this test elsewhere, along with other mismatched-return tests (float-to-int comes to mind). We do support return value conversions now, so we should have tests that exercise it. | |
Not all types are valid to cast to all other types. But beyond that, normal native platforms don't insert non-trivial casts in such situations, so even in many cases where it's valid to cast, it doesn't seem like programmers could reasonably expect a non-trivial cast to happen. What kinds of use cases are motivating this?
My goal here is not so much to make it "work" as to not produce invalid webassembly. i.e. it should compile, even it if fails at runtime to do anything useful/expected.
The motivating use case is the way 'cmake' detects functions: https://github.com/Kitware/CMake/blob/master/Modules/CheckFunctionExists.c
I tries to compile this little program (but not run it) where the function in question is declared as "int XXX()" regardless of the actual signature. This fails when detecting, for example strtod becasue the resulting wasm binary doesn't validate (which means wasm-emscripten-finalize can't load it which mean a link failure for emccc).
When you say not all types are valid to cast to all others.. how would I know which are valid? And how would this code fail if they are invalid? Would llvm bail out during the call to getCastOpcode()?
I ideally we would never actually produce invalid wasm and prefer to fail to compile.. but perhaps that should be done later on during codegen rather that during this pass?
Ah, that's useful context, thanks. That'd be good to mention in the commit message and/or comments somewhere. Also, please add a testcase which approximates that use case.
getCastOpcode asserts if you ask it for something invalid. However, what about an approach like this:
- Handle the case of a pointer-to-pointer bitcast, or vector-to-vector bitcast, that we know will always be an actual no-op, and users could maybe expect that to work.
- Convert any other case to a call to @abort.
That way it'll still compile, so if I understand correctly, the CMake test will still work. Anything that actually aborts would be well into undefined behavior territory. And, this avoids our having to support the general case of non-trivial casts over the long term. Thoughts?
Interesting, so you are proposing delaying the error until link time but still producing a valid wasm binary?
What about integer conversions? Sounds like you are saying they should also abort?
Yeah. I don't always like delaying errors, but it sounds like that's what we need to make the CMake use case work, right? If so, I'm ok with that here.
What about integer conversions? Sounds like you are saying they should also abort?
Yeah. Unless I'm missing something, this should be very rare in practice, and it's certainly undefined behavior, so hopefully it won't be important.
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.
| lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp | ||
|---|---|---|
| 113 | I actually disagree that failing at runtime is preferable, I would much rather fail as early as possible :) 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. | |
I'm a bit late to this diff but I've bisected a regression in the Rust repo to this revision -- https://bugs.llvm.org/show_bug.cgi?id=38711
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.