Allow specification of the system V AMD64 ABI constraint. #34494
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
This solves #34388 |
|
cc @rust-lang/lang This is unfortunately an "instantly stable" extension to the language, so we'll want to be sure to tread carefully here and ensure that it's named correctly/conventionally. (cc'ing to get some broader visibility) |
|
I wasn't sure of the name myself either, so that's appreciated. As far as conventions go, gcc uses |
|
bikeshed: would prefer |
|
I'll change it to |
|
There is no reason that this has to be an insta-stable addition to the language, right? We have other ABIs (e.g., |
|
☔ The latest upstream changes (presumably #35174) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The reference also has a list of ABIs, and I'd request you also update that one. |
|
I'm aware, that list was added after this pull request was originally made. |
CensoredUsername
force-pushed
the
allow_sysV64_abi
branch
from
5617f08
to
ce11bb5
Compare
|
Ok, it's been rebased to resolve the merge conflict and it has been feature gated behind feature(abi_sysv64). Additionally several tests have been added to:
One problem though is that it currently runs these tests for all platforms, instead the target architecture where it makes sense for ( |
|
☔ The latest upstream changes (presumably #36066) made this pull request unmergeable. Please resolve the merge conflicts. |
This can be specified using `extern sysV64 fn` on all platforms
…tions and add the feature(abi_sysv64) to the list of known features
…d calling convention instead of just looking at the selected platform
… valid in non-codegen tests. Remove false positive in a test that relied on the exact formatting of an error string and rewrite the sysv64 register allocation test at it was triggering undefined behaviour
…dd another test that checks if the sysv64 abi corresponds to the same rules as the C abi on unix platforms
CensoredUsername
force-pushed
the
allow_sysV64_abi
branch
from
15c1e8c
to
46a719e
Compare
|
Ok, the sysv64 ABI should now also pass the relevant tests that the C ABI passes on unix platforms. Additionally, a bug in the abi info computation should be fixed (where if you'd specify the "win64" calling convention on non-windows platforms it'd use the abi info code meant for the C (sysv64) abi). |
|
@bors r=nagisa |
|
📌 Commit 46a719e has been approved by |
|
|
||
| // Allows the sysV64 ABI to be specified on all platforms | ||
| // instead of just the platforms on which it is the C ABI | ||
| (active, abi_sysv64, "1.13.0", None) |
There was a problem hiding this comment.
@CensoredUsername you probably want to change None here to a tracking issue you should create before this gets landed, like the gates above.
|
r=me. The tests give enough confidence this is implemented properly, the ABI is feature-gated so there’s little concern about it becoming a maintenance burden and it fixes a bug wrt win64 too. |
|
@bors r=nagisa |
|
📌 Commit ad447a1 has been approved by |
|
⌛ Testing commit ad447a1 with merge 2bd2272... |
|
💔 Test failed - auto-win-gnu-64-opt |
…o ignore windows entirely instead of just msvc
|
I completely forgot that x86_64-windows-gnu is a thing and that it uses the |
|
@bors r=nagisa |
|
📌 Commit 3d766a0 has been approved by |
Allow specification of the system V AMD64 ABI constraint. This can be specified using `extern "sysV64" fn` on all platforms. This ABI is used as the C ABI on unix platforms, but can only be specified there using extern "C". It was impossible to specify on other platforms. Meanwhile the win64 ABI, which was the extern "C" ABI on the windows platform could be specified on other platforms using extern "win64". This pull request adds the the "sysV64" ABI constraint which exposes this calling convention on platforms where it is not the C ABI.
This can be specified using
extern "sysV64" fnon all platforms.This ABI is used as the C ABI on unix platforms, but can only be specified there using extern "C". It was impossible to specify on other platforms. Meanwhile the win64 ABI, which was the extern "C" ABI on the windows platform could be specified on other platforms using extern "win64".
This pull request adds the the "sysV64" ABI constraint which exposes this calling convention on platforms where it is not the C ABI.