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) Add more (still incomplete) dataflow constraints for ptr casts #947

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions c2rust-analyze/src/dataflow/type_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use rustc_middle::mir::{
};
use rustc_middle::ty::adjustment::PointerCast;
use rustc_middle::ty::{SubstsRef, Ty, TyKind};
use std::iter;

/// Visitor that walks over the MIR, computing types of rvalues/operands/places and generating
/// constraints as a side effect.
Expand Down Expand Up @@ -135,8 +136,30 @@ impl<'tcx> TypeChecker<'tcx, '_> {
PointerCast::ArrayToPointer => {}
PointerCast::Unsize => {}
}
self.do_assign_pointer_ids(to_lty.label, from_lty.label)
// TODO add other dataflow constraints

// We would normally do `self.do_assign`,
// but we can't do a normal `self.do_equivalence_nested` (inside `self.do_assign`),
// so we do the `self.do_assign_pointer_ids` from `self.do_assign`,
// and then do the body of `self.do_equivalence_nested`,
// but after the extra outer layers are stripped.

self.do_assign_pointer_ids(to_lty.label, from_lty.label);

// Each of these ptr casts needs at least the outer layer stripped (e.x. `&`, `*mut`, `fn`),
// but then after that, these ptr casts below
// needs more layers stripped to reach the types that are equivalent.
let strip_from = |lty: LTy<'tcx>| match ptr_cast {
PointerCast::ArrayToPointer => lty.args[0],
PointerCast::Unsize => lty.args[0],
_ => lty,
};
let strip_to = |lty: LTy<'tcx>| match ptr_cast {
PointerCast::Unsize => lty.args[0],
_ => lty,
};
Comment on lines +148 to +159
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be clearer to match on the "from" and "to" TyKinds, or else to have a single case per PointerCast variant and assert that the types have a particular shape (for both sanity-checking and documentation purposes). As is, it's hard for me to tell whether or not these rules are correct. Also, I think matching on TyKinds is necessary to distinguish (x: &[u8; 3]) as &[u8] from (x: &[u8; 3]) as &dyn Debug (both are PointerCast::Unsize, but they need different handling here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point about dyn fat ptrs; I hadn't thought of them. Should I just error! on as &dyn _s since there shouldn't be any dyns in lighttpd or any other c2rust transpiled codebase?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, error! on dyn seems like the best approach for now.

for (&from_lty, &to_lty) in iter::zip(from_lty.args, to_lty.args) {
self.do_unify(strip_from(from_lty), strip_to(to_lty));
}
Comment on lines +160 to +162
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar to my previous comment, I think it would be good to be clear about how many args we expect to see for each PointerCast case. For example, Unsize should always have 1 arg in both from_lty and to_lty, while for UnsafeFnPointer we only expect that from_lty and to_lty will have the same amount.

I also think it's okay to panic on the tricky cases ReifyFnPointer and ClosureFnPointer for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How tricky is ReifyFnPointer? If it's not too complex, I'd rather not error on it as it is used commonly in lighttpd and other c2rust transpiled code for fn ptrs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

With ReifyFnPointer, I haven't thought about what constraints we ought to generate for pointer arguments in the function's signature. For example, given fn f(p: *mut /*p0*/ i32) and the expression f as fn(*mut /*p1*/ i32), do we require p1's permissions to be a subset of p0's, or do we require the reverse, or do we require the two to be equal?

}
CastKind::Misc => {
match is_transmutable_ptr_cast(from_ty, to_ty) {
Expand Down
54 changes: 54 additions & 0 deletions c2rust-analyze/tests/analyze/string_casts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,57 @@ pub fn cast_from_literal() {
pub fn cast_from_literal_explicit() {
std::ptr::addr_of!(*b"\0") as *const u8 as *const core::ffi::c_char;
}

/// [`PointerCast::ReifyFnPointer`]
/// Can't figure out how to create a [`PointerCast::ReifyFnPointer`].
kkysen marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(any())]
Copy link
Contributor

Choose a reason for hiding this comment

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

where do these tests crash downstream? create an issue outlining them?

Copy link
Contributor

Choose a reason for hiding this comment

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

i see the comments outline them, so not a huge priority to make issues of them

pub fn cast_fn_item_to_fn_ptr(f: impl Fn(u8) -> i8) {
f as fn(u8) -> i8;
}

/// [`PointerCast::UnsafeFnPointer`]
///
/// ```shell
/// thread 'rustc' panicked at 'not yet implemented', c2rust-analyze/src/labeled_ty.rs:372:17
/// ```
#[cfg(any())]
pub fn cast_fn_ptr_to_unsafe_fn_ptr(f: fn(u8) -> i8) {
f as unsafe fn(u8) -> i8;
}

/// [`PointerCast::ClosureFnPointer`]
/// Unhandled very early on.
#[cfg(any())]
pub fn cast_closure_to_fn_ptr() {
(|b: u8| b as i8) as fn(u8) -> i8;
}

/// [`PointerCast::MutToConstPointer`]
///
/// ```shell
/// thread 'rustc' panicked at 'not yet implemented', c2rust-analyze/src/labeled_ty.rs:372:17
/// ```
#[cfg(any())]
pub fn cast_mut_to_const_ptr(p: *mut i32) {
p as *const i32;
}

/// Meant to be [`PointerCast::ArrayToPointer`], but is [`CastKind::Misc`].
pub fn cast_array_ptr_to_ptr(p: *const [i32; 1]) {
p as *const i32;
}

/// [`PointerCast::Unsize`]
///
/// ```shell
/// thread 'rustc' panicked at 'expected to find only one Assign statement, but got multiple', c2rust-analyze/src/rewrite/expr/hir_op.rs:126:21
/// ```
#[cfg(any())]
pub fn cast_unsize_direct(a: &[i32; 1]) {
a as &[i32];
}

/// [`PointerCast::Unsize`]
pub fn cast_unsize_indirect(a: &[i32; 1]) {
let _ = a.as_ptr();
}