diff --git a/c2rust-refactor/src/context.rs b/c2rust-refactor/src/context.rs index c4db085547..e7758c6149 100644 --- a/c2rust-refactor/src/context.rs +++ b/c2rust-refactor/src/context.rs @@ -1147,12 +1147,17 @@ pub struct CalleeInfo<'tcx> { type DefMapping = HashMap; +#[derive(Clone, Copy)] pub struct TypeCompare<'a, 'tcx: 'a, 'b> { cx: &'a RefactorCtxt<'a, 'tcx>, /// Mapping from old DefId to new DefId for defs that have been replaced /// after types were resolved. def_mapping: Option<&'b DefMapping>, + + /// Require array lengths to match exactly, instead of letting a + /// zero-length array match an array of any length. + exact_array_lens: bool, } impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { @@ -1160,6 +1165,7 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { Self { cx, def_mapping: None, + exact_array_lens: false, } } @@ -1167,6 +1173,21 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { Self { cx, def_mapping: Some(def_mapping), + exact_array_lens: false, + } + } + + /// Return a copy of this comparison that requires array lengths to match + /// exactly. + /// + /// The zero-length leniency exists for an `extern` array declaration, + /// which C lets omit the length that its definition gives. Nothing else + /// makes a zero-length array interchangeable with a longer one: they are + /// distinct Rust types, and Rust will not coerce between them. + fn with_exact_array_lens(self) -> Self { + Self { + exact_array_lens: true, + ..self } } @@ -1378,6 +1399,11 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { /// Compare two function declarations for equivalent argument and return types, /// ignoring argument names. pub fn compatible_fn_prototypes(&self, decl1: &FnDecl, decl2: &FnDecl) -> bool { + // A parameter or return type is passed by value at every call site, so + // the two signatures have to agree on it exactly; the zero-length + // array leniency only holds for an `extern` array declaration. + let strict_cmp = self.with_exact_array_lens(); + // `zip` below stops at the shorter parameter list, so the lengths have // to be compared separately. Otherwise a declaration is compatible // with any other one that merely extends it, which is exactly the @@ -1389,7 +1415,7 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { } let mut args = decl1.inputs.iter().zip(decl2.inputs.iter()); - if !args.all(|(arg1, arg2)| self.structural_eq_ast_tys(&arg1.ty, &arg2.ty, true)) { + if !args.all(|(arg1, arg2)| strict_cmp.structural_eq_ast_tys(&arg1.ty, &arg2.ty, true)) { return false; } @@ -1405,12 +1431,16 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { FnRetTy::Ty(ty) => &ty, }; - self.structural_eq_ast_tys(ty1, ty2, true) + strict_cmp.structural_eq_ast_tys(ty1, ty2, true) } /// Compare two ty function signatures for equivalent argument and return /// types, ignoring argument names. pub fn compatible_fn_sigs(&self, sig1: &FnSig<'tcx>, sig2: &FnSig<'tcx>) -> bool { + // See `compatible_fn_prototypes` for why a signature compares array + // lengths exactly. + let strict_cmp = self.with_exact_array_lens(); + if sig1.inputs().len() != sig2.inputs().len() { return false; } @@ -1420,14 +1450,14 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { } for (&arg_ty1, &arg_ty2) in sig1.inputs().iter().zip(sig2.inputs().iter()) { - if !self.structural_eq_tys_with_vis(arg_ty1, arg_ty2) { + if !strict_cmp.structural_eq_tys_with_vis(arg_ty1, arg_ty2) { return false; } } let out_ty1 = sig1.output(); let out_ty2 = sig2.output(); - self.structural_eq_tys_with_vis(out_ty1, out_ty2) + strict_cmp.structural_eq_tys_with_vis(out_ty1, out_ty2) } /// Compare two AST types for structural equivalence, ignoring names. @@ -1559,7 +1589,8 @@ impl<'a, 'tcx, 'b> TypeCompare<'a, 'tcx, 'b> { // array types with global array definitions, but it should be // apply in practice as we translate empty extern array lengths // into 0 length extern arrays. - if len1 != len2 && len1 != Some(0) && len2 != Some(0) { + let lenient = !self.exact_array_lens && (len1 == Some(0) || len2 == Some(0)); + if len1 != len2 && !lenient { trace!("Array lengths don't match: {:?} and {:?}", n1, n2); return false; } diff --git a/c2rust-refactor/src/transform/reorganize_definitions.rs b/c2rust-refactor/src/transform/reorganize_definitions.rs index df617c506c..e64f73b6fc 100644 --- a/c2rust-refactor/src/transform/reorganize_definitions.rs +++ b/c2rust-refactor/src/transform/reorganize_definitions.rs @@ -2180,7 +2180,13 @@ impl<'a, 'tcx> HeaderDeclarations<'a, 'tcx> { ( ForeignItemKind::Fn(box Fn { sig: sig1, .. }), ForeignItemKind::Fn(box Fn { sig: sig2, .. }), - ) => self.cx.compatible_fn_prototypes(&sig1.decl, &sig2.decl), + ) => compatible_foreign_fns( + self.cx, + existing_foreign, + item, + &sig1.decl, + &sig2.decl, + ), _ => existing_foreign.ast_equiv(item), }; @@ -2210,6 +2216,43 @@ enum ContainsDecl<'a> { Use(&'a mut MovedDecl), } +// Returns `true` if two foreign function declarations declare the same +// function, and so may be collapsed into one. +// +// The two declarations come from different modules, so comparing their +// written-out signatures is not enough: a parameter spelled `*mut stat` in +// each can name a different `stat` in each module, and merging on that basis +// silently repoints one module's calls at the other module's type. A foreign +// item has no body, so its signature carries no node types for +// `compatible_fn_prototypes` to compare, and it falls back to comparing the +// syntax. Compare the resolved signatures instead, and only fall back to the +// syntactic comparison when a signature cannot be resolved. +// +// Resolution fails (`no_bound_vars` returns `None`) when a signature contains +// late-bound lifetimes, e.g. an elided or function-scoped lifetime in a +// reference parameter such as `fn f(x: &u8)` or `fn f<'a>(x: &'a Foo)`. +// Transpiled declarations use raw pointers and are unaffected, but hand-edited +// code can carry references; two such signatures cannot be compared without +// instantiating their bound regions, so they take the syntactic path. +fn compatible_foreign_fns( + cx: &RefactorCtxt, + foreign1: &ForeignItem, + foreign2: &ForeignItem, + decl1: &FnDecl, + decl2: &FnDecl, +) -> bool { + let tcx = cx.ty_ctxt(); + let sig_of = |foreign: &ForeignItem| { + tcx.fn_sig(cx.node_def_id(foreign.id)) + .subst_identity() + .no_bound_vars() + }; + match (sig_of(foreign1), sig_of(foreign2)) { + (Some(sig1), Some(sig2)) => cx.compatible_fn_sigs(&sig1, &sig2), + _ => cx.compatible_fn_prototypes(decl1, decl2), + } +} + /// Returns true if the given ForeignItem can be a declaration for the given /// Item definition. fn foreign_equiv(foreign: &ForeignItem, item: &Item) -> bool { diff --git a/c2rust-refactor/tests/snapshots.rs b/c2rust-refactor/tests/snapshots.rs index 3c4f5b6671..563dfdde93 100644 --- a/c2rust-refactor/tests/snapshots.rs +++ b/c2rust-refactor/tests/snapshots.rs @@ -472,6 +472,15 @@ fn test_reorganize_foreign_fn_arity() { .test(); } +/// Two foreign declarations whose parameters are spelled the same but name +/// different types describe different functions and must not be merged. +#[test] +fn test_reorganize_foreign_fn_type_identity() { + refactor("reorganize_definitions") + .named("reorganize_foreign_fn_type_identity.rs") + .test(); +} + /// A foreign item, `static` or `fn`, that is renamed to avoid a collision must /// keep naming the symbol it linked against before the rename. #[test] diff --git a/c2rust-refactor/tests/snapshots/reorganize_foreign_fn_type_identity.rs b/c2rust-refactor/tests/snapshots/reorganize_foreign_fn_type_identity.rs new file mode 100644 index 0000000000..baa0ef1c54 --- /dev/null +++ b/c2rust-refactor/tests/snapshots/reorganize_foreign_fn_type_identity.rs @@ -0,0 +1,74 @@ +#![feature(rustc_private)] +#![feature(register_tool)] +#![register_tool(c2rust)] +#![allow(non_upper_case_globals)] +#![allow(non_camel_case_types)] +#![allow(dead_code)] + +extern crate libc; + +// Each translation unit has its own `buf`, and the two differ in field +// visibility, so they are not unified and the second is renamed. Both units +// declare `fill` taking a `*mut buf` — spelled identically, but naming a +// different type in each unit. The two declarations therefore describe +// different functions and must not be collapsed into one. + +pub mod a { + use libc; + + #[c2rust::header_src = "/home/user/some/workspace/io.h:1"] + pub mod io_h { + use super::libc; + + #[repr(C)] + #[c2rust::src_loc = "2:0"] + pub struct buf { + pub len: libc::c_int, + pad: libc::c_int, + } + + extern "C" { + #[c2rust::src_loc = "3:0"] + pub fn fill(b: *mut buf) -> libc::c_int; + } + } + + use io_h::{buf, fill}; + + pub unsafe fn run() -> libc::c_int { + let mut b = std::mem::zeroed::(); + fill(&mut b) + } +} + +pub mod b { + use libc; + + #[c2rust::header_src = "/home/user/some/workspace/io.h:1"] + pub mod io_h { + use super::libc; + + // Same fields as the other `buf`, but all of them are public, so the + // two are not interchangeable. + #[repr(C)] + #[c2rust::src_loc = "2:0"] + pub struct buf { + pub len: libc::c_int, + pub pad: libc::c_int, + } + + extern "C" { + #[c2rust::src_loc = "3:0"] + pub fn fill(b: *mut buf) -> libc::c_int; + } + } + + use io_h::{buf, fill}; + + pub unsafe fn run() -> libc::c_int { + let mut b = std::mem::zeroed::(); + fill(&mut b) + } +} + +fn main() {} diff --git a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_foreign_fn_type_identity.rs.snap b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_foreign_fn_type_identity.rs.snap new file mode 100644 index 0000000000..65ea8feff6 --- /dev/null +++ b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions-reorganize_foreign_fn_type_identity.rs.snap @@ -0,0 +1,68 @@ +--- +source: c2rust-refactor/tests/snapshots.rs +expression: c2rust-refactor reorganize_definitions --rewrite-mode alongside -- tests/snapshots/reorganize_foreign_fn_type_identity.rs --edition 2021 +--- +#![feature(rustc_private)] +#![feature(register_tool)] +#![register_tool(c2rust)] +#![allow(non_upper_case_globals)] +#![allow(non_camel_case_types)] +#![allow(dead_code)] + +pub mod io_h { + extern "C" { + pub fn fill(b: *mut crate::io_h::buf) -> libc::c_int; + + #[link_name = "fill"] + pub fn fill_1(b: *mut crate::io_h::buf_1) -> libc::c_int; + } + use ::libc; + + #[repr(C)] + + pub struct buf { + pub len: libc::c_int, + pad: libc::c_int, + } + // Same fields as the other `buf`, but all of them are public, so the + // two are not interchangeable. + #[repr(C)] + + pub struct buf_1 { + pub len: libc::c_int, + pub pad: libc::c_int, + } +} +extern crate libc; + +// Each translation unit has its own `buf`, and the two differ in field +// visibility, so they are not unified and the second is renamed. Both units +// declare `fill` taking a `*mut buf` — spelled identically, but naming a +// different type in each unit. The two declarations therefore describe +// different functions and must not be collapsed into one. + +pub mod a { + use libc; + + use crate::io_h::buf; + use crate::io_h::fill; + + pub unsafe fn run() -> libc::c_int { + let mut b = std::mem::zeroed::(); + crate::io_h::fill(&mut b) + } +} + +pub mod b { + use libc; + + use crate::io_h::buf_1; + use crate::io_h::fill_1; + + pub unsafe fn run() -> libc::c_int { + let mut b = std::mem::zeroed::(); + crate::io_h::fill_1(&mut b) + } +} + +fn main() {} diff --git a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions.rs.snap b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions.rs.snap index a7bb339d58..a22fe72a23 100644 --- a/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions.rs.snap +++ b/c2rust-refactor/tests/snapshots/snapshots__refactor-reorganize_definitions.rs.snap @@ -30,6 +30,10 @@ pub mod bar { extern "C" { pub fn statvfs(path: *const libc::c_char, buf: *mut crate::bar::statvfs) -> libc::c_int; + + #[link_name = "statvfs"] + pub fn statvfs_1(path: *const libc::c_char, buf: *mut crate::bar::statvfs_1) + -> libc::c_int; } // =============== BEGIN bar_h ================ @@ -121,7 +125,7 @@ pub mod foo { // Use the definitions that have all public fields. // The transform should not reuse any of the libc declarations. let mut buf = unsafe { std::mem::zeroed::() }; - crate::bar::statvfs(core::ptr::null(), &mut buf); + crate::bar::statvfs_1(core::ptr::null(), &mut buf); // Use the definitions that are identical to libc. let mut buf = unsafe { std::mem::zeroed::<::libc::statfs64>() };