-
Notifications
You must be signed in to change notification settings - Fork 14.1k
c_variadic: make VaList abi-compatible with C
#141980
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: main
Are you sure you want to change the base?
Conversation
|
This PR modifies cc @jieyouxu Some changes occurred in compiler/rustc_codegen_ssa |
cb90479 to
23a983d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
I've just ran
I ran the assembly test you supplied locally and got this assembly: Test assembly .file "temp.9a048525d681a25f-cgu.0"
.section .text.by_reference,"ax",@progbits
.globl by_reference
.p2align 4
.type by_reference,@function
by_reference:
.cfi_startproc
movl (%rdi), %ecx
cmpq $40, %rcx
ja .LBB0_2
movq %rcx, %rax
addq 16(%rdi), %rax
addl $8, %ecx
movl %ecx, (%rdi)
movl (%rax), %eax
retq
.LBB0_2:
movq 8(%rdi), %rax
leaq 8(%rax), %rcx
movq %rcx, 8(%rdi)
movl (%rax), %eax
retq
.Lfunc_end0:
.size by_reference, .Lfunc_end0-by_reference
.cfi_endproc
.section .text.dummy,"ax",@progbits
.globl dummy
.p2align 4
.type dummy,@function
dummy:
.cfi_startproc
retq
.Lfunc_end1:
.size dummy, .Lfunc_end1-dummy
.cfi_endproc
.globl by_value
.type by_value,@function
.set by_value, by_reference
.ident "rustc version 1.89.0-dev"
.section ".note.GNU-stack","",@progbitsLLVM appears to have merged the functions as they had identical assembly.
|
I am somewhat uncomfortable relying on that, and would personally prefer a slightly less slick API that requires fewer hacks. This feature is too niche to justify so much hackery, IMO. But maybe I am overly paranoid. If we do end up going for it, please add walls of comments everywhere that explain this, or else we can be sure some poor compiler contributor will spend hours digging through this in a few years... |
|
@beetrees hmm, I did rebase, maybe that messed something up? Can you rebase this branch to something more recent? |
23a983d to
57bc996
Compare
|
I've rebased, and the two tests still pass locally. |
|
For me too, I must have messed something up earlier. Thanks for checking! |
57bc996 to
6347a21
Compare
6347a21 to
6e1e74f
Compare
|
I've reworked this to use a |
This comment has been minimized.
This comment has been minimized.
6e1e74f to
26f7025
Compare
26f7025 to
79a6396
Compare
|
|
Rework `c_variadic` try-job: x86_64-gnu-llvm-21-3 try-job: armhf-gnu try-job: aarch64-apple try-job: x86_64-msvc-1
|
Hopefully that is sufficient then @bors r=workingjubilee |
c_variadicc_variadic: make VaList abi-compatible with C
`c_variadic`: make `VaList` abi-compatible with C tracking issue: #44930 related PR: #144529 On some platforms, the C `va_list` type is actually a single-element array of a struct (on other platforms it is just a pointer). In C, arrays passed as function arguments expirience array-to-pointer decay, which means that C will pass a pointer to the array in the caller instead of the array itself, and modifications to the array in the callee will be visible to the caller (this does not match Rust by-value semantics). However, for `va_list`, the C standard explicitly states that it is undefined behaviour to use a `va_list` after it has been passed by value to a function (in Rust parlance, the `va_list` is moved, not copied). This matches Rust's pass-by-value semantics, meaning that when the C `va_list` type is a single-element array of a struct, the ABI will match C as long as the Rust type is always be passed indirectly. In the old implementation, this ABI was achieved by having two separate types: `VaList` was the type that needed to be used when passing a `VaList` as a function parameter, whereas `VaListImpl` was the actual `va_list` type that was correct everywhere else. This however is quite confusing, as there are lots of footguns: it is easy to cause bugs by mixing them up (e.g. the C function `void foo(va_list va)` was equivalent to the Rust `fn foo(va: VaList)` whereas the C function `void bar(va_list* va)` was equivalent to the Rust `fn foo(va: *mut VaListImpl)`, not `fn foo(va: *mut VaList)` as might be expected); also converting from `VaListImpl` to `VaList` with `as_va_list()` had platform specific behaviour: on single-element array of a struct platforms it would return a `VaList` referencing the original `VaListImpl`, whereas on other platforms it would return a cioy, In this PR, there is now just a single `VaList` type (renamed from `VaListImpl`) which represents the C `va_list` type and will just work in all positions. Instead of having a separate type just to make the ABI work, #144529 adds a `#[rustc_pass_indirectly_in_non_rustic_abis]` attribute, which when applied to a struct will force the struct to be passed indirectly by non-Rustic calling conventions. This PR then implements the `VaList` rework, making use of the new attribute on all platforms where the C `va_list` type is a single-element array of a struct. Cleanup of the `VaList` API and implementation is also included in this PR: since it was decided it was OK to experiment with Rust requiring that not calling `va_end` is not undefined behaviour (#141524 (comment)), I've removed the `with_copy` method as it was redundant to the `Clone` impl (the `Drop` impl of `VaList` is a no-op as `va_end` is a no-op on all known platforms). Previous discussion: #141524 and [t-compiler > c_variadic API and ABI](https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/c_variadic.20API.20and.20ABI) Tracking issue: #44930 r? `@joshtriplett`
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
|
@bors try jobs=i686-msvc-1 |
This comment has been minimized.
This comment has been minimized.
`c_variadic`: make `VaList` abi-compatible with C try-job: i686-msvc-1
This comment has been minimized.
This comment has been minimized.
the 32-bit variant differs only in the alignment/size, and I can't test it locally
|
@bors try cancel |
|
Try build cancelled. Cancelled workflows: |
a9d1de8 to
d6db951
Compare
|
@bors try jobs=x86_64-gnu-llvm-21-3,armhf-gnu,aarch64-apple,x86_64-msvc-1,i686-msvc-1 |
`c_variadic`: make `VaList` abi-compatible with C try-job: x86_64-gnu-llvm-21-3 try-job: armhf-gnu try-job: aarch64-apple try-job: x86_64-msvc-1 try-job: i686-msvc-1
This comment has been minimized.
This comment has been minimized.
|
@bors r=workingjubilee a layout test failed on 32-bit windows because usize is smaller there. Not very interesting otherwise (all windows targets use an opaque pointer to pass Hopefully I've try-jobbed everything now? |
tracking issue: #44930
related PR: #144529
On some platforms, the C
va_listtype is actually a single-element array of a struct (on other platforms it is just a pointer). In C, arrays passed as function arguments expirience array-to-pointer decay, which means that C will pass a pointer to the array in the caller instead of the array itself, and modifications to the array in the callee will be visible to the caller (this does not match Rust by-value semantics). However, forva_list, the C standard explicitly states that it is undefined behaviour to use ava_listafter it has been passed by value to a function (in Rust parlance, theva_listis moved, not copied). This matches Rust's pass-by-value semantics, meaning that when the Cva_listtype is a single-element array of a struct, the ABI will match C as long as the Rust type is always be passed indirectly.In the old implementation, this ABI was achieved by having two separate types:
VaListwas the type that needed to be used when passing aVaListas a function parameter, whereasVaListImplwas the actualva_listtype that was correct everywhere else. This however is quite confusing, as there are lots of footguns: it is easy to cause bugs by mixing them up (e.g. the C functionvoid foo(va_list va)was equivalent to the Rustfn foo(va: VaList)whereas the C functionvoid bar(va_list* va)was equivalent to the Rustfn foo(va: *mut VaListImpl), notfn foo(va: *mut VaList)as might be expected); also converting fromVaListImpltoVaListwithas_va_list()had platform specific behaviour: on single-element array of a struct platforms it would return aVaListreferencing the originalVaListImpl, whereas on other platforms it would return a cioy,In this PR, there is now just a single
VaListtype (renamed fromVaListImpl) which represents the Cva_listtype and will just work in all positions. Instead of having a separate type just to make the ABI work, #144529 adds a#[rustc_pass_indirectly_in_non_rustic_abis]attribute, which when applied to a struct will force the struct to be passed indirectly by non-Rustic calling conventions. This PR then implements theVaListrework, making use of the new attribute on all platforms where the Cva_listtype is a single-element array of a struct.Cleanup of the
VaListAPI and implementation is also included in this PR: since it was decided it was OK to experiment with Rust requiring that not callingva_endis not undefined behaviour (#141524 (comment)), I've removed thewith_copymethod as it was redundant to theCloneimpl (theDropimpl ofVaListis a no-op asva_endis a no-op on all known platforms).Previous discussion: #141524 and t-compiler > c_variadic API and ABI
Tracking issue: #44930
r? @joshtriplett