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

Optionally derive Debug for structs that allow it #1040

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

dgherzka
Copy link
Contributor

@dgherzka dgherzka commented Nov 3, 2023

This is allowed for any struct that does not contain any unions recursively. The behavior
is optional and can be enabled with --derive Debug.

@dgherzka dgherzka changed the title Derive Debug for structs that allow it Optionally derive Debug for structs that allow it Nov 3, 2023
@dgherzka
Copy link
Contributor Author

dgherzka commented Nov 3, 2023

I actually meant to open this PR against my fork, not upstream. It may be kind of rough, but it would be helpful to know whether I'm at least on the right track!

@dgherzka
Copy link
Contributor Author

dgherzka commented Nov 3, 2023

Because I opened this against the wrong repo, it also contains #1030. Sorry, I'm a bit of a github noob.

@dgherzka
Copy link
Contributor Author

dgherzka commented Nov 3, 2023

The platform differences in va_list are making it hard to unit test this. On Linux, the struct (S3) has a lifetime parameter, but the return type of the translated fn that returns it doesn't have one, so the build fails (that must be a bug, right?). It's also unsafe to construct the object, so it can't just be a static because nothing will wrap it in unsafe {}. I'll see if just declaring pointers helps.

@kkysen
Copy link
Contributor

kkysen commented Nov 4, 2023

Because I opened this against the wrong repo, it also contains #1030. Sorry, I'm a bit of a github noob.

Usually I stack PRs, so PR #1 is merging into main from branch1 and PR #2 is merging into branch1 from branch2. Then when I merge PR #1 and delete branch1, it updates PR #2 to be merging into main from branch2. That's when the branches belong to the repo, though, and I'm not sure how it works for forks. When #1030 merges, though, this PR should fix itself, though you may need to rebase.

@@ -1194,6 +1194,12 @@ struct ConvertedVariable {
pub init: TranslationResult<WithStmts<Box<Expr>>>,
}

pub struct ConvertedStructFields {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This needed to be pub because convert_struct_fields is, but it seems like both of those should possibly be pub(crate). I didn't want to make that change because I didn't see very many instances of pub(crate) in c2rust-transpile. Should I leave that as is or make these crate-private?

Copy link
Contributor

Choose a reason for hiding this comment

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

pub should be fine here. We haven't been careful with the pub API for c2rust-transpile since it's meant to be used only from the CLI (being able to call it programmatically would be nice, but it's tricky, as the C++ clang part of it isn't entirely thread-safe). So since most of the types here are pub but probably shouldn't be, it's fine to make this one pub, too.

@dgherzka
Copy link
Contributor Author

dgherzka commented Nov 7, 2023

This is now rebased and no longer includes the commits that are already in #1030.

@dgherzka
Copy link
Contributor Author

Latest push changes this CLI argument and rebases the branch.

}

impl ExtraDerive {
fn to_transpiler_derive(&self) -> Derive {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are two different types because I didn't want to make c2rust-transpile depend on clap so it could implement ValueEnum. It works out because allowing --derive Clone wouldn't make much sense anyway if that's enabled by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. One minor (clippy) suggestion:

Suggested change
fn to_transpiler_derive(&self) -> Derive {
fn to_transpiler_derive(self) -> Derive {

Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

The way the derives are calculated now (including for the existing ones) is much more elegant (thanks!). I do have a bunch of suggestions, though, but it looks quite good so far.

.tcfg
.derives
.iter()
.flat_map(|derive| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.flat_map(|derive| {
.filter_map(|derive| {

They're actually functionally equivalent, but .filter_map is a bit clearer on the intent.

Comment on lines +1659 to +1670
.flat_map(|derive| {
let can_derive = match derive {
Derive::Clone | Derive::Copy => !contains_va_list,
Derive::Debug => can_derive_debug,
Derive::BitfieldStruct => has_bitfields,
};
if can_derive {
Some(derive.to_string())
} else {
None
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually you can split them up and it's much cleaner.

Suggested change
.flat_map(|derive| {
let can_derive = match derive {
Derive::Clone | Derive::Copy => !contains_va_list,
Derive::Debug => can_derive_debug,
Derive::BitfieldStruct => has_bitfields,
};
if can_derive {
Some(derive.to_string())
} else {
None
}
})
.filter(|derive| match derive {
Derive::Clone | Derive::Copy => !contains_va_list,
Derive::Debug => can_derive_debug,
Derive::BitfieldStruct => has_bitfields,
})
.map(|derive| format!("{derive:?}")

Comment on lines +1727 to +1737
.flat_map(|derive| {
let can_derive = match derive {
Derive::Clone | Derive::Copy | Derive::Debug => true,
Derive::BitfieldStruct => false,
};
if can_derive {
Some(derive.to_string())
} else {
None
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.flat_map(|derive| {
let can_derive = match derive {
Derive::Clone | Derive::Copy | Derive::Debug => true,
Derive::BitfieldStruct => false,
};
if can_derive {
Some(derive.to_string())
} else {
None
}
})
.filter(|derive| match derive {
Derive::Clone | Derive::Copy | Derive::Debug => true,
Derive::BitfieldStruct => false,
})
.map(|derive| format!("{derive:?}")

for field_decl_id in fields.as_ref().unwrap_or(&vec![]) {
let field_decl = self
.ast_context
.get_decl(&field_decl_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.get_decl(&field_decl_id)
.get_decl(field_decl_id)

Comment on lines +1660 to +1664
let can_derive = match derive {
Derive::Clone | Derive::Copy => !contains_va_list,
Derive::Debug => can_derive_debug,
Derive::BitfieldStruct => has_bitfields,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Writing the logic like this is much cleaner and easier to understand than how it was before. That's great!

Comment on lines +428 to +434
FieldType::Regular { field, ctype, .. } => {
// Struct is only debuggable if all regular fields are
// debuggable. Assume any fields added by the translator
// such as padding are debuggable
can_derive_debug &= self.can_struct_field_derive_debug(ctype);
field_entries.push(*field)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is more readable if it's computed similar to how contains_va_list is:

        // A struct is only debuggable if all regular fields are debuggable.
        // Assume any fields added by the translator, such as padding, are debuggable.
        let can_derive_debug = reorganized_fields.iter().all(|field| match field {
            &FieldType::Regular { ctype, .. } => self.can_struct_field_derive_debug(ctype),
            _ => true,
        });

// A union or struct containing a union cannot derive Debug
CTypeKind::Union(..) => false,

// All translated non-union C types can derive Debug
Copy link
Contributor

Choose a reason for hiding this comment

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

What about other recursive types like typedefs, arrays, etc.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh man, really good point

Comment on lines +280 to +281
/// Returns true if a struct field with this type can be part of a struct that derives `Debug`
fn can_struct_field_derive_debug(&self, ctype: CTypeId) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this only checking if a type recursively contains any unions? If so, can you rename the fn (and other variables, fields) to contains_union. That's more similar to contains_va_list and how that's used to calculate if Clone, Copy can be derived. And it can potentially be used by other derives that have similar requirements to Debug, as a bunch probably have similar behavior in regards to unions.

@@ -91,6 +93,14 @@ pub struct TranspilerConfig {
pub binaries: Vec<String>,
}

#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Display)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Display)]
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)]

Can we just use format!("{derive:?}") instead of depending on strum::Display? Or impl Display by just delegating to Debug (I also have another more complex idea that adds better checks that are useful elsewhere, but that's probably overkill for now).

} c;
} b;
} a;
} NotDebuggable1;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can test this is not Debug using static_assertions::assert_not_impl_any!. There's also a similar assert_impl_one! you can use to do the check you're already doing, that a type is Debug.

Copy link
Contributor

Choose a reason for hiding this comment

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

That way you also don't need to generate a value of the types, just the type itself.

None
}
})
.collect_vec();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.collect_vec();
.collect::<Vec<_>>();

Equally simple and doesn't require itertools; same with the other .collect_vec().

@dgherzka dgherzka marked this pull request as draft December 14, 2023 18:22
@dgherzka
Copy link
Contributor Author

Marking as a draft while I make the fixes. Thanks for all the feedback!

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