extern "C" functions don't generate the same IR definitions as clang on x86, causing problems with cross-language LTO

[glandium]

After llvm/llvm-project@6c8adc5, inlining in cross-language LTO happens in cases where it didn't happen before, including cases where things go very bad (more details in https://bugzilla.mozilla.org/show_bug.cgi?id=1789779#c7)

It seems to boil down to LLVM not liking that rust defines its extern "C" functions in significantly different ways than clang does for the C/C++ code that calls it. For example:

// rustc --emit=llvm-ir foo.rs --crate-type=lib --target=i686-unknown-linux-gnu
#[repr(C)]
pub struct Foo(pub u32);

#[no_mangle]
pub unsafe extern "C" fn foo(foo: Foo) -> u32 {
    foo.0
}

The caller:

// clang -o - -S foo.c -emit-llvm --target=i686-unknown-linux-gnu
struct Foo { int a; };

extern int foo(struct Foo f);

int var(struct Foo f) {
    return foo(f);
}

Rust defines the function as:

Foo = type { i32 }

; Function Attrs: nonlazybind uwtable
define i32 @foo(%Foo* byval(%Foo) %foo) unnamed_addr #0 {

while the caller C code does this:

%struct.Foo = type { i32 }
(...)
declare i32 @foo(i32) #1

The equivalent C code:

// clang -o - -S foo.c -emit-llvm --target=i686-unknown-linux-gnu
struct Foo { int a; };

int foo(Foo foo) {
    return foo.a;
}

defines the function as:

%struct.Foo = type { i32 }

; Function Attrs: noinline nounwind optnone uwtable
define dso_local i32 @foo(i32 %0) #0 {

(Ironically, rustc transforms a non-extern "C" version of the function to the same declaration as clang's)

Arguably, there's an underlying LLVM bug not being able to handle this case, which /could/ be considered fine, but I'm not sure it's supposed to be.

It's worth noting that rustc does not use a byval for e.g. x86_64-unknown-linux-gnu.

Cc: @nikic

[nikic]

Rust seems to just mark all struct arguments as byval:

rust/compiler/rustc_target/src/abi/call/x86.rs

Line 57 in 77e7e88

arg.make_indirect_byval();

While clang will pass certain aggregates directly: https://github.com/llvm/llvm-project/blob/1aaba40dcbe8fdc93d825d1f4e22edaa3e9aa5b1/clang/lib/CodeGen/TargetInfo.cpp#L1875-L1883

Another instance of @eddyb's favorite bug, #65111.

From your description, I don't really see how this used to work before the referenced LLVM change, as the ABIs are completely different -- I guess you just ended up interpreting the pointer address as the value contained in the struct and that happened to "work".

[glandium]

Before the change, the inlining wasn't happening so we ended up with a normal call, and the rust callee is just fine too because it compiled down to the proper ABI for the function. What screws everything is that inlining now happens.

[glandium]

@Gankra did you run the abi checker for some i686 targets?

[eddyb]

From your description, I don't really see how this used to work before the referenced LLVM change, as the ABIs are completely different -- I guess you just ended up interpreting the pointer address as the value contained in the struct and that happened to "work".

I believe %X* byval(%X) and %X are equivalent when stack-passing happens for the latter (IOW, my understanding is that byval forces stack-passing and then behaves like ref on Rust pattern bindings), though the pointer form may avoid some inefficiencies (and will always cause stack-passing).

IIRC x86 32-bit ABIs don't have any registers available for arguments "by default" (and the proliferation of e.g. thiscall, regcall, fastcall etc. is about adding registers at all).

---

Any ABI that doesn't use byval will effectively do quasi-SSA stack-passing once it runs out of registers, and so far I don't even know if we copied this from Clang or if rustc is uniquely inefficient for e.g. large arrays passed by value - the worst case would likely be one where the first few scalar components of the type still use registers, so the value is fragmented (with e.g. 8 registers reserved for argument-passing, I believe [usize; 9] will put the first 8 elements into one register each and then the 9th one on the stack).

Though this discussion is further complicated by:

• non-byval make_indirect, which replaces values with pointers before register-vs-stack decisions
  • i.e after exhausting registers, the stack-passing is still only for a pointer to the value, without copying the value in memory (like byval would)
  • this is used more by newer ABIs (including extern "Rust" at the moment)
  • in that case, also using byval would result in worse IR, e.g. %X** byval(%X*) when passing X
    • this is arguably additional points for never using byval for stack-passing individual scalars (which appears to be what Clang is doing here to get the simpler i32)
• cast_to, used to make the scalar components register-sized
  • e.g. turning [u8; 9] into [u32; 3] on a platform with 32-bit integers
  • this is why the example above is [usize; 9] and [u8; 9] (even though if LLVM ever saw [9 x i8], it would end up using one byte per register)

From a quick check, it seems (older) 32-bit ABIs are most susceptible to this inefficiency:

rust/compiler/rustc_target/src/abi/call/arm.rs

Lines 71 to 73 in fe217c2

	let align = arg.layout.align.abi.bytes();
	let total = arg.layout.size;
	arg.cast_to(Uniform { unit: if align <= 4 { Reg::i32() } else { Reg::i64() }, total });

rust/compiler/rustc_target/src/abi/call/sparc.rs

Lines 24 to 26 in fe217c2

	if arg.layout.is_aggregate() {
	let pad_i32 = !offset.is_aligned(align);
	arg.cast_to_and_pad_i32(Uniform { unit: Reg::i32(), total: size }, pad_i32);

rust/compiler/rustc_target/src/abi/call/mips.rs

Lines 24 to 26 in fe217c2

	if arg.layout.is_aggregate() {
	let pad_i32 = !offset.is_aligned(align);
	arg.cast_to_and_pad_i32(Uniform { unit: Reg::i32(), total: size }, pad_i32);

Even a 64-bit one (I assume because it's still an old ABI):

rust/compiler/rustc_target/src/abi/call/powerpc64.rs

Line 114 in fe217c2

arg.cast_to(Uniform { unit, total });

MIPS64 is also affected, but only for structs, not arrays (which makes me wonder if we're actually currently wrong for "structs that contain large arrays").

---

Given sufficient ABI information, LLVM itself should be able to "relax" byval but I doubt "explicit -> implicit" strength reductions like that would be uncontroversial.

This is one of my gripes with LLVM and ABI: it forces the frontend to think about ABI distinctions like:

• "registers vs stack" (byval)
• "register bank" (int/pointer scalars, float scalars, vectors)
• "register assignment variations" (only some ABIs: inreg, swift{error,context}, etc.)

... but only some of the time, not all of the time, and that leads to both multiple ways to represent certain behaviors (like the one discussed here, with byval vs immediate-passing), and also being forced to use convoluted conversions (like what cast_to causes) in other cases (so LLVM optimizations like inlining have to work extra hard to remove the RustType -> [usize; N] -> RustType roundtrip).

I recently wrote something related to this (Ctrl+F "ABI mapping" in #100698 (comment)), and I still believe it would be one of the nicer solutions overall: decouple "stack slots and SSA scalars/vectors" call dataflow (friendlier to IPO in general - e.g. inlining, but not only) from exporting a function with specific register/stack assignments.

[nikic]

I believe %X* byval(%X) and %X are equivalent when stack-passing happens for the latter (IOW, my understanding is that byval forces stack-passing and then behaves like ref on Rust pattern bindings), though the pointer form may avoid some inefficiencies (and will always cause stack-passing).

Thanks, this is the bit I was missing! The ABIs are incompatible at the LLVM IR level, but become compatible post-legalization.

That does limit the issue to the cross-language LTO case only.

Given sufficient ABI information, LLVM itself should be able to "relax" byval but I doubt "explicit -> implicit" strength reductions like that would be uncontroversial.

LLVM will generally try to relax byval(%T) into individual SROAd %T arguments as part of argument promotion -- but this is only for functions with internal linkage, not for the case which you're talking about (where I doubt such a transform would be legal, because it makes the call UB at the IR level due to ABI mismatch).

The rest

Yes, LLVM's ABI handling is a big pile of poo ... your suggestion does sound reasonable on the surface, but I'm not volunteering to implement it :P

Regardless of the specific IR representation, something that would help a lot is to extract Clang's ABI calculation into something reusable by other frontends (discussed a bit at https://discourse.llvm.org/t/rfc-targetinfo-library/64342, though I doubt something will come of that). While rustc couldn't use that directly (due to other codegen backends), at least it could be used to our computed ABI is correct.

[eddyb]

something that would help a lot is to extract Clang's ABI calculation into something reusable by other frontends

Heh, something like that is why rustc_target avoids dependencies on the rest of the compiler, and is generic over the concept of a semantic type and contexts that can provide more information about those semantic types (tho recent changes motivated by performance have been compromising a bit of it, you still only need an arena to use the facilities AFAICT).

My long-term hopes with rustc_target::abi::call was to split "classifying an input/output type" from "assign registers/stack to an input/output" (the latter could be done using declarative information in "classifications"), but I never got around to spend more time on it.

^{(the rough idea is that once you reduce a type and its layout to the set of possible ABI encodings that apply, it's a "resource calculus" from there on, like the "stateful" part of these algorithms ~always looks like "number of remaining registers per bank" or "stack offset/misalignment", and all the decisions are like "do I fit", i.e. "are there available resources for me to consume")}

But for rustc_target to be really good at this, we'd have to go all the way to exact register/stack mappings (which e.g. the Cranelift backend could maybe use directly) and then "reverse engineer" that back into LLVM IR (and if LLVM is willing to expose its own decisions back to us, we could check our own understanding - right now AFAIK this would only be possible by abusing assembly or DWARF, and that's not great).

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high regression-untriaged