-
Notifications
You must be signed in to change notification settings - Fork 13.6k
stabilize c-style varargs for system, sysv64, win64, efiapi, aapcs #144066
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
HIR ty lowering was modified cc @fmease |
This comment has been minimized.
This comment has been minimized.
9c870d3
to
e5330ce
Compare
e5330ce
to
496a9d6
Compare
Looks great :) Thanks for pushing this topic |
I drafted a PR of similar nature but halted on the fact that there are no tests that verify the functional correctness of code compiled with this feature. It is untested. |
@RalfJung I still wanted |
That seems like a lot of unnecessary bureaucratic overhead to me and the two PRs would necessarily conflict. Why should it be separate?
|
Regarding test coverage -- checking the generated LLVM IR represents our usual level of coverage here. I agree it'd be great to have end-to-end tests that actually invoke C code, but we rarely ever do that, we don't have great infrastructure to do it, and I am not convinced that now is the time we have to start enforcing this. |
This stabilizes extern block declarations of variadic functions with the system, sysv64, win64, efiapi, aapcs ABIs. This corresponds to the extended_varargs_abi_support and extern_system_varargs feature gates.
The feature gates were split up since it seemed like there might be further discussion needed for what exactly "system" ABI variadic functions should do, but a consensus has meanwhile been reached: they shall behave like "C" functions. IOW, the ABI of a "system" function is (bold part is new in this PR):
This had been previously stabilized without FCP in #116161, which got reverted in #136897. There was also a "fun" race condition involved with the system ABI being added to the list of variadic-supporting ABIs between the creation and merge of #116161.
There was a question raised here whether t-lang even needs to be involved for a change like this. Not sure if that has meanwhile been clarified? The behavior of the "system" ABI (a Rust-specific ABI) definitely feels like t-lang territory to me.
Fixes #100189, #136946
Cc @rust-lang/lang
Stabilization report
AFAIK there is no RFC. The tracking issues are
extended_varargs_abi_support
#100189extern_system_varargs
#136946The only controversial point is whether "system" ABI functions should support variadics.
extern "system"
as much as possible microsoft/windows-rs#3626.Note that "system" is already a magic ABI we introduced to "do the right thing". This just makes it do the right thing in more cases.
Actually defining variadic functions in Rust remains unstable, under the c_variadic feature gate.
There was no call for testing.
A search brings up https://github.com/rust-osdev/uefi-rs/blob/main/uefi-raw/src/table/boot.rs using this for "efiapi". THis doesn't seem widely used, but it is an "obvious" gap in our support for c-variadics.
All rustc does here is forward the ABI to LLVM so there's lot a lot to say here...
The check for allowed variadic ABIs is here.
The special handling of "system" is here.
This PR adds a codegen test ensuring we emit the correct LLVM IR:
tests/codegen/cffi/c-variadic-ffi.rs
.There is an existing test ensuring that we keep rejecting variadics for ABIs where that is not supported (
tests/ui/c-variadic/variadic-ffi-1.rs
).The test ensuring that we do not stabilize defining c-variadic functions is
tests/ui/feature-gates/feature-gate-c_variadic.rs
.None that I am aware of.
None that I am aware of.
@Soveu added sysv64, win64, efiapi, aapcs to the list of ABIs that allow variadics, @beepster4096 added system. @workingjubilee recently refactored the ABI handling in the compiler, also affecting this feature.
Maybe RA needs to be taught about the new allowed ABIs? No idea how precisely they mirror what exactly rustc accepts and rejects here.
Nothing new here, this just expands the existing support for calling variadic functions to more ABIs.
Nothing new here, this just expands the existing support for calling variadic functions to more ABIs.
Nothing new here, this just expands the existing support for calling variadic functions to more ABIs.
No.
None.