From 2cb053ac47de794438cbd22d46e49d10c7e0e7e6 Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Wed, 28 Jul 2021 21:14:12 -0700 Subject: [PATCH 1/2] Canonicalize types before looking for their definitions. In some esoteric cases involving nested templates, `ty.declaration().definition()` isn't enough to find the definition: we need `ty.canonical_type().declaration().definition()` instead. Closes #2078. --- src/ir/item.rs | 4 +- tests/expectations/tests/canonical-types.rs | 278 ++++++++++++++++++ .../tests/layout_cmdline_token.rs | 2 + tests/headers/canonical-types.hpp | 51 ++++ 4 files changed, 333 insertions(+), 2 deletions(-) create mode 100644 tests/expectations/tests/canonical-types.rs create mode 100644 tests/headers/canonical-types.hpp diff --git a/src/ir/item.rs b/src/ir/item.rs index 4e0ba80b42..a38c8e5fb8 100644 --- a/src/ir/item.rs +++ b/src/ir/item.rs @@ -1594,8 +1594,8 @@ impl ClangItemParser for Item { } let decl = { - let decl = ty.declaration(); - decl.definition().unwrap_or(decl) + let canonical_def = ty.canonical_type().declaration().definition(); + canonical_def.unwrap_or_else(|| ty.declaration()) }; let comment = decl.raw_comment().or_else(|| location.raw_comment()); diff --git a/tests/expectations/tests/canonical-types.rs b/tests/expectations/tests/canonical-types.rs new file mode 100644 index 0000000000..80d7fec374 --- /dev/null +++ b/tests/expectations/tests/canonical-types.rs @@ -0,0 +1,278 @@ +#![allow( + dead_code, + non_snake_case, + non_camel_case_types, + non_upper_case_globals +)] + +#[repr(C)] +#[derive(Debug, Default, Copy, Clone)] +pub struct ClassA { + pub _address: u8, +} +#[repr(C)] +#[derive(Debug, Copy, Clone)] +pub struct ClassA_ClassAInner { + pub x: *mut T, + pub _phantom_0: ::std::marker::PhantomData<::std::cell::UnsafeCell>, +} +impl Default for ClassA_ClassAInner { + fn default() -> Self { + let mut s = ::std::mem::MaybeUninit::::uninit(); + unsafe { + ::std::ptr::write_bytes(s.as_mut_ptr(), 0, 1); + s.assume_init() + } + } +} +#[repr(C)] +#[derive(Debug, Default, Copy, Clone)] +pub struct ClassB { + pub _address: u8, +} +#[repr(C)] +#[derive(Debug, Default, Copy, Clone)] +pub struct ClassC { + pub _address: u8, +} +#[repr(C)] +#[derive(Debug, Copy, Clone)] +pub struct ClassC_ClassCInnerB { + pub cache: *mut ClassC_ClassCInnerA, +} +impl Default for ClassC_ClassCInnerB { + fn default() -> Self { + let mut s = ::std::mem::MaybeUninit::::uninit(); + unsafe { + ::std::ptr::write_bytes(s.as_mut_ptr(), 0, 1); + s.assume_init() + } + } +} +#[repr(C)] +#[derive(Debug, Copy, Clone)] +pub struct ClassC_ClassCInnerA { + pub member: *mut ClassC_ClassCInnerB, +} +impl Default for ClassC_ClassCInnerA { + fn default() -> Self { + let mut s = ::std::mem::MaybeUninit::::uninit(); + unsafe { + ::std::ptr::write_bytes(s.as_mut_ptr(), 0, 1); + s.assume_init() + } + } +} +#[repr(C)] +#[derive(Debug, Copy, Clone)] +pub struct ClassC_ClassCInnerCRTP { + pub _address: u8, +} +impl Default for ClassC_ClassCInnerCRTP { + fn default() -> Self { + let mut s = ::std::mem::MaybeUninit::::uninit(); + unsafe { + ::std::ptr::write_bytes(s.as_mut_ptr(), 0, 1); + s.assume_init() + } + } +} +#[repr(C)] +#[derive(Debug, Copy, Clone)] +pub struct ClassD { + pub _address: u8, +} +#[test] +fn bindgen_test_layout_ClassD() { + assert_eq!( + ::std::mem::size_of::(), + 1usize, + concat!("Size of: ", stringify!(ClassD)) + ); + assert_eq!( + ::std::mem::align_of::(), + 1usize, + concat!("Alignment of ", stringify!(ClassD)) + ); +} +impl Default for ClassD { + fn default() -> Self { + let mut s = ::std::mem::MaybeUninit::::uninit(); + unsafe { + ::std::ptr::write_bytes(s.as_mut_ptr(), 0, 1); + s.assume_init() + } + } +} +#[test] +fn __bindgen_test_layout_ClassB_open0_ClassD_ClassCInnerCRTP_close0_instantiation( +) { + assert_eq!( + ::std::mem::size_of::(), + 1usize, + concat!("Size of template specialization: ", stringify!(ClassB)) + ); + assert_eq!( + ::std::mem::align_of::(), + 1usize, + concat!("Alignment of template specialization: ", stringify!(ClassB)) + ); +} +#[repr(C)] +#[derive(Debug, Copy, Clone)] +pub struct ClassCInnerCRTP { + pub _address: u8, +} +#[test] +fn bindgen_test_layout_ClassCInnerCRTP() { + assert_eq!( + ::std::mem::size_of::(), + 1usize, + concat!("Size of: ", stringify!(ClassCInnerCRTP)) + ); + assert_eq!( + ::std::mem::align_of::(), + 1usize, + concat!("Alignment of ", stringify!(ClassCInnerCRTP)) + ); +} +impl Default for ClassCInnerCRTP { + fn default() -> Self { + let mut s = ::std::mem::MaybeUninit::::uninit(); + unsafe { + ::std::ptr::write_bytes(s.as_mut_ptr(), 0, 1); + s.assume_init() + } + } +} +#[test] +fn __bindgen_test_layout_ClassB_open0_ClassCInnerCRTP_ClassAInner_close0_instantiation( +) { + assert_eq!( + ::std::mem::size_of::(), + 1usize, + concat!("Size of template specialization: ", stringify!(ClassB)) + ); + assert_eq!( + ::std::mem::align_of::(), + 1usize, + concat!("Alignment of template specialization: ", stringify!(ClassB)) + ); +} +#[repr(C)] +#[derive(Debug, Copy, Clone)] +pub struct ClassAInner { + pub x: *mut ClassCInnerA, +} +#[test] +fn bindgen_test_layout_ClassAInner() { + assert_eq!( + ::std::mem::size_of::(), + 8usize, + concat!("Size of: ", stringify!(ClassAInner)) + ); + assert_eq!( + ::std::mem::align_of::(), + 8usize, + concat!("Alignment of ", stringify!(ClassAInner)) + ); + assert_eq!( + unsafe { + &(*(::std::ptr::null::())).x as *const _ as usize + }, + 0usize, + concat!( + "Offset of field: ", + stringify!(ClassAInner), + "::", + stringify!(x) + ) + ); +} +impl Default for ClassAInner { + fn default() -> Self { + let mut s = ::std::mem::MaybeUninit::::uninit(); + unsafe { + ::std::ptr::write_bytes(s.as_mut_ptr(), 0, 1); + s.assume_init() + } + } +} +#[repr(C)] +#[derive(Debug, Copy, Clone)] +pub struct ClassCInnerA { + pub member: *mut ClassCInnerB, +} +#[test] +fn bindgen_test_layout_ClassCInnerA() { + assert_eq!( + ::std::mem::size_of::(), + 8usize, + concat!("Size of: ", stringify!(ClassCInnerA)) + ); + assert_eq!( + ::std::mem::align_of::(), + 8usize, + concat!("Alignment of ", stringify!(ClassCInnerA)) + ); + assert_eq!( + unsafe { + &(*(::std::ptr::null::())).member as *const _ as usize + }, + 0usize, + concat!( + "Offset of field: ", + stringify!(ClassCInnerA), + "::", + stringify!(member) + ) + ); +} +impl Default for ClassCInnerA { + fn default() -> Self { + let mut s = ::std::mem::MaybeUninit::::uninit(); + unsafe { + ::std::ptr::write_bytes(s.as_mut_ptr(), 0, 1); + s.assume_init() + } + } +} +#[repr(C)] +#[derive(Debug, Copy, Clone)] +pub struct ClassCInnerB { + pub cache: *mut ClassCInnerA, +} +#[test] +fn bindgen_test_layout_ClassCInnerB() { + assert_eq!( + ::std::mem::size_of::(), + 8usize, + concat!("Size of: ", stringify!(ClassCInnerB)) + ); + assert_eq!( + ::std::mem::align_of::(), + 8usize, + concat!("Alignment of ", stringify!(ClassCInnerB)) + ); + assert_eq!( + unsafe { + &(*(::std::ptr::null::())).cache as *const _ as usize + }, + 0usize, + concat!( + "Offset of field: ", + stringify!(ClassCInnerB), + "::", + stringify!(cache) + ) + ); +} +impl Default for ClassCInnerB { + fn default() -> Self { + let mut s = ::std::mem::MaybeUninit::::uninit(); + unsafe { + ::std::ptr::write_bytes(s.as_mut_ptr(), 0, 1); + s.assume_init() + } + } +} diff --git a/tests/expectations/tests/layout_cmdline_token.rs b/tests/expectations/tests/layout_cmdline_token.rs index 644b1b8aab..47170dd709 100644 --- a/tests/expectations/tests/layout_cmdline_token.rs +++ b/tests/expectations/tests/layout_cmdline_token.rs @@ -61,6 +61,8 @@ impl Default for cmdline_token_hdr { } } } +/// Stores a pointer to the ops struct, and the offset: the place to +/// write the parsed result in the destination structure. pub type cmdline_parse_token_hdr_t = cmdline_token_hdr; /// A token is defined by this structure. /// diff --git a/tests/headers/canonical-types.hpp b/tests/headers/canonical-types.hpp new file mode 100644 index 0000000000..c8eadd7e5e --- /dev/null +++ b/tests/headers/canonical-types.hpp @@ -0,0 +1,51 @@ +// bindgen-flags: -- -std=c++14 + +// Issue #2078. To pick up the definition of `ClassCInnerA`, +// `ty.canonical_type().declaration().definition()` is needed. + +template +class ClassA { +public: + class ClassAInner { + public: + T *x; + }; +}; + +template +class ClassB { +public: + void foo() { + ((D *)0)->quux(); + } +}; + +template +class ClassC { + struct ClassCInnerA; + + struct ClassCInnerB { + ClassCInnerA *cache; + }; + static_assert(sizeof(ClassCInnerB) > 0, ""); + + struct ClassCInnerA { + ClassCInnerB *member; + }; + +public: + class ClassCInnerCRTP : public ClassB::ClassAInner> { + public: + void quux() { + ((typename ClassA::ClassAInner *)0)->x->member; + } + }; +}; + +class ClassD : public ClassB::ClassCInnerCRTP> { +public: + void bar() { + ((ClassC::ClassCInnerCRTP *)0)->foo(); + } +}; + From 6a7dff0c8135adcedad39fd183fb72933f48aa6d Mon Sep 17 00:00:00 2001 From: Patrick Walton Date: Fri, 30 Jul 2021 19:41:10 -0700 Subject: [PATCH 2/2] Don't assume that an inner class declaration always immediately yields a complete type. It might not if we had to avoid recursion when processing types. Detect that case and bail out. This bug was being masked by the fact that we didn't always find definitions for the recursion check and so it didn't trigger, but now that this check is more reliable we have to be careful in more places. The test case was reduced from the GCC STL allocator definition. --- src/ir/comp.rs | 33 +++++++++++-------- .../tests/nested-template-typedef.rs | 17 ++++++++++ tests/headers/nested-template-typedef.hpp | 8 +++++ 3 files changed, 44 insertions(+), 14 deletions(-) create mode 100644 tests/expectations/tests/nested-template-typedef.rs create mode 100644 tests/headers/nested-template-typedef.hpp diff --git a/src/ir/comp.rs b/src/ir/comp.rs index e554f9a89c..97983308db 100644 --- a/src/ir/comp.rs +++ b/src/ir/comp.rs @@ -1408,21 +1408,26 @@ impl CompInfo { let inner = Item::parse(cur, Some(potential_id), ctx) .expect("Inner ClassDecl"); - let inner = inner.expect_type_id(ctx); - - ci.inner_types.push(inner); - - // A declaration of an union or a struct without name could - // also be an unnamed field, unfortunately. - if cur.spelling().is_empty() && - cur.kind() != CXCursor_EnumDecl - { - let ty = cur.cur_type(); - let public = cur.public_accessible(); - let offset = cur.offset_of_field().ok(); + // If we avoided recursion parsing this type (in + // `Item::from_ty_with_id()`), then this might not be a + // valid type ID, so check and gracefully handle this. + if ctx.resolve_item_fallible(inner).is_some() { + let inner = inner.expect_type_id(ctx); + + ci.inner_types.push(inner); + + // A declaration of an union or a struct without name + // could also be an unnamed field, unfortunately. + if cur.spelling().is_empty() && + cur.kind() != CXCursor_EnumDecl + { + let ty = cur.cur_type(); + let public = cur.public_accessible(); + let offset = cur.offset_of_field().ok(); - maybe_anonymous_struct_field = - Some((inner, ty, public, offset)); + maybe_anonymous_struct_field = + Some((inner, ty, public, offset)); + } } } CXCursor_PackedAttr => { diff --git a/tests/expectations/tests/nested-template-typedef.rs b/tests/expectations/tests/nested-template-typedef.rs new file mode 100644 index 0000000000..ab761d286f --- /dev/null +++ b/tests/expectations/tests/nested-template-typedef.rs @@ -0,0 +1,17 @@ +#![allow( + dead_code, + non_snake_case, + non_camel_case_types, + non_upper_case_globals +)] + +#[repr(C)] +#[derive(Debug, Default, Copy, Clone)] +pub struct Foo { + pub _address: u8, +} +#[repr(C)] +#[derive(Debug, Default, Copy, Clone)] +pub struct Foo_Bar { + pub _address: u8, +} diff --git a/tests/headers/nested-template-typedef.hpp b/tests/headers/nested-template-typedef.hpp new file mode 100644 index 0000000000..8c83de5bae --- /dev/null +++ b/tests/headers/nested-template-typedef.hpp @@ -0,0 +1,8 @@ +template +class Foo { +public: + template + struct Bar { + typedef Foo FooU; + }; +};