Uh oh!
There was an error while loading. Please reload this page.
Add new EFIAPI ABI - #65809
Merged
Merged
Conversation
Adds a new ABI for the EFIAPI calls. This ABI should reflect the latest version of the UEFI specification at the time of commit (UEFI spec 2.8, URL below). The specification says that for x86_64, we should follow the win64 ABI, while on all other supported platforms (ia32, itanium, arm, arm64 and risc-v), we should follow the C ABI. To simplify the implementation, we will simply follow the C ABI on all platforms except x86_64, even those technically unsupported by the UEFI specification. https://uefi.org/sites/default/files/resources/UEFI_Spec_2_8_final.pdf
rust-highfive
commented
Oct 25, 2019
Contributor
r? @davidtwco (rust_highfive has picked a reviewer for you, use r? to override) |
davidtwco
commented
Oct 25, 2019
Member
r? @eddyb |
eddyb
commented
Oct 25, 2019
Contributor
Also LGTM, but I'm not sure who should approve adding new ABIs. |
nagisa
reviewed
Oct 25, 2019
| } | ||
| }, | ||
| Abi::EfiApi => { | ||
| if self.arch != "x86_64" { |
Contributor
We don't currently have a whitelist (ABI x only supported on platforms y, z) but we probably want that eventually (cc #57182). Doesn't have to happen in this PR, though. |
nagisa
reviewed
Oct 25, 2019
| // CHECK: define win64 i64 @has_efiapi | ||
| #[no_mangle] | ||
| #[cfg(target_arch = "x86_64")] |
Member
There was a problem hiding this comment.
Rather than using cfg for tests here, write the test using revision feature. See https://github.com/rust-lang/rust/blob/master/src/test/codegen/optimize-attr-1.rs for an example.
| (active, object_safe_for_dispatch, "1.40.0", Some(43561), None), | ||
| /// Allows using the `efiapi` ABI. | ||
| (active, abi_efiapi, "1.40.0", Some(1), None), |
Contributor
There was a problem hiding this comment.
Please file a real tracking issue instead of directing people to #1
nagisa
commented
Oct 25, 2019
Member
I have no strong objections to adding this ABI to the compiler, although I do find the |
hanna-kruppe
commented
Oct 25, 2019
Contributor
re: name bikeshed, the name proposed in #54527 ( |
ContributorAuthor
So I figured I'd keep it the same for consistency's sake. I can rename it to |
hanna-kruppe
commented
Oct 25, 2019
Contributor
Oh dear, it's EFIAPI? Confusing, but precedent is precedent. I don't care either way, personally. |
rust-highfive
commented
Oct 25, 2019
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Centril
commented
Oct 25, 2019
Contributor
r? @nagisa |
rust-highfive
commented
Oct 25, 2019
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
nagisa
commented
Oct 25, 2019
Member
@bors r+ |
bors
commented
Oct 25, 2019
Collaborator
📌 Commit bc592e132fd20da52a6498c723efebf6e11db816 has been approved by |
Centril
commented
Oct 25, 2019
Contributor
@nagisa there's a tidy error :) |
Use revisions to run the EFIABI in multiple configurations, compiling for each supported UEFI platform, and checking the ABI generated in the LLVM IR is correct. Use no_core to make it easier to test.
rust-highfive
commented
Oct 25, 2019
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
rust-highfive
commented
Oct 25, 2019
Contributor
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
roblabla
commented
Oct 25, 2019
ContributorAuthor
^ How am I supposed to fix this? Is there a way to gate a test on the presence of a target in LLVM? |
roblabla
commented
Oct 27, 2019
ContributorAuthor
I gated the test on LLVM 9.0.0 (which is when RISC-V target was added in LLVM) and now the tests pass! I think this is good to go. |
nagisa
commented
Oct 27, 2019
Member
@bors r+ |
bors
commented
Oct 27, 2019
Collaborator
📌 Commit 1099826 has been approved by |
Centril added a commit
to Centril/rust
that referenced
this pull request
Oct 29, 2019
Add new EFIAPI ABI Fixesrust-lang#54527 Adds a new ABI, "efiapi", which reflects the calling convention as specified by [the current spec UEFI spec](https://uefi.org/sites/default/files/resources/UEFI%20Spec%202_7_A%20Sept%206.pdf#G6.999903). When compiling for x86_64, we should select the `win64` ABI, while on all other architectures (Itanium, x86, ARM and ARM64 and RISC-V), we should select the `C` ABI. Currently, this is done by just turning it into the C ABI everywhere except on x86_64, where it's turned into the win64 ABI. Should we prevent this ABI from being used on unsupported architectures, and if so, how would this be done?
Centril added a commit
to Centril/rust
that referenced
this pull request
Oct 29, 2019
Add new EFIAPI ABI Fixesrust-lang#54527 Adds a new ABI, "efiapi", which reflects the calling convention as specified by [the current spec UEFI spec](https://uefi.org/sites/default/files/resources/UEFI%20Spec%202_7_A%20Sept%206.pdf#G6.999903). When compiling for x86_64, we should select the `win64` ABI, while on all other architectures (Itanium, x86, ARM and ARM64 and RISC-V), we should select the `C` ABI. Currently, this is done by just turning it into the C ABI everywhere except on x86_64, where it's turned into the win64 ABI. Should we prevent this ABI from being used on unsupported architectures, and if so, how would this be done?
Centril added a commit
to Centril/rust
that referenced
this pull request
Oct 29, 2019
Add new EFIAPI ABI Fixesrust-lang#54527 Adds a new ABI, "efiapi", which reflects the calling convention as specified by [the current spec UEFI spec](https://uefi.org/sites/default/files/resources/UEFI%20Spec%202_7_A%20Sept%206.pdf#G6.999903). When compiling for x86_64, we should select the `win64` ABI, while on all other architectures (Itanium, x86, ARM and ARM64 and RISC-V), we should select the `C` ABI. Currently, this is done by just turning it into the C ABI everywhere except on x86_64, where it's turned into the win64 ABI. Should we prevent this ABI from being used on unsupported architectures, and if so, how would this be done?
bors added a commit
that referenced
this pull request
Oct 29, 2019
Rollup of 5 pull requests Successful merges: - #65294 (Lint ignored `#[inline]` on function prototypes) - #65318 (Call out the types that are non local on E0117) - #65531 (Update backtrace to 0.3.40) - #65562 (Improve the "try using a variant of the expected type" hint.) - #65809 (Add new EFIAPI ABI) Failed merges: r? @ghost
emilyalbini added a commit
to emilyalbini/rust
that referenced
this pull request
Nov 6, 2019
Do not require extra LLVM backends for `x.py test` to pass For long time our testing passed with a partially built LLVM ``` [llvm] targets = "X86;ARM" ``` , a [recent PR](rust-lang#65809) changed that.
JohnTitor added a commit
to JohnTitor/rust
that referenced
this pull request
Nov 6, 2019
Do not require extra LLVM backends for `x.py test` to pass For long time our testing passed with a partially built LLVM ``` [llvm] targets = "X86;ARM" ``` , a [recent PR](rust-lang#65809) changed that.
JohnTitor added a commit
to JohnTitor/rust
that referenced
this pull request
Nov 7, 2019
Do not require extra LLVM backends for `x.py test` to pass For long time our testing passed with a partially built LLVM ``` [llvm] targets = "X86;ARM" ``` , a [recent PR](rust-lang#65809) changed that.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#54527
Adds a new ABI, "efiapi", which reflects the calling convention as specified by the current spec UEFI spec. When compiling for x86_64, we should select the
win64ABI, while on all other architectures (Itanium, x86, ARM and ARM64 and RISC-V), we should select theCABI.Currently, this is done by just turning it into the C ABI everywhere except on x86_64, where it's turned into the win64 ABI. Should we prevent this ABI from being used on unsupported architectures, and if so, how would this be done?