Skip to content

Commit 8e4cfa3

Browse files
egorzhdanegorzhdan
authored
Merge pull request #78237 from swiftlang/egorzhdan/6.1-derived-out-of-order
🍒[cxx-interop] Fix memory layout for structs with out-of-order base types
2 parents f3fe377 + 8114f8e commit 8e4cfa3

File tree

10 files changed

+167
-65
lines changed

10 files changed

+167
-65
lines changed

‎lib/IRGen/GenClangDecl.cpp‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include "clang/AST/Expr.h"
2323
#include "clang/AST/ExprCXX.h"
2424
#include "clang/AST/GlobalDecl.h"
25+
#include "clang/AST/RecordLayout.h"
2526
#include "clang/AST/RecursiveASTVisitor.h"
2627
#include "clang/CodeGen/ModuleBuilder.h"
2728
#include "clang/Sema/Sema.h"
@@ -366,3 +367,41 @@ void IRGenModule::ensureImplicitCXXDestructorBodyIsDefined(
366367
auto &sema = Context.getClangModuleLoader()->getClangSema();
367368
sema.DefineImplicitDestructor(clang::SourceLocation(), destructor);
368369
}
370+
371+
/// Retrieves the base classes of a C++ struct/class ordered by their offset in
372+
/// the derived type's memory layout.
373+
SmallVector<CXXBaseRecordLayout, 1>
374+
irgen::getBasesAndOffsets(const clang::CXXRecordDecl *decl) {
375+
auto &layout = decl->getASTContext().getASTRecordLayout(decl);
376+
377+
// Collect the offsets and sizes of base classes within the memory layout
378+
// of the derived class.
379+
SmallVector<CXXBaseRecordLayout, 1> baseOffsetsAndSizes;
380+
for (auto base : decl->bases()) {
381+
if (base.isVirtual())
382+
continue;
383+
384+
auto baseType = base.getType().getCanonicalType();
385+
auto baseRecordType = cast<clang::RecordType>(baseType);
386+
auto baseRecord = baseRecordType->getAsCXXRecordDecl();
387+
assert(baseRecord && "expected a base C++ record");
388+
389+
if (baseRecord->isEmpty())
390+
continue;
391+
392+
auto offset = Size(layout.getBaseClassOffset(baseRecord).getQuantity());
393+
auto size =
394+
Size(decl->getASTContext().getTypeSizeInChars(baseType).getQuantity());
395+
396+
baseOffsetsAndSizes.push_back({baseRecord, offset, size});
397+
}
398+
399+
// In C++, base classes might get reordered if the primary base was not
400+
// the first base type on the declaration of the class.
401+
llvm::sort(baseOffsetsAndSizes, [](const CXXBaseRecordLayout &lhs,
402+
const CXXBaseRecordLayout &rhs) {
403+
return lhs.offset < rhs.offset;
404+
});
405+
406+
return baseOffsetsAndSizes;
407+
}

‎lib/IRGen/GenClass.cpp‎

Lines changed: 8 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -294,32 +294,18 @@ namespace {
294294
if (!cxxRecord)
295295
return;
296296

297-
auto &layout = cxxRecord->getASTContext().getASTRecordLayout(cxxRecord);
298-
299-
for (auto base : cxxRecord->bases()) {
300-
if (base.isVirtual())
301-
continue;
302-
303-
auto baseType = base.getType().getCanonicalType();
304-
305-
auto baseRecord = cast<clang::RecordType>(baseType)->getDecl();
306-
auto baseCxxRecord = cast<clang::CXXRecordDecl>(baseRecord);
307-
308-
if (baseCxxRecord->isEmpty())
309-
continue;
310-
311-
auto offset = Size(layout.getBaseClassOffset(baseCxxRecord).getQuantity());
312-
auto size = Size(cxxRecord->getASTContext().getTypeSizeInChars(baseType).getQuantity());
313-
314-
if (offset != CurSize) {
315-
assert(offset > CurSize);
316-
auto paddingSize = offset - CurSize;
317-
auto &opaqueTI = IGM.getOpaqueStorageTypeInfo(paddingSize, Alignment(1));
297+
auto bases = getBasesAndOffsets(cxxRecord);
298+
for (auto base : bases) {
299+
if (base.offset != CurSize) {
300+
assert(base.offset > CurSize);
301+
auto paddingSize = base.offset - CurSize;
302+
auto &opaqueTI =
303+
IGM.getOpaqueStorageTypeInfo(paddingSize, Alignment(1));
318304
auto element = ElementLayout::getIncomplete(opaqueTI);
319305
addField(element, LayoutStrategy::Universal);
320306
}
321307

322-
auto &opaqueTI = IGM.getOpaqueStorageTypeInfo(size, Alignment(1));
308+
auto &opaqueTI = IGM.getOpaqueStorageTypeInfo(base.size, Alignment(1));
323309
auto element = ElementLayout::getIncomplete(opaqueTI);
324310
addField(element, LayoutStrategy::Universal);
325311
}

‎lib/IRGen/GenStruct.cpp‎

Lines changed: 11 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -368,28 +368,6 @@ namespace {
368368
ClangFieldInfo> {
369369
const clang::RecordDecl *ClangDecl;
370370

371-
template <class Fn>
372-
void forEachNonEmptyBase(Fn fn) const {
373-
auto &layout = ClangDecl->getASTContext().getASTRecordLayout(ClangDecl);
374-
375-
if (auto cxxRecord = dyn_cast<clang::CXXRecordDecl>(ClangDecl)) {
376-
for (auto base : cxxRecord->bases()) {
377-
auto baseType = base.getType().getCanonicalType();
378-
379-
auto baseRecord = cast<clang::RecordType>(baseType)->getDecl();
380-
auto baseCxxRecord = cast<clang::CXXRecordDecl>(baseRecord);
381-
382-
if (baseCxxRecord->isEmpty())
383-
continue;
384-
385-
auto offset = layout.getBaseClassOffset(baseCxxRecord);
386-
auto size =
387-
ClangDecl->getASTContext().getTypeSizeInChars(baseType);
388-
fn(baseType, offset, size);
389-
}
390-
}
391-
}
392-
393371
public:
394372
LoadableClangRecordTypeInfo(ArrayRef<ClangFieldInfo> fields,
395373
unsigned explosionSize, llvm::Type *storageType,
@@ -446,10 +424,11 @@ namespace {
446424

447425
void addToAggLowering(IRGenModule &IGM, SwiftAggLowering &lowering,
448426
Size offset) const override {
449-
forEachNonEmptyBase([&](clang::QualType type, clang::CharUnits offset,
450-
clang::CharUnits) {
451-
lowering.addTypedData(type, offset);
452-
});
427+
if (auto cxxRecordDecl = dyn_cast<clang::CXXRecordDecl>(ClangDecl)) {
428+
for (auto base : getBasesAndOffsets(cxxRecordDecl)) {
429+
lowering.addTypedData(base.decl, base.offset.asCharUnits());
430+
}
431+
}
453432

454433
lowering.addTypedData(ClangDecl, offset.asCharUnits());
455434
}
@@ -1395,23 +1374,12 @@ class ClangRecordLowering {
13951374
void collectBases(const clang::RecordDecl *decl) {
13961375
auto &layout = decl->getASTContext().getASTRecordLayout(decl);
13971376
if (auto cxxRecord = dyn_cast<clang::CXXRecordDecl>(decl)) {
1398-
for (auto base : cxxRecord->bases()) {
1399-
if (base.isVirtual())
1400-
continue;
1401-
1402-
auto baseType = base.getType().getCanonicalType();
1403-
1404-
auto baseRecord = cast<clang::RecordType>(baseType)->getDecl();
1405-
auto baseCxxRecord = cast<clang::CXXRecordDecl>(baseRecord);
1406-
1407-
if (baseCxxRecord->isEmpty())
1408-
continue;
1409-
1410-
auto baseOffset = Size(layout.getBaseClassOffset(baseCxxRecord).getQuantity());
1411-
SubobjectAdjustment += baseOffset;
1412-
collectBases(baseCxxRecord);
1413-
collectStructFields(baseCxxRecord);
1414-
SubobjectAdjustment -= baseOffset;
1377+
auto bases = getBasesAndOffsets(cxxRecord);
1378+
for (auto base : bases) {
1379+
SubobjectAdjustment += base.offset;
1380+
collectBases(base.decl);
1381+
collectStructFields(base.decl);
1382+
SubobjectAdjustment -= base.offset;
14151383
}
14161384
}
14171385
}

‎lib/IRGen/IRGenModule.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2029,6 +2029,17 @@ class CurrentIGMPtr {
20292029
/// Workaround to disable thumb-mode until debugger support is there.
20302030
bool shouldRemoveTargetFeature(StringRef);
20312031

2032+
struct CXXBaseRecordLayout {
2033+
const clang::CXXRecordDecl *decl;
2034+
Size offset;
2035+
Size size;
2036+
};
2037+
2038+
/// Retrieves the base classes of a C++ struct/class ordered by their offset in
2039+
/// the derived type's memory layout.
2040+
SmallVector<CXXBaseRecordLayout, 1>
2041+
getBasesAndOffsets(const clang::CXXRecordDecl *decl);
2042+
20322043
} // end namespace irgen
20332044
} // end namespace swift
20342045

‎test/Interop/Cxx/class/inheritance/Inputs/fields.h‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,3 +132,27 @@ struct DerivedHasTailPadding : public BaseAlign8 {
132132
struct DerivedUsesBaseTailPadding : public DerivedHasTailPadding {
133133
short field2 = 789;
134134
}; // sizeof=16, dsize=14, align=8
135+
136+
// MARK: Types with an out-of-order inheritance.
137+
138+
struct BaseWithVirtualDestructor {
139+
int baseField = 123;
140+
141+
virtual ~BaseWithVirtualDestructor() {}
142+
};
143+
144+
struct DerivedWithVirtualDestructor : public BaseWithVirtualDestructor {
145+
int derivedField = 456;
146+
147+
~DerivedWithVirtualDestructor() override {}
148+
};
149+
150+
struct DerivedOutOfOrder : public HasOneField,
151+
public DerivedWithVirtualDestructor {
152+
// DerivedWithVirtualDestructor is the primary base class despite being the
153+
// second one the list.
154+
155+
int leafField = 789;
156+
157+
~DerivedOutOfOrder() override {}
158+
};

‎test/Interop/Cxx/class/inheritance/fields.swift‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,4 +124,11 @@ FieldsTestSuite.test("Field in tail padding of base class") {
124124
expectEqual(usesBaseTailPadding.field8, 123)
125125
}
126126

127+
FieldsTestSuite.test("Out-of-order inheritance") {
128+
let d = DerivedOutOfOrder()
129+
expectEqual(d.leafField, 789)
130+
expectEqual(d.derivedField, 456)
131+
expectEqual(d.baseField, 123)
132+
}
133+
127134
runAllTests()

‎test/Interop/Cxx/foreign-reference/Inputs/inheritance.h‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,44 @@ SubT : BaseT {
2727
SubT(const SubT &) = delete;
2828
static SubT &getSubT() { static SubT singleton; return singleton; }
2929
};
30+
31+
struct
32+
__attribute__((swift_attr("import_reference")))
33+
__attribute__((swift_attr("retain:immortal")))
34+
__attribute__((swift_attr("release:immortal")))
35+
BaseWithVirtualDestructor {
36+
int baseField = 123;
37+
38+
BaseWithVirtualDestructor() {}
39+
virtual ~BaseWithVirtualDestructor() {}
40+
};
41+
42+
struct
43+
__attribute__((swift_attr("import_reference")))
44+
__attribute__((swift_attr("retain:immortal")))
45+
__attribute__((swift_attr("release:immortal")))
46+
DerivedWithVirtualDestructor : public BaseWithVirtualDestructor {
47+
int derivedField = 456;
48+
49+
DerivedWithVirtualDestructor() : BaseWithVirtualDestructor() {}
50+
~DerivedWithVirtualDestructor() override {}
51+
};
52+
53+
struct
54+
__attribute__((swift_attr("import_reference")))
55+
__attribute__((swift_attr("retain:immortal")))
56+
__attribute__((swift_attr("release:immortal")))
57+
DerivedOutOfOrder : public BaseT, public DerivedWithVirtualDestructor {
58+
// DerivedWithVirtualDestructor is the primary base class despite being the
59+
// second one the list.
60+
61+
int leafField = 789;
62+
63+
DerivedOutOfOrder() = default;
64+
~DerivedOutOfOrder() override {}
65+
66+
static DerivedOutOfOrder& getInstance() {
67+
static DerivedOutOfOrder singleton;
68+
return singleton;
69+
}
70+
};

‎test/Interop/Cxx/foreign-reference/inheritance-irgen.swift‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
// RUN: %target-swift-emit-ir %s -I %S/Inputs -enable-experimental-cxx-interop -Xcc -fignore-exceptions -disable-availability-checking
2+
3+
// This ensured we do not crash during IRGen for inherited C++ foreign reference types.
4+
5+
import Inheritance
6+
7+
@inline(never)
8+
public func blackHole<T>(_ _: T) {}
9+
10+
let x = DerivedOutOfOrder.getInstance()
11+
12+
blackHole(x.baseField)
13+
blackHole(x.derivedField)
14+
blackHole(x.leafField)

‎test/Interop/Cxx/foreign-reference/inheritance-typechecker.swift‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,3 +21,8 @@ let b: BaseT = BaseT.getBaseT()
2121
assert(b.isBase)
2222
let bc: BaseT = cxxCast(b) // should instantiate I and O both to BaseT
2323
assert(bc.isBase)
24+
25+
let d = DerivedOutOfOrder.getInstance()
26+
let _ = d.baseField
27+
let _ = d.derivedField
28+
let _ = d.leafField

‎test/Interop/Cxx/foreign-reference/inheritance.swift‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,4 +30,11 @@ TemplatingTestSuite.test("BaseT") {
3030
assert(bc.isBase)
3131
}
3232

33+
TemplatingTestSuite.test("DerivedOutOfOrder") {
34+
let d = DerivedOutOfOrder.getInstance()
35+
expectEqual(123, d.baseField)
36+
expectEqual(456, d.derivedField)
37+
expectEqual(789, d.leafField)
38+
}
39+
3340
runAllTests()

0 commit comments

Comments
 (0)