Skip to content

Commit 65bf53d

Browse files
Xazax-hunGabor Horvath
authored andcommitted
[6.2][cxx-interop] Fix over-releasing reference members of trival C++ types
Explanation: Large trivial types were copied via memcpy instead of doing a field-wise copy. This is incorrect for types with reference fields where we also need to bump the corresponding refcounts. This PR makes sure that trival C++ types with reference members are not trivial in Swift. Issues: rdar://160315343 Original PRs: #84321 Risk: There is a low chance of breaking source compatibility as some types that used to be BitwiseCopyable are no longer considered as such. However, code relying on that probably has latent memory safety bugs. Testing: Added a compiler test. Reviewers: @egorzhdan, @j-hui
1 parent 9016636 commit 65bf53d

File tree

4 files changed

+126
-10
lines changed

4 files changed

+126
-10
lines changed

lib/IRGen/GenStruct.cpp

Lines changed: 30 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include "swift/AST/Decl.h"
2323
#include "swift/AST/IRGenOptions.h"
2424
#include "swift/AST/Pattern.h"
25+
#include "swift/AST/ReferenceCounting.h"
2526
#include "swift/AST/SemanticAttrs.h"
2627
#include "swift/AST/SubstitutionMap.h"
2728
#include "swift/AST/Types.h"
@@ -369,6 +370,7 @@ namespace {
369370
: public StructTypeInfoBase<LoadableClangRecordTypeInfo, LoadableTypeInfo,
370371
ClangFieldInfo> {
371372
const clang::RecordDecl *ClangDecl;
373+
bool HasReferenceField;
372374

373375
public:
374376
LoadableClangRecordTypeInfo(ArrayRef<ClangFieldInfo> fields,
@@ -377,14 +379,14 @@ namespace {
377379
Alignment align,
378380
IsTriviallyDestroyable_t isTriviallyDestroyable,
379381
IsCopyable_t isCopyable,
380-
const clang::RecordDecl *clangDecl)
382+
const clang::RecordDecl *clangDecl,
383+
bool hasReferenceField)
381384
: StructTypeInfoBase(StructTypeInfoKind::LoadableClangRecordTypeInfo,
382385
fields, explosionSize, FieldsAreABIAccessible,
383386
storageType, size, std::move(spareBits), align,
384-
isTriviallyDestroyable,
385-
isCopyable,
386-
IsFixedSize, IsABIAccessible),
387-
ClangDecl(clangDecl) {}
387+
isTriviallyDestroyable, isCopyable, IsFixedSize,
388+
IsABIAccessible),
389+
ClangDecl(clangDecl), HasReferenceField(hasReferenceField) {}
388390

389391
TypeLayoutEntry
390392
*buildTypeLayoutEntry(IRGenModule &IGM,
@@ -448,6 +450,7 @@ namespace {
448450
const ClangFieldInfo &field) const {
449451
llvm_unreachable("non-fixed field in Clang type?");
450452
}
453+
bool hasReferenceField() const { return HasReferenceField; }
451454
};
452455

453456
class AddressOnlyPointerAuthRecordTypeInfo final
@@ -1320,6 +1323,11 @@ class ClangRecordLowering {
13201323
Size NextOffset = Size(0);
13211324
Size SubobjectAdjustment = Size(0);
13221325
unsigned NextExplosionIndex = 0;
1326+
// Types that are trivial in C++ but are containing fields to reference types
1327+
// are not trivial in Swift, they cannot be copied using memcpy as we need to
1328+
// do the proper retain operations.
1329+
bool hasReferenceField = false;
1330+
13231331
public:
13241332
ClangRecordLowering(IRGenModule &IGM, StructDecl *swiftDecl,
13251333
const clang::RecordDecl *clangDecl,
@@ -1360,11 +1368,12 @@ class ClangRecordLowering {
13601368
return LoadableClangRecordTypeInfo::create(
13611369
FieldInfos, NextExplosionIndex, llvmType, TotalStride,
13621370
std::move(SpareBits), TotalAlignment,
1363-
(SwiftDecl && SwiftDecl->getValueTypeDestructor())
1364-
? IsNotTriviallyDestroyable : IsTriviallyDestroyable,
1365-
(SwiftDecl && !SwiftDecl->canBeCopyable())
1366-
? IsNotCopyable : IsCopyable,
1367-
ClangDecl);
1371+
(SwiftDecl &&
1372+
(SwiftDecl->getValueTypeDestructor() || hasReferenceField))
1373+
? IsNotTriviallyDestroyable
1374+
: IsTriviallyDestroyable,
1375+
(SwiftDecl && !SwiftDecl->canBeCopyable()) ? IsNotCopyable : IsCopyable,
1376+
ClangDecl, hasReferenceField);
13681377
}
13691378

13701379
private:
@@ -1489,6 +1498,17 @@ class ClangRecordLowering {
14891498
SwiftType.getFieldType(swiftField, IGM.getSILModule(),
14901499
IGM.getMaximalTypeExpansionContext())));
14911500
addField(swiftField, offset, fieldTI, isZeroSized);
1501+
auto fieldTy =
1502+
swiftField->getInterfaceType()->lookThroughSingleOptionalType();
1503+
if (fieldTy->isAnyClassReferenceType() &&
1504+
fieldTy->getReferenceCounting() != ReferenceCounting::None)
1505+
hasReferenceField = true;
1506+
else if (auto structDecl = fieldTy->getStructOrBoundGenericStruct();
1507+
structDecl && structDecl->hasClangNode() &&
1508+
getStructTypeInfoKind(fieldTI) ==
1509+
StructTypeInfoKind::LoadableClangRecordTypeInfo)
1510+
if (fieldTI.as<LoadableClangRecordTypeInfo>().hasReferenceField())
1511+
hasReferenceField = true;
14921512
return;
14931513
}
14941514

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
#pragma once
2+
3+
#include <stdio.h>
4+
#include <swift/bridging>
5+
6+
class SharedFRT {
7+
public:
8+
SharedFRT() : _refCount(1) { logMsg("Ctor"); }
9+
10+
private:
11+
void logMsg(const char *s) const {
12+
printf("RefCount: %d, message: %s\n", _refCount, s);
13+
}
14+
15+
~SharedFRT() { logMsg("Dtor"); }
16+
SharedFRT(const SharedFRT &) = delete;
17+
SharedFRT &operator=(const SharedFRT &) = delete;
18+
SharedFRT(SharedFRT &&) = delete;
19+
SharedFRT &operator=(SharedFRT &&) = delete;
20+
21+
int _refCount;
22+
23+
friend void retainSharedFRT(SharedFRT *_Nonnull);
24+
friend void releaseSharedFRT(SharedFRT *_Nonnull);
25+
} SWIFT_SHARED_REFERENCE(retainSharedFRT, releaseSharedFRT);
26+
27+
class MyToken {
28+
public:
29+
MyToken() = default;
30+
MyToken(MyToken const &) {}
31+
};
32+
33+
inline void retainSharedFRT(SharedFRT *_Nonnull x) {
34+
++x->_refCount;
35+
x->logMsg("retain");
36+
}
37+
38+
inline void releaseSharedFRT(SharedFRT *_Nonnull x) {
39+
--x->_refCount;
40+
x->logMsg("release");
41+
if (x->_refCount == 0)
42+
delete x;
43+
}
44+
45+
struct LargeStructWithRefCountedField {
46+
void const *a;
47+
void const *b;
48+
unsigned long c;
49+
unsigned d;
50+
SharedFRT *e;
51+
};
52+
53+
struct LargeStructWithRefCountedFieldNested {
54+
int a;
55+
LargeStructWithRefCountedField b;
56+
};
57+
58+
inline LargeStructWithRefCountedField getStruct() {
59+
return {0, 0, 0, 0, new SharedFRT()};
60+
}
61+
62+
inline LargeStructWithRefCountedFieldNested getNestedStruct() {
63+
return {0, {0, 0, 0, 0, new SharedFRT()}};
64+
}

test/Interop/Cxx/foreign-reference/Inputs/module.modulemap

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,3 +78,8 @@ module VirtMethodWithRvalRef {
7878
header "virtual-methods-with-rvalue-reference.h"
7979
requires cplusplus
8080
}
81+
82+
module LoggingFrts {
83+
header "logging-frts.h"
84+
requires cplusplus
85+
}
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// RUN: %target-run-simple-swift(-I %swift_src_root/lib/ClangImporter/SwiftBridging -I %S/Inputs -cxx-interoperability-mode=default -Xfrontend -disable-availability-checking -Onone) | %FileCheck %s
2+
3+
// REQUIRES: executable_test
4+
5+
import LoggingFrts
6+
7+
func takesLargeStructWithRefCountedField(_ x: LargeStructWithRefCountedField) {
8+
var a = x
9+
}
10+
11+
takesLargeStructWithRefCountedField(getStruct())
12+
// CHECK: RefCount: 1, message: Ctor
13+
// CHECK-NEXT: RefCount: 2, message: retain
14+
// CHECK-NEXT: RefCount: 1, message: release
15+
// CHECK-NEXT: RefCount: 0, message: release
16+
// CHECK-NEXT: RefCount: 0, message: Dtor
17+
18+
func takesLargeStructWithRefCountedFieldNested(_ x: LargeStructWithRefCountedFieldNested) {
19+
var a = x
20+
}
21+
22+
takesLargeStructWithRefCountedFieldNested(getNestedStruct())
23+
// CHECK: RefCount: 1, message: Ctor
24+
// CHECK-NEXT: RefCount: 2, message: retain
25+
// CHECK-NEXT: RefCount: 1, message: release
26+
// CHECK-NEXT: RefCount: 0, message: release
27+
// CHECK-NEXT: RefCount: 0, message: Dtor

0 commit comments

Comments
 (0)