[SILGen] Fix the type of closure thunks that are passed const reference structs #75491
Conversation
|
@swift-ci please test |
ahatanaka
requested review from
zoecarver,
hyp,
egorzhdan and
jckarter
as code owners
| @@ -1797,7 +1807,9 @@ ManagedValue emitCFunctionPointer(SILGenFunction &SGF, | |||
| #endif | |||
| semanticExpr = conv->getSubExpr()->getSemanticsProvidingExpr(); | |||
| } | |||
|
|
|||
|
|
|||
| const clang::Type *destFnType = nullptr; | |||
There was a problem hiding this comment.
This ought to be named something that makes it clear that this is a Clang type.
| else | ||
| loweredDestTy = SGF.getLoweredType( | ||
| AbstractionPattern(destTy->getCanonicalType(), clangInfo.getType()), | ||
| destTy); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actually, t still has the clang type, but it's dropped somewhere else later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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();
There was a problem hiding this comment.
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.
|
@swift-ci please test |
|
@swift-ci please smoke test |
structs 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
The precondition checks that a clang type must be passed when creating the foreign entry point for a closure.
This is needed to fix the static assertions in the file that were failing because a new member was added to SILDeclRef.
ahatanaka
force-pushed
the
cxx-const-ref
branch
from
233dd61 to
b0e0408
Compare
|
@swift-ci please test |
|
@swift-ci Please Build Toolchain macOS Platform |
…ce 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
…ce 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
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