Skip to content

Commit c86184e

Browse files
hyphyp
committed
[interop][SwiftToCxx] consuming parameters should be passed using an additional copy
Fixes swiftlang#69372
1 parent 7b3c47f commit c86184e

12 files changed

+412
-43
lines changed

‎include/swift/IRGen/IRABIDetailsProvider.h

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,13 +96,16 @@ class LoweredFunctionSignature {
9696

9797
inline const ParamDecl &getParamDecl() const { return paramDecl; }
9898

99+
inline ParameterConvention getConvention() const { return convention; }
100+
99101
private:
100102
DirectParameter(IRABIDetailsProviderImpl &owner,
101103
const irgen::TypeInfo &typeDetails,
102-
const ParamDecl &paramDecl);
104+
const ParamDecl &paramDecl, ParameterConvention convention);
103105
IRABIDetailsProviderImpl &owner;
104106
const irgen::TypeInfo &typeDetails;
105107
const ParamDecl &paramDecl;
108+
ParameterConvention convention;
106109
friend class LoweredFunctionSignature;
107110
};
108111

@@ -111,9 +114,13 @@ class LoweredFunctionSignature {
111114
public:
112115
inline const ParamDecl &getParamDecl() const { return paramDecl; }
113116

117+
inline ParameterConvention getConvention() const { return convention; }
118+
114119
private:
115-
IndirectParameter(const ParamDecl &paramDecl);
120+
IndirectParameter(const ParamDecl &paramDecl,
121+
ParameterConvention convention);
116122
const ParamDecl &paramDecl;
123+
ParameterConvention convention;
117124
friend class LoweredFunctionSignature;
118125
};
119126

‎lib/IRGen/IRABIDetailsProvider.cpp

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -313,12 +313,13 @@ bool LoweredFunctionSignature::DirectResultType::enumerateRecordMembers(
313313

314314
LoweredFunctionSignature::DirectParameter::DirectParameter(
315315
IRABIDetailsProviderImpl &owner, const irgen::TypeInfo &typeDetails,
316-
const ParamDecl &paramDecl)
317-
: owner(owner), typeDetails(typeDetails), paramDecl(paramDecl) {}
316+
const ParamDecl &paramDecl, ParameterConvention convention)
317+
: owner(owner), typeDetails(typeDetails), paramDecl(paramDecl),
318+
convention(convention) {}
318319

319320
LoweredFunctionSignature::IndirectParameter::IndirectParameter(
320-
const ParamDecl &paramDecl)
321-
: paramDecl(paramDecl) {}
321+
const ParamDecl &paramDecl, ParameterConvention convention)
322+
: paramDecl(paramDecl), convention(convention) {}
322323

323324
bool LoweredFunctionSignature::DirectParameter::enumerateRecordMembers(
324325
llvm::function_ref<void(clang::CharUnits, clang::CharUnits, Type)> callback)
@@ -401,10 +402,11 @@ void LoweredFunctionSignature::visitParameterList(
401402
: silParamMapping[currentSilParam];
402403
++currentSilParam;
403404
if (!isIndirect) {
404-
DirectParameter param(owner, abiParam.typeInfo, *paramDecl);
405+
DirectParameter param(owner, abiParam.typeInfo, *paramDecl,
406+
abiParam.convention);
405407
directParamVisitor(param);
406408
} else {
407-
IndirectParameter param(*paramDecl);
409+
IndirectParameter param(*paramDecl, abiParam.convention);
408410
indirectParamVisitor(param);
409411
}
410412
}
@@ -432,8 +434,11 @@ void LoweredFunctionSignature::visitParameterList(
432434

433435
if (abiDetails.hasTrailingSelfParam) {
434436
assert(!abiDetails.hasContextParam);
435-
assert(FD->hasImplicitSelfDecl());
436-
indirectParamVisitor(IndirectParameter(*FD->getImplicitSelfDecl()));
437+
indirectParamVisitor(IndirectParameter(
438+
*FD->getImplicitSelfDecl(),
439+
FD->getImplicitSelfDecl()->getValueOwnership() == ValueOwnership::Owned
440+
? ParameterConvention::Direct_Owned
441+
: ParameterConvention::Direct_Guaranteed));
437442
} else if (abiDetails.hasContextParam) {
438443
contextParamVisitor(ContextParameter());
439444
}

‎lib/PrintAsClang/PrintClangFunction.cpp

Lines changed: 86 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -966,7 +966,7 @@ void DeclAndTypeClangFunctionPrinter::printTypeImplTypeSpecifier(
966966

967967
void DeclAndTypeClangFunctionPrinter::printCxxToCFunctionParameterUse(
968968
Type type, StringRef name, const ModuleDecl *moduleContext, bool isInOut,
969-
bool isIndirect, std::string directTypeEncoding, bool isSelf) {
969+
bool isIndirect, std::string directTypeEncoding, bool forceSelf) {
970970
auto namePrinter = [&]() { ClangSyntaxPrinter(os).printIdentifier(name); };
971971
if (!isKnownCxxType(type, typeMapping) &&
972972
!hasKnownOptionalNullableCxxMapping(type)) {
@@ -1007,9 +1007,9 @@ void DeclAndTypeClangFunctionPrinter::printCxxToCFunctionParameterUse(
10071007
} else {
10081008
ClangValueTypePrinter(os, cPrologueOS, interopContext)
10091009
.printParameterCxxToCUseScaffold(
1010-
moduleContext,
1011-
[&]() { printTypeImplTypeSpecifier(type, moduleContext); },
1012-
namePrinter, isSelf);
1010+
moduleContext,
1011+
[&]() { printTypeImplTypeSpecifier(type, moduleContext); },
1012+
namePrinter, forceSelf);
10131013
}
10141014
if (!directTypeEncoding.empty())
10151015
os << ')';
@@ -1153,6 +1153,82 @@ void DeclAndTypeClangFunctionPrinter::printCxxThunkBody(
11531153
break;
11541154
}
11551155
}
1156+
1157+
auto getParamName = [&](const ParamDecl &param, size_t paramIndex,
1158+
bool isConsumed) {
1159+
std::string paramName;
1160+
if (isConsumed)
1161+
paramName = "consumedParamCopy_";
1162+
if (param.isSelfParameter()) {
1163+
if (isConsumed)
1164+
paramName += "this";
1165+
else
1166+
paramName = "*this";
1167+
} else if (param.getName().empty()) {
1168+
llvm::raw_string_ostream paramOS(paramName);
1169+
if (!isConsumed)
1170+
paramOS << "_";
1171+
paramOS << paramIndex;
1172+
} else {
1173+
StringRef nameStr = param.getName().str();
1174+
if (isConsumed)
1175+
paramName += nameStr.str();
1176+
else
1177+
paramName = nameStr;
1178+
renameCxxParameterIfNeeded(FD, paramName);
1179+
}
1180+
return paramName;
1181+
};
1182+
1183+
// Check if we need to copy any parameters that are consumed by Swift,
1184+
// to ensure that Swift does not destroy the value that's owned by C++.
1185+
// FIXME: Support non-copyable types here as well between C++ -> Swift.
1186+
// FIXME: class types can be optimized down to an additional retain right
1187+
// here.
1188+
size_t paramIndex = 1;
1189+
auto emitParamCopyForConsume = [&](const ParamDecl &param) {
1190+
auto name = getParamName(param, paramIndex, /*isConsumed=*/false);
1191+
auto consumedName = getParamName(param, paramIndex, /*isConsumed=*/true);
1192+
std::string paramType;
1193+
1194+
llvm::raw_string_ostream typeOS(paramType);
1195+
1196+
CFunctionSignatureTypePrinter typePrinter(
1197+
typeOS, cPrologueOS, typeMapping, OutputLanguageMode::Cxx,
1198+
interopContext, CFunctionSignatureTypePrinterModifierDelegate(),
1199+
moduleContext, declPrinter, FunctionSignatureTypeUse::TypeReference);
1200+
auto result = typePrinter.visit(param.getInterfaceType(), OTK_None,
1201+
/*isInOutParam=*/false);
1202+
assert(!result.isUnsupported());
1203+
typeOS.flush();
1204+
1205+
os << " alignas(alignof(" << paramType << ")) char copyBuffer_"
1206+
<< consumedName << "[sizeof(" << paramType << ")];\n";
1207+
os << " auto &" << consumedName << " = *(new(copyBuffer_" << consumedName
1208+
<< ") " << paramType << "(" << name << "));\n";
1209+
os << " swift::" << cxx_synthesis::getCxxImplNamespaceName()
1210+
<< "::ConsumedValueStorageDestroyer<" << paramType << "> storageGuard_"
1211+
<< consumedName << "(" << consumedName << ");\n";
1212+
};
1213+
signature.visitParameterList(
1214+
[&](const LoweredFunctionSignature::IndirectResultValue &) {},
1215+
[&](const LoweredFunctionSignature::DirectParameter &param) {
1216+
if (isConsumedParameter(param.getConvention()))
1217+
emitParamCopyForConsume(param.getParamDecl());
1218+
++paramIndex;
1219+
},
1220+
[&](const LoweredFunctionSignature::IndirectParameter &param) {
1221+
if (isConsumedParameter(param.getConvention()))
1222+
emitParamCopyForConsume(param.getParamDecl());
1223+
++paramIndex;
1224+
},
1225+
[&](const LoweredFunctionSignature::GenericRequirementParameter
1226+
&genericRequirementParam) {},
1227+
[&](const LoweredFunctionSignature::MetadataSourceParameter
1228+
&metadataSrcParam) {},
1229+
[&](const LoweredFunctionSignature::ContextParameter &) {},
1230+
[&](const LoweredFunctionSignature::ErrorResultValue &) {});
1231+
11561232
auto printCallToCFunc = [&](llvm::Optional<StringRef> additionalParam) {
11571233
if (indirectFunctionVar)
11581234
os << "(* " << *indirectFunctionVar << ')';
@@ -1168,13 +1244,14 @@ void DeclAndTypeClangFunctionPrinter::printCxxThunkBody(
11681244
needsComma = true;
11691245
};
11701246
auto printParamUse = [&](const ParamDecl &param, bool isIndirect,
1247+
bool isConsumed,
11711248

11721249
std::string directTypeEncoding) {
11731250
emitNewParam();
1174-
std::string paramName;
11751251
if (param.isSelfParameter()) {
11761252
bool needsStaticSelf = isa<ConstructorDecl>(FD) || isStaticMethod;
11771253
if (needsStaticSelf) {
1254+
// Static self value is just the type's metadata value.
11781255
os << "swift::TypeMetadataTrait<";
11791256
CFunctionSignatureTypePrinter typePrinter(
11801257
os, cPrologueOS, typeMapping, OutputLanguageMode::Cxx,
@@ -1187,19 +1264,13 @@ void DeclAndTypeClangFunctionPrinter::printCxxThunkBody(
11871264
os << ">::getTypeMetadata()";
11881265
return;
11891266
}
1190-
paramName = "*this";
1191-
} else if (param.getName().empty()) {
1192-
llvm::raw_string_ostream paramOS(paramName);
1193-
paramOS << "_" << paramIndex;
1194-
} else {
1195-
paramName = param.getName().str().str();
1196-
renameCxxParameterIfNeeded(FD, paramName);
11971267
}
1268+
auto paramName = getParamName(param, paramIndex, isConsumed);
11981269
++paramIndex;
11991270
printCxxToCFunctionParameterUse(param.getInterfaceType(), paramName,
12001271
param.getModuleContext(), param.isInOut(),
12011272
isIndirect, directTypeEncoding,
1202-
param.isSelfParameter());
1273+
!isConsumed && param.isSelfParameter());
12031274
};
12041275

12051276
signature.visitParameterList(
@@ -1211,10 +1282,12 @@ void DeclAndTypeClangFunctionPrinter::printCxxThunkBody(
12111282
},
12121283
[&](const LoweredFunctionSignature::DirectParameter &param) {
12131284
printParamUse(param.getParamDecl(), /*isIndirect=*/false,
1285+
isConsumedParameter(param.getConvention()),
12141286
encodeTypeInfo(param, moduleContext, typeMapping));
12151287
},
12161288
[&](const LoweredFunctionSignature::IndirectParameter &param) {
12171289
printParamUse(param.getParamDecl(), /*isIndirect=*/true,
1290+
isConsumedParameter(param.getConvention()),
12181291
/*directTypeEncoding=*/"");
12191292
},
12201293
[&](const LoweredFunctionSignature::GenericRequirementParameter

‎lib/PrintAsClang/PrintClangValueType.cpp

Lines changed: 16 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -502,16 +502,16 @@ void ClangValueTypePrinter::printValueTypeDecl(
502502
if (!isOpaqueLayout)
503503
printCValueTypeStorageStruct(cPrologueOS, typeDecl, *typeSizeAlign);
504504

505-
printTypeGenericTraits(os, typeDecl, typeMetadataFuncName,
506-
typeMetadataFuncGenericParams,
507-
typeDecl->getModuleContext(), declAndTypePrinter);
505+
printTypeGenericTraits(
506+
os, typeDecl, typeMetadataFuncName, typeMetadataFuncGenericParams,
507+
typeDecl->getModuleContext(), declAndTypePrinter, isOpaqueLayout);
508508
}
509509

510510
void ClangValueTypePrinter::printParameterCxxToCUseScaffold(
511511
const ModuleDecl *moduleContext, llvm::function_ref<void()> typePrinter,
512-
llvm::function_ref<void()> cxxParamPrinter, bool isSelf) {
512+
llvm::function_ref<void()> cxxParamPrinter, bool forceSelf) {
513513
// A Swift value type is passed to its underlying Swift function
514-
if (isSelf) {
514+
if (forceSelf) {
515515
os << "_getOpaquePointer()";
516516
} else {
517517
// FIXME: can we propagate the _impl request here?
@@ -598,7 +598,8 @@ void ClangValueTypePrinter::printTypePrecedingGenericTraits(
598598
void ClangValueTypePrinter::printTypeGenericTraits(
599599
raw_ostream &os, const TypeDecl *typeDecl, StringRef typeMetadataFuncName,
600600
ArrayRef<GenericRequirement> typeMetadataFuncRequirements,
601-
const ModuleDecl *moduleContext, DeclAndTypePrinter &declAndTypePrinter) {
601+
const ModuleDecl *moduleContext, DeclAndTypePrinter &declAndTypePrinter,
602+
bool isOpaqueLayout) {
602603
auto *NTD = dyn_cast<NominalTypeDecl>(typeDecl);
603604
ClangSyntaxPrinter printer(os);
604605
// FIXME: avoid popping out of the module's namespace here.
@@ -668,14 +669,16 @@ void ClangValueTypePrinter::printTypeGenericTraits(
668669
os << "::";
669670
printer.printBaseName(typeDecl);
670671
os << "> = true;\n";
671-
if (NTD && NTD->isResilient()) {
672+
}
673+
if (isOpaqueLayout) {
674+
assert(NTD && "not a nominal type?");
675+
assert(!isa<ClassDecl>(typeDecl) && !typeDecl->hasClangNode());
676+
if (printer.printNominalTypeOutsideMemberDeclTemplateSpecifiers(NTD))
672677
os << "template<>\n";
673-
os << "static inline const constexpr bool isOpaqueLayout<";
674-
printer.printBaseName(typeDecl->getModuleContext());
675-
os << "::";
676-
printer.printBaseName(typeDecl);
677-
os << "> = true;\n";
678-
}
678+
os << "static inline const constexpr bool isOpaqueLayout<";
679+
printer.printNominalTypeReference(NTD,
680+
/*moduleContext=*/nullptr);
681+
os << "> = true;\n";
679682
}
680683

681684
// FIXME: generic support.

‎lib/PrintAsClang/PrintClangValueType.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,8 @@ class ClangValueTypePrinter {
9696
static void printTypeGenericTraits(
9797
raw_ostream &os, const TypeDecl *typeDecl, StringRef typeMetadataFuncName,
9898
ArrayRef<GenericRequirement> typeMetadataFuncRequirements,
99-
const ModuleDecl *moduleContext, DeclAndTypePrinter &declAndTypePrinter);
99+
const ModuleDecl *moduleContext, DeclAndTypePrinter &declAndTypePrinter,
100+
bool isOpaqueLayout = false);
100101

101102
static void printTypePrecedingGenericTraits(raw_ostream &os,
102103
const NominalTypeDecl *typeDecl,

‎lib/PrintAsClang/_SwiftCxxInteroperability.h

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,22 @@ SWIFT_INLINE_THUNK void *_Nonnull getOpaquePointer(T &value) {
279279
return reinterpret_cast<void *>(&value);
280280
}
281281

282+
/// Helper struct that destroys any additional storage allocated (e.g. for
283+
/// resilient value types) for a Swift value owned by C++ code after the Swift
284+
/// value was consumed and thus the original C++ destructor is not ran.
285+
template <class T> class ConsumedValueStorageDestroyer {
286+
public:
287+
SWIFT_INLINE_THUNK ConsumedValueStorageDestroyer(T &val) noexcept
288+
: value(val) {}
289+
SWIFT_INLINE_THUNK ~ConsumedValueStorageDestroyer() noexcept {
290+
if constexpr (isOpaqueLayout<T>)
291+
reinterpret_cast<OpaqueStorage &>(value).~OpaqueStorage();
292+
}
293+
294+
private:
295+
T &value;
296+
};
297+
282298
} // namespace _impl
283299

284300
#pragma clang diagnostic pop

‎test/Interop/SwiftToCxx/generics/generic-struct-in-cxx.swift

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -274,6 +274,11 @@ public func inoutConcretePair(_ x: UInt16, _ y: inout GenericPair<UInt16, UInt16
274274
// CHECK-NEXT: }
275275
// CHECK-NEXT: };
276276
// CHECK-NEXT: namespace _impl{
277+
// CHECK-NEXT: template<class T_0_0, class T_0_1>
278+
// CHECK-NEXT: #ifdef __cpp_concepts
279+
// CHECK-NEXT: requires swift::isUsableInGenericContext<T_0_0> && swift::isUsableInGenericContext<T_0_1>
280+
// CHECK-NEXT: #endif // __cpp_concepts
281+
// CHECK-NEXT: static inline const constexpr bool isOpaqueLayout<Generics::GenericPair<T_0_0, T_0_1>> = true;
277282
// CHECK-NEXT: } // namespace
278283
// CHECK-NEXT: #pragma clang diagnostic pop
279284
// CHECK-NEXT: } // namespace swift
@@ -380,7 +385,10 @@ public func inoutConcretePair(_ x: UInt16, _ y: inout GenericPair<UInt16, UInt16
380385
// CHECK-NEXT: static_assert(swift::isUsableInGenericContext<T_0_0>, "type cannot be used in a Swift generic context");
381386
// CHECK-NEXT: static_assert(swift::isUsableInGenericContext<T_0_1>, "type cannot be used in a Swift generic context");
382387
// CHECK-NEXT: #endif
383-
// CHECK-NEXT: return _impl::$s8Generics11GenericPairV1yq_vs(swift::_impl::getOpaquePointer(value), swift::TypeMetadataTrait<GenericPair<T_0_0, T_0_1>>::getTypeMetadata(), _getOpaquePointer());
388+
// CHECK-NEXT: alignas(alignof(T_0_1)) char copyBuffer_consumedParamCopy_value[sizeof(T_0_1)];
389+
// CHECK-NEXT: auto &consumedParamCopy_value = *(new(copyBuffer_consumedParamCopy_value) T_0_1(value));
390+
// CHECK-NEXT: swift::_impl::ConsumedValueStorageDestroyer<T_0_1> storageGuard_consumedParamCopy_value(consumedParamCopy_value);
391+
// CHECK-NEXT: return _impl::$s8Generics11GenericPairV1yq_vs(swift::_impl::getOpaquePointer(consumedParamCopy_value), swift::TypeMetadataTrait<GenericPair<T_0_0, T_0_1>>::getTypeMetadata(), _getOpaquePointer());
384392
// CHECK-NEXT: }
385393
// CHECK-NEXT: template<class T_0_0, class T_0_1>
386394
// CHECK-NEXT: #ifdef __cpp_concepts
@@ -391,8 +399,14 @@ public func inoutConcretePair(_ x: UInt16, _ y: inout GenericPair<UInt16, UInt16
391399
// CHECK-NEXT: static_assert(swift::isUsableInGenericContext<T_0_0>, "type cannot be used in a Swift generic context");
392400
// CHECK-NEXT: static_assert(swift::isUsableInGenericContext<T_0_1>, "type cannot be used in a Swift generic context");
393401
// CHECK-NEXT: #endif
402+
// CHECK-NEXT: alignas(alignof(T_0_0)) char copyBuffer_consumedParamCopy_x[sizeof(T_0_0)];
403+
// CHECK-NEXT: auto &consumedParamCopy_x = *(new(copyBuffer_consumedParamCopy_x) T_0_0(x));
404+
// CHECK-NEXT: swift::_impl::ConsumedValueStorageDestroyer<T_0_0> storageGuard_consumedParamCopy_x(consumedParamCopy_x);
405+
// CHECK-NEXT: alignas(alignof(T_0_1)) char copyBuffer_consumedParamCopy_y[sizeof(T_0_1)];
406+
// CHECK-NEXT: auto &consumedParamCopy_y = *(new(copyBuffer_consumedParamCopy_y) T_0_1(y));
407+
// CHECK-NEXT: swift::_impl::ConsumedValueStorageDestroyer<T_0_1> storageGuard_consumedParamCopy_y(consumedParamCopy_y);
394408
// CHECK-NEXT: return _impl::_impl_GenericPair<T_0_0, T_0_1>::returnNewValue([&](char * _Nonnull result) SWIFT_INLINE_THUNK_ATTRIBUTES {
395-
// CHECK-NEXT: _impl::$s8Generics11GenericPairVyACyxq_Gx_Siq_tcfC(result, swift::_impl::getOpaquePointer(x), i, swift::_impl::getOpaquePointer(y), swift::TypeMetadataTrait<T_0_0>::getTypeMetadata(), swift::TypeMetadataTrait<T_0_1>::getTypeMetadata());
409+
// CHECK-NEXT: _impl::$s8Generics11GenericPairVyACyxq_Gx_Siq_tcfC(result, swift::_impl::getOpaquePointer(consumedParamCopy_x), i, swift::_impl::getOpaquePointer(consumedParamCopy_y), swift::TypeMetadataTrait<T_0_0>::getTypeMetadata(), swift::TypeMetadataTrait<T_0_1>::getTypeMetadata());
396410
// CHECK-NEXT: });
397411
// CHECK-NEXT: }
398412
// CHECK-NEXT: template<class T_0_0, class T_0_1>
@@ -470,4 +484,7 @@ public func inoutConcretePair(_ x: UInt16, _ y: inout GenericPair<UInt16, UInt16
470484
// CHECK-NEXT: static_assert(swift::isUsableInGenericContext<T_0_0>, "type cannot be used in a Swift generic context");
471485
// CHECK-NEXT: static_assert(swift::isUsableInGenericContext<T_0_1>, "type cannot be used in a Swift generic context");
472486
// CHECK-NEXT: #endif
473-
// CHECK-NEXT: return _impl::$s8Generics11GenericPairV11computedVarxvs(swift::_impl::getOpaquePointer(newValue), swift::TypeMetadataTrait<GenericPair<T_0_0, T_0_1>>::getTypeMetadata(), _getOpaquePointer());
487+
// CHECK-NEXT: alignas(alignof(T_0_0)) char copyBuffer_consumedParamCopy_newValue[sizeof(T_0_0)];
488+
// CHECK-NEXT: auto &consumedParamCopy_newValue = *(new(copyBuffer_consumedParamCopy_newValue) T_0_0(newValue));
489+
// CHECK-NEXT: swift::_impl::ConsumedValueStorageDestroyer<T_0_0> storageGuard_consumedParamCopy_newValue(consumedParamCopy_newValue);
490+
// CHECK-NEXT: return _impl::$s8Generics11GenericPairV11computedVarxvs(swift::_impl::getOpaquePointer(consumedParamCopy_newValue), swift::TypeMetadataTrait<GenericPair<T_0_0, T_0_1>>::getTypeMetadata(), _getOpaquePointer());

0 commit comments

Comments
 (0)