Skip to content

c-variadic: error when we can't guarantee that the backend does the right thing #152935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
folkertdev wants to merge 1 commit into
base: main
Choose a base branch
from
Open

c-variadic: error when we can't guarantee that the backend does the right thing #152935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_ast_passes/src/ast_validation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,7 @@ impl<'a> AstValidator<'a> {
match fn_ctxt {
FnCtxt::Foreign => return,
FnCtxt::Free | FnCtxt::Assoc(_) => {
if !self.sess.target.arch.supports_c_variadic_definitions() {
if !self.sess.target.supports_c_variadic_definitions() {
self.dcx().emit_err(errors::CVariadicNotSupported {
variadic_span: variadic_param.span,
target: &*self.sess.target.llvm_target,
Expand Down
7 changes: 4 additions & 3 deletions compiler/rustc_codegen_llvm/src/va_arg.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1186,9 +1186,10 @@ pub(super) fn emit_va_arg<'ll, 'tcx>(
// Clang uses the LLVM implementation for these architectures.
Copy link
Member

@RalfJung RalfJung Feb 21, 2026

Choose a reason for hiding this comment

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

Is this "clang does it so it's probably fine" or "people familiar with the target have confirmed it's fine"?

Copy link
Contributor Author

@folkertdev folkertdev Feb 21, 2026

Choose a reason for hiding this comment

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

It is "clang does this so we're matching the de-facto standard C compiler that rust code would interact with". We could pull that implementation into rustc as well if you prefer. I've traced how this implementation is picked here https://github.com/rust-lang/rust/pull/150831/changes#r2673777819.

The mips case right above will be handled by #152576.

Copy link
Member

@RalfJung RalfJung Feb 21, 2026

Choose a reason for hiding this comment

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

We could pull that implementation into rustc as well if you prefer.

Happy to leave that choice to you.^^ It is very strange to me that clang mostly does not use the LLVM impl here, given that they are part of the same project, so this is all a bit confusing.

Copy link
Contributor Author

@folkertdev folkertdev Feb 21, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Pinging some target maintainers:

@jonathanpallant @Patryk27 @glaubitz @ricky26

apparently msp430-none-elf does not have a target maintainer. For m68k I can't get programs to link (some incorrect relocation issue, might be an LLVM problem).

Can you confirm that this test (with your target) works?

./x test tests/run-make/c-link-to-rust-va-list-fn --target mipsel-unknown-linux-gnu

this likely needs some sort of config in bootstrap.toml, I have some variations on

[target.mips64-unknown-linux-gnuabi64]
runner = "qemu-mips64 -L /usr/mips64-linux-gnuabi64"

[target.mipsel-unknown-linux-gnu]
runner = "qemu-mipsel -cpu 24Kf -L /usr/mipsel-linux-gnu"

[target.mips-unknown-linux-gnu]
runner = "qemu-mips -L /usr/mips-linux-gnu"

If we don't get any response here, it's probably safer to exclude these targets from stabilization for now, because looking at it again, I think this implementation is also a fallback in clang/llvm so there is a good chance that it's actually untested.

Copy link
Contributor

@Patryk27 Patryk27 Feb 21, 2026

Choose a reason for hiding this comment

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

Will check tomorrow!

Copy link
Contributor

@glaubitz glaubitz Feb 21, 2026

Choose a reason for hiding this comment

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

For m68k I can't get programs to link (some incorrect relocation issue, might be an LLVM problem).

Yes, it's a known bug, see: llvm/llvm-project#181481

Copy link
Contributor

@Patryk27 Patryk27 Feb 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

It seems that that test is not applicable to AVR (it's a non-std target), but in general the AVR backend for LLVM implements variadics:

https://github.com/llvm/llvm-project/blob/8701cfc0008c4756f04f9839fd4afbb3f112bdad/llvm/lib/Target/AVR/AVRCallingConv.td#L28

... and we strive to be compatible with avr-gcc, so I'd expect things to just-work.

Copy link
Contributor Author

@folkertdev folkertdev Feb 22, 2026

Choose a reason for hiding this comment

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

We're hesitant with accepting targets because the LLVM va_arg implementation is known to miscompile programs on many platforms. In large part that is because a correct implementation would require layout information that LLVM just won't have. Hence, for many targets the actual logic is implemented in clang and we replicate that logic in rustc.

I just tried this locally, compilation still fails but it gets far enough along to emit these errors:

error[E0277]: the trait bound `i16: VaArgSafe` is not satisfied
   --> checkrust.rs:123:27
    |
123 |     continue_if!(ap.arg::<c_int>() == 4);
    |                     ---   ^^^^^ the nightly-only, unstable trait `VaArgSafe` is not implemented for `i16`
    |                     |
    |                     required by a bound introduced by this call
    |
    = help: the following other types implement trait `VaArgSafe`:
              f64
              i32
              i64
              isize
              u32
              u64
              usize

I suppose this really should work, and we're currently implicitly assuming that c_int is i32.

What types implement VaArgSafe is based on section 7.16.1 of the C23 spec

If type is not compatible with the type of the actual next argument (as promoted according to the default argument promotions), the behavior is undefined

Our reading is that, on most targets, it is UB to read an i16 because it is incompatible with what was passed (even if an i16 was passed, it has been promoted to c_int (we implicitly assume this is i32) through the default argument promotions).

I'll make a separate PR to fix that and hopefully we can figure out some approach to testing avr-none there too.

bx.va_arg(addr.immediate(), bx.cx.layout_of(target_ty).llvm_type(bx.cx))
}
Arch::Other(_) => {
// For custom targets, use the LLVM va_arg instruction as a fallback.
bx.va_arg(addr.immediate(), bx.cx.layout_of(target_ty).llvm_type(bx.cx))
Arch::Other(ref arch) => {
// Just to be safe we error out explicitly here, instead of crossing our fingers that
// the default LLVM implementation has the correct behavior for this target.
bug!("c-variadic functions are not currently implemented for custom target {arch}")
}
}
}
45 changes: 27 additions & 18 deletions compiler/rustc_target/src/spec/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1939,24 +1939,6 @@ impl Arch {
}
}

pub fn supports_c_variadic_definitions(&self) -> bool {
use Arch::*;

match self {
// These targets just do not support c-variadic definitions.
Bpf | SpirV => false,

// We don't know if the target supports c-variadic definitions, but we don't want
// to needlessly restrict custom target.json configurations.
Other(_) => true,

AArch64 | AmdGpu | Arm | Arm64EC | Avr | CSky | Hexagon | LoongArch32 | LoongArch64
| M68k | Mips | Mips32r6 | Mips64 | Mips64r6 | Msp430 | Nvptx64 | PowerPC
| PowerPC64 | RiscV32 | RiscV64 | S390x | Sparc | Sparc64 | Wasm32 | Wasm64 | X86
| X86_64 | Xtensa => true,
}
}

/// Whether `#[rustc_scalable_vector]` is supported for a target architecture
pub fn supports_scalable_vectors(&self) -> bool {
use Arch::*;
Expand Down Expand Up @@ -2170,6 +2152,33 @@ impl Target {

Ok(dl)
}

pub fn supports_c_variadic_definitions(&self) -> bool {
use Arch::*;

match self.arch {
// The c-variadic ABI for this target may change in the future, per this comment in
// clang:
//
// > To be compatible with GCC's behaviors, we force arguments with
// > 2×XLEN-bit alignment and size at most 2×XLEN bits like `long long`,
// > `unsigned long long` and `double` to have 4-byte alignment. This
// > behavior may be changed when RV32E/ILP32E is ratified.
RiscV32 if self.llvm_abiname == "ilp32e" => false,

// These targets just do not support c-variadic definitions.
Bpf | SpirV => false,

// We don't know how c-variadics work for this target. Using the default LLVM
// fallback implementation may work, but just to be safe we disallow this.
Other(_) => false,

AArch64 | AmdGpu | Arm | Arm64EC | Avr | CSky | Hexagon | LoongArch32 | LoongArch64
| M68k | Mips | Mips32r6 | Mips64 | Mips64r6 | Msp430 | Nvptx64 | PowerPC
| PowerPC64 | RiscV32 | RiscV64 | S390x | Sparc | Sparc64 | Wasm32 | Wasm64 | X86
| X86_64 | Xtensa => true,
}
}
}

pub trait HasTargetSpec {
Expand Down
Loading