Audit ABI implementations in rustc_target (vs Clang).

[eddyb]

In #63649 (comment) I wrote:

The wasm-bindgen mismatch could've been avoided, had we validated the call ABI by comparing it with what Clang does, when it was initially implemented (to be clear, I think I dropped the ball here).

Irrespective of what happens to this PR, I think we should do an audit of all our call ABIs, and institute a policy to prevent this from happening again in the future.
cc @rust-lang/compiler @rust-lang/wg-codegen

The context there was that our wasm32 ABI has been wrong since it was added, and wasm-bindgen now relies on the wrong ABI.

I opened this issue so we don't lose track of this, because I'm sure I will.

[nagisa]

If my memory serves me right, I asked about a formal ABI specification back when this target was being added and there wasn’t one at the time so we ended up with something implementation defined. Implementation of the ABI in Clang is not a formal spec, either, so I don’t see much purpose in changing one implementation-defined ABI to another one.

Maybe makes sense for other formally specified ABIs, though.

[joshtriplett]

Implementation of the ABI in Clang is not a formal spec, either, so I don’t see much purpose in changing one implementation-defined ABI to another one.

Interoperability between the two?

[eddyb]

More data points:

• mips64 has an impossible-for-C-FFI rule about arrays, see Rust arrays on C FFI are super confusing #58905 (comment)
• the entirety of PowerPC C ZST ABI fixes #64259, where the GNU C extension for ZSTs ended up being discussed a bunch
  • just found that trying to pass a ZST crashes clang for SPARC64: https://godbolt.org/z/uzbPMP
• MSP430 doesn't match GCC, haven't looked at clang yet
  • according to the ABI spec (thanks @gnzlbg), GCC is wrong?
  • however, it doesn't use the "cast" functionality so it's possible that trying to pass a (u8, u8) on MSP430 would use two registers simply by letting LLVM see aggregate fields
    • this may be a problem for many other ABIs, we need to investigate more thoroughly, and maybe make the "cast" functionality of rustc_target::abi::call mandatory for multi-scalar aggregates passed directly, to avoid that footgun

[eddyb]

Here's one more:

• we always treat returning ZSTs like void-returning functions in C, but is that correct? Types equal to/wrapping () result in "not FFI-safe" warning #66202 (comment)

[eddyb]

• non-C-like enums weren't implemented, e.g. in x86_64 (SysV): SystemV ABI Mismatch on x86 with a repr(C) enum for extern "C"/FFI functions. #68190 (comment)

[Gankra]

While looking into #54341 I wrote a tool for actually generating+compiling+running+checking rust<->C ffi interfaces:

https://github.com/Gankra/abi-checker

It needs some polish and UX work for it to be totally useful in the context of automated tooling/reporting, but I think it's a pretty useful baseline for just objectively verifying whether C and Rust agree on how different concepts get passed across the FFI boundary. I tried to document/comment it a bunch in case anyone finds it useful to extend and iterate on.

[eddyb]

We should probably run @Gankra's tool instead of our own (less comprehensive) tests. cc @rust-lang/compiler

Anyway came here to add one more to the pile:

• SPARC64 aggregate recursion code gets offsets wrong in several ways (I'm surprised the test added in that PR even works) more complete sparc64 ABI fix for aggregates with floating point members #94216 (review)

[Gankra]

I filed a few new issues on abi-checker and made a metabug for what needs to be done to get this up to snuff for y'all. Please feel free to drive-by it with whatever you want, and to file more issues on the repo. This is not my job atm, but if there's enough interest I can maybe make an argument to mozilla to allocate some resources to it.

Metabug: things rust-lang needs/wants to use this in CI

[Gankra]

Just a little update on abi-checker:

• We now set the exit code if any tests fail
• We now have a system for specifying test expectations
• We now have machine-readable output via either --output-format=json (very verbose megareport) or --output-format=rustc-json (uses libtest's format, kinda jank)
• rustc_codegen_cranelift now integrates abi-checker into its own CI

All the major issues I could think of are now at least minimally resolved, and integrating this tool into rustc's CI should be relatively low-effort (see the above link to how rustc_codegen_cranelift did it).

[Gankra]

Possibly one annoyance remaining is that we currently use rustc by literally invoking Command::new(rustc), which works well for a rustup user but may be annoying for rustc itself. I can potentially change this to accept some sort of configuration if desired. Or I could use the RUSTC that built picks up. It's already mildly important that abi-checker itself and the tests it builds are themselves abi-compatible, because the former dlopen's the latter. However the tests are compiled as c staticlibs that get linked into cdylibs, so the ABI is as stable as it gets.