Skip to content

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

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.

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
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented May 26, 2025

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",

@rustbot 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
@workingjubilee
Copy link
Member Author

I'd like to see you
@bors try

bors added a commit that referenced this pull request May 26, 2025
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
@bors

This comment was marked as outdated.

@workingjubilee

This comment was marked as resolved.

@workingjubilee

This comment was marked as resolved.

@workingjubilee

This comment was marked as resolved.

bors added a commit that referenced this pull request May 26, 2025
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
@bors

This comment was marked as outdated.

@workingjubilee
Copy link
Member Author

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

@bors
Copy link
Collaborator

bors commented May 26, 2025

⌛ Trying commit e9817cc with merge 9938b8a...

bors added a commit that referenced this pull request May 26, 2025
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
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment was marked as outdated.

@rust-log-analyzer

This comment has been minimized.


/// ABIs relevant to Windows or x86 targets
X86(X86Call),
}
Copy link
Member

Choose a reason for hiding this comment

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

😍

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@workingjubilee

This comment was marked as resolved.

@workingjubilee

This comment was marked as resolved.

@workingjubilee
Copy link
Member Author

@bors try

@bors
Copy link
Collaborator

bors commented May 26, 2025

⌛ Trying commit 17fab7d with merge c4f8500...

bors added a commit that referenced this pull request May 26, 2025
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 workingjubilee removed PG-exploit-mitigations Project group: Exploit mitigations O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) labels May 26, 2025
@RalfJung
Copy link
Member

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.

@workingjubilee
Copy link
Member Author

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
Copy link
Member Author

this review. I like it. another!

@workingjubilee
Copy link
Member Author

@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
Copy link
Member

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.

/// 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 {
Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@workingjubilee workingjubilee May 30, 2025

Choose a reason for hiding this comment

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

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
Copy link
Member Author

@bors2 try

@rust-bors
Copy link

rust-bors bot commented May 30, 2025

⌛ Trying commit da98b14 with merge b508329

rust-bors bot added a commit that referenced this pull request May 30, 2025
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
Copy link

rust-bors bot commented May 30, 2025

💔 Test failed

@workingjubilee
Copy link
Member Author

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

@workingjubilee
Copy link
Member Author

@bors2 try

@rust-bors
Copy link

rust-bors bot commented May 30, 2025

⌛ Trying commit da98b14 with merge 5527051

rust-bors bot added a commit that referenced this pull request May 30, 2025
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
Copy link

rust-bors bot commented May 30, 2025

☀️ Try build successful

  • CI
    Build commit: 5527051 (552705171bcd9f242c39823a3a8b383699c82fdb)

@workingjubilee
Copy link
Member Author

huzzah!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ABI Area: Concerning the application binary interface (ABI) A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. A-target-specs Area: Compile-target specifications A-targets Area: Concerning the implications of different compiler targets O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) PG-exploit-mitigations Project group: Exploit mitigations 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.

7 participants