Skip to content

target specs: stricter checks for LLVM ABI values, and correlate that with cfg(target_abi)#153769

Open
RalfJung wants to merge 3 commits intorust-lang:mainfrom
RalfJung:target-spec-abi-checks
Open

target specs: stricter checks for LLVM ABI values, and correlate that with cfg(target_abi)#153769
RalfJung wants to merge 3 commits intorust-lang:mainfrom
RalfJung:target-spec-abi-checks

Conversation

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 12, 2026

The first commit renames target.abi to target.cfg_abi to make it less likely that someone will use it to determine things about the actual ccABI, i.e. the calling convention used on the target. target.abi does not control that calling convention, it just sometimes informs the user about that calling convention (and also about other aspects of the ABI).

The second commit tightens the checks for llvm_abiname, llvm_floatabi and rustc_abi in our target specs. Those are the fields that actually control the ccABI. With this commit, we now have an allowlist of value for these fields for all architectures (we previously only had that for some architectures). We also check that cfg(target_abi) suitably correlates with the actual ccABI. I based this check on our in-tree targets. For all ccABIs where we had a bunch of "random" values that don't directly correlate to the ccABI (like "uwp"), I also allowed cfg(target_abi) to remain empty, and whenever it is allowed to be empty I also allowed arbitrary other values for JSON targets. However, there's still a risk that JSON targets will fail this new check -- the idea is that we'll then get bugreports and can adjust the check as needed.

I had to adjust the target specs for non-ARM32 Apple targets as those were all setting llvm_floatabi, which to my knowledge makes no sense -- LLVM only actually does anything with that field on ARM32. I also adjusted the target specs for MIPS32 targets: one of them was setting llvm_abiname, and then it seems safer and more consistent to set that for all targets, so I set it to "o32" everywhere which seems to be the default.

Cc @workingjubilee

@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2026

Some changes occurred in cfg and check-cfg configuration

cc @Urgau

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@rustbot rustbot added A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-apple Operating system: Apple / Darwin (macOS, iOS, tvOS, visionOS, watchOS) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 12, 2026
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 12, 2026
@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2026

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 69 candidates
  • Random selection from 16 candidates

Comment on lines +130 to +135
llvm_floatabi: if arch.target_arch() == crate::spec::Arch::Arm {
Some(FloatAbi::Hard)
} else {
// `llvm_floatabi` makes no sense on x86 and aarch64.
None
},
Copy link
Member Author

@RalfJung RalfJung Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikic just to double check -- it is correct that x86 and aarch64 ignore LLVM's FloatAbi field, right? A quick grep seems to confirm this but I don't want to accidentally break the ABI here.^^

@RalfJung RalfJung force-pushed the target-spec-abi-checks branch 2 times, most recently from ae18b24 to 7fce460 Compare March 12, 2026 11:19
"AIX targets always use the AIX ABI and `llvm_abiname` should be left empty",
check_matches!(
(&*self.llvm_abiname, &self.cfg_abi),
("", Abi::VecDefault | Abi::VecExtAbi),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly we don't seem to have any target that uses VecDefault. I just inferred from other places in the compiler that this is an AIX ABI.
@Gelbpunkt @daltenty any idea why this even exists as a possible value for cfg(target_abi)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, VecDefault is an AIX ABI. We use VecExtAbi which corresponds to the AIX Extended Altivec ABI. See https://www.ibm.com/docs/en/aix/7.1.0?topic=processor-legacy-abi-compatibility-interoperability for some more information. I think we can safely get rid of VecDefault, the baseline for the AIX targets supported includes AltiVec so I see no reason to ever use the legacy ABI (and we currently don't).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll remove it.
How would the ABI even be controlled, i.e. how would LLVM be informed about which ABI to use?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the LLVM ABI to vec-extabi will use that one, leaving it empty / at the default should be the legacy ABI as far as I can tell. LLVM's -vec-extabi flag is not the default (and neither is Clang's -mabi=vec-extabi).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not even IBM uses it for the Rustc on AIX:

bash-5.3$ rustc --print target-list | grep aix
powerpc64-ibm-aix
bash-5.3$ RUSTC_BOOTSTRAP=1 rustc -Z unstable-options --target powerpc64-ibm-aix --print target-spec-json
{
  "abi": "vec-extabi",
  "arch": "powerpc64",
  "archive-format": "aix_big",
  "binary-format": "xcoff",
  "code-model": "large",
  "cpu": "pwr7",
  "crt-objects-fallback": "false",
  "crt-static-respected": true,
  "data-layout": "E-m:a-Fi64-i64:64-i128:128-n32:64-S128-v256:256:256-v512:512:512",
  "default-dwarf-version": 3,
  "dll-suffix": ".a",
  "dynamic-linking": true,
  "eh-frame-header": false,
  "has-thread-local": true,
  "is-like-aix": true,
  "linker": "ld",
  "linker-flavor": "unix",
  "linker-is-gnu": false,
  "llvm-target": "powerpc64-ibm-aix",
  "max-atomic-width": 64,
  "metadata": {
    "description": "64-bit AIX (7.2 and newer)",
    "host_tools": false,
    "std": null,
    "tier": 3
  },
  "os": "aix",
  "pre-link-args": {
    "unix": [
      "-b64",
      "-bpT:0x100000000",
      "-bpD:0x110000000",
      "-bcdtors:all:0:s"
    ]
  },
  "pre-link-objects": {
    "dynamic-nopic-exe": [
      "/usr/lib/crt0_64.o",
      "/usr/lib/crti_64.o"
    ],
    "dynamic-pic-exe": [
      "/usr/lib/crt0_64.o",
      "/usr/lib/crti_64.o"
    ]
  },
  "target-endian": "big",
  "target-family": [
    "unix"
  ],
  "target-pointer-width": 64,
  "vendor": "ibm"
}
bash-5.3$ rustc --version
rustc 1.92.0 (afb17749a 2026-02-05) (IBM Open SDK for Rust on AIX 1.92.0.0 (5900-BND, 5765-J24))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, I'm confused now. We leave the LLVM ABI name empty, so we should actually be using the legacy ABI for codegen?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only valid LLVM ABI names for PowerPC are "elfv1" and "elfv2", as far as I can tell by grepping the LLVM source code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, looking a bit closer, we'd have to set EnableAIXExtendedAltivecABI to 1 in TargetOpts (https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/Target/TargetOptions.h#L179). We should probably do that if we're targeting AIX somewhere here: https://github.com/rust-lang/rust/blob/main/compiler/rustc_llvm/llvm-wrapper/PassWrapper.cpp#L315

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no, what a mess -- an architecture-specific field in the global TargetOptions struct?!? No idea how we'd best wire that up.
Could you file an issue? That change is definitely outside the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #153784 which I believe "fixes" this

(Some(RustcAbi::Softfloat), Abi::SoftFloat)
| (
None,
Abi::Ilp32
Copy link
Member Author

@RalfJung RalfJung Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what the "ilp32" value for aarch64 means. It sure sounds like it talks about the actual calling convention, but it does not seem to correlate with anything that would impact the actual calling convention...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems "ilp32" was introduced in c3fbafd. @joshtriplett maybe you could help us out here. :)

The value is only used by two targets, aarch64_be_unknown_linux_gnu_ilp32 and aarch64_unknown_linux_gnu_ilp32. Are we relying on LLVM implicitly configuring the ABI based on llvm_target?

Copy link
Member

@taiki-e taiki-e Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is about pointer and c_long widths and the same as target_abi="x32" in x86_64 (x86_64-unknown-linux-gnux32).

AFAIK it is common to use target_pointer_width to detect this, and the target_abi value about this is neither reliable nor convenient (e.g., non-Linux AArch64 ILP32 targets (arm64_32-apple-watchos) doesn't have this cfg, and value is different between platforms (x32 vs ilp32. mips64 n32, rv64ilp32, and ppc64 lv2 may also have different values)), so I do not feel it necessary to set target_abi for this purpose. However, @Amanieu who added AArch64 Linux ILP32 targets in #81455 or @joshtriplett who added these values in c3fbafd may have a different opinion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is about pointer and c_long widths and the same as target_abi="x32" in x86_64

Ah! That makes sense, thanks.

I don't plan to change any of the existing target_abi values in this PR, I just want to map out how we use that value and ensure we are and remain consistent about it.

@RalfJung RalfJung force-pushed the target-spec-abi-checks branch from 7fce460 to a16a988 Compare March 12, 2026 11:39
check!(self.rustc_abi.is_none(), "`rustc_abi` is unused on MIPS");
check_matches!(
(&*self.llvm_abiname, &self.cfg_abi),
("o32", Abi::Unspecified | Abi::Other(_)),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is my understanding based on reading the LLVM sources that o32 is the default value for the ABI for MIPS32, so I forced all targets to set this explicitly and updated our built-in targets accordingly. Cc @LukasWoodtli

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the n32 ABI is only intended for CPUs running in 64-bit mode, o32 is the native 32-bit CPU ABI

@RalfJung RalfJung force-pushed the target-spec-abi-checks branch 3 times, most recently from d0775f2 to a03f108 Compare March 12, 2026 13:18
@RalfJung RalfJung force-pushed the target-spec-abi-checks branch from a03f108 to 5dd6bdc Compare March 12, 2026 13:18
&& sess.target.os == Os::Windows
&& sess.target.env == Env::Gnu
&& sess.target.abi != Abi::Llvm);
&& sess.target.cfg_abi != Abi::Llvm);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I follow correctly your PR description, this and all the other conditions on cfg_abi should eventually disappear, as they do not represent the actual abi?

Copy link
Member Author

@RalfJung RalfJung Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not represent the calling convention. There are more things to an ABI. So, this check here might be entirely reasonable -- I am not sure. But for something like "are arguments passed in this or that register", cfg_abi shouldn't be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. O-apple Operating system: Apple / Darwin (macOS, iOS, tvOS, visionOS, watchOS) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants