Skip to content
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

Implement Display for rustc_target::callconv::Conv #135808

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+35 −4
Open

Implement Display for rustc_target::callconv::Conv #135808

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
4 changes: 2 additions & 2 deletions compiler/rustc_const_eval/src/interpret/call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,8 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
if caller_fn_abi.conv != callee_fn_abi.conv {
throw_ub_custom!(
fluent::const_eval_incompatible_calling_conventions,
callee_conv = format!("{:?}", callee_fn_abi.conv),
caller_conv = format!("{:?}", caller_fn_abi.conv),
callee_conv = format!("{}", callee_fn_abi.conv),
caller_conv = format!("{}", caller_fn_abi.conv),
)
}

Expand Down
32 changes: 32 additions & 0 deletions compiler/rustc_target/src/callconv/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use std::fmt::Display;
use std::str::FromStr;
use std::{fmt, iter};

Expand Down Expand Up @@ -881,6 +882,37 @@ impl FromStr for Conv {
}
}

impl Display for Conv {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.write_str(match self {
Conv::C => "C",
Conv::Rust => "Rust",
Conv::Cold => "Cold",
Conv::PreserveMost => "PreserveMost",
Conv::PreserveAll => "PreserveAll",
Conv::ArmAapcs => "ArmAapcs",
Conv::CCmseNonSecureCall => "CCmseNonSecureCall",
Conv::CCmseNonSecureEntry => "CCmseNonSecureEntry",
Conv::Msp430Intr => "Msp430Intr",
Conv::PtxKernel => "PtxKernel",
Conv::GpuKernel => "GpuKernel",
Conv::X86Fastcall => "X86Fastcall",
Conv::X86Intr => "X86Intr",
Conv::X86Stdcall => "X86Stdcall",
Conv::X86ThisCall => "X86ThisCall",
Conv::X86VectorCall => "X86VectorCall",
Conv::X86_64SysV => "X86_64SysV",
Conv::X86_64Win64 => "X86_64Win64",
Conv::AvrInterrupt => "AvrInterrupt",
Conv::AvrNonBlockingInterrupt => "AvrNonBlockingInterrupt",
Conv::RiscvInterrupt { kind: RiscvInterruptKind::Machine } => "RiscvInterrupt(machine)",
Conv::RiscvInterrupt { kind: RiscvInterruptKind::Supervisor } => {
"RiscvInterrupt(supervisor)"
}
})
}
}

// Some types are used a lot. Make sure they don't unintentionally get bigger.
#[cfg(target_pointer_width = "64")]
mod size_asserts {
Expand Down
3 changes: 1 addition & 2 deletions src/tools/miri/src/helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -920,8 +920,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
fn check_abi<'a>(&self, fn_abi: &FnAbi<'tcx, Ty<'tcx>>, exp_abi: Conv) -> InterpResult<'a, ()> {
if fn_abi.conv != exp_abi {
throw_ub_format!(
"calling a function with ABI {:?} using caller ABI {:?}",
exp_abi,
"calling a function with ABI {exp_abi} using caller ABI {}",
Copy link
Member

@workingjubilee workingjubilee Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@oli-obk This error is user-facing, correct?

If so, the proposed formatting is not desirable, it should be using ExternAbi's formatting instead.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't about ExternAbi though, it is about the underlying calling convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope it'd help by providing more context. Initially we indeed used ExternAbi here for check_abi, but after #133103 we passed FnAbi instead of ExternAbi, so we can check the number of fixed args like what is done in rust-lang/miri#4122.

Copy link
Member

Choose a reason for hiding this comment

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

Also, this is how we allow e.g. calling a C function through a C-unwind fn pointer.

Copy link
Member

@workingjubilee workingjubilee Jan 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This isn't about ExternAbi though, it is about the underlying calling convention.

The underlying calling convention is still mappable to one of the ExternAbi strings, and we can still express conflicts in terms of that. Dropping data randomly, like erasing "rust-cold" to "Cold", or "x86-interrupt" to "X86Intr", will mostly be confusing.

Copy link
Member

@RalfJung RalfJung Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The concrete error this comment is attached to does not have a ui test; the other one changed in this PR is tested at

rust/src/tools/miri/tests/fail/function_calls/check_callback_abi.rs

Line 12 in 522af10

//~^ ERROR: calling a function with calling convention C using calling convention Rust

and

https://github.com/rust-lang/rust/blob/e8ff9b1836fc8e69ab4fe78c35a6799277c8e700/src/tools/miri/tests/fail/function_calls/exported_symbol_abi_mismatch.rs

Copy link
Member

Choose a reason for hiding this comment

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

huh, those two say "calling convention" but this error says "ABI"? is that what you mean by "the concrete error"... does not have a UI test?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this one here is triggered when you call a built-in shim the wrong way (rather than some normal Rust function), which we don't have a test for. We should add one.

And indeed it should say "calling convention".

Copy link
Contributor Author

@tiif tiif Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So we should change the error message in check_abito use "calling convention" and add a test that will invoke the check_abi here.

Other than that, should we continue with impl Display for Conv or map the calling convention to ExternAbi?

Copy link
Member

Choose a reason for hiding this comment

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

Please implement Display by mapping Conv back to the matching ExternAbi variant (without unwind). This will sometimes give a surprising result, but it will still be comprehensible. If this is properly tested, then I can elaborate on the error message in a followup that gives the rest of the information necessary to fix the problem.

Of course, if even this is considered impossible by my peers, then I can relent for now and simply make possible the impossible later.

fn_abi.conv
);
}
Expand Down
Loading