Skip to content

Commit abab6b3

Browse files
zoecarverzoecarver
committed
[cxx-interop] Bail earlier when importing invalid records.
If we have a C++ record decl that's invalid (because of a deleted destructor or copy constructor), bail before we import any of its members or cache the decl. This way, we don't accidentally import any "nested" decls.
1 parent 2d463d7 commit abab6b3

File tree

6 files changed

+101
-57
lines changed

6 files changed

+101
-57
lines changed

‎lib/ClangImporter/ImportDecl.cpp

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -3334,6 +3334,22 @@ namespace {
33343334
return result;
33353335
}
33363336

3337+
bool isCxxRecordImportable(const clang::CXXRecordDecl *decl) {
3338+
if (auto dtor = decl->getDestructor()) {
3339+
if (dtor->isDeleted() || dtor->getAccess() != clang::AS_public) {
3340+
return false;
3341+
}
3342+
}
3343+
3344+
// If we have no way of copying the type we can't import the class
3345+
// at all because we cannot express the correct semantics as a swift
3346+
// struct.
3347+
return llvm::none_of(decl->ctors(), [](clang::CXXConstructorDecl *ctor) {
3348+
return ctor->isCopyConstructor() &&
3349+
(ctor->isDeleted() || ctor->getAccess() != clang::AS_public);
3350+
});
3351+
}
3352+
33373353
Decl *VisitRecordDecl(const clang::RecordDecl *decl) {
33383354
// Track whether this record contains fields we can't reference in Swift
33393355
// as stored properties.
@@ -3626,30 +3642,9 @@ namespace {
36263642

36273643
result->setHasUnreferenceableStorage(hasUnreferenceableStorage);
36283644

3629-
if (cxxRecordDecl) {
3645+
if (cxxRecordDecl)
36303646
result->setIsCxxNonTrivial(!cxxRecordDecl->isTriviallyCopyable());
36313647

3632-
for (auto ctor : cxxRecordDecl->ctors()) {
3633-
if (ctor->isCopyConstructor()) {
3634-
// If we have no way of copying the type we can't import the class
3635-
// at all because we cannot express the correct semantics as a swift
3636-
// struct.
3637-
if (ctor->isDeleted() || ctor->getAccess() != clang::AS_public)
3638-
return nullptr;
3639-
}
3640-
if (ctor->getAccess() != clang::AS_public) {
3641-
result->setIsCxxNonTrivial(true);
3642-
break;
3643-
}
3644-
}
3645-
3646-
if (auto dtor = cxxRecordDecl->getDestructor()) {
3647-
if (dtor->isDeleted() || dtor->getAccess() != clang::AS_public) {
3648-
return nullptr;
3649-
}
3650-
}
3651-
}
3652-
36533648
return result;
36543649
}
36553650

@@ -3704,6 +3699,11 @@ namespace {
37043699
}
37053700
}
37063701

3702+
// It is import that we bail on an unimportable record *before* we import
3703+
// any of its members or cache the decl.
3704+
if (!isCxxRecordImportable(decl))
3705+
return nullptr;
3706+
37073707
return VisitRecordDecl(decl);
37083708
}
37093709

‎test/Interop/Cxx/class/Inputs/invalid-nested-struct.h

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
// When we import this class, make sure that we bail before trying to import
2+
// its sub-decls (i.e., "ForwardDeclaredSibling").
3+
struct CannotImport {
4+
void test(struct ForwardDeclaredSibling *) {}
5+
6+
~CannotImport() = delete;
7+
CannotImport(CannotImport const &) = delete;
8+
};
9+
10+
// We shouldn't be able to import this class either because it also doesn't have
11+
// a copy ctor or destructor.
12+
struct ForwardDeclaredSibling : CannotImport {};
13+
14+
// This is a longer regression test to make sure we don't improperly cache a
15+
// typedef that's invalid.
16+
namespace RegressionTest {
17+
18+
template <class From>
19+
struct pointer_traits {
20+
template <class To>
21+
struct rebind {
22+
typedef To other;
23+
};
24+
};
25+
26+
template <class T, class U>
27+
struct Convert {
28+
typedef typename pointer_traits<T>::template rebind<U>::other type;
29+
};
30+
31+
template <class>
32+
struct Forward;
33+
34+
template <class V>
35+
struct SomeTypeTrait {
36+
typedef Forward<V> *F;
37+
typedef typename Convert<V, F>::type A;
38+
};
39+
40+
template <class V>
41+
struct Forward {
42+
typedef typename SomeTypeTrait<V>::A A;
43+
44+
private:
45+
~Forward() = delete;
46+
Forward(Forward const &) = delete;
47+
};
48+
49+
template <class V>
50+
struct SomeOtherTypeTrait : SomeTypeTrait<V> {
51+
typedef typename SomeTypeTrait<V>::A A;
52+
};
53+
54+
// Just to instantiate all the templates.
55+
struct FinalUser {
56+
typedef Forward<void *> F;
57+
typedef SomeOtherTypeTrait<void *> O;
58+
typedef SomeTypeTrait<void *> Z;
59+
};
60+
61+
void test(typename FinalUser::Z) {}
62+
63+
} // namespace RegressionTest

‎test/Interop/Cxx/class/Inputs/module.modulemap

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,3 +67,8 @@ module LinkedRecords {
6767
header "linked-records.h"
6868
requires cplusplus
6969
}
70+
71+
module InvalidNestedStruct {
72+
header "invalid-nested-struct.h"
73+
requires cplusplus
74+
}

‎test/Interop/Cxx/class/invalid-nested-struct-module-interface.swift

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
// RUN: %target-swift-ide-test -print-module -module-to-print=InvalidNestedStruct -I %S/Inputs/ -source-filename=x -enable-cxx-interop | %FileCheck %s
2+
3+
// CHECK-NOT: CannotImport
4+
// CHECK-NOT: ForwardDeclaredSibling

‎test/Interop/Cxx/class/type-classification-loadable-silgen.swift

Lines changed: 0 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -52,21 +52,6 @@ func pass(s: StructWithSubobjectDefaultedCopyConstructor) {
5252
// CHECK: bb0(%0 : $StructWithSubobjectDefaultedCopyConstructor):
5353
}
5454

55-
// CHECK-LABEL: sil hidden [ossa] @$s4main4pass{{.*[ (]}}StructWithMoveConstructor)
56-
func pass(s: StructWithMoveConstructor) {
57-
// CHECK: bb0(%0 : $*StructWithMoveConstructor):
58-
}
59-
60-
// CHECK-LABEL: sil hidden [ossa] @$s4main4pass{{.*[ (]}}StructWithInheritedMoveConstructor)
61-
func pass(s: StructWithInheritedMoveConstructor) {
62-
// CHECK: bb0(%0 : $*StructWithInheritedMoveConstructor):
63-
}
64-
65-
// CHECK-LABEL: sil hidden [ossa] @$s4main4pass{{.*[ (]}}StructWithSubobjectMoveConstructor)
66-
func pass(s: StructWithSubobjectMoveConstructor) {
67-
// CHECK: bb0(%0 : $*StructWithSubobjectMoveConstructor):
68-
}
69-
7055
// CHECK-LABEL: sil hidden [ossa] @$s4main4pass{{.*[ (]}}StructWithCopyAssignment)
7156
func pass(s: StructWithCopyAssignment) {
7257
// CHECK: bb0(%0 : $*StructWithCopyAssignment):
@@ -82,21 +67,6 @@ func pass(s: StructWithSubobjectCopyAssignment) {
8267
// CHECK: bb0(%0 : $*StructWithSubobjectCopyAssignment):
8368
}
8469

85-
// CHECK-LABEL: sil hidden [ossa] @$s4main4pass{{.*[ (]}}StructWithMoveAssignment)
86-
func pass(s: StructWithMoveAssignment) {
87-
// CHECK: bb0(%0 : $*StructWithMoveAssignment):
88-
}
89-
90-
// CHECK-LABEL: sil hidden [ossa] @$s4main4pass{{.*[ (]}}StructWithInheritedMoveAssignment)
91-
func pass(s: StructWithInheritedMoveAssignment) {
92-
// CHECK: bb0(%0 : $*StructWithInheritedMoveAssignment):
93-
}
94-
95-
// CHECK-LABEL: sil hidden [ossa] @$s4main4pass{{.*[ (]}}StructWithSubobjectMoveAssignment)
96-
func pass(s: StructWithSubobjectMoveAssignment) {
97-
// CHECK: bb0(%0 : $*StructWithSubobjectMoveAssignment):
98-
}
99-
10070
// CHECK-LABEL: sil hidden [ossa] @$s4main4pass{{.*[ (]}}StructWithDestructor)
10171
func pass(s: StructWithDestructor) {
10272
// CHECK: bb0(%0 : $*StructWithDestructor):
@@ -133,8 +103,3 @@ func pass(s: StructWithSubobjectDefaultedDestructor) {
133103
func pass(s: StructTriviallyCopyableMovable) {
134104
// CHECK: bb0(%0 : $StructTriviallyCopyableMovable):
135105
}
136-
137-
// CHECK-LABEL: sil hidden [ossa] @$s4main4pass{{.*[ (]}}StructNonCopyableNonMovable)
138-
func pass(s: StructNonCopyableNonMovable) {
139-
// CHECK: bb0(%0 : $StructNonCopyableNonMovable):
140-
}

‎test/Interop/Cxx/class/type-classification-module-interface.swift

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,13 @@
55
// CHECK-NOT: StructWithInheritedPrivateDefaultedCopyConstructor
66
// CHECK-NOT: StructWithSubobjectPrivateDefaultedCopyConstructor
77
// CHECK-NOT: StructNonCopyableTriviallyMovable
8+
// CHECK-NOT: StructNonCopyableNonMovable
9+
// CHECK-NOT: StructWithMoveConstructor
10+
// CHECK-NOT: StructWithInheritedMoveConstructor
11+
// CHECK-NOT: StructWithSubobjectMoveConstructor
12+
// CHECK-NOT: StructWithMoveAssignment
13+
// CHECK-NOT: StructWithInheritedMoveAssignment
14+
// CHECK-NOT: StructWithSubobjectMoveAssignment
815
// CHECK-NOT: StructWithPrivateDefaultedDestructor
916
// CHECK-NOT: StructWithInheritedPrivateDefaultedDestructor
1017
// CHECK-NOT: StructWithSubobjectPrivateDefaultedDestructor

0 commit comments

Comments
 (0)