[cxx-interop] Avoid unchecked recursion when importing C++ classes with circular inheritance #81188
Conversation
j-hui
requested review from
zoecarver,
egorzhdan,
Xazax-hun,
fahadnayyar,
susmonteiro,
hnrklssn,
hborla,
slavapestov,
xedin,
beccadax and
ian-twilightcoder
as code owners
|
@swift-ci please test |
j-hui
force-pushed
the
fix-recursive-inheritance
branch
from
81b6280 to
8ac798b
Compare
|
@swift-ci please test |
|
nit to self: I could clarify the commit message a little bit, to something like:
Alas the spurious CI failure deities have given me an opportunity to nitpick my commit message |
j-hui
changed the title
j-hui
force-pushed
the
fix-recursive-inheritance
branch
from
8ac798b to
a39be57
Compare
|
@swift-ci please test |
| @@ -8774,3 +8781,18 @@ bool importer::isClangNamespace(const DeclContext *dc) { | |||
|
|
|||
| return false; | |||
| } | |||
|
|
|||
| bool importer::isSymbolicCircularBase(const clang::CXXRecordDecl *symbolicClass, | |||
There was a problem hiding this comment.
Should we only do this when we are importing something in symbolic mode?
There was a problem hiding this comment.
Yeah that's a good point. I'm not entirely sure whether we should, because the symbolic mode is only the trigger for this issue rather than the actual problem itself (which is that we're not handling CxxRecordDecls that describe uninstantiated templates).
In any case, I can add the check just for this to be an extra conservative patch, since I presume we'll need to cherry-pick this.
There was a problem hiding this comment.
I think we should be consistent here in terms of symbolic vs non-symbolic mode, i.e. bailing in both modes, like this patch does currently.
| @@ -8774,3 +8781,18 @@ bool importer::isClangNamespace(const DeclContext *dc) { | |||
|
|
|||
| return false; | |||
| } | |||
|
|
|||
| bool importer::isSymbolicCircularBase(const clang::CXXRecordDecl *symbolicClass, | |||
There was a problem hiding this comment.
I think we should be consistent here in terms of symbolic vs non-symbolic mode, i.e. bailing in both modes, like this patch does currently.
|
@swift-ci please test macos platform |
…th circular inheritance It is possible for a C++ class template to inherit from a specialization of itself. Normally, these are imported to Swift as separate (unrelated) types, but when symbolic import is enabled, unspecialized templates are imported in place of their specializations, leading to circularly inheriting classes to seemingly inherit from themselves. This patch adds a check to guard against the most common case of circular inheritance, when a class template directly inherits from itself. This pattern appears in a recent version of libc++, necessitating this patch. However, the solution here is imperfect as it does not handle more complex/contrived circular inheritance patterns. This patch also adds a test case exercising this pattern. The -index-store-path flag causes swift-frontend to index the C++ module with symbolic import enabled, without the fix in this patch, that test triggers an assertion failure due to the circular reference (and can infinitely recurse in the StorageVisitor when assertions are disabled). rdar://148026461
j-hui
force-pushed
the
fix-recursive-inheritance
branch
from
a39be57 to
1f2107f
Compare
| @@ -6364,6 +6364,11 @@ TinyPtrVector<ValueDecl *> ClangRecordMemberLookup::evaluate( | |||
| continue; | |||
|
|
|||
| auto *baseRecord = baseType->getAs<clang::RecordType>()->getDecl(); | |||
|
|
|||
| if (isSymbolicCircularBase(cxxRecord, baseRecord)) | |||
There was a problem hiding this comment.
My last force push updated the test case to cover one more case (a class that inherits from a circularly inheriting class), and fixed this code to check for the right base. It used to check the C++ class of inheritingDecl, but now it checks the C++ class of recordDecl, where:
template <typename T> class Circular;
template <> class Circular<void> { ... };
template <typename T> class Circular<T> : Circular<void> { ... };
typedef Circular<SomeType> RecordDecl;
class InheritingDecl : RecordDecl { ... };
Neither check is complete, but heuristically checking recordDecl is probably better (at least that's what matches the circular inheritance case in std::exception).
|
@swift-ci please test |
|
@swift-ci please test |
|
(re-running because I seem to have run into an error fixed by swiftlang/swift-package-manager@bdfd754) |
|
@swift-ci please test macos platform |
It is possible for a C++ class template to inherit from a specialization of itself. Normally, these are imported to Swift as separate (unrelated) types, but when symbolic import is enabled, unspecialized templates are imported in place of their specializations, leading to circularly inheriting classes to seemingly inherit from themselves.
This patch adds a check to guard against the most common case of circular inheritance, when a class template directly inherits from itself. This pattern appears in a recent version of libc++, necessitating this patch. However, the solution here is imperfect as it does not handle more complex/contrived circular inheritance patterns.
This patch also adds a test case exercising this pattern. The -index-store-path flag causes swift-frontend to index the C++ module with symbolic import enabled, without the fix in this patch, that test triggers an assertion failure due to the circular reference (and can infinitely recurse in the StorageVisitor when assertions are disabled).
rdar://148026461