Skip to content

Commit 34e4729

Browse files
DougGregorDougGregor
committed
[C++ interop] Fix manual mangling of C++ base method thunks
When creating a C++ method that thunks from a particular C++ derived class to call a method on one of its base classes, we do some manual name mangling. This name mangling did not properly capture all of the aspects of the thunk, so it was prone to collisions. In moving to SIL asmname, we got a different set of collisions from the ones that existed previously. Expanding the mangling to account for small differences (const or not) as well as the base class type eliminates these collisions. Most of the test changes account for the new mangling, but the member-inheritance test indicates that this change fixes an existing miscompile as well.
1 parent 0ac8a83 commit 34e4729

File tree

6 files changed

+83
-27
lines changed

6 files changed

+83
-27
lines changed

‎lib/ClangImporter/SwiftDeclSynthesizer.cpp‎

Lines changed: 75 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2066,34 +2066,91 @@ clang::CXXMethodDecl *SwiftDeclSynthesizer::synthesizeCXXForwardingMethod(
20662066

20672067
// Create a new method in the derived class that calls the base method.
20682068
clang::DeclarationName name = method->getNameInfo().getName();
2069+
std::string newName;
2070+
llvm::raw_string_ostream os(newName);
2071+
bool useExistingName = false;
20692072
if (name.isIdentifier()) {
2070-
std::string newName;
2071-
llvm::raw_string_ostream os(newName);
20722073
os << (forwardingMethodKind == ForwardingMethodKind::Virtual
20732074
? "__synthesizedVirtualCall_"
20742075
: "__synthesizedBaseCall_")
20752076
<< name.getAsIdentifierInfo()->getName();
2076-
name = clang::DeclarationName(
2077-
&ImporterImpl.getClangPreprocessor().getIdentifierTable().get(
2078-
os.str()));
2079-
} else if (name.getCXXOverloadedOperator() == clang::OO_Subscript) {
2080-
name = clang::DeclarationName(
2081-
&ImporterImpl.getClangPreprocessor().getIdentifierTable().get(
2082-
(forwardingMethodKind == ForwardingMethodKind::Virtual
2077+
} else {
2078+
switch (auto op = name.getCXXOverloadedOperator()) {
2079+
case clang::OO_Subscript:
2080+
os << (forwardingMethodKind == ForwardingMethodKind::Virtual
20832081
? "__synthesizedVirtualCall_operatorSubscript"
2084-
: "__synthesizedBaseCall_operatorSubscript")));
2085-
} else if (name.getCXXOverloadedOperator() == clang::OO_Star) {
2086-
name = clang::DeclarationName(
2087-
&ImporterImpl.getClangPreprocessor().getIdentifierTable().get(
2088-
(forwardingMethodKind == ForwardingMethodKind::Virtual
2082+
: "__synthesizedBaseCall_operatorSubscript");
2083+
if (forceConstQualifier)
2084+
os << "C";
2085+
break;
2086+
2087+
case clang::OO_Star:
2088+
os << (forwardingMethodKind == ForwardingMethodKind::Virtual
20892089
? "__synthesizedVirtualCall_operatorStar"
2090-
: "__synthesizedBaseCall_operatorStar")));
2091-
} else if (name.getCXXOverloadedOperator() == clang::OO_Call) {
2092-
assert(forwardingMethodKind != ForwardingMethodKind::Virtual);
2090+
: "__synthesizedBaseCall_operatorStar");
2091+
if (forceConstQualifier)
2092+
os << "C";
2093+
break;
2094+
2095+
case clang::OO_Call:
2096+
assert(forwardingMethodKind != ForwardingMethodKind::Virtual);
2097+
os << "__synthesizedBaseCall_operatorCall";
2098+
if (forceConstQualifier)
2099+
os << "C";
2100+
break;
2101+
2102+
case clang::OO_Plus:
2103+
case clang::OO_Minus:
2104+
case clang::OO_Slash:
2105+
case clang::OO_PlusEqual:
2106+
case clang::OO_MinusEqual:
2107+
case clang::OO_StarEqual:
2108+
case clang::OO_SlashEqual:
2109+
case clang::OO_Percent:
2110+
case clang::OO_Caret:
2111+
case clang::OO_Amp:
2112+
case clang::OO_Pipe:
2113+
case clang::OO_Tilde:
2114+
case clang::OO_Exclaim:
2115+
case clang::OO_Less:
2116+
case clang::OO_Greater:
2117+
case clang::OO_LessLess:
2118+
case clang::OO_GreaterGreater:
2119+
case clang::OO_EqualEqual:
2120+
case clang::OO_PlusPlus:
2121+
case clang::OO_ExclaimEqual:
2122+
case clang::OO_LessEqual:
2123+
case clang::OO_GreaterEqual:
2124+
case clang::OO_AmpAmp:
2125+
case clang::OO_PipePipe:
2126+
os << importer::getOperatorName(ImporterImpl.SwiftContext, op).str();
2127+
break;
2128+
2129+
default:
2130+
useExistingName = true;
2131+
break;
2132+
}
2133+
}
2134+
2135+
if (!useExistingName) {
2136+
// The created method is inside the derived class already. If that's
2137+
// different from the base class, also include the base class in the
2138+
// mangling to keep this separate from other similar functions cloned from
2139+
// other base classes.
2140+
if (derivedClass != baseClass) {
2141+
os << "_";
2142+
std::unique_ptr<clang::ItaniumMangleContext> mangler{
2143+
clang::ItaniumMangleContext::create(clangCtx, clangCtx.getDiagnostics())};
2144+
auto derivedType = clangCtx.getTypeDeclType(baseClass)
2145+
.getCanonicalType();
2146+
mangler->mangleCanonicalTypeName(derivedType, os);
2147+
}
2148+
20932149
name = clang::DeclarationName(
20942150
&ImporterImpl.getClangPreprocessor().getIdentifierTable().get(
2095-
"__synthesizedBaseCall_operatorCall"));
2151+
os.str()));
20962152
}
2153+
20972154
auto methodType = method->getType();
20982155
// Check if we need to drop the reference from the return type
20992156
// of the new method. This is needed when a synthesized `operator []`

‎test/Interop/Cxx/class/inheritance/functions-objc-irgen.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55

66
import FunctionsObjC
77

8-
// CHECK: define linkonce_odr {{.*}}ptr @_ZNK7Derived36__synthesizedBaseCall_virtual_methodEv(ptr {{.*}}%[[THIS:.*]])
8+
// CHECK: define linkonce_odr {{.*}}ptr @_ZNK7Derived46__synthesizedBaseCall_virtual_method__ZTS4BaseEv(ptr {{.*}}%[[THIS:.*]])
99
// CHECK: %[[THIS_ADDR:.*]] = alloca ptr,
1010
// CHECK: store ptr %[[THIS]], ptr %[[THIS_ADDR]],
1111
// CHECK: %[[THIS1:.*]] = load ptr, ptr %[[THIS_ADDR]],
@@ -15,7 +15,7 @@ import FunctionsObjC
1515
// CHECK: %[[CALL:.*]] = call {{.*}}ptr %[[V0]](ptr {{.*}}%[[THIS1]])
1616
// CHECK: ret ptr %[[CALL]]
1717

18-
// CHECK: define linkonce_odr {{.*}}ptr @_ZNK7Derived40__synthesizedBaseCall_non_virtual_methodEv(ptr {{.*}}%[[THIS:.*]])
18+
// CHECK: define linkonce_odr {{.*}}ptr @_ZNK7Derived50__synthesizedBaseCall_non_virtual_method__ZTS4BaseEv(ptr {{.*}}%[[THIS:.*]])
1919
// CHECK: %[[THIS_ADDR:.*]] = alloca ptr,
2020
// CHECK: store ptr %[[THIS]], ptr %[[THIS_ADDR]],
2121
// CHECK: %[[THIS1:.*]] = load ptr, ptr %[[THIS_ADDR]],

‎test/Interop/Cxx/class/inheritance/subscript-irgen.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,6 @@ func testGetX() -> CInt {
99

1010
let _ = testGetX()
1111

12-
// CHECK: define {{.*}}linkonce_odr{{.*}} i32 @{{(.*)(30CopyTrackedDerivedDerivedClass39__synthesizedBaseCall_operatorSubscript|__synthesizedBaseCall_operatorSubscript@CopyTrackedDerivedDerivedClass)(.*)}}(ptr {{.*}} %[[THIS_PTR:.*]], i32 {{.*}})
12+
// CHECK: define {{.*}}linkonce_odr{{.*}} i32 @_ZNK30CopyTrackedDerivedDerivedClass67__synthesizedBaseCall_operatorSubscriptC__ZTS20CopyTrackedBaseClassEi(ptr {{.*}} %[[THIS_PTR:.*]], i32 {{.*}})
1313
// CHECK: %[[ADD_PTR:.*]] = getelementptr inbounds i8, ptr %{{.*}}, i{{32|64}} 4
1414
// CHECK: {{.*}}call {{.*}} i32 @{{.*}}CopyTrackedBaseClass{{.*}}(ptr {{.*}} %[[ADD_PTR]], i32 {{.*}}){{.*}}

‎test/Interop/Cxx/class/inheritance/virtual-methods-irgen.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ d4.f()
2323
// CHECK: call swiftcc {{.*}} @"$sSo8Derived4V1fs5Int32VyF"
2424

2525
// CHECK: define {{.*}} @"$sSo8Derived4V1fs5Int32VyF"(ptr swiftself dereferenceable
26-
// CHECK: call {{.*}} @{{_ZN8Derived423__synthesizedBaseCall_fEv|"\?__synthesizedBaseCall_f@Derived4@@QEAAHXZ"}}
26+
// CHECK: call {{.*}} @{{_ZN8Derived434__synthesizedBaseCall_f__ZTS5Base3Ev|"\?__synthesizedBaseCall_ZTS5Base3Ev@@QEAAHXZ"}}
2727

2828
// CHECK: define {{.*}}void @{{_ZN7DerivedIiE3fooEv|"\?foo@\?\$Derived@H@@UEAAXXZ"}}
2929
// CHECK: call void @{{_Z21testFunctionCollectedv|"\?testFunctionCollected@@YAXXZ"}}

‎test/Interop/Cxx/foreign-reference/function-inheritance-irgen.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,6 @@ func testGetX() -> CInt {
1010
let _ = testGetX()
1111

1212

13-
// CHECK: define {{.*}}linkonce_odr{{.*}} i32 @{{(.*)(30CopyTrackedDerivedDerivedClass26__synthesizedBaseCall_getX|__synthesizedBaseCall_getX@CopyTrackedDerivedDerivedClass)(.*)}}(ptr {{.*}} %[[THIS_PTR:.*]])
13+
// CHECK: define {{.*}}linkonce_odr{{.*}} i32 @{{(.*)(_ZNK30CopyTrackedDerivedDerivedClass53__synthesizedBaseCall_getX__ZTS20CopyTrackedBaseClassEv|.*__synthesizedBaseCall_getX.*@CopyTrackedDerivedDerivedClass)(.*)}}(ptr {{.*}} %[[THIS_PTR:.*]])
1414
// CHECK: %[[ADD_PTR:.*]] = getelementptr inbounds i8, ptr %{{.*}}, i{{32|64}} 4
1515
// CHECK: call{{.*}} i32 @{{.*}}(ptr {{.*}} %[[ADD_PTR]])

‎test/Interop/Cxx/foreign-reference/member-inheritance.swift‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -151,10 +151,9 @@ if #available(SwiftStdlib 5.8, *) {
151151
expectEqual(d1.virtualMethod(), 111)
152152
expectEqual(d1.swiftBarRename(), 113)
153153
expectEqual(d1.swiftParamsRename(a1: 42), 42)
154-
// FIXME the method calls below return incorrect values
155-
expectEqual(d1.swiftVirtualMethod(), 111) // should be 121
156-
expectEqual(d1.A2BarRename(), 113) // should be 123
157-
expectEqual(d1.swiftParamsRename(a2: 42), 42) // should be 43
154+
expectEqual(d1.swiftVirtualMethod(), 121)
155+
expectEqual(d1.A2BarRename(), 123)
156+
expectEqual(d1.swiftParamsRename(a2: 42), 43)
158157
}
159158
}
160159

0 commit comments

Comments
 (0)