tests: FiveU16s extern ABI test fails on big endian PPC64 ELFv2 (musl)

[awilfox]

I tried this code:

        RUST_BACKTRACE=1 \   
        python3 x.py test -j 64 tests/ui

I expected to see this happen: test suite passing

Instead, this happened:

failures:
    [ui] tests/ui/abi/extern/extern-return-FiveU16s.rs
    [ui] tests/ui/abi/extern/extern-pass-FiveU16s.rs

The return failure is:

thread 'main' panicked at /home/awilcox/Code/contrib/rust/tests/ui/abi/extern/extern-return-FiveU16s.rs:20:9:
assertion `left == right` failed
  left: 40
 right: 10
stack backtrace:
   0: rust_begin_unwind
             at /home/awilcox/Code/contrib/rust/library/std/src/panicking.rs:662:5
   1: core::panicking::panic_fmt
             at /home/awilcox/Code/contrib/rust/library/core/src/panicking.rs:74:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
   4: extern_return_FiveU16s::main

The pass failure is:

thread 'main' panicked at /home/awilcox/Code/contrib/rust/tests/ui/abi/extern/extern-pass-FiveU16s.rs:28:9:
assertion `left == right` failed
  left: FiveU16s { one: 22, two: 23, three: 24, four: 25, five: 26 }
 right: FiveU16s { one: 25, two: 26, three: 0, four: 0, five: 0 }
stack backtrace:
   0: rust_begin_unwind
             at /home/awilcox/Code/contrib/rust/library/std/src/panicking.rs:662:5
   1: core::panicking::panic_fmt
             at /home/awilcox/Code/contrib/rust/library/core/src/panicking.rs:74:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
   4: extern_pass_FiveU16s::main

Meta

rustc --version --verbose:

rustc 1.82.0-nightly (249cf71f1 2024-07-30) (Adélie 1.82.0 EXPERIMENTAL)
binary: rustc
commit-hash: 249cf71f11a29b3fb68e8a35969569d8bb7958ee
commit-date: 2024-07-30
host: powerpc64-foxkit-linux-musl
release: 1.82.0-nightly
LLVM version: 18.1.8

This occurs in every version since the test was added. I see this with 1.79.0 and 1.80.0 as well as my 1.81.0 beta build (rustc 1.81.0-beta.2 (08328a323 2024-07-25) (Adelie 1.81.0_beta20240730-r0 [BETA])).

I haven't dug in to all of the nitty-gritty yet, but I have a feeling

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

Lines 72 to 73 in 0514789

	let unit = if cx.data_layout().endian == Endian::Big {
	Reg { kind: RegKind::Integer, size }

may be potentially incorrect?

[awilfox]

Indeed, if I remove those two lines, it passes the tests correctly:

--- a/compiler/rustc_target/src/abi/call/powerpc64.rs
+++ b/compiler/rustc_target/src/abi/call/powerpc64.rs
@@ -69,9 +69,7 @@ fn classify_ret<'a, Ty, C>(cx: &C, ret: &mut ArgAbi<'a, Ty>, abi: ABI)
     let size = ret.layout.size;
     let bits = size.bits();
     if bits <= 128 {
-        let unit = if cx.data_layout().endian == Endian::Big {
-            Reg { kind: RegKind::Integer, size }
-        } else if bits <= 8 {
+        let unit = if bits <= 8 {
             Reg::i8()
         } else if bits <= 16 {
             Reg::i16()

But I'm not sure why they are there and if this could possibly cause breakage elsewhere. I'd be happy to open a PR with this change for comment if that is the best way forward, or we can continue to discuss here.

[awilfox]

Indeed, there is fallout - this test used to pass:

failures:
    [run-make] tests/run-make/extern-fn-struct-passing-abi

test result: FAILED. 284 passed; 1 failed; 69 ignored; 0 measured; 0 filtered out; finished in 70.17s

thread 'main' panicked at test.rs:155:9:
assertion `left == right` failed
  left: IntOdd { a: 0, b: 1, c: 2 }
 right: IntOdd { a: 1, b: 2, c: 3 }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
make: *** [Makefile:6: all] Error 1

[awilfox]

Reading over the ELF ABI specification and how LLVM and GCC implement this, arguments and return values should be treated the same. So, trying to duplicate the logic from classify_arg in classify_ret yielded me:

--- a/compiler/rustc_target/src/abi/call/powerpc64.rs
+++ b/compiler/rustc_target/src/abi/call/powerpc64.rs
@@ -69,19 +69,15 @@ fn classify_ret<'a, Ty, C>(cx: &C, ret: &mut ArgAbi<'a, Ty>, abi: ABI)
     let size = ret.layout.size;
     let bits = size.bits();
     if bits <= 128 {
-        let unit = if cx.data_layout().endian == Endian::Big {
-            Reg { kind: RegKind::Integer, size }
-        } else if bits <= 8 {
-            Reg::i8()
-        } else if bits <= 16 {
-            Reg::i16()
-        } else if bits <= 32 {
-            Reg::i32()
+        if bits <= 64 {
+            ret.cast_to(Uniform::new(Reg { kind: RegKind::Integer, size }, size))
         } else {
-            Reg::i64()
+            let reg = if ret.layout.align.abi.bytes() > 8 { Reg::i128() } else { Reg::i64() };
+            ret.cast_to(Uniform::consecutive(
+                reg,
+                size.align_to(Align::from_bytes(reg.size.bytes()).unwrap()),
+            ))
         };
-
-        ret.cast_to(Uniform::new(unit, size));
         return;
     }
 

and gave me:

...
test [run-make] tests/run-make/extern-fn-struct-passing-abi ... ok
...
test [ui] tests/ui/abi/extern/extern-return-TwoU16s.rs ... ok
test [ui] tests/ui/abi/extern/extern-pass-FiveU16s.rs ... ok
...
Build completed successfully in 0:02:44

which is actually the first time I've ever seen Rust complete its test suite fully on a big endian PPC64/musl environment!

Since the logic doesn't vary between endian in classify_arg, I'm not sure why it should in classify_ret. Unfortunately, I don't have any spare hardware to try and run little-endian, so I can't test if this change actually does the right thing on LE.

I suppose since the original code had a BE case which was suspiciously similar to the first half of classify_arg, I could do the 128 >= size > 64 code as another branch there. But that feels ugly, and I'd like to know if this simpler way would work on both endians before trying to shoehorn deeper nested branches.