Page MenuHomePhabricator

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

Authored by sbc100 on Jul 18 2018, 4:10 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Jul 18 2018, 4:10 PM
lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp
147 ↗(On Diff #156175)

Doesn't seem to fail in this case. Probably worth leaving the debug print though, just changing the wording to be more accurate.

150 ↗(On Diff #156175)

Should annotate the false values with the argument names, like /*SrcIsSigned = */ false

156 ↗(On Diff #156175)

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.

test/CodeGen/WebAssembly/function-bitcasts-varargs.ll
23 ↗(On Diff #156175)

This is probably less fragile test-wise, as well as being easier to read later.

test/CodeGen/WebAssembly/unsupported-function-bitcasts.ll
25 ↗(On Diff #156175)

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.

sunfish requested changes to this revision.Jul 19 2018, 1:06 PM

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?

This revision now requires changes to proceed.Jul 19 2018, 1:06 PM

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?

Interesting, so you are proposing delaying the error until link time but still producing a valid wasm binary?

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.

sbc100 updated this revision to Diff 158547.Aug 1 2018, 8:35 AM

gererate runtime calls to abort rather than failing to compile
add more tests

sbc100 updated this revision to Diff 158561.Aug 1 2018, 10:07 AM
  • revert
sbc100 retitled this revision from [WebAssembly] Handle return type conversions in FixFunctionBitcasts pass to [WebAssembly] Ensure all bitcasts that would be invalid in wasm are removed by FixFunctionBitcasts pass.Aug 1 2018, 3:22 PM
sbc100 added a reviewer: jgravelle-google.
sbc100 updated this revision to Diff 158651.Aug 1 2018, 3:23 PM
  • remove logging
dschuff accepted this revision.Aug 1 2018, 3:49 PM

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 ↗(On Diff #158579)

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.

sbc100 updated this revision to Diff 158792.Aug 2 2018, 10:04 AM
  • update comment
This revision was not accepted when it landed; it landed in state Needs Review.Aug 2 2018, 10:38 AM
This revision was automatically updated to reflect the committed changes.

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