c-variadic: more precise compatibility check in const-eval#155832
c-variadic: more precise compatibility check in const-eval#155832rust-bors[bot] merged 1 commit intorust-lang:mainfrom
Conversation
|
|
This comment has been minimized.
This comment has been minimized.
d15adf3 to
6aedccd
Compare
There was a problem hiding this comment.
This looks very nice, thanks! However I do think we can clean up those value checks.
@rustbot author
|
Reminder, once the PR becomes ready for a review, use |
This comment has been minimized.
This comment has been minimized.
11aa613 to
9e64ee5
Compare
| /// `T` and `U` are compatible, e.g. | ||
| /// | ||
| /// - They're the same type. | ||
| /// - One is `usize`/`isize`, the other same-sized fixed-width integer on that target. |
There was a problem hiding this comment.
This is new, and not reflected in the user-facing docs I think?
Also this should insist on same-signedness.
There was a problem hiding this comment.
This is me extrapolating, but on the ABI level there is no usize/isize right, it's just the register-sized integer type for the current target. So on a 64-bit target usize and u64 should be interchangeable I think?
There was a problem hiding this comment.
I guess the C wording implies that integer types of the same size and signedness are compatible.
| /// - `T` is a pointer to [`c_void`] and `U` is a pointer to [`i8]` or [`u8`]. | ||
| /// - `T` is a pointer to [`i8]` or [`u8`] and `U` is a pointer to [`c_void`]. |
There was a problem hiding this comment.
| /// - `T` is a pointer to [`c_void`] and `U` is a pointer to [`i8]` or [`u8`]. | |
| /// - `T` is a pointer to [`i8]` or [`u8`] and `U` is a pointer to [`c_void`]. | |
| /// - `T` is a pointer to [`c_void`] and `U` is a pointer to [`i8`] or [`u8`], or vice versa. |
| /// - `T` is a signed integer and `U` the corresponding unsigned integer, | ||
| /// - `T` is an unsigned integer and `U` the corresponding signed integer, |
There was a problem hiding this comment.
| /// - `T` is a signed integer and `U` the corresponding unsigned integer, | |
| /// - `T` is an unsigned integer and `U` the corresponding signed integer, | |
| /// - `T` is a signed integer and `U` the corresponding unsigned integer, or vice versa. |
| /// When `T` and `U` are both integer types, the value that the caller passes must be | ||
| /// representable by `T` and `U`. |
There was a problem hiding this comment.
Seems better to move that into the bullet it applies to, rather than adjusting the rules after the fact.
This comment has been minimized.
This comment has been minimized.
73da4e3 to
f52d039
Compare
|
@rustbot ready |
This comment has been minimized.
This comment has been minimized.
|
CI is red. EDIT: actually that seems spurious. |
|
yeah, also the last commit only changed comments, so it's likely to pass. This does probably need a squash + rebase before merging. |
| /// - `T` and `U` are integer types of the same size. The value that the caller passes | ||
| /// must be representable by `T` and `U`. |
There was a problem hiding this comment.
| /// - `T` and `U` are integer types of the same size. The value that the caller passes | |
| /// must be representable by `T` and `U`. | |
| /// - `T` and `U` are integer types of the same size. The value that the caller passes | |
| /// must be representable by both `T` and `U`. |
| /// - `T` and `U` are integer types of the same size. The value that the caller passes | ||
| /// must be representable by `T` and `U`. | ||
| /// - `T` and `U` are both pointers, and their target types are compatible. | ||
| /// - `T` is a pointer to [`c_void`] and `U` is a pointer to [`i8]` or [`u8`], or vice versa. |
There was a problem hiding this comment.
| /// - `T` is a pointer to [`c_void`] and `U` is a pointer to [`i8]` or [`u8`], or vice versa. | |
| /// - `T` is a pointer to [`c_void`] and `U` is a pointer to [`i8`] or [`u8`], or vice versa. |
| .validate_c_variadic_compatible_ty(*caller_target_ty, *callee_target_ty)? | ||
| { | ||
| VarArgCompatible::Incompatible => interp_ok(VarArgCompatible::Incompatible), | ||
| _ => interp_ok(VarArgCompatible::Compatible), |
There was a problem hiding this comment.
I just realized this diverges from the docs in a subtle way -- you are skipping the check that the value is representable in both types. C has separate notions of "compatible types" and "va_arg allowed" to account for that.
Please make that match here exhaustive and add a comment explaining why we can skip the value check. And then we'll also have to reword the docs.
This comment has been minimized.
This comment has been minimized.
|
Nice, thanks. :) r=me after squashing. |
b85cd8a to
c560774
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. |
|
@bors r=RalfJung |
…RalfJung c-variadic: more precise compatibility check in const-eval tracking issue: rust-lang#44930 This came up in the stabilization report discussion rust-lang#155697 (comment). As a reminder, this is what C says (in section 7.16.1.1 of the [C23 standard](https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3220.pdf#page=302). > If type is not compatible with the type of the actual next argument (as promoted according to the default argument promotions), the behavior is undefined, except for the following cases: > > - both types are pointers to qualified or unqualified versions of compatible types; > - one type is compatible with a signed integer type, the other type is compatible with the > corresponding unsigned integer type, and the value is representable in both types; > - one type is pointer to qualified or unqualified void and the other is a pointer to a qualified or > unqualified character type; > - 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 I think the last rule is not relevant for us, we don't really have an equivalent of `nullptr_t` as far as I know. r? RalfJung cc @tgross35
…RalfJung c-variadic: more precise compatibility check in const-eval tracking issue: rust-lang#44930 This came up in the stabilization report discussion rust-lang#155697 (comment). As a reminder, this is what C says (in section 7.16.1.1 of the [C23 standard](https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3220.pdf#page=302). > If type is not compatible with the type of the actual next argument (as promoted according to the default argument promotions), the behavior is undefined, except for the following cases: > > - both types are pointers to qualified or unqualified versions of compatible types; > - one type is compatible with a signed integer type, the other type is compatible with the > corresponding unsigned integer type, and the value is representable in both types; > - one type is pointer to qualified or unqualified void and the other is a pointer to a qualified or > unqualified character type; > - 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 I think the last rule is not relevant for us, we don't really have an equivalent of `nullptr_t` as far as I know. r? RalfJung cc @tgross35
…uwer Rollup of 22 pull requests Successful merges: - #154149 (resolve: Extend `ambiguous_import_visibilities` deprecation lint to glob-vs-glob ambiguities) - #155189 (simd_reduce_min/max: remove float support) - #155453 (apply Cortex-A53 errata 843419 mitigation to the AArch64 Linux targets) - #155562 (Add a missing `GenericTypeVisitable`, and avoid having interner traits for `FnSigKind` and `Abi`) - #155608 (rustc_middle: Implement the `partial_cmp` operation for `DefId`s) - #155721 (When archive format is wrong produce an error instead of ICE) - #155794 (privacy: share effective visibility initialization) - #155832 (c-variadic: more precise compatibility check in const-eval) - #155856 (std_detect: support detecting more features on aarch64 Windows) - #155861 (Suggest `[const] Trait` bounds in more places) - #155899 (`dlltool`: Set the working directory to workaround `--temp-prefix` bug) - #155916 (Update with new LLVM 22 target for `wasm32-wali-linux-musl` target) - #155935 (remap OUT_DIR paths to fix build script path leakage in crate metadata. ) - #155950 (use the new `//@ needs-asm-mnemonic: ret` more) - #155958 (ci(free-disk-space): remove more tools and fix warnings) - #155966 (miri subtree update) - #155711 (bump curl-sys and openssl-sys to support OpenSSL 4.0.x) - #155831 (Add `AcceptContext::expect_key_value`) - #155877 (Avoid misleading return-type note for foreign `Fn` callees) - #155949 (Update `opt_ast_lowering_delayed_lints` query to allow "stealing" lints, allowing to use `FnOnce` instead of `Fn`) - #155951 (Make `FlatMapInPlaceVec` an unsafe trait.) - #155967 (Fix `doc_cfg` feature for extern items)
…uwer Rollup of 21 pull requests Successful merges: - #155966 (miri subtree update) - #154149 (resolve: Extend `ambiguous_import_visibilities` deprecation lint to glob-vs-glob ambiguities) - #155189 (simd_reduce_min/max: remove float support) - #155562 (Add a missing `GenericTypeVisitable`, and avoid having interner traits for `FnSigKind` and `Abi`) - #155608 (rustc_middle: Implement the `partial_cmp` operation for `DefId`s) - #155721 (When archive format is wrong produce an error instead of ICE) - #155794 (privacy: share effective visibility initialization) - #155832 (c-variadic: more precise compatibility check in const-eval) - #155856 (std_detect: support detecting more features on aarch64 Windows) - #155861 (Suggest `[const] Trait` bounds in more places) - #155899 (`dlltool`: Set the working directory to workaround `--temp-prefix` bug) - #155916 (Update with new LLVM 22 target for `wasm32-wali-linux-musl` target) - #155935 (remap OUT_DIR paths to fix build script path leakage in crate metadata. ) - #155950 (use the new `//@ needs-asm-mnemonic: ret` more) - #155958 (ci(free-disk-space): remove more tools and fix warnings) - #155711 (bump curl-sys and openssl-sys to support OpenSSL 4.0.x) - #155831 (Add `AcceptContext::expect_key_value`) - #155877 (Avoid misleading return-type note for foreign `Fn` callees) - #155949 (Update `opt_ast_lowering_delayed_lints` query to allow "stealing" lints, allowing to use `FnOnce` instead of `Fn`) - #155951 (Make `FlatMapInPlaceVec` an unsafe trait.) - #155967 (Fix `doc_cfg` feature for extern items)
…uwer Rollup of 21 pull requests Successful merges: - rust-lang/rust#155966 (miri subtree update) - rust-lang/rust#154149 (resolve: Extend `ambiguous_import_visibilities` deprecation lint to glob-vs-glob ambiguities) - rust-lang/rust#155189 (simd_reduce_min/max: remove float support) - rust-lang/rust#155562 (Add a missing `GenericTypeVisitable`, and avoid having interner traits for `FnSigKind` and `Abi`) - rust-lang/rust#155608 (rustc_middle: Implement the `partial_cmp` operation for `DefId`s) - rust-lang/rust#155721 (When archive format is wrong produce an error instead of ICE) - rust-lang/rust#155794 (privacy: share effective visibility initialization) - rust-lang/rust#155832 (c-variadic: more precise compatibility check in const-eval) - rust-lang/rust#155856 (std_detect: support detecting more features on aarch64 Windows) - rust-lang/rust#155861 (Suggest `[const] Trait` bounds in more places) - rust-lang/rust#155899 (`dlltool`: Set the working directory to workaround `--temp-prefix` bug) - rust-lang/rust#155916 (Update with new LLVM 22 target for `wasm32-wali-linux-musl` target) - rust-lang/rust#155935 (remap OUT_DIR paths to fix build script path leakage in crate metadata. ) - rust-lang/rust#155950 (use the new `//@ needs-asm-mnemonic: ret` more) - rust-lang/rust#155958 (ci(free-disk-space): remove more tools and fix warnings) - rust-lang/rust#155711 (bump curl-sys and openssl-sys to support OpenSSL 4.0.x) - rust-lang/rust#155831 (Add `AcceptContext::expect_key_value`) - rust-lang/rust#155877 (Avoid misleading return-type note for foreign `Fn` callees) - rust-lang/rust#155949 (Update `opt_ast_lowering_delayed_lints` query to allow "stealing" lints, allowing to use `FnOnce` instead of `Fn`) - rust-lang/rust#155951 (Make `FlatMapInPlaceVec` an unsafe trait.) - rust-lang/rust#155967 (Fix `doc_cfg` feature for extern items)
View all comments
tracking issue: #44930
This came up in the stabilization report discussion #155697 (comment).
As a reminder, this is what C says (in section 7.16.1.1 of the C23 standard.
I think the last rule is not relevant for us, we don't really have an equivalent of
nullptr_tas far as I know.r? RalfJung
cc @tgross35