-
Notifications
You must be signed in to change notification settings - Fork 242
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
base: master
Are you sure you want to change the base?
Changes from 12 commits
db2097b
a3d33b9
124cf0e
afca80f
2615986
cafde45
4637bb1
52ddae3
670708d
beca631
501ec52
ceb8c28
3b3e1c0
191e2f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -11,6 +11,7 @@ use dtoa; | |||||||||||||||||||||||||||||||||||||
use failure::{err_msg, format_err, Fail}; | ||||||||||||||||||||||||||||||||||||||
use indexmap::indexmap; | ||||||||||||||||||||||||||||||||||||||
use indexmap::{IndexMap, IndexSet}; | ||||||||||||||||||||||||||||||||||||||
use itertools::Itertools; | ||||||||||||||||||||||||||||||||||||||
use log::{error, info, trace, warn}; | ||||||||||||||||||||||||||||||||||||||
use proc_macro2::{Punct, Spacing::*, Span, TokenStream, TokenTree}; | ||||||||||||||||||||||||||||||||||||||
use syn::spanned::Spanned as _; | ||||||||||||||||||||||||||||||||||||||
|
@@ -27,12 +28,12 @@ use c2rust_ast_builder::{mk, properties::*, Builder}; | |||||||||||||||||||||||||||||||||||||
use c2rust_ast_printer::pprust::{self}; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
use crate::c_ast::iterators::{DFExpr, SomeId}; | ||||||||||||||||||||||||||||||||||||||
use crate::c_ast::*; | ||||||||||||||||||||||||||||||||||||||
use crate::cfg; | ||||||||||||||||||||||||||||||||||||||
use crate::convert_type::TypeConverter; | ||||||||||||||||||||||||||||||||||||||
use crate::renamer::Renamer; | ||||||||||||||||||||||||||||||||||||||
use crate::with_stmts::WithStmts; | ||||||||||||||||||||||||||||||||||||||
use crate::{c_ast, format_translation_err}; | ||||||||||||||||||||||||||||||||||||||
use crate::{c_ast::*, Derive}; | ||||||||||||||||||||||||||||||||||||||
use crate::{ExternCrate, ExternCrateDetails, TranspilerConfig}; | ||||||||||||||||||||||||||||||||||||||
use c2rust_ast_exporter::clang_ast::LRValue; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
@@ -1194,6 +1195,12 @@ struct ConvertedVariable { | |||||||||||||||||||||||||||||||||||||
pub init: TranslationResult<WithStmts<Box<Expr>>>, | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
pub struct ConvertedStructFields { | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needed to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||||||||||||||||||||||||||||||||
pub field_entries: Vec<Field>, | ||||||||||||||||||||||||||||||||||||||
pub contains_va_list: bool, | ||||||||||||||||||||||||||||||||||||||
pub can_derive_debug: bool, | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
impl<'c> Translation<'c> { | ||||||||||||||||||||||||||||||||||||||
pub fn new( | ||||||||||||||||||||||||||||||||||||||
mut ast_context: TypedAstContext, | ||||||||||||||||||||||||||||||||||||||
|
@@ -1628,14 +1635,12 @@ impl<'c> Translation<'c> { | |||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
// Gather up all the field names and field types | ||||||||||||||||||||||||||||||||||||||
let (field_entries, contains_va_list) = | ||||||||||||||||||||||||||||||||||||||
self.convert_struct_fields(decl_id, fields, platform_byte_size)?; | ||||||||||||||||||||||||||||||||||||||
let ConvertedStructFields { | ||||||||||||||||||||||||||||||||||||||
field_entries, | ||||||||||||||||||||||||||||||||||||||
contains_va_list, | ||||||||||||||||||||||||||||||||||||||
can_derive_debug, | ||||||||||||||||||||||||||||||||||||||
} = self.convert_struct_fields(decl_id, fields, platform_byte_size)?; | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
let mut derives = vec![]; | ||||||||||||||||||||||||||||||||||||||
if !contains_va_list { | ||||||||||||||||||||||||||||||||||||||
derives.push("Copy"); | ||||||||||||||||||||||||||||||||||||||
derives.push("Clone"); | ||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||
let has_bitfields = | ||||||||||||||||||||||||||||||||||||||
fields | ||||||||||||||||||||||||||||||||||||||
.iter() | ||||||||||||||||||||||||||||||||||||||
|
@@ -1644,10 +1649,27 @@ impl<'c> Translation<'c> { | |||||||||||||||||||||||||||||||||||||
_ => unreachable!("Found non-field in record field list"), | ||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||
if has_bitfields { | ||||||||||||||||||||||||||||||||||||||
derives.push("BitfieldStruct"); | ||||||||||||||||||||||||||||||||||||||
self.use_crate(ExternCrate::C2RustBitfields); | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
let derives = self | ||||||||||||||||||||||||||||||||||||||
.tcfg | ||||||||||||||||||||||||||||||||||||||
.derives | ||||||||||||||||||||||||||||||||||||||
.iter() | ||||||||||||||||||||||||||||||||||||||
.flat_map(|derive| { | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
They're actually functionally equivalent, but |
||||||||||||||||||||||||||||||||||||||
let can_derive = match derive { | ||||||||||||||||||||||||||||||||||||||
Derive::Clone | Derive::Copy => !contains_va_list, | ||||||||||||||||||||||||||||||||||||||
Derive::Debug => can_derive_debug, | ||||||||||||||||||||||||||||||||||||||
Derive::BitfieldStruct => has_bitfields, | ||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+1660
to
+1664
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! |
||||||||||||||||||||||||||||||||||||||
if can_derive { | ||||||||||||||||||||||||||||||||||||||
Some(derive.to_string()) | ||||||||||||||||||||||||||||||||||||||
} else { | ||||||||||||||||||||||||||||||||||||||
None | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+1659
to
+1670
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||||||||||||||||||||||||||||||||||
.collect_vec(); | ||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Equally simple and doesn't require |
||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
let mut reprs = vec![simple_metaitem("C")]; | ||||||||||||||||||||||||||||||||||||||
let max_field_alignment = if is_packed { | ||||||||||||||||||||||||||||||||||||||
// `__attribute__((packed))` forces a max alignment of 1, | ||||||||||||||||||||||||||||||||||||||
|
@@ -1697,10 +1719,28 @@ impl<'c> Translation<'c> { | |||||||||||||||||||||||||||||||||||||
]; | ||||||||||||||||||||||||||||||||||||||
let repr_attr = mk().meta_list("repr", outer_reprs); | ||||||||||||||||||||||||||||||||||||||
let outer_field = mk().pub_().enum_field(mk().ident_ty(inner_name)); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
let outer_struct_derives = self | ||||||||||||||||||||||||||||||||||||||
.tcfg | ||||||||||||||||||||||||||||||||||||||
.derives | ||||||||||||||||||||||||||||||||||||||
.iter() | ||||||||||||||||||||||||||||||||||||||
.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 | ||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||
Comment on lines
+1727
to
+1737
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||||||
.collect_vec(); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
let outer_struct = mk() | ||||||||||||||||||||||||||||||||||||||
.span(span) | ||||||||||||||||||||||||||||||||||||||
.pub_() | ||||||||||||||||||||||||||||||||||||||
.call_attr("derive", vec!["Copy", "Clone"]) | ||||||||||||||||||||||||||||||||||||||
.call_attr("derive", outer_struct_derives) | ||||||||||||||||||||||||||||||||||||||
.meta_item_attr(AttrStyle::Outer, repr_attr) | ||||||||||||||||||||||||||||||||||||||
.struct_item(name, vec![outer_field], true); | ||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -6,13 +6,14 @@ use std::collections::HashSet; | |||||
use std::ops::Index; | ||||||
|
||||||
use super::named_references::NamedReference; | ||||||
use super::TranslationError; | ||||||
use crate::c_ast::{BinOp, CDeclId, CDeclKind, CExprId, CRecordId, CTypeId}; | ||||||
use super::{ConvertedStructFields, TranslationError}; | ||||||
use crate::c_ast::{BinOp, CDeclId, CDeclKind, CExprId, CRecordId, CTypeId, CTypeKind}; | ||||||
use crate::diagnostics::TranslationResult; | ||||||
use crate::translator::{ExprContext, Translation, PADDING_SUFFIX}; | ||||||
use crate::with_stmts::WithStmts; | ||||||
use c2rust_ast_builder::mk; | ||||||
use c2rust_ast_printer::pprust; | ||||||
use failure::format_err; | ||||||
use syn::{ | ||||||
self, AttrStyle, BinOp as RBinOp, Expr, ExprAssign, ExprAssignOp, ExprBinary, ExprBlock, | ||||||
ExprCast, ExprMethodCall, ExprUnary, Field, Meta, NestedMeta, Stmt, Type, | ||||||
|
@@ -276,6 +277,52 @@ impl<'a> Translation<'a> { | |||||
Ok(reorganized_fields) | ||||||
} | ||||||
|
||||||
/// 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 { | ||||||
Comment on lines
+280
to
+281
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this only checking if a type recursively contains any |
||||||
let ty = self.ast_context.resolve_type(ctype); | ||||||
let can_derive_debug = match ty.kind { | ||||||
// Recurse into struct fields. A struct is debuggable iff all of its fields are | ||||||
CTypeKind::Struct(decl_id) => { | ||||||
let decl = self | ||||||
.ast_context | ||||||
.get_decl(&decl_id) | ||||||
.ok_or_else(|| format_err!("Missing decl {:?}", decl_id)) | ||||||
.unwrap(); | ||||||
match &decl.kind { | ||||||
CDeclKind::Struct { fields, .. } => { | ||||||
for field_decl_id in fields.as_ref().unwrap_or(&vec![]) { | ||||||
let field_decl = self | ||||||
.ast_context | ||||||
.get_decl(&field_decl_id) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
.ok_or_else(|| format_err!("Missing decl {:?}", field_decl_id)) | ||||||
.unwrap(); | ||||||
match &field_decl.kind { | ||||||
CDeclKind::Field { typ, .. } => { | ||||||
let can_derive_debug = | ||||||
self.can_struct_field_derive_debug(typ.ctype); | ||||||
if !can_derive_debug { | ||||||
return false; | ||||||
} | ||||||
} | ||||||
_ => panic!("not a field"), | ||||||
} | ||||||
} | ||||||
true | ||||||
} | ||||||
_ => panic!("not a struct"), | ||||||
} | ||||||
} | ||||||
|
||||||
// A union or struct containing a union cannot derive Debug | ||||||
CTypeKind::Union(..) => false, | ||||||
|
||||||
// All translated non-union C types can derive Debug | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about other recursive types like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh man, really good point |
||||||
_ => true, | ||||||
}; | ||||||
|
||||||
can_derive_debug | ||||||
} | ||||||
|
||||||
/// Here we output a struct derive to generate bitfield data that looks like this: | ||||||
/// | ||||||
/// ```no_run | ||||||
|
@@ -296,7 +343,7 @@ impl<'a> Translation<'a> { | |||||
struct_id: CRecordId, | ||||||
field_ids: &[CDeclId], | ||||||
platform_byte_size: u64, | ||||||
) -> TranslationResult<(Vec<Field>, bool)> { | ||||||
) -> TranslationResult<ConvertedStructFields> { | ||||||
let mut field_entries = Vec::with_capacity(field_ids.len()); | ||||||
// We need to clobber bitfields in consecutive bytes together (leaving | ||||||
// regular fields alone) and add in padding as necessary | ||||||
|
@@ -317,6 +364,7 @@ impl<'a> Translation<'a> { | |||||
field_name | ||||||
}; | ||||||
|
||||||
let mut can_derive_debug = true; | ||||||
for field_type in reorganized_fields { | ||||||
match field_type { | ||||||
FieldType::BitfieldGroup { | ||||||
|
@@ -377,10 +425,20 @@ impl<'a> Translation<'a> { | |||||
|
||||||
field_entries.push(field); | ||||||
} | ||||||
FieldType::Regular { field, .. } => field_entries.push(*field), | ||||||
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) | ||||||
} | ||||||
Comment on lines
+428
to
+434
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 // 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,
}); |
||||||
} | ||||||
} | ||||||
Ok((field_entries, contains_va_list)) | ||||||
Ok(ConvertedStructFields { | ||||||
field_entries, | ||||||
contains_va_list, | ||||||
can_derive_debug, | ||||||
}) | ||||||
} | ||||||
|
||||||
/// Here we output a block to generate a struct literal initializer in. | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -3,7 +3,9 @@ use log::LevelFilter; | |||||
use regex::Regex; | ||||||
use std::{fs, path::PathBuf}; | ||||||
|
||||||
use c2rust_transpile::{Diagnostic, ReplaceMode, TranspilerConfig}; | ||||||
use c2rust_transpile::{Derive, Diagnostic, ReplaceMode, TranspilerConfig}; | ||||||
|
||||||
const DEFAULT_DERIVES: &[Derive] = &[Derive::Clone, Derive::Copy, Derive::BitfieldStruct]; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are less default derives as they are necessary derives ( impl Derive {
/// The derives that are necessary for compilation.
const NECESSARY: &[Self] = &[Self::Clone, Self::Copy, Self::BitfieldStruct];
} |
||||||
|
||||||
#[derive(Debug, Parser)] | ||||||
#[clap( | ||||||
|
@@ -155,6 +157,17 @@ struct Args { | |||||
/// Fail when the control-flow graph generates branching constructs | ||||||
#[clap(long)] | ||||||
fail_on_multiple: bool, | ||||||
|
||||||
/// Add extra derived traits to generated structs in addition to the default | ||||||
/// set of derives (Copy, Clone, BitfieldStruct). Specify multiple times to | ||||||
/// add more than one derive. A struct will derive all traits in the set for | ||||||
/// which it is eligible. | ||||||
/// | ||||||
/// For example, a struct containing a union cannot derive Debug, so | ||||||
/// `#[derive(Debug)]` will not be added to that struct regardless of | ||||||
/// whether `--derive Debug` is specified. | ||||||
#[clap(long = "derive", value_enum, value_name = "TRAIT")] | ||||||
extra_derives: Vec<ExtraDerive>, | ||||||
Comment on lines
+168
to
+170
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It still is |
||||||
} | ||||||
|
||||||
#[derive(Debug, PartialEq, Eq, ValueEnum, Clone)] | ||||||
|
@@ -164,9 +177,29 @@ enum InvalidCodes { | |||||
CompileError, | ||||||
} | ||||||
|
||||||
#[derive(Clone, Copy, Debug, ValueEnum)] | ||||||
#[clap(rename_all = "PascalCase")] | ||||||
enum ExtraDerive { | ||||||
Debug, | ||||||
} | ||||||
|
||||||
impl ExtraDerive { | ||||||
fn to_transpiler_derive(&self) -> Derive { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense. One minor (
Suggested change
|
||||||
match self { | ||||||
Self::Debug => Derive::Debug, | ||||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
fn main() { | ||||||
let args = Args::parse(); | ||||||
|
||||||
let derives = DEFAULT_DERIVES | ||||||
.iter() | ||||||
.cloned() | ||||||
.chain(args.extra_derives.iter().map(|d| d.to_transpiler_derive())) | ||||||
.collect(); | ||||||
|
||||||
// Build a TranspilerConfig from the command line | ||||||
let mut tcfg = TranspilerConfig { | ||||||
dump_untyped_context: args.dump_untyped_clang_ast, | ||||||
|
@@ -216,6 +249,7 @@ fn main() { | |||||
emit_no_std: args.emit_no_std, | ||||||
enabled_warnings: args.warn.into_iter().collect(), | ||||||
log_level: args.log_level, | ||||||
derives, | ||||||
}; | ||||||
// binaries imply emit-build-files | ||||||
if !tcfg.binaries.is_empty() { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
//! derive:Debug | ||
|
||
#include <stdarg.h> | ||
|
||
typedef struct { | ||
int a; | ||
} Debuggable1; | ||
|
||
typedef struct { | ||
va_list v; | ||
} Debuggable2; | ||
|
||
Debuggable1 *kDebuggable1; | ||
Debuggable2 *kDebuggable2; | ||
|
||
// A struct containing a union cannot derive Debug, so make | ||
// sure we don't generate a #[derive] for it | ||
typedef struct { | ||
struct { | ||
struct { | ||
union { | ||
int d; | ||
float e; | ||
} c; | ||
} b; | ||
} a; | ||
} NotDebuggable1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can test this is not There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
||
NotDebuggable1 kNotDebuggable1; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
//! feature_c_variadic | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed? |
||
|
||
use crate::debug_derive::{rust_kDebuggable1, rust_kDebuggable2}; | ||
use std::fmt::Debug; | ||
|
||
pub fn test_debuggable() { | ||
unsafe { | ||
// Make sure all debuggable structs implement `Debug` | ||
let _debuggable: Vec<*mut dyn Debug> = vec![rust_kDebuggable1, rust_kDebuggable2]; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just use
format!("{derive:?}")
instead of depending onstrum::Display
? Orimpl Display
by just delegating toDebug
(I also have another more complex idea that adds better checks that are useful elsewhere, but that's probably overkill for now).