Attempt to implement support for self-referential union types - #3419
Attempt to implement support for self-referential union types#3419erickt wants to merge 1 commit into
Conversation
|
r? @emilio |
cff08ee to
d21afc5
Compare
In llvm/llvm-project#185449, LLVM's libc++ changed a header to use a recursive self-referential type, which looks approximately like: ``` template <class A0, class... As> union RUnion { A0 arg; RUnion<As...> u; }; template <class A> union RUnion<A> { A arg; }; struct Wrap { RUnion<int, float> 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 rust-lang#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
| if !ty.can_derive_copy(ctx) { | ||
| return false; | ||
| } | ||
| if ctx.in_codegen_phase() && |
There was a problem hiding this comment.
It feels like a huge smell to change the answer here depending on which phase are we on...
There was a problem hiding this comment.
Yeah I agree, but I'm still new in the code so I wasn't sure if there's a better way to do this. Do you recommend an approach? I could see how invasive it'd be for us to split this code into a parsing context, and a type checking context, but that might be a bit invasive. I'll dig around in the meantime.
There was a problem hiding this comment.
So for the background, as best as I can tell that we're doing two passes through the types. First, when we're parsing the types, we need to accommodate forward declared types, so when we see them, we end up pushing in a None into BindgenContext::items, which gets populated later on during the parse. Second, during code generation, we walk through the items, and temporarily pop them from the items list when we recurse into types.
In the first version of this patch, I just changed a bunch of the callsites from using resolve_type to safe_resolve_type, where we just exit the function if the type is loaned out. But I'm a little nervous that this could lead towards us hiding bugs where we actually are referencing undefined types, so I added in tracking which types we're loaning out so at least we won't hide that issue.
| 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() && |
There was a problem hiding this comment.
Same here, I don't understand why this is needed?
There was a problem hiding this comment.
I'll reply in the other question, but I'll keep this thread open in case there's other issues we want to discuss.
Sure, I'll add it in a separate PR. Also, LLVM 22 also recently came out, but it looks like the helper action doesn't support it yet KyleMayes/install-llvm-action#103. |
3 participants
In llvm/llvm-project#185449, LLVM's libc++ changed a header to use a recursive self-referential type, which looks approximately like:
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_itemremoves the type from the context. Later on, when parsing theCompKind::Union,CompInfo::layoutwould callresolve_typeon an item that was loaned out, so it panics. This bug was filed in #3397.I've attempted to fix this bug with the help of Gemini agent, and it seems that we might be able to swap out
resolve_typewithsafe_resolve_typein a few locations to get it to stop erroring out. This seems to make sense as best as I understand this situation, although there may be a chance that returningNonein these callsites might be incorrect. But I couldn't find a counter example that shows incorrect code. As a safeguard, this also tracks which items we have loaned out, and makes sure thatsafe_resolve_typewill panic if these types weren't loaned out.Fixes #3397