Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi` #141569

workingjubilee · 2025-05-26T05:40:28Z

Our conv_from_spec_abi, adjust_abi, and is_abi_supported combine to give us a very confusing way of reasoning about what actual calling convention we want to lower our code to and whether we want to compile the resulting code at all. Instead of leaving this code as a miniature adventure game in which someone tries to combine stateful mutations into a Rube Goldberg machine that will let them escape the maze and arrive at the promised land of codegen, we let AbiMap devour this complexity. Once you have an AbiMap, you can answer which ExternAbis will lower to what CanonAbis (and whether they will lower at all).

Removed:

conv_from_spec_abi replaced by AbiMap::canonize_abi
adjust_abi replaced by same
Conv::PreserveAll as unused
Conv::Cold as unused
enum Conv replaced by enum CanonAbi

target-spec.json changes:

If you have a target-spec.json then now your "entry-abi" key will be specified in terms of one of the "{abi}" strings Rust recognizes, e.g.

    "entry-abi": "C",
    "entry-abi": "win64",
    "entry-abi": "aapcs",

workingjubilee · 2025-05-26T05:44:14Z

I'd like to see you
@bors try

Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi` I am having second thoughts about some of my design choices, here, but I think it's useless to _internally_ debate them further. r? `@ghost` try-job: test-various try-job: aarch64-apple try-job: aarch64-gnu try-job: armhf-gnu try-job: x86_64-msvc-1 try-job: x86_64-msvc-2 try-job: x86_64-mingw-1 try-job: x86_64-mingw-2 try-job: i686-msvc-1 try-job: i686-msvc-2

workingjubilee · 2025-05-26T06:22:31Z

I am foolish and impatient.
@bors try-
@bors try

bors · 2025-05-26T06:23:41Z

⌛ Trying commit e9817cc with merge 9938b8a...

Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi` I am having second thoughts about some of my design choices, here, but I think it's useless to _internally_ debate them further. r? `@ghost` try-job: test-various try-job: aarch64-apple try-job: aarch64-gnu try-job: armhf-gnu try-job: x86_64-msvc-1 try-job: x86_64-msvc-2 try-job: x86_64-mingw-1 try-job: x86_64-mingw-2 try-job: i686-msvc-1 try-job: i686-msvc-2

compiler/rustc_abi/src/map.rs

bjorn3 · 2025-05-26T09:19:17Z

compiler/rustc_abi/src/canon_abi.rs

+
+    /// ABIs relevant to Windows or x86 targets
+    X86(X86Call),
+}


Ah so that decision actually does look right to someone who is not inside my head? Great.

compiler/rustc_target/src/spec/targets/i686_pc_windows_msvc.rs

workingjubilee · 2025-05-26T18:49:32Z

@bors try

bors · 2025-05-26T18:50:44Z

⌛ Trying commit 17fab7d with merge c4f8500...

Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi` I am having second thoughts about some of my design choices, here, but I think it's useless to _internally_ debate them further. r? `@ghost` try-job: test-various try-job: aarch64-apple try-job: aarch64-gnu try-job: armhf-gnu try-job: x86_64-msvc-1 try-job: x86_64-msvc-2 try-job: x86_64-mingw-1 try-job: x86_64-mingw-2 try-job: i686-msvc-1 try-job: i686-msvc-2

RalfJung · 2025-05-28T14:30:24Z

Let me rephrase my concern: in its current form, what doc comment would you put on AbiMap? It should have some explanation of its purpose.

compiler/rustc_target/src/spec/abi_map.rs

workingjubilee · 2025-05-28T21:42:52Z

I admit that canonize_abi function is bigger than I thought it would be.

That's the thing about an abstraction, it's bigger on the inside :^)

workingjubilee · 2025-05-28T22:09:37Z

this review. I like it. another!

compiler/rustc_target/src/spec/abi_map.rs

workingjubilee · 2025-05-30T16:58:37Z

@RalfJung Ultimately, I want to explore pushing more things into being trait-defined that are implemented by each target backend, so AbiMap may wind up hosting a Box<dyn Trait>. It's just easier for me to have things in the current form for experimenting with in later PRs.

RalfJung · 2025-05-30T17:02:56Z

Hm, I'm somewhat skeptical of that but sure.^^

Unfortunately I'm not sure when I will have time to review this properly as I'm busy the entire weekend and the next week.

compiler/rustc_abi/src/canon_abi.rs

this allows removing CanonAbi::try_from_extern_abi and its FromStr

RalfJung · 2025-05-30T19:38:50Z

compiler/rustc_abi/src/canon_abi.rs

+    /// A target-insensitive mapping of CanonAbi to ExternAbi, convenient for "forwarding" impls.
+    /// Importantly, the set of CanonAbi values is a logical *subset* of ExternAbi values,
+    /// so this is injective: if you take an ExternAbi to a CanonAbi and back, you have lost data.
+    const fn to_erased_extern_abi(self) -> ExternAbi {


So this is now the only odd new translation function left here, I think? I'm wondering how we can eradicate this one as well. It's only used for printing. Where do we care about that? Is it only that Miri error message? Ideally that would have a way of recovering the original ExternAbi...

My earlier versions of this had a to_denormalizing_map, yes, which would allow naming all the "{abi}"s which map to the CanonAbi of interest.

would be happy to pursue eliminating this in a followup if the PR otherwise looks good in its current state except for this function daring to exist (well, and needing a rebase-squash badly). that Miri error message would indeed be a relatively important part of any further changes and I think it would want a little more conversation around it.

workingjubilee · 2025-05-30T20:07:54Z

@bors2 try

rust-bors · 2025-05-30T20:07:57Z

⌛ Trying commit da98b14 with merge b508329…

Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi` Our `conv_from_spec_abi`, `adjust_abi`, and `is_abi_supported` combine to give us a very confusing way of reasoning about what _actual_ calling convention we want to lower our code to and whether we want to compile the resulting code at all. Instead of leaving this code as a miniature adventure game in which someone tries to combine stateful mutations into a Rube Goldberg machine that will let them escape the maze and arrive at the promised land of codegen, we let `AbiMap` devour this complexity. Once you have an `AbiMap`, you can answer which `ExternAbi`s will lower to what `CanonAbi`s (and whether they will lower at all). Removed: - `conv_from_spec_abi` replaced by `AbiMap::canonize_abi` - `adjust_abi` replaced by same - `Conv::PreserveAll` as unused - `Conv::Cold` as unused - `enum Conv` replaced by `enum CanonAbi` target-spec.json changes: - If you have a target-spec.json then now your "entry-abi" key will be specified in terms of one of the `"{abi}"` strings Rust recognizes, e.g. ```json "entry-abi": "C", "entry-abi": "win64", "entry-abi": "aapcs", ``` try-job: test-various try-job: aarch64-apple try-job: aarch64-gnu-debug try-job: arm-android try-job: armhf-gnu try-job: x86_64-msvc-1 try-job: x86_64-msvc-2 try-job: x86_64-mingw-1 try-job: x86_64-mingw-2 try-job: i686-msvc-1 try-job: i686-msvc-2 try-job: i686-mingw-1 try-job: i686-mingw-2

rust-bors · 2025-05-30T20:08:44Z

💔 Test failed

CI ❌

workingjubilee · 2025-05-30T20:10:49Z

oops, right, that's tier 2 now...

workingjubilee · 2025-05-30T20:10:58Z

@bors2 try

rust-bors · 2025-05-30T20:11:01Z

⌛ Trying commit da98b14 with merge 5527051…

Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi` Our `conv_from_spec_abi`, `adjust_abi`, and `is_abi_supported` combine to give us a very confusing way of reasoning about what _actual_ calling convention we want to lower our code to and whether we want to compile the resulting code at all. Instead of leaving this code as a miniature adventure game in which someone tries to combine stateful mutations into a Rube Goldberg machine that will let them escape the maze and arrive at the promised land of codegen, we let `AbiMap` devour this complexity. Once you have an `AbiMap`, you can answer which `ExternAbi`s will lower to what `CanonAbi`s (and whether they will lower at all). Removed: - `conv_from_spec_abi` replaced by `AbiMap::canonize_abi` - `adjust_abi` replaced by same - `Conv::PreserveAll` as unused - `Conv::Cold` as unused - `enum Conv` replaced by `enum CanonAbi` target-spec.json changes: - If you have a target-spec.json then now your "entry-abi" key will be specified in terms of one of the `"{abi}"` strings Rust recognizes, e.g. ```json "entry-abi": "C", "entry-abi": "win64", "entry-abi": "aapcs", ``` try-job: test-various try-job: aarch64-apple try-job: aarch64-gnu-debug try-job: arm-android try-job: armhf-gnu try-job: x86_64-msvc-1 try-job: x86_64-msvc-2 try-job: x86_64-mingw-1 try-job: x86_64-mingw-2 try-job: i686-msvc-1 try-job: i686-msvc-2 try-job: i686-gnu-1 try-job: i686-gnu-2

rust-bors · 2025-05-30T22:57:38Z

☀️ Try build successful

CI ✅
Build commit: 5527051 (552705171bcd9f242c39823a3a8b383699c82fdb)

workingjubilee · 2025-05-30T22:59:23Z

huzzah!

rustbot added O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) PG-exploit-mitigations Project group: Exploit mitigations T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 26, 2025

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

workingjubilee force-pushed the canonicalize-abi branch from 7e6762a to 71abc30 Compare May 26, 2025 06:06

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

workingjubilee force-pushed the canonicalize-abi branch from 71abc30 to e9817cc Compare May 26, 2025 06:21

This comment has been minimized.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment has been minimized.

Sign in to view

bjorn3 reviewed May 26, 2025

View reviewed changes

compiler/rustc_abi/src/map.rs Outdated Show resolved Hide resolved

bjorn3 reviewed May 26, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

bjorn3 reviewed May 26, 2025

View reviewed changes

compiler/rustc_target/src/spec/targets/i686_pc_windows_msvc.rs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

workingjubilee force-pushed the canonicalize-abi branch from 180ed85 to 17fab7d Compare May 26, 2025 18:46

workingjubilee removed PG-exploit-mitigations Project group: Exploit mitigations O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) labels May 26, 2025

RalfJung reviewed May 28, 2025

View reviewed changes

compiler/rustc_target/src/spec/abi_map.rs Outdated Show resolved Hide resolved

Explain why AbiMap tentatively exists

dda0821

workingjubilee commented May 28, 2025

View reviewed changes

compiler/rustc_target/src/spec/abi_map.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 30, 2025

View reviewed changes

compiler/rustc_abi/src/canon_abi.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 30, 2025

View reviewed changes

compiler/rustc_abi/src/canon_abi.rs Outdated Show resolved Hide resolved

RalfJung reviewed May 30, 2025

View reviewed changes

compiler/rustc_abi/src/canon_abi.rs Show resolved Hide resolved

workingjubilee added 6 commits May 30, 2025 12:31

edit comment on CanonAbi::try_from_extern_abi

70f1470

Add AbiMapping for encoding permissive mappings

63e031a

actually encode permissive AbiMapping variants

313d6a3

Fuss over the comments on CanonAbi some more

4fcc795

make entry_abi a lowering of the ABI string

9212ced

this allows removing CanonAbi::try_from_extern_abi and its FromStr

Use derives on CanonAbi

397e975

RalfJung reviewed May 30, 2025

View reviewed changes

workingjubilee force-pushed the canonicalize-abi branch from 5c54b5e to 397e975 Compare May 30, 2025 19:39

compiler: actually remove Conv now that it is irrelevant

da98b14

Replace ad-hoc ABI "adjustments" with an AbiMap to CanonAbi #141569

Are you sure you want to change the base?

Replace ad-hoc ABI "adjustments" with an AbiMap to CanonAbi #141569

Conversation

workingjubilee commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

workingjubilee commented May 26, 2025

Uh oh!

This comment was marked as outdated.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as outdated.

workingjubilee commented May 26, 2025

Uh oh!

bors commented May 26, 2025

Uh oh!

This comment has been minimized.

This comment was marked as outdated.

This comment has been minimized.

Uh oh!

bjorn3 May 26, 2025

Choose a reason for hiding this comment

Uh oh!

workingjubilee May 26, 2025

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

workingjubilee commented May 26, 2025

Uh oh!

bors commented May 26, 2025

Uh oh!

RalfJung commented May 28, 2025

Uh oh!

Uh oh!

workingjubilee commented May 28, 2025

Uh oh!

workingjubilee commented May 28, 2025

Uh oh!

Uh oh!

workingjubilee commented May 30, 2025

Uh oh!

RalfJung commented May 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RalfJung May 30, 2025

Choose a reason for hiding this comment

Uh oh!

workingjubilee May 30, 2025

Choose a reason for hiding this comment

Uh oh!

workingjubilee May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

workingjubilee commented May 30, 2025

Uh oh!

rust-bors bot commented May 30, 2025

Uh oh!

rust-bors bot commented May 30, 2025

Uh oh!

workingjubilee commented May 30, 2025

Uh oh!

workingjubilee commented May 30, 2025

Uh oh!

rust-bors bot commented May 30, 2025

Uh oh!

rust-bors bot commented May 30, 2025

Uh oh!

workingjubilee commented May 30, 2025

Uh oh!

Uh oh!

Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi` #141569

Replace ad-hoc ABI "adjustments" with an `AbiMap` to `CanonAbi` #141569

workingjubilee commented May 26, 2025 •

edited

Loading

workingjubilee May 30, 2025 •

edited

Loading