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

Conversation

kkysen
Copy link
Contributor

@kkysen kkysen commented Jun 9, 2023

3c03860 adds the simple (still incomplete) dataflow constraints for CastKind::Misc (fixes the minimum outlined in #839 (comment)). The full constraints are trickier since the types can be recursive, so I wanted to split this simpler part out into its own smaller PR first.

b20cea7 adds complete dataflow constraints for CastKind::Pointer, which is modeled on do_assign, but adjusted given its a ptr cast (fixes #839 (comment)).

…ointer` (previously incomplete).

More tests have also been added to `string_casts.rs`
exercising as many of the `PointerCast`s as possible,
but many of these crash later on, so they're disabled.

/// [`PointerCast::ReifyFnPointer`]
/// Can't figure out how to create a [`PointerCast::ReifyFnPointer`].
#[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

Copy link
Contributor

@aneksteind aneksteind left a comment

Choose a reason for hiding this comment

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

would like @spernsteiner's okay but this LGTM as a start

Comment on lines +148 to +159
// 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,
};
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.

Comment on lines +160 to +162
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));
}
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?

@kkysen kkysen force-pushed the kkysen/analyze-ptr-casts-dataflow branch from e02903c to 19ce2bd Compare June 12, 2023 20: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.

3 participants