Skip to content

Commit 41104a5

Browse files
author
Gabor Horvath
committed
[SILGen] Fix the type of closure thunks that are passed const reference structs
This PR is another attempt at landing #76903. The changes compared to the original PR: * Instead of increasing the size of SILDeclRef, store the necessary type information in the conversion AST nodes. * The PR above introduced a crash during indexing system modules that references foreign types coming from modules imported as implementation only. These entities are implementation details so they do not need to be included during serialization. This PR adds a test and adds logic to exclude such clang types in the serialization process. rdar://131321096&141786724
1 parent aa16ff5 commit 41104a5

File tree

14 files changed

+328
-41
lines changed

14 files changed

+328
-41
lines changed

‎include/swift/AST/Expr.h

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
#include "swift/AST/FunctionRefInfo.h"
3030
#include "swift/AST/ProtocolConformanceRef.h"
3131
#include "swift/AST/ThrownErrorDestination.h"
32+
#include "swift/AST/Type.h"
3233
#include "swift/AST/TypeAlignments.h"
3334
#include "swift/Basic/Debug.h"
3435
#include "swift/Basic/InlineBitfield.h"
@@ -3367,9 +3368,8 @@ class UnresolvedTypeConversionExpr : public ImplicitConversionExpr {
33673368
/// FIXME: This should be a CapturingExpr.
33683369
class FunctionConversionExpr : public ImplicitConversionExpr {
33693370
public:
3370-
FunctionConversionExpr(Expr *subExpr, Type type)
3371-
: ImplicitConversionExpr(ExprKind::FunctionConversion, subExpr, type) {}
3372-
3371+
FunctionConversionExpr(Expr *subExpr, Type type);
3372+
33733373
static bool classof(const Expr *E) {
33743374
return E->getKind() == ExprKind::FunctionConversion;
33753375
}
@@ -4290,6 +4290,12 @@ class ClosureExpr : public AbstractClosureExpr {
42904290
/// The body of the closure.
42914291
BraceStmt *Body;
42924292

4293+
/// Used when lowering ClosureExprs to C function pointers.
4294+
/// This is required to access the ClangType from SILDeclRef.
4295+
/// TODO: this will be redundant after we preserve ClangTypes
4296+
/// in the canonical types.
4297+
FunctionConversionExpr *ConvertedTo;
4298+
42934299
friend class GlobalActorAttributeRequest;
42944300

42954301
bool hasNoGlobalActorAttribute() const {
@@ -4301,19 +4307,19 @@ class ClosureExpr : public AbstractClosureExpr {
43014307
}
43024308

43034309
public:
4304-
ClosureExpr(const DeclAttributes &attributes,
4305-
SourceRange bracketRange, VarDecl *capturedSelfDecl,
4306-
ParameterList *params, SourceLoc asyncLoc, SourceLoc throwsLoc,
4307-
TypeExpr *thrownType, SourceLoc arrowLoc, SourceLoc inLoc,
4308-
TypeExpr *explicitResultType, DeclContext *parent)
4309-
: AbstractClosureExpr(ExprKind::Closure, Type(), /*Implicit=*/false,
4310-
parent),
4311-
Attributes(attributes), BracketRange(bracketRange),
4312-
CapturedSelfDecl(capturedSelfDecl),
4313-
AsyncLoc(asyncLoc), ThrowsLoc(throwsLoc), ArrowLoc(arrowLoc),
4314-
InLoc(inLoc), ThrownType(thrownType),
4315-
ExplicitResultTypeAndBodyState(explicitResultType, BodyState::Parsed),
4316-
Body(nullptr) {
4310+
ClosureExpr(const DeclAttributes &attributes, SourceRange bracketRange,
4311+
VarDecl *capturedSelfDecl, ParameterList *params,
4312+
SourceLoc asyncLoc, SourceLoc throwsLoc, TypeExpr *thrownType,
4313+
SourceLoc arrowLoc, SourceLoc inLoc, TypeExpr *explicitResultType,
4314+
DeclContext *parent)
4315+
: AbstractClosureExpr(ExprKind::Closure, Type(), /*Implicit=*/false,
4316+
parent),
4317+
Attributes(attributes), BracketRange(bracketRange),
4318+
CapturedSelfDecl(capturedSelfDecl), AsyncLoc(asyncLoc),
4319+
ThrowsLoc(throwsLoc), ArrowLoc(arrowLoc), InLoc(inLoc),
4320+
ThrownType(thrownType),
4321+
ExplicitResultTypeAndBodyState(explicitResultType, BodyState::Parsed),
4322+
Body(nullptr), ConvertedTo(nullptr) {
43174323
setParameterList(params);
43184324
Bits.ClosureExpr.HasAnonymousClosureVars = false;
43194325
Bits.ClosureExpr.ImplicitSelfCapture = false;
@@ -4528,6 +4534,9 @@ class ClosureExpr : public AbstractClosureExpr {
45284534
ExplicitResultTypeAndBodyState.setInt(v);
45294535
}
45304536

4537+
const FunctionConversionExpr *getConvertedTo() const { return ConvertedTo; }
4538+
void setConvertedTo(FunctionConversionExpr *e) { ConvertedTo = e; }
4539+
45314540
static bool classof(const Expr *E) {
45324541
return E->getKind() == ExprKind::Closure;
45334542
}

‎lib/AST/Expr.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1456,6 +1456,16 @@ DestructureTupleExpr::create(ASTContext &ctx,
14561456
srcExpr, dstExpr, ty);
14571457
}
14581458

1459+
FunctionConversionExpr::FunctionConversionExpr(Expr *subExpr, Type type)
1460+
: ImplicitConversionExpr(ExprKind::FunctionConversion, subExpr, type) {
1461+
while (auto *PE = dyn_cast<ParenExpr>(subExpr))
1462+
subExpr = PE->getSubExpr();
1463+
if (auto *CLE = dyn_cast<CaptureListExpr>(subExpr))
1464+
subExpr = CLE->getClosureBody();
1465+
if (auto *CE = dyn_cast<ClosureExpr>(subExpr))
1466+
CE->setConvertedTo(this);
1467+
}
1468+
14591469
SourceRange TupleExpr::getSourceRange() const {
14601470
auto start = LParenLoc;
14611471
if (start.isInvalid()) {

‎lib/SIL/IR/SILFunctionType.cpp

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
//
1717
//===----------------------------------------------------------------------===//
1818

19+
#include "swift/AST/Expr.h"
20+
#include "swift/AST/Type.h"
1921
#define DEBUG_TYPE "libsil"
2022

2123
#include "swift/AST/AnyFunctionRef.h"
@@ -4321,12 +4323,10 @@ static CanSILFunctionType getUncachedSILFunctionTypeForConstant(
43214323
// The type of the native-to-foreign thunk for a swift closure.
43224324
if (constant.isForeign && constant.hasClosureExpr() &&
43234325
shouldStoreClangType(TC.getDeclRefRepresentation(constant))) {
4324-
auto clangType = TC.Context.getClangFunctionType(
4325-
origLoweredInterfaceType->getParams(),
4326-
origLoweredInterfaceType->getResult(),
4327-
FunctionTypeRepresentation::CFunctionPointer);
4328-
AbstractionPattern pattern =
4329-
AbstractionPattern(origLoweredInterfaceType, clangType);
4326+
assert(!extInfoBuilder.getClangTypeInfo().empty() &&
4327+
"clang type not found");
4328+
AbstractionPattern pattern = AbstractionPattern(
4329+
origLoweredInterfaceType, extInfoBuilder.getClangTypeInfo().getType());
43304330
return getSILFunctionTypeForAbstractCFunction(
43314331
TC, pattern, origLoweredInterfaceType, extInfoBuilder, constant);
43324332
}
@@ -4834,9 +4834,21 @@ getAbstractionPatternForConstant(ASTContext &ctx, SILDeclRef constant,
48344834
if (!constant.isForeign)
48354835
return AbstractionPattern(fnType);
48364836

4837+
if (const auto *closure = constant.getClosureExpr()) {
4838+
if (const auto *convertedTo = closure->getConvertedTo()) {
4839+
auto clangInfo = convertedTo->getType()
4840+
->castTo<AnyFunctionType>()
4841+
->getExtInfo()
4842+
.getClangTypeInfo();
4843+
if (!clangInfo.empty())
4844+
return AbstractionPattern(fnType, clangInfo.getType());
4845+
}
4846+
}
4847+
48374848
auto bridgedFn = getBridgedFunction(constant);
48384849
if (!bridgedFn)
48394850
return AbstractionPattern(fnType);
4851+
48404852
const clang::Decl *clangDecl = bridgedFn->getClangDecl();
48414853
if (!clangDecl)
48424854
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;
@@ -1453,6 +1454,11 @@ emitObjCThunkArguments(SILGenFunction &SGF, SILLocation loc, SILDeclRef thunk,
14531454
auto buf = SGF.emitTemporaryAllocation(loc, native.getType());
14541455
native.forwardInto(SGF, loc, buf);
14551456
native = SGF.emitManagedBufferWithCleanup(buf);
1457+
} else if (!fnConv.isSILIndirect(nativeInputs[i]) &&
1458+
native.getType().isAddress()) {
1459+
// Load the value if the argument has an address type and the native
1460+
// function expects the argument to be passed directly.
1461+
native = SGF.emitManagedLoadCopy(loc, native.getValue());
14561462
}
14571463

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

‎lib/SILGen/SILGenExpr.cpp

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1737,7 +1737,19 @@ static ManagedValue convertCFunctionSignature(SILGenFunction &SGF,
17371737
FunctionConversionExpr *e,
17381738
SILType loweredResultTy,
17391739
llvm::function_ref<ManagedValue ()> fnEmitter) {
1740-
SILType loweredDestTy = SGF.getLoweredType(e->getType());
1740+
SILType loweredDestTy;
1741+
auto destTy = e->getType();
1742+
auto clangInfo =
1743+
destTy->castTo<AnyFunctionType>()->getExtInfo().getClangTypeInfo();
1744+
if (clangInfo.empty())
1745+
loweredDestTy = SGF.getLoweredType(destTy);
1746+
else
1747+
// This won't be necessary after we stop dropping clang types when
1748+
// canonicalizing function types.
1749+
loweredDestTy = SGF.getLoweredType(
1750+
AbstractionPattern(destTy->getCanonicalType(), clangInfo.getType()),
1751+
destTy);
1752+
17411753
ManagedValue result;
17421754

17431755
// We're converting between C function pointer types. They better be
@@ -1808,7 +1820,7 @@ ManagedValue emitCFunctionPointer(SILGenFunction &SGF,
18081820
#endif
18091821
semanticExpr = conv->getSubExpr()->getSemanticsProvidingExpr();
18101822
}
1811-
1823+
18121824
if (auto declRef = dyn_cast<DeclRefExpr>(semanticExpr)) {
18131825
setLocFromConcreteDeclRef(declRef->getDeclRef());
18141826
} else if (auto memberRef = dyn_cast<MemberRefExpr>(semanticExpr)) {

‎lib/Serialization/Deserialization.cpp

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

85098509
llvm::Expected<const clang::Type *>
85108510
ModuleFile::getClangType(ClangTypeID TID) {
8511-
if (!getContext().LangOpts.UseClangFunctionTypes)
8512-
return nullptr;
8513-
85148511
if (TID == 0)
85158512
return nullptr;
85168513

‎lib/Serialization/Serialization.cpp

Lines changed: 51 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
#include "swift/AST/ASTMangler.h"
1818
#include "swift/AST/ASTVisitor.h"
1919
#include "swift/AST/AutoDiff.h"
20+
#include "swift/AST/Decl.h"
2021
#include "swift/AST/DiagnosticsCommon.h"
2122
#include "swift/AST/DiagnosticsSema.h"
2223
#include "swift/AST/Expr.h"
@@ -41,6 +42,7 @@
4142
#include "swift/AST/SynthesizedFileUnit.h"
4243
#include "swift/AST/TypeCheckRequests.h"
4344
#include "swift/AST/TypeVisitor.h"
45+
#include "swift/AST/Types.h"
4446
#include "swift/Basic/Assertions.h"
4547
#include "swift/Basic/Defer.h"
4648
#include "swift/Basic/FileSystem.h"
@@ -5553,6 +5555,31 @@ static TypeAliasDecl *findTypeAliasForBuiltin(ASTContext &Ctx, Type T) {
55535555
return cast<TypeAliasDecl>(CurModuleResults[0]);
55545556
}
55555557

5558+
namespace {
5559+
struct ImplementationOnlyWalker : TypeWalker {
5560+
bool hadImplementationOnlyDecl = false;
5561+
const ModuleDecl *currentModule;
5562+
ImplementationOnlyWalker(const ModuleDecl *M) : currentModule(M) {}
5563+
Action walkToTypePre(Type ty) override {
5564+
if (auto *typeAlias = dyn_cast<TypeAliasType>(ty)) {
5565+
if (importedImplementationOnly(typeAlias->getDecl()))
5566+
return Action::Stop;
5567+
} else if (auto *nominal = ty->getAs<NominalType>()) {
5568+
if (importedImplementationOnly(nominal->getDecl()))
5569+
return Action::Stop;
5570+
}
5571+
return Action::Continue;
5572+
}
5573+
bool importedImplementationOnly(const Decl *D) {
5574+
if (currentModule->isImportedImplementationOnly(D->getModuleContext())) {
5575+
hadImplementationOnlyDecl = true;
5576+
return true;
5577+
}
5578+
return false;
5579+
}
5580+
};
5581+
} // namespace
5582+
55565583
class Serializer::TypeSerializer : public TypeVisitor<TypeSerializer> {
55575584
Serializer &S;
55585585

@@ -5906,10 +5933,23 @@ class Serializer::TypeSerializer : public TypeVisitor<TypeSerializer> {
59065933
using namespace decls_block;
59075934

59085935
auto resultType = S.addTypeRef(fnTy->getResult());
5909-
auto clangType =
5910-
S.getASTContext().LangOpts.UseClangFunctionTypes
5911-
? S.addClangTypeRef(fnTy->getClangTypeInfo().getType())
5912-
: ClangTypeID(0);
5936+
bool shouldSerializeClangType = true;
5937+
if (S.hadImplementationOnlyImport && S.M &&
5938+
S.M->getResilienceStrategy() != ResilienceStrategy::Resilient) {
5939+
// Deserializing clang types from implementation only modules could crash
5940+
// as the transitive clang module might not be available to retrieve the
5941+
// declarations from. In an optimal world we would make the deseriaization
5942+
// more resilient to these problems but the failure is in Clang's
5943+
// deserialization code path that is not architected with potentially
5944+
// missing declarations in mind.
5945+
ImplementationOnlyWalker walker{S.M};
5946+
Type(const_cast<FunctionType *>(fnTy)).walk(walker);
5947+
if (walker.hadImplementationOnlyDecl)
5948+
shouldSerializeClangType = false;
5949+
}
5950+
auto clangType = shouldSerializeClangType
5951+
? S.addClangTypeRef(fnTy->getClangTypeInfo().getType())
5952+
: ClangTypeID(0);
59135953

59145954
auto isolation = encodeIsolation(fnTy->getIsolation());
59155955

@@ -6991,8 +7031,13 @@ void Serializer::writeAST(ModuleOrSourceFile DC) {
69917031
nextFile->getTopLevelDeclsWithAuxiliaryDecls(fileDecls);
69927032

69937033
for (auto D : fileDecls) {
6994-
if (isa<ImportDecl>(D) || isa<MacroExpansionDecl>(D) ||
6995-
isa<TopLevelCodeDecl>(D) || isa<UsingDecl>(D)) {
7034+
if (const auto *ID = dyn_cast<ImportDecl>(D)) {
7035+
if (ID->getAttrs().hasAttribute<ImplementationOnlyAttr>())
7036+
hadImplementationOnlyImport = true;
7037+
continue;
7038+
}
7039+
if (isa<MacroExpansionDecl>(D) || isa<TopLevelCodeDecl>(D) ||
7040+
isa<UsingDecl>(D)) {
69967041
continue;
69977042
}
69987043

‎lib/Serialization/Serialization.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,8 @@ class Serializer : public SerializerBase {
116116
/// an error in the AST.
117117
bool hadError = false;
118118

119+
bool hadImplementationOnlyImport = false;
120+
119121
/// Helper for serializing entities in the AST block object graph.
120122
///
121123
/// Keeps track of assigning IDs to newly-seen entities, and collecting

‎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
}
@@ -75,4 +79,13 @@ inline void releaseSharedRef(SharedRef *_Nonnull x) {
7579
}
7680
}
7781

82+
void cfuncConstRefNonTrivial(void (*_Nonnull)(const NonTrivial &));
83+
void cfuncConstRefTrivial(void (*_Nonnull)(const Trivial &));
84+
void blockConstRefNonTrivial(void (^_Nonnull)(const NonTrivial &));
85+
void blockConstRefTrivial(void (^_Nonnull)(const Trivial &));
86+
#if __OBJC__
87+
void cfuncConstRefStrong(void (*_Nonnull)(const ARCStrong &));
88+
void blockConstRefStrong(void (^_Nonnull)(const ARCStrong &));
89+
#endif
90+
7891
#endif // __CLOSURE__

0 commit comments

Comments
 (0)