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

analyze: implement Box<T> rewrites #1106

Merged
merged 20 commits into from
Oct 24, 2024
Merged

analyze: implement Box<T> rewrites #1106

merged 20 commits into from
Oct 24, 2024

Conversation

spernsteiner
Copy link
Collaborator

This branch adds rewrites to convert pointers with the FREE permission into Box<T>. This follows the dynamic ownership tracking proposal in #1097.

Specific changes:

  • Adds a new dyn_owned: bool field to TypeDesc, which indicates that the pointer type should be wrapped in a dynamic ownership wrapper.
  • Extends ZeroizeType handling to support zero-initializing some pointer types (necessary to allow malloc/calloc of structs that contain pointers).
  • Adds support for Box and dyn_owned related casts in mir_op and rewrite::expr::convert.
  • Adds custom rewrite rules for malloc, calloc, free, and realloc.

@spernsteiner spernsteiner force-pushed the analyze-rewrite-box-base branch from 8a8bc96 to b3aecb0 Compare August 6, 2024 00:12
@spernsteiner spernsteiner changed the base branch from analyze-rewrite-box-base to master August 6, 2024 16:13
)
}
mir_op::RewriteKind::DynOwnedWrap => {
Rewrite::Call("std::result::Result::<_, ()>::Ok".to_string(), vec![hir_rw])
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't this just be an Option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It could, but DynOwned can be composed with Option<T> if we encounter a nullable owned pointer, and Option<DynOwned<T>> is clearer than Option<Option<T>>. But c2rust-analyze doesn't yet have a run-time support library where we could define DynOwned<T>, so for now we use the somewhat ugly Result<T, ()> in place of DynOwned<T>.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be useful to add a comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a comment on the translation of DynOwned types: 95d4038

@spernsteiner
Copy link
Collaborator Author

Looks like the Darwin CI error here is the same issue that's being fixed in #1146. Once that's merged, I'll rebase this and try the CI again.

@thedataking
Copy link
Contributor

Looks like the Darwin CI error here is the same issue that's being fixed in #1146. Once that's merged, I'll rebase this and try the CI again.

You are right. #1146 is merged now.

@spernsteiner spernsteiner merged commit 0872f48 into master Oct 24, 2024
8 of 9 checks passed
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