-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
c_variadic: impl va_copy and va_end as Rust intrinsics
#150436
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
| /// Basic implementation of a `va_list`. | ||
| #[repr(transparent)] | ||
| #[derive(Debug)] | ||
| #[derive(Debug, Clone, Copy)] | ||
| struct VaListInner { | ||
| ptr: *const c_void, | ||
| } |
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.
| /// https://github.com/llvm/llvm-project/blob/0cdc1b6dd4a870fc41d4b15ad97e0001882aba58/clang/lib/CodeGen/Targets/Hexagon.cpp#L407-L417 | ||
| #[repr(C)] | ||
| #[derive(Debug)] | ||
| #[derive(Debug, Clone, Copy)] |
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.
| /// https://github.com/llvm/llvm-project/blob/af9a4263a1a209953a1d339ef781a954e31268ff/llvm/lib/Target/Xtensa/XtensaISelLowering.cpp#L1211-L1215 | ||
| #[repr(C)] | ||
| #[derive(Debug)] | ||
| #[derive(Debug, Clone, Copy)] |
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.
| /// https://docs.google.com/gview?embedded=true&url=https://github.com/IBM/s390x-abi/releases/download/v1.7/lzsabi_s390x.pdf | ||
| #[repr(C)] | ||
| #[derive(Debug)] | ||
| #[derive(Debug, Clone, Copy)] |
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.
| /// [GCC header]: https://web.mit.edu/darwin/src/modules/gcc/gcc/ginclude/va-ppc.h | ||
| #[repr(C)] | ||
| #[derive(Debug)] | ||
| #[derive(Debug, Clone, Copy)] |
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.
| /// http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf | ||
| #[repr(C)] | ||
| #[derive(Debug)] | ||
| #[derive(Debug, Clone, Copy)] |
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.
This comment has been minimized.
This comment has been minimized.
7a78640 to
8e09112
Compare
|
The codegen of |
|
|
Didn't we say we would avoid that assumption and also support implementations that do malloc/free in va_copy / va_end? See #t-compiler/const-eval > c-variadics in const-eval @ 💬. |
|
That's right, we don't actually impl Copy for VaList, so that a future target could implement a meaningful Clone and Drop. It's just that in practice for all current targets Clone happens to be a memcpy and Drop a no-op |
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.
Offering a suggested historical note.
Please feel free to trim it or move it into the PR description itself, down to the amounts you find worthwhile or just amusing to keep (including 0 lines).
r=me after
Thanks!
| // | ||
| // Rust requires that not calling `va_end` on a `va_list` does not cause undefined | ||
| // behaviour: it is safe to leak values. | ||
| } |
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.
| } | |
| // The only target with a possibly-non-conformant C implementation | |
| // was Pyramid Technology (usually `#ifdef pyr` in old C headers) | |
| // which used `#define`s to replace `va_start` and `va_end` with `{ }` | |
| // and thus needed to be called in the same scope. | |
| // This should still be irrelevant for *binary* compatibility. | |
| // | |
| // Pyramid Technology was acquired by Siemens AG in 1995. | |
| // Its proprietary arch was replaced by MIPS in 1991[^1][^2] | |
| // and DCOS/X, its Unix implementation, was replaced by SINIX in 1995[^3]. | |
| // | |
| // [^1]: ComputerWorld, Vol XXV, No. 16, 1991 April 22nd, from digitized text: https://archive.org/stream/computerworld2516unse/computerworld2516unse_djvu.txt | |
| // [^2]: "Pyramid Server Uses Mips Chip" from digitized collection: https://archive.computerhistory.org/resources/access/text/2022/03/102739303-05-01-acc.pdf | |
| // [^3]: https://en.wikipedia.org/wiki/DC/OSx | |
| } |
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.
This is fun, but I don't think it is relevant for this comment. The same-scope restriction is a C implementation detail (portable macro assembler and all that), rust (and modern C implementations) don't have that problem.
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.
I'm satisfied with it being part of the PR history, then.
I just wanted to make sure it was incredibly, completely, totally dead. :^)
|
@folkertdev what is your plan for va_copy regarding the const-eval support for this? |
| /// | ||
| #[rustc_intrinsic] | ||
| #[rustc_nounwind] | ||
| pub unsafe fn va_copy<'f>(dest: *mut VaList<'f>, src: &VaList<'f>); |
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.
The docs for va_end still reference va_copy. Does va_end even still make sense after this PR?
Also, seems like it is now newly legal to call va_arg multiple times on the same raw VaList representation after duplicating it. The entire protocol for how the code may interact with va_arg is changed quite a bit. That should also be reflected in the docs.
c_variadic: use Clone instead of va_copyc_variadic: use Clone instead of LLVM va_copy
8e09112 to
00673e0
Compare
This comment has been minimized.
This comment has been minimized.
|
Based on further discussion in #t-compiler/const-eval > c-variadics in const-eval, a slight change of plans
I've changed the signature of the intrinsic to be more ergonomic, and made it safe because while the hook is used to detect UB, the intrinsic itself is completely safe. |
This comment has been minimized.
This comment has been minimized.
00673e0 to
19e7342
Compare
This comment has been minimized.
This comment has been minimized.
|
Typo in a field name? |
19e7342 to
97e26cb
Compare
|
cool! @bors r+ |
|
Commit 97e26cb has been unapproved. |
|
Locally with #150601 I can now get this to emit the error from the comment. So this setup should work in practice. fn manual_copy() {
const unsafe extern "C" fn helper(ap: ...) {
// A copy created using Clone is valid, and can be used to read arguments.
let mut copy = ap.clone();
assert!(copy.arg::<i32>() == 1i32);
let mut u = core::mem::MaybeUninit::uninit();
unsafe { core::ptr::copy_nonoverlapping(&ap, u.as_mut_ptr(), 1) };
// Manually creating the copy is fine.
let mut copy = unsafe { u.assume_init() };
// Even using it is fine.
let _ = copy.arg::<i32>();
// But then using (or dropping) the original is not.
drop(copy);
drop(ap);
}
const { unsafe { helper(1, 2, 3) } };
//~^ ERROR va_end on unknown va_list allocation ALLOC0 [E0080]
}The above also requires some changes to |
| /// the one in `src`, but can be advanced independently. | ||
| #[rustc_intrinsic] | ||
| #[rustc_nounwind] | ||
| pub fn va_copy<'f>(src: &VaList<'f>) -> VaList<'f> { |
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.
The more I think about it, maybe this function should now have different name? It no longer is va_copy, really: it does not lower to the LLVM va_copy, and even has a different signature now. va_list_clone perhaps? with va_end becoming va_list_drop?
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.
It still is an implementation, in effect, of the va_copy function-macro from C, right?
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.
essentially, yes.
Perhaps I should push the va_end changes here (those turned out to be smaller than anticipated) for a more complete picture?
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.
Sure!
97e26cb to
936b529
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Right so I've now included the changes to |
c_variadic: use Clone instead of LLVM va_copyc_variadic: impl va_copy and va_end as Rust intrinsics
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.
Hmm. Yeah, I think the names should remain the same as they are now because those are the names most familiar to C programmers, and I think that (soon-to-be-former, hopefully) C programmers poking at the source will want to see them instead of some offbeat va_list_clone call.
I do understand the implication that it is a "false friend", because someone might assume it is LLVM's impl. However, here we do document it is not, and people who learn how Rust's intrinsics work will know that it is at most a delegation to LLVM's intrinsics (and how to find what they do). They will first see the Rust source, anyways!
And in a sense, I think they aren't false friends: they are faithful implementations of va_copy and va_end... in Rust, a language which isn't C, but nonetheless implements some C semantics by assuming the C implementation can be made-compatible with Rust semantics.
We could name them rust_va_copy and rust_va_end if you really want to discourage people from assuming they are merely LLVM calls. I also do appreciate that you have spent a while with your head "in LLVM" and you have to make such a distinction between @llvm.va_copy and our code anyways. I think LLVM makes the right choice here... even if the intrinsic, in practice, isn't actually what clang uses 99.9% of the time (in terms of compiler invocations, anyways? maybe it's used for more targets in absolute count than I think it is, but then again LLVM doesn't have such a highly reified notion of target tuples like we do...).
|
...I also am aware that |
|
Makes sense, and yeah there are no obviously better names, so maybe consistency is more valuable. Let's see what Ralf thinks. |
|
Given that this is an internal API I'd rather have it use Rust-y terms than follow C terminology. We use the LLVM names for many of the intrinsics and this can be quite confusing. We can always explain the relation to But given that this is an internal API I also do not care very strongly. |
tracking issue: #44930
Implement
va_copyas (the rust equivalent of)memcpy, which is the behavior of all current LLVM targets. By providing our own implementation, we can guarantee its behavior. These guarantees are important for implementing c-variadics in e.g. const-eval.Discussed in #t-compiler/const-eval > c-variadics in const-eval.
I've also updated the comment for
Dropa bit. The background here is that the C standard requires thatva_endis used in the same function (and really, in the same scope) as the correspondingva_startorva_copy. That is because historicallyva_startwould start a scope, whichva_endwould then close. e.g.https://softwarepreservation.computerhistory.org/c_plus_plus/cfront/release_3.0.3/source/incl-master/proto-headers/stdarg.sol
The C standard still has to consider such implementations, but for Rust they are irrelevant. Hence we can use
Cloneforva_copyandDropforva_end.