Optionally derive `Debug` for structs that allow it #1040

dgherzka · 2023-11-03T19:54:00Z

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 · 2023-11-03T20:05:32Z

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 · 2023-11-03T20:10:17Z

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

dgherzka · 2023-11-03T23:16:30Z

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 · 2023-11-04T02:37:25Z

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.

c2rust-transpile/src/translator/structs.rs

c2rust-transpile/src/translator/mod.rs

dgherzka · 2023-11-07T16:22:04Z

c2rust-transpile/src/translator/mod.rs

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

+pub struct ConvertedStructFields {


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?

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 · 2023-11-07T16:22:35Z

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

tests/structs/src/debug_derive_bad.c

c2rust/src/bin/c2rust-transpile.rs

dgherzka · 2023-11-29T18:07:57Z

Latest push changes this CLI argument and rebases the branch.

dgherzka · 2023-11-29T18:11:41Z

c2rust/src/bin/c2rust-transpile.rs

+}
+
+impl ExtraDerive {
+    fn to_transpiler_derive(&self) -> Derive {


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.

That makes sense. One minor (clippy) suggestion:

Suggested change

fn to_transpiler_derive(&self) -> Derive {

fn to_transpiler_derive(self) -> Derive {

kkysen

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.

kkysen · 2023-12-01T08:49:32Z

c2rust-transpile/src/translator/mod.rs

+                    .tcfg
+                    .derives
+                    .iter()
+                    .flat_map(|derive| {


Suggested change

.flat_map(|derive| {

.filter_map(|derive| {

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

kkysen · 2023-12-01T10:34:02Z

c2rust-transpile/src/translator/mod.rs

+                    .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
+                        }
+                    })


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:?}")

kkysen · 2023-12-01T10:35:01Z

c2rust-transpile/src/translator/mod.rs

+                        .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
+                            }
+                        })


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:?}")

kkysen · 2023-12-01T10:37:31Z

c2rust-transpile/src/translator/structs.rs

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


Suggested change

.get_decl(&field_decl_id)

.get_decl(field_decl_id)

kkysen · 2023-12-01T10:38:14Z

c2rust-transpile/src/translator/mod.rs

+                        let can_derive = match derive {
+                            Derive::Clone | Derive::Copy => !contains_va_list,
+                            Derive::Debug => can_derive_debug,
+                            Derive::BitfieldStruct => has_bitfields,
+                        };


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

kkysen · 2023-12-01T10:49:23Z

c2rust-transpile/src/translator/structs.rs

+                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)
+                }


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, });

kkysen · 2023-12-01T10:53:29Z

c2rust-transpile/src/translator/structs.rs

+            // A union or struct containing a union cannot derive Debug
+            CTypeKind::Union(..) => false,
+
+            // All translated non-union C types can derive Debug


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

Oh man, really good point

kkysen · 2023-12-01T10:56:30Z

c2rust-transpile/src/translator/structs.rs

+    /// 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 {


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.

kkysen · 2023-12-01T10:58:33Z

c2rust-transpile/src/lib.rs

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

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


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).

kkysen · 2023-12-01T11:03:14Z

tests/structs/src/debug_derive.c

+            } c;
+        } b;
+    } a;
+} NotDebuggable1;


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.

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

kkysen · 2023-12-01T11:32:20Z

c2rust-transpile/src/translator/mod.rs

+                            None
+                        }
+                    })
+                    .collect_vec();


Suggested change

.collect_vec();

.collect::<Vec<_>>();

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

dgherzka · 2023-12-14T18:23:05Z

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

dgherzka changed the title ~~Derive Debug for structs that allow it~~ Optionally derive Debug for structs that allow it Nov 3, 2023

dgherzka mentioned this pull request Nov 3, 2023

derive(Debug) for translated types #1034

Open

kkysen reviewed Nov 4, 2023

View reviewed changes

c2rust-transpile/src/translator/structs.rs Outdated Show resolved Hide resolved

ahomescu reviewed Nov 7, 2023

View reviewed changes

c2rust-transpile/src/translator/mod.rs Outdated Show resolved Hide resolved

dgherzka force-pushed the debug_derive branch from ec8fdd3 to 785b4cf Compare November 7, 2023 16:18

dgherzka commented Nov 7, 2023

View reviewed changes

dgherzka requested review from kkysen and ahomescu November 7, 2023 16:23

kkysen reviewed Nov 16, 2023

View reviewed changes

tests/structs/src/debug_derive_bad.c Outdated Show resolved Hide resolved

kkysen reviewed Nov 16, 2023

View reviewed changes

c2rust/src/bin/c2rust-transpile.rs Outdated Show resolved Hide resolved

dgherzka added 12 commits November 28, 2023 17:44

[WIP] derive(Debug)

db2097b

More work on derive(Debug)

a3d33b9

Some cleanup, renaming

124cf0e

Test that va_list can be debugged

afca80f

Make derive(Debug) optional

2615986

cargo fmt

cafde45

Add feature_c_variadic to test

4637bb1

Work around unsafe static constructor for struct with va_list field

52ddae3

Try to fix tests again

670708d

Return struct fields in struct

beca631

Change CLI argument to --derive

501ec52

Make tests actually make sense

ceb8c28

dgherzka force-pushed the debug_derive branch from 785b4cf to ceb8c28 Compare November 29, 2023 18:06

dgherzka commented Nov 29, 2023

View reviewed changes

Don't use removeprefix

3b3e1c0

kkysen requested changes Dec 1, 2023

View reviewed changes

kkysen reviewed Dec 1, 2023

View reviewed changes

Make derive order stable

191e2f3

dgherzka marked this pull request as draft December 14, 2023 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally derive `Debug` for structs that allow it #1040

Optionally derive `Debug` for structs that allow it #1040

dgherzka commented Nov 3, 2023 •

edited

dgherzka commented Nov 3, 2023

dgherzka commented Nov 3, 2023

dgherzka commented Nov 3, 2023

kkysen commented Nov 4, 2023

dgherzka Nov 7, 2023

kkysen Nov 16, 2023

dgherzka commented Nov 7, 2023

dgherzka commented Nov 29, 2023

dgherzka Nov 29, 2023

kkysen Dec 1, 2023

kkysen left a comment

kkysen Dec 1, 2023

kkysen Dec 1, 2023

kkysen Dec 1, 2023

kkysen Dec 1, 2023

kkysen Dec 1, 2023

kkysen Dec 1, 2023

kkysen Dec 1, 2023

dgherzka Dec 1, 2023

kkysen Dec 1, 2023

kkysen Dec 1, 2023

kkysen Dec 1, 2023

kkysen Dec 1, 2023

kkysen Dec 1, 2023

dgherzka commented Dec 14, 2023

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

		/// 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 {

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

Optionally derive Debug for structs that allow it #1040

Are you sure you want to change the base?

Optionally derive Debug for structs that allow it #1040

Conversation

dgherzka commented Nov 3, 2023 • edited

dgherzka commented Nov 3, 2023

dgherzka commented Nov 3, 2023

dgherzka commented Nov 3, 2023

kkysen commented Nov 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgherzka commented Nov 7, 2023

dgherzka commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkysen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgherzka commented Dec 14, 2023

Optionally derive `Debug` for structs that allow it #1040

Optionally derive `Debug` for structs that allow it #1040

dgherzka commented Nov 3, 2023 •

edited