Skip to content
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

(c2rust-analyze) Relax the transmutable checks from two-way to one-way, now allowing for arrays and slices to decay #841

Merged

Conversation

kkysen
Copy link
Contributor

@kkysen kkysen commented Feb 16, 2023

Relaxed the transmutable checks from two-way to one-way, now allowing for arrays and slices to decay.

This expands the definition of safe transmutability to be one-way. That is, it checks if *T as *U is safe, rather than also *U as *T.

Thus, we can now allow for casts decaying pointers to arrays and slices to pointers to their element type.

do_unify is modified to also be one-way, which it was already in all call sites.

New tests are also added to string_casts.rs for all the types of ptr-to-ptr casts.

Out of the full string cast, b"" as *const u8 as *const core::ffi::c_char, this adds support for the as *const u8 (from &[u8; _]), so only support for the string literal itself remains.

@kkysen kkysen changed the title (c2rust-analyze) Relaxed the transmutable checks from two-way to one-way, now allowing for arrays and slices to decay (c2rust-analyze) Relax the transmutable checks from two-way to one-way, now allowing for arrays and slices to decay Feb 16, 2023
Comment on lines 205 to 225
/// type has a pointer, this function unifies the `PointerId`s that `lty1` and `lty2` have at
/// that position. For example, given `lty1 = *mut /*l1*/ *const /*l2*/ u8` and `lty2 = *mut
/// /*l3*/ *const /*l4*/ u8`, this function will unify `l1` with `l3` and `l2` with `l4`.
fn do_unify(&mut self, lty1: LTy<'tcx>, lty2: LTy<'tcx>) {
let ty1 = lty1.ty;
let ty2 = lty2.ty;
assert!(are_transmutable(
self.acx.tcx().erase_regions(ty1),
self.acx.tcx().erase_regions(ty2),
), "types not transmutable (compatible), so PointerId unification cannot be done: {ty1:?} !~ {ty2:?}");
for (sub_lty1, sub_lty2) in lty1.iter().zip(lty2.iter()) {
fn do_unify(&mut self, pl_lty: LTy<'tcx>, rv_lty: LTy<'tcx>) {
let pl_ty = pl_lty.ty;
let rv_ty = rv_lty.ty;
assert!(is_transmutable_to(
self.acx.tcx().erase_regions(rv_ty),
self.acx.tcx().erase_regions(pl_ty),
), "types not transmutable (compatible), so PointerId unification cannot be done: *{rv_ty:?} as *{pl_ty:?}");
for (sub_lty1, sub_lty2) in pl_lty.iter().zip(rv_lty.iter()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused here: are pl_lty née lty1 and rv_lty née lty2 the (possibly) pointer types, or types that have already been dereferenced? If the former, I don't see how their transmute compatibility can be handled by is_transmutable_to née are_transmutable, because that function doesn't do any dereferencing. If they've already been dereferenced, then the comment giving the example of lty1 = *mut /*l1*/ *const /*l2*/ u8 seems misleading.

In any event, update the lty1/lty2 names in the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused here: are pl_lty née lty1 and rv_lty née lty2 the (possibly) pointer types, or types that have already been dereferenced? If the former, I don't see how their transmute compatibility can be handled by is_transmutable_to née are_transmutable, because that function doesn't do any dereferencing. If they've already been dereferenced, then the comment giving the example of lty1 = *mut /*l1*/ *const /*l2*/ u8 seems misleading.

pl_ty and rv_ty are the possibly pointer types, but tcx.erase_regions strips the pointer (if it's there) from what I can tell.

In any event, update the lty1/lty2 names in the comment.

Thanks for catching that, I'll do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong, but I don't think erase_regions strips an outer pointer: looking at this source, I would expect fold_ty to do dispatch on the outer kind, but it only either dispatches to itself via the generic visitor (super_fold_with), or to a new instance of itself (self.tcx.erase_regions_ty(ty)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In any event, update the lty1/lty2 names in the comment.

Fixed in f05df65.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could be wrong, but I don't think erase_regions strips an outer pointer

Yeah, you're right. I misunderstood.

It seems that the pl_ty and rv_ty types passed to do_unify have already been dereferenced, which happens in do_equivalence_nested and other call sites. I'm not sure what the example means. @spernsteiner?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*const *mut *mut u8 is a legal type. do_equivalence_nested will strip off the outermost *const and pass *mut *mut u8 to do_unify.

@kkysen kkysen force-pushed the kkysen/analyze-one-way-string-casts branch 2 times, most recently from b84b412 to 45d9f83 Compare February 18, 2023 08:51
…e-way, now allowing for arrays and slices to decay.

This expands the definition of safe transmutability to be one-way.
That is, it checks if `*T as *U` is safe, rather than also `*U as *T`.

Thus, we can now allow for casts decaying
pointers to arrays and slices to pointers to their element type.

`do_unify` is modified to also be one-way,
which it was already in all call sites.

New tests are also added to `string_casts.rs`
for all the types of ptr-to-ptr casts.

Out of the full string cast, `b"" as *const u8 as *const core::ffi::c_char`,
this adds support for the `as *const u8` (from `&[u8; _]`),
so only support for the string literal itself remains.
… expanded defintion of safe transmutability.
@kkysen kkysen force-pushed the kkysen/analyze-one-way-string-casts branch from 45d9f83 to 26a4275 Compare February 19, 2023 05:11
@kkysen
Copy link
Contributor Author

kkysen commented Apr 26, 2023

I'm going to merge this into #839, then merge master into #839 (vs. a rebase since it's quite old and diverged and I don't want to lose the old history), fixup #839, and then have it reviewed before merging into master.

@kkysen kkysen merged commit eae9234 into kkysen/analyze-string-casts Apr 26, 2023
@kkysen kkysen deleted the kkysen/analyze-one-way-string-casts branch April 26, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants