1.18.0 armv7 regression: extern-fn-struct-passing-abi assertion failed, passed in 1.17.0

[infinity0]

Same errors on both Debian and Fedora:

failures:

---- [run-make] run-make/extern-fn-struct-passing-abi stdout ----
[..]
thread 'main' panicked at 'assertion failed: `(left == right)` (left: `FloatPoint { x: 0, y: 0.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000341032099879 }`, right: `FloatPoint { x: 5, y: -3 }`)', test.rs:115

The corresponding test succeeded in 1.17 on Debian, though it appears Fedora did not run this test (and ran fewer tests overall) for 1.17 and earlier.

cc @cuviper

[sanxiyn]

Can you try with -Z fuel=test=0? This will let us know if it is due to field reordering new in 1.18.

[infinity0]

I added this flag to src/test/run-make/extern-fn-struct-passing-abi/Makefile, I get an extra message optimization-fuel-exhausted: Reorder fields of "Rect" in the output of compiletest, but the same assertion failure still occurs.

I didn't recompile rustc itself with the flag that you mentioned, however. Did you want me to try that too?

[sanxiyn]

Hm, no. I think you just proved this is not due to field reordering.

[parched]

Those tests look like they are only for x86, so I wonder why they are being run now on arm.

[parched]

Oh actually, looking more closely they are just commented for x86 but should always be valid.

[cuviper]

There was only one commit touching librustc_trans/cabi_arm.rs between 1.17 and 1.18, f0636b6 from #40658, but that was almost a complete rewrite. Maybe nightlies before and after that merge can help confirm if that's the regression.

[cuviper]

The corresponding test succeeded in 1.17

This FloatPoint part of this test wasn't added until 0303a33, after 1.17. Using the current test sources with older stable compilers, I can reproduce the same failure back to 1.14. 1.13 had known codegen issues with armhf. In 1.12, it hits test.c:290: float_point: Assertion `p.x == 5.' failed.

Anyway, it seems this is not strictly a regression, but rather something that never worked, just wasn't tested until FloatPoint was added to the test in 1.18.

@eddyb, since you added that, any idea what could be missing on ARM?

[eddyb]

The only way to really know is to read the clang source.

[cuviper]

Looking at disassembly, the C side is expecting the struct FloatPoint parameter in VFP registers d0 and d1, and is returning the same. The Rust side is passing the arg in general r0/etc registers and a memory address for it to return to.

The ARM Procedure Call Standard 6.1.2.1 describes the homogeneous aggregate conditions for using VFP, and I found where clang checks for that. We're also already doing this for AArch64 per #24181.

I'll work on adding this for ARM hf targets.

[eddyb]

Ah, missing homogenous aggregates would explain it, and adding support for it is fine as long as there is no old version it would break for.

[cuviper]

What's the right way for cabi_arm.rs to know whether it is compiling for a VFP ABI?
In clang, ARMABIInfo has enum ABIKind for this.

[eddyb]

cc @alexcrichton @michaelwoerister for #43329 (comment) - I don't think we have a way to tell, other than by matching on the target triple or something. Maybe there's a target feature?

[alexcrichton]

AFAIK it's related to the target today, but I'll admit that my knowledge of VFP and ARM things is tenuous at best. My understand though is that:

• arm-unknown-linux-gnueabi - not VFP
• arm-unknown-linux-gnueabihf - VFP
• armv7-unknown-linux-gnueabihf - VFP

In that, I think "hf" at the end of the target indicates VFP which I'm taking as transitively implying "hard float"

[cuviper]

Shouldn't it be on a per-function basis? What if someone used extern "aapcs"?

LLVM has three ARM-specific CallingConv values:

    /// ARM_APCS - ARM Procedure Calling Standard calling convention (obsolete,
    /// but still used on some targets).
    ARM_APCS = 66,

    /// ARM_AAPCS - ARM Architecture Procedure Calling Standard calling
    /// convention (aka EABI). Soft float variant.
    ARM_AAPCS = 67,

    /// ARM_AAPCS_VFP - Same as ARM_AAPCS, but uses hard floating point ABI.
    ARM_AAPCS_VFP = 68,

AFAICS Rust only explicitly sets ARM_AAPCS, internally ArmAapcsCallConv, but I believe the default "C" calling convention for hard-float targets will be ARM_AAPCS_VFP. We probably should have an explicit extern "aapcs-vfp" too.

cc @TimNN and @japaric, as I know you two have dealt with ARM ABIs in the past...

[parched]

@alexcrichton @cuviper Yeah that's right, if it's extern "C" then it's based on the environment part of the LLVM triple. So basically if it ends in "hf" then it's aapcs-vfp otherwise it's aapcs. I guess extern "aapcs-vfp" will get added when someone needs it, extern "aapcs" was added for the compiler builtins because some functions are extern "aapcs" regardless of the target environment.

[cuviper]

OK, so for now it's roughly target_triple.ends_with("hf") && !func.is_explicitly_aapcs() -- now I'll figure out how to actually code that. :)

[nikomatsakis]

Compiler team meeting: Assigning to @eddyb since I can't assign @cuviper, but @eddyb thinks @cuviper is working on it. =)

triage: P-high -- I-wrong on P1 target.