Skip to content

Commit e074426

Browse files
ahatanakaahatanaka
authored
[SILGen] Fix the type of closure thunks that are passed const reference structs (#75491)
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. rdar://131321096
1 parent 89708dd commit e074426

File tree

9 files changed

+164
-27
lines changed

9 files changed

+164
-27
lines changed

‎include/swift/SIL/SILBridging.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1081,7 +1081,7 @@ struct BridgedSuccessorArray {
10811081
};
10821082

10831083
struct BridgedDeclRef {
1084-
uint64_t storage[3];
1084+
uint64_t storage[4];
10851085

10861086
#ifdef USED_IN_CPP_SOURCE
10871087
BridgedDeclRef(swift::SILDeclRef declRef) {
@@ -1098,7 +1098,7 @@ struct BridgedDeclRef {
10981098
};
10991099

11001100
struct BridgedVTableEntry {
1101-
uint64_t storage[5];
1101+
uint64_t storage[6];
11021102

11031103
enum class Kind {
11041104
Normal,
@@ -1146,7 +1146,7 @@ struct OptionalBridgedVTable {
11461146
};
11471147

11481148
struct BridgedWitnessTableEntry {
1149-
uint64_t storage[5];
1149+
uint64_t storage[6];
11501150

11511151
enum class Kind {
11521152
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;
@@ -204,6 +208,9 @@ struct SILDeclRef {
204208
const GenericSignatureImpl *, CustomAttr *>
205209
pointer;
206210

211+
// Type of closure thunk.
212+
const clang::Type *thunkType = nullptr;
213+
207214
/// Returns the type of AST node location being stored by the SILDeclRef.
208215
LocKind getLocKind() const {
209216
if (loc.is<ValueDecl *>())
@@ -257,11 +264,10 @@ struct SILDeclRef {
257264
/// for the containing ClassDecl.
258265
/// - If 'loc' is a global VarDecl, this returns its GlobalAccessor
259266
/// SILDeclRef.
260-
explicit SILDeclRef(
261-
Loc loc,
262-
bool isForeign = false,
263-
bool isDistributed = false,
264-
bool isDistributedLocal = false);
267+
explicit SILDeclRef(Loc loc, bool isForeign = false,
268+
bool isDistributed = false,
269+
bool isDistributedLocal = false,
270+
const clang::Type *thunkType = nullptr);
265271

266272
/// See above put produces a prespecialization according to the signature.
267273
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
@@ -3965,12 +3965,10 @@ static CanSILFunctionType getUncachedSILFunctionTypeForConstant(
39653965
// The type of the native-to-foreign thunk for a swift closure.
39663966
if (constant.isForeign && constant.hasClosureExpr() &&
39673967
shouldStoreClangType(TC.getDeclRefRepresentation(constant))) {
3968-
auto clangType = TC.Context.getClangFunctionType(
3969-
origLoweredInterfaceType->getParams(),
3970-
origLoweredInterfaceType->getResult(),
3971-
FunctionTypeRepresentation::CFunctionPointer);
3972-
AbstractionPattern pattern =
3973-
AbstractionPattern(origLoweredInterfaceType, clangType);
3968+
assert(!extInfoBuilder.getClangTypeInfo().empty() &&
3969+
"clang type not found");
3970+
AbstractionPattern pattern = AbstractionPattern(
3971+
origLoweredInterfaceType, extInfoBuilder.getClangTypeInfo().getType());
39743972
return getSILFunctionTypeForAbstractCFunction(
39753973
TC, pattern, origLoweredInterfaceType, extInfoBuilder, constant);
39763974
}
@@ -4476,9 +4474,13 @@ getAbstractionPatternForConstant(ASTContext &ctx, SILDeclRef constant,
44764474
if (!constant.isForeign)
44774475
return AbstractionPattern(fnType);
44784476

4477+
if (constant.thunkType)
4478+
return AbstractionPattern(fnType, constant.thunkType);
4479+
44794480
auto bridgedFn = getBridgedFunction(constant);
44804481
if (!bridgedFn)
44814482
return AbstractionPattern(fnType);
4483+
44824484
const clang::Decl *clangDecl = bridgedFn->getClangDecl();
44834485
if (!clangDecl)
44844486
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

‎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+
}

‎test/Interop/Cxx/stdlib/use-std-function.swift

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,12 +64,11 @@ StdFunctionTestSuite.test("FunctionStringToString init from closure and pass as
6464
expectEqual(std.string("prefixabcabc"), res)
6565
}
6666

67-
// FIXME: assertion for address-only closure params (rdar://124501345)
68-
//StdFunctionTestSuite.test("FunctionStringToStringConstRef init from closure and pass as parameter") {
69-
// let res = invokeFunctionTwiceConstRef(.init({ $0 + std.string("abc") }),
70-
// std.string("prefix"))
71-
// expectEqual(std.string("prefixabcabc"), res)
72-
//}
67+
StdFunctionTestSuite.test("FunctionStringToStringConstRef init from closure and pass as parameter") {
68+
let res = invokeFunctionTwiceConstRef(.init({ $0 + std.string("abc") }),
69+
std.string("prefix"))
70+
expectEqual(std.string("prefixabcabc"), res)
71+
}
7372
#endif
7473

7574
runAllTests()

0 commit comments

Comments
 (0)