Add extra types to VaArgSafe - #162858
Add extra types to VaArgSafe#162858clarfonthey wants to merge 1 commit into
VaArgSafe#162858Conversation
|
@rfcbot merge libs,lang |
|
@clarfonthey has proposed to merge this. The next step is review by the rest of the tagged team members:
No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns. |
|
r? @Darksonn rustbot has assigned @Darksonn. Use Why was this reviewer chosen?The reviewer was selected based on:
|
006eb59 to
8007a9d
Compare
VaArgSafeVaArgSafe
|
The docs for
this should also be adjusted, but otherwise yeah this makes sense. @rfcbot reviewed |
b020d39 to
f84f398
Compare
|
Updated docs to remove note on everything being |
f84f398 to
7de1f76
Compare
|
The safety comments of |
There was a problem hiding this comment.
Can you add tests for these in tests/ui/c-variadic/roundtrip.rs?
There was a problem hiding this comment.
The result is very cursed but hopefully still achieves the desired result. Would appreciate an explicit review on those.
This comment has been minimized.
This comment has been minimized.
| #[stable(feature = "c_variadic_nonzero", since = "CURRENT_RUSTC_VERSION")] | ||
| unsafe impl<T: ZeroablePrimitive + VaArgSafe> VaArgSafe for NonZero<T> {} | ||
| #[stable(feature = "c_variadic_nonzero", since = "CURRENT_RUSTC_VERSION")] | ||
| unsafe impl<T: ZeroablePrimitive + VaArgSafe> VaArgSafe for Option<NonZero<T>> {} | ||
|
|
||
| #[stable(feature = "c_variadic_nonnull", since = "1.99.0")] | ||
| unsafe impl<T> VaArgSafe for NonNull<T> {} | ||
| #[stable(feature = "c_variadic_nonnull", since = "1.99.0")] | ||
| unsafe impl<T> VaArgSafe for Option<NonNull<T>> {} | ||
| #[stable(feature = "c_variadic", since = "1.99.0")] | ||
| unsafe impl<T> VaArgSafe for *mut T {} | ||
| #[stable(feature = "c_variadic", since = "1.99.0")] | ||
| unsafe impl<T> VaArgSafe for *const T {} | ||
| #[stable(feature = "c_variadic_refs", since = "CURRENT_RUSTC_VERSION")] | ||
| unsafe impl<T> VaArgSafe for &mut T {} | ||
| #[stable(feature = "c_variadic_refs", since = "CURRENT_RUSTC_VERSION")] | ||
| unsafe impl<T> VaArgSafe for &T {} |
There was a problem hiding this comment.
I assume the intent is that e.g. NonNull<T> is compatible with *mut T. That will need adjustments in the docs and corresponding adjustments in the const-eval / Miri checking logic.
There was a problem hiding this comment.
I thought that we already had this guarantee, but if we don't, I would rather not overcomplicate this PR and can remove it. (Note: this only needs to hold for sized pointees.)
There was a problem hiding this comment.
Rust guarantees they have the same layout, so it's just the docs of next_arg that should document that this equivalence applies here.
There was a problem hiding this comment.
Same layout != same ABI.
But we also guarantee that they have the same ABI.
This applies to most of your additions, e.g. NonZero<i32> vs i32.
There was a problem hiding this comment.
So, looking at the docs:
Unlike *mut T, the pointer must always be non-null, even if the pointer is never dereferenced. This is so that enums may use this forbidden value as a discriminant – Option<NonNull> has the same size as *mut T. However the pointer may still dangle if it isn’t dereferenced.
That feels pretty much like an explicit guarantee that *mut T, NonNull<T>, and Option<NonNull<T>> are layout-compatible, but I guess we should probably make it more explicit in the reference.
There was a problem hiding this comment.
From my understanding, variadic arguments simply require layout compatibility, since for example i32 and u32 are interchangeable in them.
If only :)
The comment on next_arg goes into detail on what it means for types to be compatible. It is much more subtle than layout compatibility. E.g. i32 and u32 are only compatible if the runtime value is representable in both types. (so like 3 is compatible but -3 is not.
There was a problem hiding this comment.
Oh, yes, that is cursed.
There was a problem hiding this comment.
Morally the same is true for the nonzero types and references (which cannot be NULL), but those requirements are already captured by the type itself in these new cases.
There was a problem hiding this comment.
The docs very explicitly say
The actual type of the argument U is compatible with T (as defined below).
So without changing the docs, if the caller uses NonZeroI32 and the callee uses next_arg::<i32>(), we very clearly have UB. (NonZeroI32 is not an "integer type". That term refers to primitive types, iN and uN.)
There was a problem hiding this comment.
Yes, I did explicitly go through and update the docs for those cases: the general rule for those is that if not all bit patterns are valid, you are guaranteeing that you have passed a valid bit pattern, at risk of UB.
2d31c99 to
ba1acb2
Compare
This comment has been minimized.
This comment has been minimized.
ba1acb2 to
8403324
Compare
|
PR for destabilizing |
This comment has been minimized.
This comment has been minimized.
d138f08 to
7400129
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6c40ebc to
b8c0d1d
Compare
|
Some changes occurred to the CTFE machinery Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred in compiler/rustc_attr_ir |
This comment has been minimized.
This comment has been minimized.
b8c0d1d to
2afc9be
Compare
This comment has been minimized.
This comment has been minimized.
2afc9be to
17e5462
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Because I'm feeling particularly spicy, is there a reason we shouldn't support Since we're already allowing invalid bit patterns, might as well. |
|
|
|
Never mind, then. That makes that easy. |
17e5462 to
71abb74
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. |
This comment has been minimized.
This comment has been minimized.
71abb74 to
3f842a8
Compare
There was a problem hiding this comment.
Congrats, you managed to achieve what many thought was impossible -- you made me think "that feels almost like there's too many comments here now." ;)
(Not really, it's fine, just more than I wold have written.)
Also when comments are full sentences please end them in a period. Too often have I seen people add more sentences to such comments without adding the periods which makes for extremely confusing times.
| } | ||
|
|
||
| match self.tcx.tcx.as_lang_item(adt.did()) { | ||
| // `NonNull` is allowed as just a pointer with fewer an alid values |
There was a problem hiding this comment.
| // `NonNull` is allowed as just a pointer with fewer an alid values | |
| // `NonNull` is allowed as just a pointer with fewer valid values. |
I assume?
| if self.layout_of(caller_type)?.size != self.layout_of(callee_type)?.size { | ||
| // All other cases require that the layout of the types match; | ||
| // in practice, since the only types that could get to this point are integers and pointers, | ||
| // the alignment check isn't necessary, but might as well verify |
There was a problem hiding this comment.
| // the alignment check isn't necessary, but might as well verify | |
| // the alignment check isn't necessary, but might as well verify. |
| let is_c_char = |ty: Ty<'_>| matches!(ty.kind(), ty::Uint(UintTy::U8) | ty::Int(IntTy::I8)); | ||
| // Since we've already checked for identical types and have narrowed down the layouts as | ||
| // being identical, we can filter out any non-integer, non-pointer types and similarly | ||
| // normalize integer and pointer types to make matching easier |
There was a problem hiding this comment.
| // normalize integer and pointer types to make matching easier | |
| // normalize integer and pointer types to make matching easier. |
| if (caller_target_ty.is_c_void(self.tcx.tcx) | ||
| || callee_target_ty.is_c_void(self.tcx.tcx)) | ||
| && (caller_target_ty.is_byte_sized_integral() | ||
| || callee_target_ty.is_byte_sized_integral()) |
There was a problem hiding this comment.
I think the version with the && inside and the || outside would be easier to match up with the text from the C standard that we are quoting above.
There was a problem hiding this comment.
Fair; I mostly just wanted to cover the case where the types were the same but technically under this condition without having to do any recursion.
| // Note: we already know that the layout is identical | ||
| if caller_signed == callee_signed { | ||
| // So, if the signedness is the same, this means that one of the types is | ||
| // `usize` or `isize`, which have the same ABI as their same-layout counterparts |
There was a problem hiding this comment.
| // `usize` or `isize`, which have the same ABI as their same-layout counterparts | |
| // `usize` or `isize`, which have the same ABI as their same-layout counterparts. |
| interp_ok(VarArgCompatible::Compatible) | ||
| } else { | ||
| // And if the signedness is different, this means that we're casting by changing | ||
| // the sign but not the size of the integer type, which is fine |
There was a problem hiding this comment.
| // the sign but not the size of the integer type, which is fine | |
| // the sign but not the size of the integer type, which is fine. |
| // - "or, the type of the next argument is nullptr_t and type is a pointer type that has the same | ||
| // representation and alignment requirements as a pointer to a character type" | ||
| // This one does not have an equivalent form in Rust. | ||
| _ => interp_ok(VarArgCompatible::Incompatible), |
There was a problem hiding this comment.
The fallback arm still exists, you just reordered it wrt the comment?
There was a problem hiding this comment.
Yeah, fair. I guess that my thought process was that the branch was covering more before, but, not really.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| /// - [`c_int`], [`c_long`] and [`c_longlong`] | ||
| /// - [`c_uint`], [`c_ulong`] and [`c_ulonglong`] | ||
| /// - [`c_double`] | ||
| /// - `*const T` and `*mut T` |
There was a problem hiding this comment.
We should probably expand this with the new types we guarantee impls for.
View all comments
Option 3 as proposed by @RalfJung here: #162478 (comment)
This adds the following types to
VaArgSafe:next_arg)NonNull<T>(for similar reasons)Option<NonNull<T>>(since it's ABI-compatible)NonZero<T>(since it's ABI-compatible)Option<NonZero<T>>(since it's ABI-compatible)Additionally, as part of implementing this in the compiler,
NonZerois promoted from a diagnostic item to a lang item. I'm under the impression this is fine since we're already doing a lang FCP (and none of the lang folks have chimed in yet) but it's worth mentioning.Also have a Zulip thread for the compiler impl: #t-compiler/help > `VaArgSafe` change + random `Ty` questions @ 💬