This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Narrow the scope of WebAssemblyFixFunctionBitcasts
ClosedPublic

Authored by jgravelle-google on Oct 6 2017, 11:36 AM.

Details

Summary

The pass to fix function bitcasts generates thunks for functions that
are called directly with a mismatching signature. It was also generating
thunks in cases where the function was address-taken, causing aliasing
problems in otherwise valid cases.
This patch tightens the restrictions for when the pass runs.

Event Timeline

dschuff added inline comments.Oct 6 2017, 4:38 PM
lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp
73

How about invoke? Also, are we filtering out intrinsics anywhere? (An IntrinsicInst is a CallInst but I forget if its opcode is Instruction::Call or not)

sunfish edited edge metadata.Oct 6 2017, 4:46 PM

Is the assumption here that the eventual caller will cast the argument back to the original type?

For example, in test_argument, if call_func bitcasts the i32 ()* argument to void (i32)* before calling it, then it doesn't want a wrapper. However, if it doesn't do that cast, and just calls it as i32 ()*, then it does seem to need the wrapper. So I think it's a guess either way. Do you think functions doing an extra cast are more common that functions that don't?

Is the assumption here that the eventual caller will cast the argument back to the original type?

Pretty much. In C, that is explicitly defined to be well-defined, and otherwise it's undefined behavior. Native targets ignore missing args and/or read junk bits from the argument registers, wasm traps at runtime, or with this pass we synthesize/ignore args.

For example, in test_argument, if call_func bitcasts the i32 ()* argument to void (i32)* before calling it, then it doesn't want a wrapper. However, if it doesn't do that cast, and just calls it as i32 ()*, then it does seem to need the wrapper. So I think it's a guess either way. Do you think functions doing an extra cast are more common that functions that don't?

And if call_func stores it to memory, or prints it to screen, or compares it to the pointer value of has_i32_arg, we have no idea. Because it's a guess either way, and because we don't have a way to reverse the bitcast-function, I think the most reasonable thing to do is nothing.
Functions doing an extra cast are probably equally common (I have no data for this), but more importantly they're not-undefined, so I think we should make sure to support those, and any undefined use cases that we can support are a bonus.

lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp
73

Not sure, will check and add tests

That makes sense. This patch looks good to me, with Invokes handled.

lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp
73

The CallSite class is one way to handle both Calls and Invokes at the same time.

Also, it's invalid to do anything with the address of an intrinsic except directly call it, so we shouldn't have to worry about them.

  • Add invoke test
dschuff accepted this revision.Oct 9 2017, 12:46 PM
This revision is now accepted and ready to land.Oct 9 2017, 12:46 PM
  • Use CallSites, properly test exceptions

Cool, and the code is even shorter this way too πŸ‘

This revision was automatically updated to reflect the committed changes.