From 0acdfdd74054caec85a75ba20c7ac236f6ebd893 Mon Sep 17 00:00:00 2001 From: Erick Tryzelaar Date: Thu, 30 Jul 2026 20:53:45 +0000 Subject: [PATCH] Fix variadic template union layout recursion and generic field codegen In llvm/llvm-project#185449, LLVM's libc++ changed a header to use a recursive self-referential type, which looks approximately like: ``` template union RUnion { A0 arg; RUnion u; }; template union RUnion { A arg; }; struct Wrap { RUnion u; }; ``` This code caused bindgen to panic. The problem seems to be that bindgen can't handle the recursive self-referential union. Digging into the code, it seems that when this type is being parsed, `with_loaned_item` removes the type from the context. Later on, when parsing the CompKind::Union, CompInfo::layout would call resolve_type on an item that was loaned out, so it panics. This bug was filed in #3397. This patch avoids that by first keeping track of which items we have loaed out with `with_loaned_item`, then changing `safe_resolve_type` to panic if we don't have an entry for that type, or it has been loaned out. Then we've updated the call sites to use it. This also adds LLVM-21 tests, since clang generates different code than LLVM-20. Test: cargo test -p bindgen-tests TAG=agy CONV=619bdf72-4d2d-494f-8cb0-f61a6db9674c --- .github/workflows/bindgen.yml | 8 +-- bindgen-tests/Cargo.toml | 1 + .../libclang-21/issue-544-stylo-creduce-2.rs | 17 +++++ .../tests/libclang-21/nsBaseHashtable.rs | 57 +++++++++++++++++ .../tests/recursive_alias_canonicalization.rs | 32 ++++++++++ .../tests/expectations/tests/transform-op.rs | 64 +++---------------- .../tests/variadic_template_union.rs | 59 +++++++++++++++++ .../variadic_template_union_alignment.rs | 53 +++++++++++++++ .../recursive_alias_canonicalization.hpp | 11 ++++ .../tests/headers/variadic_template_union.hpp | 9 +++ .../variadic_template_union_alignment.hpp | 33 ++++++++++ bindgen-tests/tests/tests.rs | 10 ++- bindgen/ir/comp.rs | 58 ++++++++++++++++- bindgen/ir/context.rs | 52 +++++++++++++-- bindgen/ir/enum_ty.rs | 4 +- bindgen/ir/ty.rs | 14 ++-- 16 files changed, 402 insertions(+), 80 deletions(-) create mode 100644 bindgen-tests/tests/expectations/tests/libclang-21/issue-544-stylo-creduce-2.rs create mode 100644 bindgen-tests/tests/expectations/tests/libclang-21/nsBaseHashtable.rs create mode 100644 bindgen-tests/tests/expectations/tests/recursive_alias_canonicalization.rs create mode 100644 bindgen-tests/tests/expectations/tests/variadic_template_union.rs create mode 100644 bindgen-tests/tests/expectations/tests/variadic_template_union_alignment.rs create mode 100644 bindgen-tests/tests/headers/recursive_alias_canonicalization.hpp create mode 100644 bindgen-tests/tests/headers/variadic_template_union.hpp create mode 100644 bindgen-tests/tests/headers/variadic_template_union_alignment.hpp diff --git a/third_party/rust/bindgen/ir/comp.rs b/third_party/rust/bindgen/ir/comp.rs index 2760775cea..5fb4d8b73d 100644 --- a/third_party/rust/bindgen/ir/comp.rs +++ b/third_party/rust/bindgen/ir/comp.rs @@ -1082,6 +1082,12 @@ impl CompInfo { return None; } + // Uninstantiated template unions do not have layout in C++ and can cause + // infinite layout recursion if fields reference the union template. + if self.kind == CompKind::Union && !self.template_params.is_empty() { + return None; + } + let mut max_size = 0; // Don't allow align(0) let mut max_align = 1; @@ -1707,13 +1713,61 @@ impl CompInfo { ctx.options().default_non_copy_union_style }; + let field_can_copy = |field_data: &FieldData| -> bool { + let ty = field_data.ty(); + if !ty.can_derive_copy(ctx) { + return false; + } + if ctx.in_codegen_phase() && + ctx.uses_any_template_parameters(ty.into()) + { + return false; + } + true + }; + let all_can_copy = self.fields().iter().all(|f| match *f { + Field::DataMember(ref field_data) => field_can_copy(field_data), + Field::Bitfields(_) => true, + }); + + let has_generic_params = self.fields().iter().any(|f| match *f { Field::DataMember(ref field_data) => { - field_data.ty().can_derive_copy(ctx) + ctx.in_codegen_phase() && + ctx.uses_any_template_parameters(field_data.ty().into()) } - Field::Bitfields(_) => true, + Field::Bitfields(_) => false, }); + let has_by_value_recursive_field = + self.fields().iter().any(|f| match *f { + Field::DataMember(ref field_data) => { + let mut ty = ctx.safe_resolve_type(field_data.ty()); + while let Some(t) = ty { + if t.name().is_some_and(|n| name.ends_with(n)) { + return true; + } + match *t.kind() { + super::ty::TypeKind::ResolvedTypeRef(inner) | + super::ty::TypeKind::Alias(inner) => { + ty = ctx.safe_resolve_type(inner); + } + _ => return false, + } + } + false + } + Field::Bitfields(_) => false, + }); + + // If the union has generic template parameters and does not contain a recursive + // template field by value (which would produce E0072 in Rust), emit as a native + // Rust union with ManuallyDrop fields ((true, false)) so the Rust compiler + // computes the correct size and alignment upon instantiation. + if has_generic_params && !has_by_value_recursive_field { + return (true, false); + } + if !all_can_copy && union_style == NonCopyUnionStyle::BindgenWrapper { return (false, false); } diff --git a/third_party/rust/bindgen/ir/context.rs b/third_party/rust/bindgen/ir/context.rs index b5b6b4a000..3d5f6a8e79 100644 --- a/third_party/rust/bindgen/ir/context.rs +++ b/third_party/rust/bindgen/ir/context.rs @@ -349,6 +349,13 @@ pub(crate) struct BindgenContext { /// potentially break that assumption. currently_parsed_types: Vec, + /// A stack of items currently loaned out by `with_loaned_item`. + /// + /// This allows us to distinguish between: + /// 1) An existing item that is currently being traversed recursively. + /// 2) An unknown or invalid item ID (which should panic). + currently_loaned_items: Vec, + /// A map with all the already parsed macro names. This is done to avoid /// hard errors while parsing duplicated macros, as well to allow macro /// expression parsing. @@ -586,6 +593,7 @@ If you encounter an error missing from this list, please file an issue or a PR!" current_module: root_module_id, semantic_parents: Default::default(), currently_parsed_types: vec![], + currently_loaned_items: vec![], parsed_macros: Default::default(), replacements: Default::default(), collected_typerefs: false, @@ -984,9 +992,13 @@ If you encounter an error missing from this list, please file an issue or a PR!" F: (FnOnce(&BindgenContext, &mut Item) -> T), { let mut item = self.items[id.0].take().unwrap(); + self.currently_loaned_items.push(id); let result = f(self, &mut item); + let popped = self.currently_loaned_items.pop(); + debug_assert_eq!(popped, Some(id)); + let existing = self.items[id.0].replace(item); assert!(existing.is_none()); @@ -1464,13 +1476,38 @@ If you encounter an error missing from this list, please file an issue or a PR!" self.resolve_item(func_id).kind().expect_function() } - /// Resolve the given `ItemId` as a type, or `None` if there is no item with - /// the given ID. + /// Returns `true` if `id` is currently loaned out by `with_loaned_item`. + pub(crate) fn is_currently_loaned_item>( + &self, + id: Id, + ) -> bool { + self.currently_loaned_items.contains(&id.into()) + } + + /// Resolve the given `ItemId` as a type. + /// + /// Returns `Some(&Type)` if the item resolves to a type. + /// Returns `None` if the item is currently loaned out by `with_loaned_item` + /// (indicating an intentional recursive reference). /// - /// Panics if the ID resolves to an item that is not a type. + /// Panics if bindgen has no awareness of the given type ID during codegen. pub(crate) fn safe_resolve_type(&self, type_id: TypeId) -> Option<&Type> { - self.resolve_item_fallible(type_id) - .map(|t| t.kind().expect_type()) + let id: ItemId = type_id.into(); + match self.items.get(id.0) { + Some(Some(item)) => item.kind().as_type(), + // When an item slot is `None` because it is currently loaned out by + // `with_loaned_item`, intentionally return `None` to signal recursion. + Some(None) if self.is_currently_loaned_item(id) => None, + _ => { + // During AST parsing and template resolution (`!self.in_codegen_phase()`), + // forward-declared items or template references may not yet be populated + // in `self.items`, so returning `None` is expected. During codegen, + // however, all valid IR items must be present in `self.items`. + assert!(!self.in_codegen_phase(), "Not an item: {type_id:?}"); + + None + } + } } /// Resolve the given `ItemId` into an `Item`, or `None` if no such item @@ -1624,8 +1661,9 @@ If you encounter an error missing from this list, please file an issue or a PR!" ty: &clang::Type, location: Cursor, ) -> Option { - let num_expected_args = - self.resolve_type(template).num_self_template_params(self); + let num_expected_args = self + .safe_resolve_type(template) + .map_or(0, |t| t.num_self_template_params(self)); if num_expected_args == 0 { warn!( "Tried to instantiate a template for which we could not \ diff --git a/third_party/rust/bindgen/ir/enum_ty.rs b/third_party/rust/bindgen/ir/enum_ty.rs index 8566622100..20b2de73b3 100644 --- a/third_party/rust/bindgen/ir/enum_ty.rs +++ b/third_party/rust/bindgen/ir/enum_ty.rs @@ -71,8 +71,8 @@ impl Enum { .and_then(|et| Item::from_ty(&et, declaration, None, ctx).ok()); let mut variants = vec![]; - let variant_ty = - repr.and_then(|r| ctx.resolve_type(r).safe_canonical_type(ctx)); + let variant_ty = repr + .and_then(|r| ctx.safe_resolve_type(r)?.safe_canonical_type(ctx)); let is_bool = variant_ty.is_some_and(Type::is_bool); // Assume signedness since the default type by the C standard is an int. diff --git a/third_party/rust/bindgen/ir/ty.rs b/third_party/rust/bindgen/ir/ty.rs index 805138b7a9..8c0a2fd0df 100644 --- a/third_party/rust/bindgen/ir/ty.rs +++ b/third_party/rust/bindgen/ir/ty.rs @@ -209,9 +209,9 @@ impl Type { None } } - TypeKind::ResolvedTypeRef(inner) => { - ctx.resolve_type(inner).is_incomplete_array(ctx) - } + TypeKind::ResolvedTypeRef(inner) => ctx + .safe_resolve_type(inner) + .and_then(|t| t.is_incomplete_array(ctx)), _ => None, } } @@ -223,7 +223,7 @@ impl Type { TypeKind::Comp(ref ci) => ci.layout(ctx), TypeKind::Array(inner, 0) => Some(Layout::new( 0, - ctx.resolve_type(inner).layout(ctx)?.align, + ctx.safe_resolve_type(inner)?.layout(ctx)?.align, )), // FIXME(emilio): This is a hack for anonymous union templates. // Use the actual pointer size! @@ -232,7 +232,7 @@ impl Type { ctx.target_pointer_size(), )), TypeKind::ResolvedTypeRef(inner) => { - ctx.resolve_type(inner).layout(ctx) + ctx.safe_resolve_type(inner)?.layout(ctx) } _ => None, } @@ -327,10 +327,10 @@ impl Type { TypeKind::ResolvedTypeRef(inner) | TypeKind::Alias(inner) | TypeKind::TemplateAlias(inner, _) => { - ctx.resolve_type(inner).safe_canonical_type(ctx) + ctx.safe_resolve_type(inner)?.safe_canonical_type(ctx) } TypeKind::TemplateInstantiation(ref inst) => ctx - .resolve_type(inst.template_definition()) + .safe_resolve_type(inst.template_definition())? .safe_canonical_type(ctx), TypeKind::UnresolvedTypeRef(..) => None,