Skip to content

[SILGen] Fix the type of closure thunks that are passed const reference structs #75491

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions include/swift/SIL/SILBridging.h
Original file line number Diff line number Diff line change
Expand Up @@ -1081,7 +1081,7 @@ struct BridgedSuccessorArray {
};

struct BridgedDeclRef {
uint64_t storage[3];
uint64_t storage[4];

#ifdef USED_IN_CPP_SOURCE
BridgedDeclRef(swift::SILDeclRef declRef) {
Expand All @@ -1098,7 +1098,7 @@ struct BridgedDeclRef {
};

struct BridgedVTableEntry {
uint64_t storage[5];
uint64_t storage[6];

enum class Kind {
Normal,
Expand Down Expand Up @@ -1146,7 +1146,7 @@ struct OptionalBridgedVTable {
};

struct BridgedWitnessTableEntry {
uint64_t storage[5];
uint64_t storage[6];

enum class Kind {
invalid,
Expand Down
16 changes: 11 additions & 5 deletions include/swift/SIL/SILDeclRef.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ namespace llvm {
class raw_ostream;
}

namespace clang {
class Type;
}

namespace swift {
enum class EffectsKind : uint8_t;
class AbstractFunctionDecl;
Expand Down Expand Up @@ -204,6 +208,9 @@ struct SILDeclRef {
const GenericSignatureImpl *, CustomAttr *>
pointer;

// Type of closure thunk.
const clang::Type *thunkType = nullptr;

/// Returns the type of AST node location being stored by the SILDeclRef.
LocKind getLocKind() const {
if (loc.is<ValueDecl *>())
Expand Down Expand Up @@ -257,11 +264,10 @@ struct SILDeclRef {
/// for the containing ClassDecl.
/// - If 'loc' is a global VarDecl, this returns its GlobalAccessor
/// SILDeclRef.
explicit SILDeclRef(
Loc loc,
bool isForeign = false,
bool isDistributed = false,
bool isDistributedLocal = false);
explicit SILDeclRef(Loc loc, bool isForeign = false,
bool isDistributed = false,
bool isDistributedLocal = false,
const clang::Type *thunkType = nullptr);

/// See above put produces a prespecialization according to the signature.
explicit SILDeclRef(Loc loc, GenericSignature prespecializationSig);
Expand Down
10 changes: 8 additions & 2 deletions lib/SIL/IR/SILDeclRef.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -135,11 +135,15 @@ SILDeclRef::SILDeclRef(ValueDecl *vd, SILDeclRef::Kind kind, bool isForeign,
isAsyncLetClosure(0), pointer(derivativeId) {}

SILDeclRef::SILDeclRef(SILDeclRef::Loc baseLoc, bool asForeign,
bool asDistributed, bool asDistributedKnownToBeLocal)
bool asDistributed, bool asDistributedKnownToBeLocal,
const clang::Type *thunkType)
: isRuntimeAccessible(false),
backDeploymentKind(SILDeclRef::BackDeploymentKind::None),
defaultArgIndex(0), isAsyncLetClosure(0),
pointer((AutoDiffDerivativeFunctionIdentifier *)nullptr) {
pointer((AutoDiffDerivativeFunctionIdentifier *)nullptr),
thunkType(thunkType) {
assert((!thunkType || baseLoc.is<AbstractClosureExpr *>()) &&
"thunk type is needed only for closures");
if (auto *vd = baseLoc.dyn_cast<ValueDecl*>()) {
if (auto *fd = dyn_cast<FuncDecl>(vd)) {
// Map FuncDecls directly to Func SILDeclRefs.
Expand Down Expand Up @@ -169,6 +173,8 @@ SILDeclRef::SILDeclRef(SILDeclRef::Loc baseLoc, bool asForeign,
llvm_unreachable("invalid loc decl for SILDeclRef!");
}
} else if (auto *ACE = baseLoc.dyn_cast<AbstractClosureExpr *>()) {
assert((!asForeign || thunkType) &&
"thunk type needed for foreign type for closures");
loc = ACE;
kind = Kind::Func;
if (ACE->getASTContext().LangOpts.hasFeature(
Expand Down
14 changes: 8 additions & 6 deletions lib/SIL/IR/SILFunctionType.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3965,12 +3965,10 @@ static CanSILFunctionType getUncachedSILFunctionTypeForConstant(
// The type of the native-to-foreign thunk for a swift closure.
if (constant.isForeign && constant.hasClosureExpr() &&
shouldStoreClangType(TC.getDeclRefRepresentation(constant))) {
auto clangType = TC.Context.getClangFunctionType(
origLoweredInterfaceType->getParams(),
origLoweredInterfaceType->getResult(),
FunctionTypeRepresentation::CFunctionPointer);
AbstractionPattern pattern =
AbstractionPattern(origLoweredInterfaceType, clangType);
assert(!extInfoBuilder.getClangTypeInfo().empty() &&
"clang type not found");
AbstractionPattern pattern = AbstractionPattern(
origLoweredInterfaceType, extInfoBuilder.getClangTypeInfo().getType());
return getSILFunctionTypeForAbstractCFunction(
TC, pattern, origLoweredInterfaceType, extInfoBuilder, constant);
}
Expand Down Expand Up @@ -4476,9 +4474,13 @@ getAbstractionPatternForConstant(ASTContext &ctx, SILDeclRef constant,
if (!constant.isForeign)
return AbstractionPattern(fnType);

if (constant.thunkType)
return AbstractionPattern(fnType, constant.thunkType);

auto bridgedFn = getBridgedFunction(constant);
if (!bridgedFn)
return AbstractionPattern(fnType);

const clang::Decl *clangDecl = bridgedFn->getClangDecl();
if (!clangDecl)
return AbstractionPattern(fnType);
Expand Down
10 changes: 8 additions & 2 deletions lib/SILGen/SILGenBridging.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1315,8 +1315,9 @@ static SILValue emitObjCUnconsumedArgument(SILGenFunction &SGF,
SILLocation loc,
SILValue arg) {
auto &lowering = SGF.getTypeLowering(arg->getType());
// If address-only, make a +1 copy and operate on that.
if (lowering.isAddressOnly() && SGF.useLoweredAddresses()) {
// If arg is non-trivial and has an address type, make a +1 copy and operate
// on that.
if (!lowering.isTrivial() && arg->getType().isAddress()) {
auto tmp = SGF.emitTemporaryAllocation(loc, arg->getType().getObjectType());
SGF.B.createCopyAddr(loc, arg, tmp, IsNotTake, IsInitialization);
return tmp;
Expand Down Expand Up @@ -1448,6 +1449,11 @@ emitObjCThunkArguments(SILGenFunction &SGF, SILLocation loc, SILDeclRef thunk,
auto buf = SGF.emitTemporaryAllocation(loc, native.getType());
native.forwardInto(SGF, loc, buf);
native = SGF.emitManagedBufferWithCleanup(buf);
} else if (!fnConv.isSILIndirect(nativeInputs[i]) &&
native.getType().isAddress()) {
// Load the value if the argument has an address type and the native
// function expects the argument to be passed directly.
native = SGF.emitManagedLoadCopy(loc, native.getValue());
}

if (nativeInputs[i].isConsumedInCaller()) {
Expand Down
26 changes: 23 additions & 3 deletions lib/SILGen/SILGenExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1723,7 +1723,19 @@ static ManagedValue convertCFunctionSignature(SILGenFunction &SGF,
FunctionConversionExpr *e,
SILType loweredResultTy,
llvm::function_ref<ManagedValue ()> fnEmitter) {
SILType loweredDestTy = SGF.getLoweredType(e->getType());
SILType loweredDestTy;
auto destTy = e->getType();
auto clangInfo =
destTy->castTo<AnyFunctionType>()->getExtInfo().getClangTypeInfo();
if (clangInfo.empty())
loweredDestTy = SGF.getLoweredType(destTy);
else
// This won't be necessary after we stop dropping clang types when
// canonicalizing function types.
loweredDestTy = SGF.getLoweredType(
AbstractionPattern(destTy->getCanonicalType(), clangInfo.getType()),
destTy);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be necessary. If we're lowering a foreign function type, we should be honoring the Clang type stored in it automatically without requiring that we pass down separately as an abstraction pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm passing the type in the abstraction pattern because the stored clang type gets dropped in TypeConverter::getTypeLowering if I just pass the type without the abstraction pattern to getLoweredType.

  const TypeLowering &
  getTypeLowering(Type t, TypeExpansionContext forExpansion) {
    AbstractionPattern pattern(t->getCanonicalType());
    return getTypeLowering(pattern, t, forExpansion);
  }

The call to t->getCanonicalType() drops the clang type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, t still has the clang type, but it's dropped somewhere else later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to understand that. I don't think there's any good reason for us to drop clang types, especially during type lowering.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting LangOptions::UseClangFunctionTypes or passing -use-clang-function-types prevents clang types from getting dropped, but it also causes other regression tests to crash/fail.

It looks like it hasn't been turned on by default because of unresolved issues.

    /// Use Clang function types for computing canonical types.
    /// If this option is false, the clang function types will still be computed
    /// but will not be used for checking type equality.
    /// [TODO: Clang-type-plumbing] Turn on for feature rollout.
    bool UseClangFunctionTypes = false;

Copy link
Contributor Author

@ahatanaka ahatanaka Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the SILDeclRef that's declared later in the function?

// Produce a reference to the C-compatible entry point for the function.
SILDeclRef constant(loc, /*foreign*/ true);

loc passed to the constructor is an AbstractClosureExpr, whose type doesn't have a clang type. The type of parameter FunctionConversionExpr *e passed to this function has the clang type.

Copy link
Contributor

@rjmccall rjmccall Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think this shouldn't be necessary — lowering a @convention(c) function type that has a Clang type attached should definitely be producing a type that uses the conventions of that Clang type. Can you identify what exactly drops this? It's one thing if it's just not being propagated into the SILFunctionType, but it seems bizarre that we'd not use this information when it's available in type lowering.

Maybe the query about whether we have a Clang type on an AbstractionPattern just needs to consider the possibility that we have a @convention(c) function type that's carrying such a type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clang type is dropped in TypeConverter::getTypeLowering.

  getTypeLowering(Type t, TypeExpansionContext forExpansion) {
    AbstractionPattern pattern(t->getCanonicalType());
    return getTypeLowering(pattern, t, forExpansion);
  }

The call to t->getCanonicalType() drops the clang type because UseClangFunctionTypes isn't set.

I experimented with creating an AbstractionPattern with a clang type in that function if it exists and it seemed to work although I didn't test it extensively.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the type t passed to the overload of getTypeLowering loses its clang type when getCanonicalType is called again.

const TypeLowering &
TypeConverter::getTypeLowering(AbstractionPattern origType,
                               Type origSubstType,
                               TypeExpansionContext forExpansion) {
  CanType substType = origSubstType->getCanonicalType();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, okay. You'd said up-thread that it wasn't dropped by canonicalization, but if it is, I agree that we can't do much here. Please at least add a comment explaining that this won't be necessary when we switch to Clang types, because the Clang function type should survive canonicalization.


ManagedValue result;

// We're converting between C function pointer types. They better be
Expand Down Expand Up @@ -1794,7 +1806,9 @@ ManagedValue emitCFunctionPointer(SILGenFunction &SGF,
#endif
semanticExpr = conv->getSubExpr()->getSemanticsProvidingExpr();
}


const clang::Type *destFnType = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ought to be named something that makes it clear that this is a Clang type.


if (auto declRef = dyn_cast<DeclRefExpr>(semanticExpr)) {
setLocFromConcreteDeclRef(declRef->getDeclRef());
} else if (auto memberRef = dyn_cast<MemberRefExpr>(semanticExpr)) {
Expand All @@ -1808,12 +1822,18 @@ ManagedValue emitCFunctionPointer(SILGenFunction &SGF,
loc = closure;
return ManagedValue();
});
auto clangInfo = conversionExpr->getType()
->castTo<FunctionType>()
->getExtInfo()
.getClangTypeInfo();
if (!clangInfo.empty())
destFnType = clangInfo.getType();
} else {
llvm_unreachable("c function pointer converted from a non-concrete decl ref");
}

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

Expand Down
13 changes: 13 additions & 0 deletions test/Interop/Cxx/class/Inputs/closure.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@ struct NonTrivial {
int *p;
};

struct Trivial {
int i;
};

void cfunc(void (^ _Nonnull block)(NonTrivial)) noexcept {
block(NonTrivial());
}
Expand Down Expand Up @@ -45,4 +49,13 @@ void cfuncARCWeak(ARCWeak) noexcept;
void (* _Nonnull getFnPtr() noexcept)(NonTrivial) noexcept;
void (* _Nonnull getFnPtr2() noexcept)(ARCWeak) noexcept;

void cfuncConstRefNonTrivial(void (*_Nonnull)(const NonTrivial &));
void cfuncConstRefTrivial(void (*_Nonnull)(const Trivial &));
void blockConstRefNonTrivial(void (^_Nonnull)(const NonTrivial &));
void blockConstRefTrivial(void (^_Nonnull)(const Trivial &));
#if __OBJC__
void cfuncConstRefStrong(void (*_Nonnull)(const ARCStrong &));
void blockConstRefStrong(void (^_Nonnull)(const ARCStrong &));
#endif

#endif // __CLOSURE__
85 changes: 85 additions & 0 deletions test/Interop/Cxx/class/closure-thunk-macosx.swift
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,88 @@ public func testClosureToFuncPtr() {
public func testClosureToBlockReturnNonTrivial() {
cfuncReturnNonTrivial({() -> NonTrivial in return NonTrivial() })
}

// CHECK-LABEL: sil private [thunk] [ossa] @$s4main22testConstRefNonTrivialyyFySo0eF0VcfU_To : $@convention(c) (@in_guaranteed NonTrivial) -> () {
// CHECK: bb0(%[[V0:.*]] : $*NonTrivial):
// CHECK: %[[V1:.*]] = alloc_stack $NonTrivial
// CHECK: copy_addr %[[V0]] to [init] %[[V1]] : $*NonTrivial
// CHECK: %[[V3:.*]] = function_ref @$s4main22testConstRefNonTrivialyyFySo0eF0VcfU_ : $@convention(thin) (@in_guaranteed NonTrivial) -> ()
// CHECK: %[[V4:.*]] = apply %[[V3]](%[[V1]]) : $@convention(thin) (@in_guaranteed NonTrivial) -> ()
// CHECK: destroy_addr %[[V1]] : $*NonTrivial
// CHECK: dealloc_stack %[[V1]] : $*NonTrivial
// CHECK: return %[[V4]] : $()

public func testConstRefNonTrivial() {
cfuncConstRefNonTrivial({S in });
}

// CHECK-LABEL: sil private [thunk] [ossa] @$s4main19testConstRefTrivialyyFySo0E0VcfU_To : $@convention(c) (@in_guaranteed Trivial) -> () {
// CHECK: bb0(%[[V0:.*]] : $*Trivial):
// CHECK: %[[V1:.*]] = load [trivial] %[[V0]] : $*Trivial
// CHECK: %[[V2:.*]] = function_ref @$s4main19testConstRefTrivialyyFySo0E0VcfU_ : $@convention(thin) (Trivial) -> ()
// CHECK: %[[V3:.*]] = apply %[[V2]](%[[V1]]) : $@convention(thin) (Trivial) -> ()
// CHECK: return %[[V3]] : $()

public func testConstRefTrivial() {
cfuncConstRefTrivial({S in });
}

// CHECK-LABEL: sil private [thunk] [ossa] @$s4main18testConstRefStrongyyFySo9ARCStrongVcfU_To : $@convention(c) (@in_guaranteed ARCStrong) -> () {
// CHECK: bb0(%[[V0:.*]] : $*ARCStrong):
// CHECK: %[[V1:.*]] = alloc_stack $ARCStrong
// CHECK: copy_addr %[[V0]] to [init] %[[V1]] : $*ARCStrong
// CHECK: %[[V3:.*]] = load [copy] %[[V1]] : $*ARCStrong
// CHECK: %[[V4:.*]] = begin_borrow %[[V3]] : $ARCStrong
// CHECK: %[[V5:.*]] = function_ref @$s4main18testConstRefStrongyyFySo9ARCStrongVcfU_ : $@convention(thin) (@guaranteed ARCStrong) -> ()
// CHECK: %[[V6:.*]] = apply %[[V5]](%[[V4]]) : $@convention(thin) (@guaranteed ARCStrong) -> ()
// CHECK: end_borrow %[[V4]] : $ARCStrong
// CHECK: destroy_value %[[V3]] : $ARCStrong
// CHECK: destroy_addr %[[V1]] : $*ARCStrong
// CHECK: dealloc_stack %[[V1]] : $*ARCStrong
// CHECK: return %[[V6]] : $()

public func testConstRefStrong() {
cfuncConstRefStrong({S in });
}

// 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) -> () {
// CHECK: bb0(%[[V0:.*]] : $*@block_storage @callee_guaranteed (@in_guaranteed NonTrivial) -> (), %[[V1:.*]] : $*NonTrivial):
// CHECK: %[[V2:.*]] = project_block_storage %[[V0]] : $*@block_storage @callee_guaranteed (@in_guaranteed NonTrivial) -> ()
// CHECK: %[[V3:.*]] = load [copy] %[[V2]] : $*@callee_guaranteed (@in_guaranteed NonTrivial) -> ()
// CHECK: %[[V4:.*]] = begin_borrow %[[V3]] : $@callee_guaranteed (@in_guaranteed NonTrivial) -> ()
// CHECK: apply %[[V4]](%[[V1]]) : $@callee_guaranteed (@in_guaranteed NonTrivial) -> ()
// CHECK: end_borrow %[[V4]] : $@callee_guaranteed (@in_guaranteed NonTrivial) -> ()
// CHECK: destroy_value %[[V3]] : $@callee_guaranteed (@in_guaranteed NonTrivial) -> ()

public func testBlockConstRefNonTrivial() {
blockConstRefNonTrivial({S in });
}

// CHECK-LABEL: sil shared [transparent] [serialized] [reabstraction_thunk] [ossa] @$sSo7TrivialVIegy_ABIeyBn_TR : $@convention(c) (@inout_aliasable @block_storage @callee_guaranteed (Trivial) -> (), @in_guaranteed Trivial) -> () {
// CHECK: bb0(%[[V0:.*]] : $*@block_storage @callee_guaranteed (Trivial) -> (), %[[V1:.*]] : $*Trivial):
// CHECK: %[[V2:.*]] = project_block_storage %[[V0]] : $*@block_storage @callee_guaranteed (Trivial) -> ()
// CHECK: %[[V3:.*]] = load [copy] %[[V2]] : $*@callee_guaranteed (Trivial) -> ()
// CHECK: %[[V4:.*]] = load [trivial] %[[V1]] : $*Trivial
// CHECK: %[[V5:.*]] = begin_borrow %[[V3]] : $@callee_guaranteed (Trivial) -> ()
// CHECK: apply %[[V5]](%[[V4]]) : $@callee_guaranteed (Trivial) -> ()
// CHECK: end_borrow %[[V5]] : $@callee_guaranteed (Trivial) -> ()
// CHECK: destroy_value %[[V3]] : $@callee_guaranteed (Trivial) -> ()

public func testBlockConstRefTrivial() {
blockConstRefTrivial({S in });
}

// 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) -> () {
// CHECK: bb0(%[[V0:.*]] : $*@block_storage @callee_guaranteed (@guaranteed ARCStrong) -> (), %[[V1:.*]] : $*ARCStrong):
// CHECK: %[[V2:.*]] = project_block_storage %[[V0]] : $*@block_storage @callee_guaranteed (@guaranteed ARCStrong) -> ()
// CHECK: %[[V3:.*]] = load [copy] %[[V2]] : $*@callee_guaranteed (@guaranteed ARCStrong) -> ()
// CHECK: %[[V4:.*]] = load_borrow %[[V1]] : $*ARCStrong
// CHECK: %[[V5:.*]] = begin_borrow %[[V3]] : $@callee_guaranteed (@guaranteed ARCStrong) -> ()
// CHECK: apply %[[V5]](%[[V4]]) : $@callee_guaranteed (@guaranteed ARCStrong) -> ()
// CHECK: end_borrow %[[V5]] : $@callee_guaranteed (@guaranteed ARCStrong) -> ()
// CHECK: end_borrow %[[V4]] : $ARCStrong
// CHECK: destroy_value %[[V3]] : $@callee_guaranteed (@guaranteed ARCStrong) -> ()

public func testBlockConstRefStrong() {
blockConstRefStrong({S in });
}
11 changes: 5 additions & 6 deletions test/Interop/Cxx/stdlib/use-std-function.swift
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,11 @@ StdFunctionTestSuite.test("FunctionStringToString init from closure and pass as
expectEqual(std.string("prefixabcabc"), res)
}

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

runAllTests()