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

tiif · 2025-01-21T09:00:19Z

rustbot · 2025-01-21T09:00:28Z

rustbot has assigned @fmease.
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

rustbot · 2025-01-21T09:00:30Z

Some changes occurred to the CTFE machinery

cc @rust-lang/wg-const-eval

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

tiif · 2025-01-21T09:09:19Z

It's miri related, and Ralf looks busy, so r? @oli-obk

oli-obk · 2025-01-21T09:38:39Z

@bors r+ rollup

has no test changes as for almost all cases the Debug impl has the same output as the Display impl, adding a new test just for risc doesn't really bring much value imo, so shipping as is

bors · 2025-01-21T09:38:42Z

📌 Commit bbde683 has been approved by oli-obk

It is now in the queue for this repository.

workingjubilee · 2025-01-21T10:34:29Z

src/tools/miri/src/helpers.rs

@@ -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 {}",


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

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

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

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.

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

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.

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.

According to conv_from_spec_abi, the reverse mapping of Conv::Rust can potentially be ExternAbi::RustIntrinsic / RustCall / Rust. Similarly, Conv::C can be mapped to ExternAbi::C / Unadjusted / Cdecl.

Also, in adjust_abi, ExternAbi::StdCall / Thiscall / Fastcall / VectorCall can occasionally get mapped to ExternAbi::C. That means the reverse mapping of Conv::C to ExternAbican possibly be any of them.

If I ignore all of the issues above, the simplest reverse mapping that I can think of is:

Conv::Rust => ExternAbi::Rust

Conv::PreserveMost => ExternAbi::RustCold

Conv::X86Stdcall => ExternAbi::Stdcall

Conv::X86Fastcall => ExternAbi::Fastcall

Conv::X86VectorCall => ExternAbi::Vectorcall

Conv::X86ThisCall => ExternAbi::Thiscall

Conv::C => ExternAbi::C

Conv::X86_64Win64 => ExternAbi::Win64

Conv::X86_64SysV => ExternAbi::SysV64

Conv::ArmAapcs => ExternAbi::Aapcs

Conv::CCmseNonSecureCall => ExternAbi::CCmseNonSecureCall

Conv::CCmseNonSecureEntry => ExternAbi::CCmseNonSecureEntry

Conv::PtxKernel => ExternAbi::PtxKernel

Conv::Msp430Intr => ExternAbi::Msp430Interrupt

Conv::X86Intr => ExternAbi::X86Interrupt

Conv::GpuKernel => ExternAbi::GpuKernel

Conv::AvrInterrupt => ExternAbi::AvrInterrupt

Conv::AvrNonBlockingInterrupt => ExternAbi::AvrNonBlockingInterrupt

Conv::RiscvInterrupt { kind: RiscvInterruptKind::Machine } => ExternAbi::RiscvInterruptM

Conv::RiscvInterrupt { kind: RiscvInterruptKind::Supervisor } => ExternAbi::RiscvInterruptS

Conv::Cold and Conv::PreserveAll: Not sure how to deal with them, can we ignore them?

But I am not sure if this is still correct, so cc @workingjubilee for a vibe check :)

Yes. Please ignore Conv::Cold and Conv::PreserveAll, they are actually dead code. I am working through a refactor of some parts of the compiler that involve what is currently called Conv and discovered they are dead on the way.

Once I am done they will also make it possible to always recover the list of valid ExternAbi names from the actually-used ABI.

Nice! Since there is an ongoing refactor involving Conv, would it be more convenient if I put this on hold and wait for that to finish? I don't mind working on this sooner or later.

EDIT: I went ahead and made the change anyway :)

oli-obk · 2025-01-21T11:29:42Z

@bors r-

src/tools/miri/tests/fail/shims/callconv_mismatch.rs

LFS6502 · 2025-03-01T09:35:11Z

@tiif Thanks for your contribution. Are you able to address the code review?

tiif · 2025-03-01T14:11:26Z

Yup, I have been planning to do this, but unfortunately I still have quite a bit of stuff on my queue, I'd convert this to draft for now.

tiif · 2025-03-11T16:50:09Z

src/tools/miri/tests/fail/function_calls/check_arg_abi.rs

We already have relevant test in miri, so I removed similar test in previous commit.

tiif · 2025-03-13T01:50:50Z

@rustbot ready

rustbot assigned fmease Jan 21, 2025

rustbot added 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. labels Jan 21, 2025

rustbot assigned oli-obk and unassigned fmease Jan 21, 2025

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 21, 2025

workingjubilee reviewed Jan 21, 2025

View reviewed changes

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 21, 2025

This comment has been minimized.

Sign in to view

oli-obk reviewed Feb 3, 2025

View reviewed changes

src/tools/miri/tests/fail/shims/callconv_mismatch.rs Outdated Show resolved Hide resolved

tiif marked this pull request as draft March 1, 2025 14:11

tiif commented Mar 11, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

oli-obk assigned workingjubilee and unassigned oli-obk Mar 12, 2025

tiif added 4 commits March 12, 2025 14:38

impl Display for Conv

701b194

Add test and change ub message wording

17c6eae

Remove unnecessary assert

43c51e9

Implement Display by mapping Conv to ExternAbi

d463c5d

tiif added 5 commits March 12, 2025 14:38

Remove test

3079213

fmt

c8197b7

Remove invalid Conv

71c4b24

bless test

650c8a2

Bless more test

e74fe7a

rust-cloud-vms bot force-pushed the conv_display branch from 473998c to e74fe7a Compare March 12, 2025 17:14

tiif marked this pull request as ready for review March 12, 2025 17:16

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 13, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tiif commented Jan 21, 2025

rustbot commented Jan 21, 2025

rustbot commented Jan 21, 2025

tiif commented Jan 21, 2025

oli-obk commented Jan 21, 2025

bors commented Jan 21, 2025

workingjubilee Jan 21, 2025 •

edited

Loading

RalfJung Jan 21, 2025

tiif Jan 21, 2025

RalfJung Jan 21, 2025

workingjubilee Jan 21, 2025 •

edited

Loading

workingjubilee Jan 22, 2025

tiif Mar 9, 2025

workingjubilee Mar 9, 2025 •

edited

Loading

workingjubilee Mar 9, 2025

tiif Mar 10, 2025 •

edited

Loading

oli-obk commented Jan 21, 2025

This comment has been minimized.

LFS6502 commented Mar 1, 2025

tiif commented Mar 1, 2025

tiif Mar 11, 2025

This comment has been minimized.

This comment has been minimized.

tiif commented Mar 13, 2025

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

Are you sure you want to change the base?

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

Conversation

tiif commented Jan 21, 2025

rustbot commented Jan 21, 2025

rustbot commented Jan 21, 2025

tiif commented Jan 21, 2025

oli-obk commented Jan 21, 2025

bors commented Jan 21, 2025

workingjubilee Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

RalfJung Jan 21, 2025

Choose a reason for hiding this comment

tiif Jan 21, 2025

Choose a reason for hiding this comment

RalfJung Jan 21, 2025

Choose a reason for hiding this comment

workingjubilee Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

workingjubilee Jan 22, 2025

Choose a reason for hiding this comment

tiif Mar 9, 2025

Choose a reason for hiding this comment

workingjubilee Mar 9, 2025 • edited Loading

Choose a reason for hiding this comment

workingjubilee Mar 9, 2025

Choose a reason for hiding this comment

tiif Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

oli-obk commented Jan 21, 2025

This comment has been minimized.

LFS6502 commented Mar 1, 2025

tiif commented Mar 1, 2025

tiif Mar 11, 2025

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

tiif commented Mar 13, 2025

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

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

workingjubilee Jan 21, 2025 •

edited

Loading

workingjubilee Jan 21, 2025 •

edited

Loading

workingjubilee Mar 9, 2025 •

edited

Loading

tiif Mar 10, 2025 •

edited

Loading