-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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
base: master
Are you sure you want to change the base?
Conversation
I'd like to see you |
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
This comment was marked as outdated.
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 resolved.
7e6762a
to
71abc30
Compare
This comment was marked as resolved.
This comment was marked as resolved.
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
This comment was marked as outdated.
This comment was marked as outdated.
71abc30
to
e9817cc
Compare
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
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
|
||
/// ABIs relevant to Windows or x86 targets | ||
X86(X86Call), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
There was a problem hiding this comment.
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.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
180ed85
to
17fab7d
Compare
@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
Let me rephrase my concern: in its current form, what doc comment would you put on |
That's the thing about an abstraction, it's bigger on the inside :^) |
this review. I like it. another! |
@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 |
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. |
this allows removing CanonAbi::try_from_extern_abi and its FromStr
/// 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 { |
There was a problem hiding this comment.
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
...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
5c54b5e
to
397e975
Compare
@bors2 try |
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
💔 Test failed
|
oops, right, that's tier 2 now... |
@bors2 try |
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
huzzah! |
Our
conv_from_spec_abi
,adjust_abi
, andis_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 letAbiMap
devour this complexity. Once you have anAbiMap
, you can answer whichExternAbi
s will lower to whatCanonAbi
s (and whether they will lower at all).Removed:
conv_from_spec_abi
replaced byAbiMap::canonize_abi
adjust_abi
replaced by sameConv::PreserveAll
as unusedConv::Cold
as unusedenum Conv
replaced byenum CanonAbi
target-spec.json changes:
"{abi}"
strings Rust recognizes, e.g.