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: misc fixes for simple_buffer #1148

Merged
merged 11 commits into from
Nov 26, 2024
Merged

Conversation

spernsteiner
Copy link
Collaborator

This branch has fixes for various minor issues uncovered while testing on simple_buffer.c (#1103).

  • Fix borrowck handling of casts to *mut my_struct when my_struct has lifetimes
  • Fix various cases of compile errors introduced by DynOwned rewrites
  • Fix precedence of method calls when generating rewritten source code
  • Treat realloc(NULL, n) like malloc(n)
  • Support nullable pointers in the ZeroizeTy initializers used for malloc/memset rewrites

@spernsteiner spernsteiner requested a review from ahomescu October 22, 2024 22:53
@@ -361,7 +361,7 @@ impl<S: Sink> Emitter<'_, S> {
})
}
Rewrite::MethodCall(ref method, ref receiver_rw, ref arg_rws) => {
self.emit(receiver_rw, 0)?;
self.emit(receiver_rw, 3)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the magic 3 value here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Operator precedence, roughly. Things that bind less tightly than . need to be parenthesized when used as a method receiver. E.g. (2 + 2).method()

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 for it, and/or change it to a const with a good name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changing all the precedence values to named consts might be a good idea. I think that should be a separate PR, though

@@ -364,9 +365,14 @@ impl<'tcx> ConvertVisitor<'tcx> {
}
let expr = match (src_single, dest_single) {
(false, false) => {
let src = if option {
"src_ptr.unwrap_or(Box::new([]))"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this just be src_ptr.unwrap_or_default()? Or use unwrap_or_else so you're not constructing a Box every time?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Zero-sized boxes don't allocate - that includes boxed zero-length arrays, as in this case. As for unwrap_or_default, I generally prefer .unwrap_or_else(Vec::new) etc. to make it clear what the result type is intended to be.

#[c2rust_analyze_test::skip_borrowck]
// CHECK-LABEL: fn test_is_null(
pub unsafe fn test_is_null(cond: bool) {
// CHECK: let mut ptr: core::option::Option<core::result::Result<std::boxed::Box<(i32)>,()>> =
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it Box<(i32)>? Is that inner tuple needed? (or is it something else)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's not a tuple (1-tuples require a trailing comma, like (i32,)). When reusing part of a type annotation as written in the original source code (in this case, the i32 from *mut i32), we wrap the reused type in parentheses so we don't have to worry about precedence or other interactions with the surrounding rewrite. In theory, these parentheses should be easy to detect and remove with an "unused parens" lint in the cases where they're not needed.

@spernsteiner spernsteiner changed the base branch from analyze-simple-buffer-fixes-base to master November 22, 2024 18:41
@spernsteiner spernsteiner changed the base branch from master to analyze-simple-buffer-fixes-base November 22, 2024 18:41
@spernsteiner spernsteiner force-pushed the analyze-simple-buffer-fixes branch from 377e7d3 to faf35b8 Compare November 26, 2024 23:01
@spernsteiner spernsteiner force-pushed the analyze-simple-buffer-fixes-base branch from 7271647 to 644d80b Compare November 26, 2024 23:01
@spernsteiner spernsteiner changed the base branch from analyze-simple-buffer-fixes-base to master November 26, 2024 23:02
@spernsteiner spernsteiner force-pushed the analyze-simple-buffer-fixes branch from faf35b8 to 14cf393 Compare November 26, 2024 23:05
@spernsteiner spernsteiner merged commit 68de84c into master Nov 26, 2024
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.

2 participants