Guarantee result nonzero for FFI#110968
Conversation
Implements the required comipler changes for result_ffi_guarantees (rust-lang#3391) Allows `Result<T, E>` to be used in extern "C" functions where one of (T, E) is a non-zero type and one of (T, E) is a zero sized type. e.g. `Result<NonZeroI32, ()>` Adds test cases to ensure that the abi for Result and Option types with nonzero representation remains consistent
|
r? @jackh726 (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
r? compiler |
| let is_zst = |field: &FieldDef, variant: &VariantDef| { | ||
| let param_env = cx.tcx.param_env(variant.def_id); | ||
| let field_ty = field.ty(cx.tcx, substs); | ||
| cx.tcx | ||
| .layout_of(param_env.and(field_ty)) | ||
| .map_or(false, |layout| layout.is_zst()) | ||
| }; |
There was a problem hiding this comment.
This is not sufficient, see the RFC:
Result<T, E>where eitherTorEmeet all of the following conditions:
- Is a zero-sized type with alignment 1 (a "1-ZST").
- Has no fields.
- Does not have the
#[non_exhaustive]attribute.
(emph. mine)
| @@ -770,8 +773,26 @@ pub(crate) fn repr_nullable_ptr<'tcx>( | |||
| debug!("is_repr_nullable_ptr(cx, ty = {:?})", ty); | |||
There was a problem hiding this comment.
Please also update the documentation comment on this function.
| ) -> FfiResult<'tcx> { | ||
| let field_ty = field.ty(self.cx.tcx, substs); | ||
| if field_ty.has_opaque_types() { | ||
| if field_ty.is_unit() && allow_unit_type { |
There was a problem hiding this comment.
This only checks for unit, shouldn't it be the same check as above? (ZST + no fields + exhaustive)
| @@ -770,8 +773,26 @@ pub(crate) fn repr_nullable_ptr<'tcx>( | |||
| debug!("is_repr_nullable_ptr(cx, ty = {:?})", ty); | |||
There was a problem hiding this comment.
Also, the doc comment seems to suggest we have similar code in codegen, but I don't see it changed in this PR.
WaffleLapkin
added
S-waiting-on-author
|
cc @Lokathor |
|
☔ The latest upstream changes (presumably #113591) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@zacklukem FYI: when a PR is ready for review, send a message containing |
| [variant1, variant2] => match (&variant1.fields.raw[..], &variant2.fields.raw[..]) { | ||
| // If one variant is empty and the other is a single field (e.g. Option<T>) | ||
| ([], [field]) | ([field], []) => field.ty(cx.tcx, substs), | ||
| // If one variant has a ZST and the other has a single field (e.g. Result<T, ()> or Result<(), T>) |
There was a problem hiding this comment.
All of these uses of "ZST" here should become "1-ZST" -- it is quite important that we exclude ZST with alignment requirements. There now is a layout.is_1zst method that can and should be used here.
|
Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks |
Dylan-DPC
added
S-inactive
9 participants
Implements the required comipler changes for result_ffi_guarantees (rust-lang/rfcs#3391)
Allows
Result<T, E>to be used inextern "C"functions where one of (T, E) is a non-zero type and one of (T, E) is a zero sized type. e.g.Result<NonZeroI32, ()>Adds test cases to ensure that the abi for Result and Option types with nonzero representation remains consistent
tracking issue: #110503