[cxx-interop] Detect IndirectFields nested in non-importable anon-union. #37287
Conversation
|
I left a few comments. Thanks for working on this. I think this is the correct fix (or at least in the right direction). But we might need to change when we do the analysis to figure out if the member/parent is valid. |
|
|
||
| struct S { | ||
| union { | ||
| C *t; |
There was a problem hiding this comment.
I believe because it thinks the destructor is deleted at
in
isCxxRecordImportable
I'm a bit confused. Where is the destructor deleted?
If that's the problem I think we actually need to fix this differently: figure out why clang thinks the destructor is deleted here (or maybe I'm just missing something 😁). Then we should try to reproduce this assertion again using a "differently invalid" type.
There was a problem hiding this comment.
Actually I think it's because the destructor for an anonymous union here is not deleted (sorry read the condition inverted), and since I dont think theres no way to do a ~() = delete; I can't try this out otherwise for an anonymous union. I think the reason for this is that in this similar instance:
@interface C
@end
union {
C *t;
char c;
};
The anon union contains an arc managed pointer so the destructor can't be trivial but I am not certain of this.
There was a problem hiding this comment.
If the destructor is not deleted, then why can't we import it? I want to make sure we completely understand the problem here.
There was a problem hiding this comment.
Here's an idea about why this might be failing, it comes from the observation that a similar C++ program doesn't fail:
struct C {
C(const C&) = delete;
};
struct S {
union {
OC *t;
char c;
};
S(const S &s) {}
~S() { }
int f() { return 42; }
};
S *getSPtr();
In the above program, C isn't able to be imported, but its pointer type is still imported as OpaquePointer so we get:
struct S {
struct __Unnamed_union___Anonymous_field0 {
var t: OpaquePointer!
var c: CChar
init()
init(t: OpaquePointer!)
init(c: CChar)
}
var __Anonymous_field0: S.__Unnamed_union___Anonymous_field0
var t: OpaquePointer!
var c: CChar
mutating func f() -> Int32
}
func getSPtr() -> UnsafeMutablePointer<S>!
However, as I understand it (and this might not be correct), this is not the case for ObjC types: if they can't be imported, their type is nullptr. This might be the reason it's only happening for ObjC.
However, I'm still not sure we know the answer to the question: why is isCxxRecordImportable returning false?
plotfi
force-pushed
the
cxx-interop-anon-union
branch
from
d2ba0f6 to
6c3bcc4
Compare
|
@swift-ci test |
|
Build failed |
|
Build failed |
plotfi
force-pushed
the
cxx-interop-anon-union
branch
from
6c3bcc4 to
730c8bb
Compare
|
@swift-ci please test |
|
@compnerd @zoecarver merge please? :-) |
lib/ClangImporter/ImportDecl.cpp
Outdated
| // If we encounter an IndirectFieldDecl, ensure that its parent is | ||
| // importable before attempting to import it because they are dependent | ||
| // when it comes to getter/setter generation. | ||
| if (const auto *Ind = dyn_cast<clang::IndirectFieldDecl>(nd)) { |
There was a problem hiding this comment.
I actually misunderstood here, do you want Ind to be ind?
| char c; | ||
| }; | ||
| S(const S &s) {} | ||
| ~S() { } |
There was a problem hiding this comment.
Additionally, it's odd that this doesn't fail when the special members are gone, this contributes to my thinking that we don't know the whole story here.
There was a problem hiding this comment.
Ah yeah I agreed. I thought there was something going on where the special members of the containing struct affected the anon-union's. If it is ok with you to land this so I can be unblocked, I will continue investigating in the background if that is ok?
|
@plotfi the implementation looks good to me, so if you're blocked on this, I'm happy to commit it as a stop-gap solution. However, as my comments say, I'd like to investigate further. I don't fully understand why this type is failing to import. |
|
Please fix the minor naming thing, then I'll land it. Thanks @plotfi! |
plotfi
force-pushed
the
cxx-interop-anon-union
branch
from
730c8bb to
d18e383
Compare
Updated. Please invoke swift-ci again. Thank you. :-) |
lib/ClangImporter/ImportDecl.cpp
Outdated
| // importable before attempting to import it because they are dependent | ||
| // when it comes to getter/setter generation. | ||
| if (const auto *ind = dyn_cast<clang::IndirectFieldDecl>(nd)) { | ||
| const clang::CXXRecordDecl *ParentUnion = |
There was a problem hiding this comment.
ParentUnion -> parentUnion.
(Sorry to be pedantic.)
There was a problem hiding this comment.
Ah, no problem totally fine. I am still adjusting from the LLVM starts with caps style.
This patch to the Clang Importer avoids an assert by detecting when an IndirectField belongs to an anonymous union that is not importable. How it does this is by determining if an IndirectFieldDecl's anonymous parent field is non-importable (based on isCxxRecordImportable) and if so skips importing the IndirectFieldDecl. This avoids the mentioned assert because makeIndirectFieldAccessors expects a FieldDecl of __Unnamed_union___Anonymous_field that is only populated when the top-level union is imported from an IndirectFieldDecl's parent. IndirectFieldDecls for the members of an anonymous union are dependent on their parent and should not be imported if their parent can not also be imported. This corner case can happen in cases when an anonymous union contains an ARC pointer, which results in a non-trivial but also inaccessible dtor.
plotfi
force-pushed
the
cxx-interop-anon-union
branch
from
d18e383 to
7d7a6c6
Compare
|
@swift-ci please smoke test and merge. |
@zoecarver @egorzhdan @compnerd