Skip to content

Commit 9c44b79

Browse files
ahatanakaahatanaka
authored
[SILGen] Fix the type of closure thunks that are passed const reference structs (#76903)
The thunk's parameter needs the @in_guaranteed convention if it's a const reference parameter. However, that convention wasn't being used because clang importer was removing the const reference from the type and SILGen was computing the type of the parameter based on the type without const reference. This commit fixes the bug by passing the clang function type to SILDeclRef so that it can be used to compute the correct thunk type. This fixes a crash when a closure is passed to a C function taking a pointer to a function that has a const reference struct parameter. This recommits e074426 with fixes to serialization/deserialization of function types. The fixes prevent clang types of functions from being dropped during serialization. rdar://131321096
1 parent ee4d4a3 commit 9c44b79

File tree

13 files changed

+189
-34
lines changed

13 files changed

+189
-34
lines changed

‎include/swift/SIL/SILBridging.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1011,7 +1011,7 @@ struct BridgedSuccessorArray {
10111011
};
10121012

10131013
struct BridgedDeclRef {
1014-
uint64_t storage[3];
1014+
uint64_t storage[4];
10151015

10161016
#ifdef USED_IN_CPP_SOURCE
10171017
BridgedDeclRef(swift::SILDeclRef declRef) {
@@ -1029,7 +1029,7 @@ struct BridgedDeclRef {
10291029
};
10301030

10311031
struct BridgedVTableEntry {
1032-
uint64_t storage[5];
1032+
uint64_t storage[6];
10331033

10341034
enum class Kind {
10351035
Normal,
@@ -1077,7 +1077,7 @@ struct OptionalBridgedVTable {
10771077
};
10781078

10791079
struct BridgedWitnessTableEntry {
1080-
uint64_t storage[5];
1080+
uint64_t storage[6];
10811081

10821082
enum class Kind {
10831083
invalid,

‎include/swift/SIL/SILDeclRef.h‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@ namespace llvm {
3232
class raw_ostream;
3333
}
3434

35+
namespace clang {
36+
class Type;
37+
}
38+
3539
namespace swift {
3640
enum class EffectsKind : uint8_t;
3741
class AbstractFunctionDecl;
@@ -208,6 +212,9 @@ struct SILDeclRef {
208212
const GenericSignatureImpl *, CustomAttr *>
209213
pointer;
210214

215+
// Type of closure thunk.
216+
const clang::Type *thunkType = nullptr;
217+
211218
/// Returns the type of AST node location being stored by the SILDeclRef.
212219
LocKind getLocKind() const {
213220
if (loc.is<ValueDecl *>())
@@ -261,11 +268,10 @@ struct SILDeclRef {
261268
/// for the containing ClassDecl.
262269
/// - If 'loc' is a global VarDecl, this returns its GlobalAccessor
263270
/// SILDeclRef.
264-
explicit SILDeclRef(
265-
Loc loc,
266-
bool isForeign = false,
267-
bool isDistributed = false,
268-
bool isDistributedLocal = false);
271+
explicit SILDeclRef(Loc loc, bool isForeign = false,
272+
bool isDistributed = false,
273+
bool isDistributedLocal = false,
274+
const clang::Type *thunkType = nullptr);
269275

270276
/// See above put produces a prespecialization according to the signature.
271277
explicit SILDeclRef(Loc loc, GenericSignature prespecializationSig);

‎lib/SIL/IR/SILDeclRef.cpp‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -135,11 +135,15 @@ SILDeclRef::SILDeclRef(ValueDecl *vd, SILDeclRef::Kind kind, bool isForeign,
135135
isAsyncLetClosure(0), pointer(derivativeId) {}
136136

137137
SILDeclRef::SILDeclRef(SILDeclRef::Loc baseLoc, bool asForeign,
138-
bool asDistributed, bool asDistributedKnownToBeLocal)
138+
bool asDistributed, bool asDistributedKnownToBeLocal,
139+
const clang::Type *thunkType)
139140
: isRuntimeAccessible(false),
140141
backDeploymentKind(SILDeclRef::BackDeploymentKind::None),
141142
defaultArgIndex(0), isAsyncLetClosure(0),
142-
pointer((AutoDiffDerivativeFunctionIdentifier *)nullptr) {
143+
pointer((AutoDiffDerivativeFunctionIdentifier *)nullptr),
144+
thunkType(thunkType) {
145+
assert((!thunkType || baseLoc.is<AbstractClosureExpr *>()) &&
146+
"thunk type is needed only for closures");
143147
if (auto *vd = baseLoc.dyn_cast<ValueDecl*>()) {
144148
if (auto *fd = dyn_cast<FuncDecl>(vd)) {
145149
// Map FuncDecls directly to Func SILDeclRefs.
@@ -169,6 +173,8 @@ SILDeclRef::SILDeclRef(SILDeclRef::Loc baseLoc, bool asForeign,
169173
llvm_unreachable("invalid loc decl for SILDeclRef!");
170174
}
171175
} else if (auto *ACE = baseLoc.dyn_cast<AbstractClosureExpr *>()) {
176+
assert((!asForeign || thunkType) &&
177+
"thunk type needed for foreign type for closures");
172178
loc = ACE;
173179
kind = Kind::Func;
174180
if (ACE->getASTContext().LangOpts.hasFeature(

‎lib/SIL/IR/SILFunctionType.cpp‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3988,12 +3988,10 @@ static CanSILFunctionType getUncachedSILFunctionTypeForConstant(
39883988
// The type of the native-to-foreign thunk for a swift closure.
39893989
if (constant.isForeign && constant.hasClosureExpr() &&
39903990
shouldStoreClangType(TC.getDeclRefRepresentation(constant))) {
3991-
auto clangType = TC.Context.getClangFunctionType(
3992-
origLoweredInterfaceType->getParams(),
3993-
origLoweredInterfaceType->getResult(),
3994-
FunctionTypeRepresentation::CFunctionPointer);
3995-
AbstractionPattern pattern =
3996-
AbstractionPattern(origLoweredInterfaceType, clangType);
3991+
assert(!extInfoBuilder.getClangTypeInfo().empty() &&
3992+
"clang type not found");
3993+
AbstractionPattern pattern = AbstractionPattern(
3994+
origLoweredInterfaceType, extInfoBuilder.getClangTypeInfo().getType());
39973995
return getSILFunctionTypeForAbstractCFunction(
39983996
TC, pattern, origLoweredInterfaceType, extInfoBuilder, constant);
39993997
}
@@ -4501,9 +4499,13 @@ getAbstractionPatternForConstant(ASTContext &ctx, SILDeclRef constant,
45014499
if (!constant.isForeign)
45024500
return AbstractionPattern(fnType);
45034501

4502+
if (constant.thunkType)
4503+
return AbstractionPattern(fnType, constant.thunkType);
4504+
45044505
auto bridgedFn = getBridgedFunction(constant);
45054506
if (!bridgedFn)
45064507
return AbstractionPattern(fnType);
4508+
45074509
const clang::Decl *clangDecl = bridgedFn->getClangDecl();
45084510
if (!clangDecl)
45094511
return AbstractionPattern(fnType);

‎lib/SILGen/SILGenBridging.cpp‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1315,8 +1315,9 @@ static SILValue emitObjCUnconsumedArgument(SILGenFunction &SGF,
13151315
SILLocation loc,
13161316
SILValue arg) {
13171317
auto &lowering = SGF.getTypeLowering(arg->getType());
1318-
// If address-only, make a +1 copy and operate on that.
1319-
if (lowering.isAddressOnly() && SGF.useLoweredAddresses()) {
1318+
// If arg is non-trivial and has an address type, make a +1 copy and operate
1319+
// on that.
1320+
if (!lowering.isTrivial() && arg->getType().isAddress()) {
13201321
auto tmp = SGF.emitTemporaryAllocation(loc, arg->getType().getObjectType());
13211322
SGF.B.createCopyAddr(loc, arg, tmp, IsNotTake, IsInitialization);
13221323
return tmp;
@@ -1448,6 +1449,11 @@ emitObjCThunkArguments(SILGenFunction &SGF, SILLocation loc, SILDeclRef thunk,
14481449
auto buf = SGF.emitTemporaryAllocation(loc, native.getType());
14491450
native.forwardInto(SGF, loc, buf);
14501451
native = SGF.emitManagedBufferWithCleanup(buf);
1452+
} else if (!fnConv.isSILIndirect(nativeInputs[i]) &&
1453+
native.getType().isAddress()) {
1454+
// Load the value if the argument has an address type and the native
1455+
// function expects the argument to be passed directly.
1456+
native = SGF.emitManagedLoadCopy(loc, native.getValue());
14511457
}
14521458

14531459
if (nativeInputs[i].isConsumedInCaller()) {

‎lib/SILGen/SILGenExpr.cpp‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1723,7 +1723,19 @@ static ManagedValue convertCFunctionSignature(SILGenFunction &SGF,
17231723
FunctionConversionExpr *e,
17241724
SILType loweredResultTy,
17251725
llvm::function_ref<ManagedValue ()> fnEmitter) {
1726-
SILType loweredDestTy = SGF.getLoweredType(e->getType());
1726+
SILType loweredDestTy;
1727+
auto destTy = e->getType();
1728+
auto clangInfo =
1729+
destTy->castTo<AnyFunctionType>()->getExtInfo().getClangTypeInfo();
1730+
if (clangInfo.empty())
1731+
loweredDestTy = SGF.getLoweredType(destTy);
1732+
else
1733+
// This won't be necessary after we stop dropping clang types when
1734+
// canonicalizing function types.
1735+
loweredDestTy = SGF.getLoweredType(
1736+
AbstractionPattern(destTy->getCanonicalType(), clangInfo.getType()),
1737+
destTy);
1738+
17271739
ManagedValue result;
17281740

17291741
// We're converting between C function pointer types. They better be
@@ -1794,7 +1806,9 @@ ManagedValue emitCFunctionPointer(SILGenFunction &SGF,
17941806
#endif
17951807
semanticExpr = conv->getSubExpr()->getSemanticsProvidingExpr();
17961808
}
1797-
1809+
1810+
const clang::Type *destFnType = nullptr;
1811+
17981812
if (auto declRef = dyn_cast<DeclRefExpr>(semanticExpr)) {
17991813
setLocFromConcreteDeclRef(declRef->getDeclRef());
18001814
} else if (auto memberRef = dyn_cast<MemberRefExpr>(semanticExpr)) {
@@ -1808,12 +1822,18 @@ ManagedValue emitCFunctionPointer(SILGenFunction &SGF,
18081822
loc = closure;
18091823
return ManagedValue();
18101824
});
1825+
auto clangInfo = conversionExpr->getType()
1826+
->castTo<FunctionType>()
1827+
->getExtInfo()
1828+
.getClangTypeInfo();
1829+
if (!clangInfo.empty())
1830+
destFnType = clangInfo.getType();
18111831
} else {
18121832
llvm_unreachable("c function pointer converted from a non-concrete decl ref");
18131833
}
18141834

18151835
// Produce a reference to the C-compatible entry point for the function.
1816-
SILDeclRef constant(loc, /*foreign*/ true);
1836+
SILDeclRef constant(loc, /*foreign*/ true, false, false, destFnType);
18171837
SILConstantInfo constantInfo =
18181838
SGF.getConstantInfo(SGF.getTypeExpansionContext(), constant);
18191839

‎lib/Serialization/Deserialization.cpp‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8139,9 +8139,6 @@ class SwiftToClangBasicReader :
81398139

81408140
llvm::Expected<const clang::Type *>
81418141
ModuleFile::getClangType(ClangTypeID TID) {
8142-
if (!getContext().LangOpts.UseClangFunctionTypes)
8143-
return nullptr;
8144-
81458142
if (TID == 0)
81468143
return nullptr;
81478144

‎lib/Serialization/Serialization.cpp‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5696,10 +5696,7 @@ class Serializer::TypeSerializer : public TypeVisitor<TypeSerializer> {
56965696
using namespace decls_block;
56975697

56985698
auto resultType = S.addTypeRef(fnTy->getResult());
5699-
auto clangType =
5700-
S.getASTContext().LangOpts.UseClangFunctionTypes
5701-
? S.addClangTypeRef(fnTy->getClangTypeInfo().getType())
5702-
: ClangTypeID(0);
5699+
auto clangType = S.addClangTypeRef(fnTy->getClangTypeInfo().getType());
57035700

57045701
auto isolation = encodeIsolation(fnTy->getIsolation());
57055702

‎test/Interop/Cxx/class/Inputs/closure.h‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ struct NonTrivial {
1010
int *p;
1111
};
1212

13+
struct Trivial {
14+
int i;
15+
};
16+
1317
void cfunc(void (^ _Nonnull block)(NonTrivial)) noexcept {
1418
block(NonTrivial());
1519
}
@@ -45,4 +49,13 @@ void cfuncARCWeak(ARCWeak) noexcept;
4549
void (* _Nonnull getFnPtr() noexcept)(NonTrivial) noexcept;
4650
void (* _Nonnull getFnPtr2() noexcept)(ARCWeak) noexcept;
4751

52+
void cfuncConstRefNonTrivial(void (*_Nonnull)(const NonTrivial &));
53+
void cfuncConstRefTrivial(void (*_Nonnull)(const Trivial &));
54+
void blockConstRefNonTrivial(void (^_Nonnull)(const NonTrivial &));
55+
void blockConstRefTrivial(void (^_Nonnull)(const Trivial &));
56+
#if __OBJC__
57+
void cfuncConstRefStrong(void (*_Nonnull)(const ARCStrong &));
58+
void blockConstRefStrong(void (^_Nonnull)(const ARCStrong &));
59+
#endif
60+
4861
#endif // __CLOSURE__

‎test/Interop/Cxx/class/closure-thunk-macosx.swift‎

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,3 +35,88 @@ public func testClosureToFuncPtr() {
3535
public func testClosureToBlockReturnNonTrivial() {
3636
cfuncReturnNonTrivial({() -> NonTrivial in return NonTrivial() })
3737
}
38+
39+
// CHECK-LABEL: sil private [thunk] [ossa] @$s4main22testConstRefNonTrivialyyFySo0eF0VcfU_To : $@convention(c) (@in_guaranteed NonTrivial) -> () {
40+
// CHECK: bb0(%[[V0:.*]] : $*NonTrivial):
41+
// CHECK: %[[V1:.*]] = alloc_stack $NonTrivial
42+
// CHECK: copy_addr %[[V0]] to [init] %[[V1]] : $*NonTrivial
43+
// CHECK: %[[V3:.*]] = function_ref @$s4main22testConstRefNonTrivialyyFySo0eF0VcfU_ : $@convention(thin) (@in_guaranteed NonTrivial) -> ()
44+
// CHECK: %[[V4:.*]] = apply %[[V3]](%[[V1]]) : $@convention(thin) (@in_guaranteed NonTrivial) -> ()
45+
// CHECK: destroy_addr %[[V1]] : $*NonTrivial
46+
// CHECK: dealloc_stack %[[V1]] : $*NonTrivial
47+
// CHECK: return %[[V4]] : $()
48+
49+
public func testConstRefNonTrivial() {
50+
cfuncConstRefNonTrivial({S in });
51+
}
52+
53+
// CHECK-LABEL: sil private [thunk] [ossa] @$s4main19testConstRefTrivialyyFySo0E0VcfU_To : $@convention(c) (@in_guaranteed Trivial) -> () {
54+
// CHECK: bb0(%[[V0:.*]] : $*Trivial):
55+
// CHECK: %[[V1:.*]] = load [trivial] %[[V0]] : $*Trivial
56+
// CHECK: %[[V2:.*]] = function_ref @$s4main19testConstRefTrivialyyFySo0E0VcfU_ : $@convention(thin) (Trivial) -> ()
57+
// CHECK: %[[V3:.*]] = apply %[[V2]](%[[V1]]) : $@convention(thin) (Trivial) -> ()
58+
// CHECK: return %[[V3]] : $()
59+
60+
public func testConstRefTrivial() {
61+
cfuncConstRefTrivial({S in });
62+
}
63+
64+
// CHECK-LABEL: sil private [thunk] [ossa] @$s4main18testConstRefStrongyyFySo9ARCStrongVcfU_To : $@convention(c) (@in_guaranteed ARCStrong) -> () {
65+
// CHECK: bb0(%[[V0:.*]] : $*ARCStrong):
66+
// CHECK: %[[V1:.*]] = alloc_stack $ARCStrong
67+
// CHECK: copy_addr %[[V0]] to [init] %[[V1]] : $*ARCStrong
68+
// CHECK: %[[V3:.*]] = load [copy] %[[V1]] : $*ARCStrong
69+
// CHECK: %[[V4:.*]] = begin_borrow %[[V3]] : $ARCStrong
70+
// CHECK: %[[V5:.*]] = function_ref @$s4main18testConstRefStrongyyFySo9ARCStrongVcfU_ : $@convention(thin) (@guaranteed ARCStrong) -> ()
71+
// CHECK: %[[V6:.*]] = apply %[[V5]](%[[V4]]) : $@convention(thin) (@guaranteed ARCStrong) -> ()
72+
// CHECK: end_borrow %[[V4]] : $ARCStrong
73+
// CHECK: destroy_value %[[V3]] : $ARCStrong
74+
// CHECK: destroy_addr %[[V1]] : $*ARCStrong
75+
// CHECK: dealloc_stack %[[V1]] : $*ARCStrong
76+
// CHECK: return %[[V6]] : $()
77+
78+
public func testConstRefStrong() {
79+
cfuncConstRefStrong({S in });
80+
}
81+
82+
// CHECK-LABEL: sil shared [transparent] [serialized] [reabstraction_thunk] [ossa] @$sSo10NonTrivialVIegn_ABIeyBn_TR : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (@in_guaranteed NonTrivial) -> (), @in_guaranteed NonTrivial) -> () {
83+
// CHECK: bb0(%[[V0:.*]] : $*@block_storage @callee_guaranteed (@in_guaranteed NonTrivial) -> (), %[[V1:.*]] : $*NonTrivial):
84+
// CHECK: %[[V2:.*]] = project_block_storage %[[V0]] : $*@block_storage @callee_guaranteed (@in_guaranteed NonTrivial) -> ()
85+
// CHECK: %[[V3:.*]] = load [copy] %[[V2]] : $*@callee_guaranteed (@in_guaranteed NonTrivial) -> ()
86+
// CHECK: %[[V4:.*]] = begin_borrow %[[V3]] : $@callee_guaranteed (@in_guaranteed NonTrivial) -> ()
87+
// CHECK: apply %[[V4]](%[[V1]]) : $@callee_guaranteed (@in_guaranteed NonTrivial) -> ()
88+
// CHECK: end_borrow %[[V4]] : $@callee_guaranteed (@in_guaranteed NonTrivial) -> ()
89+
// CHECK: destroy_value %[[V3]] : $@callee_guaranteed (@in_guaranteed NonTrivial) -> ()
90+
91+
public func testBlockConstRefNonTrivial() {
92+
blockConstRefNonTrivial({S in });
93+
}
94+
95+
// CHECK-LABEL: sil shared [transparent] [serialized] [reabstraction_thunk] [ossa] @$sSo7TrivialVIegy_ABIeyBn_TR : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (Trivial) -> (), @in_guaranteed Trivial) -> () {
96+
// CHECK: bb0(%[[V0:.*]] : $*@block_storage @callee_guaranteed (Trivial) -> (), %[[V1:.*]] : $*Trivial):
97+
// CHECK: %[[V2:.*]] = project_block_storage %[[V0]] : $*@block_storage @callee_guaranteed (Trivial) -> ()
98+
// CHECK: %[[V3:.*]] = load [copy] %[[V2]] : $*@callee_guaranteed (Trivial) -> ()
99+
// CHECK: %[[V4:.*]] = load [trivial] %[[V1]] : $*Trivial
100+
// CHECK: %[[V5:.*]] = begin_borrow %[[V3]] : $@callee_guaranteed (Trivial) -> ()
101+
// CHECK: apply %[[V5]](%[[V4]]) : $@callee_guaranteed (Trivial) -> ()
102+
// CHECK: end_borrow %[[V5]] : $@callee_guaranteed (Trivial) -> ()
103+
// CHECK: destroy_value %[[V3]] : $@callee_guaranteed (Trivial) -> ()
104+
105+
public func testBlockConstRefTrivial() {
106+
blockConstRefTrivial({S in });
107+
}
108+
109+
// CHECK-LABEL: sil shared [transparent] [serialized] [reabstraction_thunk] [ossa] @$sSo9ARCStrongVIegg_ABIeyBn_TR : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (@guaranteed ARCStrong) -> (), @in_guaranteed ARCStrong) -> () {
110+
// CHECK: bb0(%[[V0:.*]] : $*@block_storage @callee_guaranteed (@guaranteed ARCStrong) -> (), %[[V1:.*]] : $*ARCStrong):
111+
// CHECK: %[[V2:.*]] = project_block_storage %[[V0]] : $*@block_storage @callee_guaranteed (@guaranteed ARCStrong) -> ()
112+
// CHECK: %[[V3:.*]] = load [copy] %[[V2]] : $*@callee_guaranteed (@guaranteed ARCStrong) -> ()
113+
// CHECK: %[[V4:.*]] = load_borrow %[[V1]] : $*ARCStrong
114+
// CHECK: %[[V5:.*]] = begin_borrow %[[V3]] : $@callee_guaranteed (@guaranteed ARCStrong) -> ()
115+
// CHECK: apply %[[V5]](%[[V4]]) : $@callee_guaranteed (@guaranteed ARCStrong) -> ()
116+
// CHECK: end_borrow %[[V5]] : $@callee_guaranteed (@guaranteed ARCStrong) -> ()
117+
// CHECK: end_borrow %[[V4]] : $ARCStrong
118+
// CHECK: destroy_value %[[V3]] : $@callee_guaranteed (@guaranteed ARCStrong) -> ()
119+
120+
public func testBlockConstRefStrong() {
121+
blockConstRefStrong({S in });
122+
}

0 commit comments

Comments
 (0)