Skip to content

Commit

Permalink
[Serialization] Handle XREFs to private types
Browse files Browse the repository at this point in the history
We can encounter these when the compiler modifies an inlinable
function to break apart a struct and the struct uses a private
type for one of its fields. It's questionable whether we /should/
handle this, but meanwhile this /is/ a non-intrusive fix that
preserves the performance of non-resilient libraries.

(That is, it appears this worked in Swift 4.0, though perhaps
not all of the same optimizations kicked in.)

https://bugs.swift.org/browse/SR-6874
  • Loading branch information
jrose-apple committed Feb 7, 2018
1 parent d9067d9 commit b9d3c6e
Show file tree
Hide file tree
Showing 7 changed files with 156 additions and 17 deletions.
3 changes: 2 additions & 1 deletion include/swift/Serialization/ModuleFormat.h
Expand Up @@ -54,7 +54,7 @@ const uint16_t VERSION_MAJOR = 0;
/// in source control, you should also update the comment to briefly
/// describe what change you made. The content of this comment isn't important;
/// it just ensures a conflict if two people change the module format.
const uint16_t VERSION_MINOR = 397; // No resilience expansion in SILDeclRef
const uint16_t VERSION_MINOR = 398; // Private discriminators for type xrefs

using DeclIDField = BCFixed<31>;

Expand Down Expand Up @@ -1288,6 +1288,7 @@ namespace decls_block {
using XRefTypePathPieceLayout = BCRecordLayout<
XREF_TYPE_PATH_PIECE,
IdentifierIDField, // name
IdentifierIDField, // private discriminator
BCFixed<1> // restrict to protocol extension
>;

Expand Down
49 changes: 39 additions & 10 deletions lib/Serialization/Deserialization.cpp
Expand Up @@ -1264,18 +1264,22 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
case XREF_TYPE_PATH_PIECE:
case XREF_VALUE_PATH_PIECE: {
IdentifierID IID;
IdentifierID privateDiscriminator = 0;
TypeID TID = 0;
bool isType = (recordID == XREF_TYPE_PATH_PIECE);
bool inProtocolExt = false;
bool isStatic = false;
if (isType)
XRefTypePathPieceLayout::readRecord(scratch, IID, inProtocolExt);
XRefTypePathPieceLayout::readRecord(scratch, IID, privateDiscriminator,
inProtocolExt);
else
XRefValuePathPieceLayout::readRecord(scratch, TID, IID, inProtocolExt,
isStatic);

DeclBaseName name = getDeclBaseName(IID);
pathTrace.addValue(name);
if (privateDiscriminator)
pathTrace.addValue(getIdentifier(privateDiscriminator));

Type filterTy;
if (!isType) {
Expand All @@ -1290,9 +1294,14 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
pathTrace.addType(filterTy);
}

baseModule->lookupQualified(ModuleType::get(baseModule), name,
NL_QualifiedDefault | NL_KnownNoDependency,
/*typeResolver=*/nullptr, values);
if (privateDiscriminator) {
baseModule->lookupMember(values, baseModule, name,
getIdentifier(privateDiscriminator));
} else {
baseModule->lookupQualified(ModuleType::get(baseModule), name,
NL_QualifiedDefault | NL_KnownNoDependency,
/*typeResolver=*/nullptr, values);
}
filterValues(filterTy, nullptr, nullptr, isType, inProtocolExt, isStatic,
None, values);
break;
Expand Down Expand Up @@ -1348,7 +1357,7 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
switch (recordID) {
case XREF_TYPE_PATH_PIECE: {
IdentifierID IID;
XRefTypePathPieceLayout::readRecord(scratch, IID, None);
XRefTypePathPieceLayout::readRecord(scratch, IID, None, None);
result = getIdentifier(IID);
break;
}
Expand Down Expand Up @@ -1405,8 +1414,13 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
// Fast path for nested types that avoids deserializing all
// members of the parent type.
IdentifierID IID;
IdentifierID privateDiscriminator;
bool onlyInNominal = false;
XRefTypePathPieceLayout::readRecord(scratch, IID, onlyInNominal);
XRefTypePathPieceLayout::readRecord(scratch, IID, privateDiscriminator,
onlyInNominal);
if (privateDiscriminator)
goto giveUpFastPath;

Identifier memberName = getIdentifier(IID);
pathTrace.addValue(memberName);

Expand Down Expand Up @@ -1449,21 +1463,25 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {

pathTrace.removeLast();
}
giveUpFastPath:
LLVM_FALLTHROUGH;
}
case XREF_VALUE_PATH_PIECE:
case XREF_INITIALIZER_PATH_PIECE: {
TypeID TID = 0;
DeclBaseName memberName;
Identifier privateDiscriminator;
Optional<swift::CtorInitializerKind> ctorInit;
bool isType = false;
bool inProtocolExt = false;
bool isStatic = false;
switch (recordID) {
case XREF_TYPE_PATH_PIECE: {
IdentifierID IID;
XRefTypePathPieceLayout::readRecord(scratch, IID, inProtocolExt);
IdentifierID IID, discriminatorID;
XRefTypePathPieceLayout::readRecord(scratch, IID, discriminatorID,
inProtocolExt);
memberName = getDeclBaseName(IID);
privateDiscriminator = getIdentifier(discriminatorID);
isType = true;
break;
}
Expand All @@ -1490,6 +1508,8 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
}

pathTrace.addValue(memberName);
if (!privateDiscriminator.empty())
pathTrace.addPrivateDiscriminator(privateDiscriminator);

Type filterTy;
if (!isType) {
Expand Down Expand Up @@ -1519,8 +1539,17 @@ ModuleFile::resolveCrossReference(ModuleDecl *baseModule, uint32_t pathLen) {
getXRefDeclNameForError());
}

auto members = nominal->lookupDirect(memberName);
values.append(members.begin(), members.end());
if (!privateDiscriminator.empty()) {
ModuleDecl *searchModule = M;
if (!searchModule)
searchModule = nominal->getModuleContext();
searchModule->lookupMember(values, nominal, memberName,
privateDiscriminator);

} else {
auto members = nominal->lookupDirect(memberName);
values.append(members.begin(), members.end());
}
filterValues(filterTy, M, genericSig, isType, inProtocolExt, isStatic,
ctorInit, values);
break;
Expand Down
21 changes: 15 additions & 6 deletions lib/Serialization/DeserializationErrors.h
Expand Up @@ -37,6 +37,7 @@ class XRefTracePath {
Accessor,
Extension,
GenericParam,
PrivateDiscriminator,
Unknown
};

Expand All @@ -55,11 +56,12 @@ class XRefTracePath {
: kind(K),
data(llvm::PointerLikeTypeTraits<T>::getAsVoidPointer(value)) {}

Identifier getAsIdentifier() const {
DeclBaseName getAsBaseName() const {
switch (kind) {
case Kind::Value:
case Kind::Operator:
return getDataAs<Identifier>();
case Kind::PrivateDiscriminator:
return getDataAs<DeclBaseName>();
case Kind::Type:
case Kind::OperatorFilter:
case Kind::Accessor:
Expand All @@ -73,7 +75,7 @@ class XRefTracePath {
void print(raw_ostream &os) const {
switch (kind) {
case Kind::Value:
os << getDataAs<Identifier>();
os << getDataAs<DeclBaseName>();
break;
case Kind::Type:
os << "with type " << getDataAs<Type>();
Expand Down Expand Up @@ -137,6 +139,9 @@ class XRefTracePath {
case Kind::GenericParam:
os << "generic param #" << getDataAs<uintptr_t>();
break;
case Kind::PrivateDiscriminator:
os << "(in " << getDataAs<Identifier>() << ")";
break;
case Kind::Unknown:
os << "unknown xref kind " << getDataAs<uintptr_t>();
break;
Expand Down Expand Up @@ -180,17 +185,21 @@ class XRefTracePath {
path.push_back({ PathPiece::Kind::GenericParam, index });
}

void addPrivateDiscriminator(Identifier name) {
path.push_back({ PathPiece::Kind::PrivateDiscriminator, name });
}

void addUnknown(uintptr_t kind) {
path.push_back({ PathPiece::Kind::Unknown, kind });
}

Identifier getLastName() const {
DeclBaseName getLastName() const {
for (auto &piece : reversed(path)) {
Identifier result = piece.getAsIdentifier();
DeclBaseName result = piece.getAsBaseName();
if (!result.empty())
return result;
}
return Identifier();
return DeclBaseName();
}

void removeLast() {
Expand Down
17 changes: 17 additions & 0 deletions lib/Serialization/Serialization.cpp
Expand Up @@ -1868,8 +1868,16 @@ void Serializer::writeCrossReference(const DeclContext *DC, uint32_t pathLen) {

auto generic = cast<GenericTypeDecl>(DC);
abbrCode = DeclTypeAbbrCodes[XRefTypePathPieceLayout::Code];

Identifier discriminator;
if (generic->isOutermostPrivateOrFilePrivateScope()) {
auto *containingFile = cast<FileUnit>(generic->getModuleScopeContext());
discriminator = containingFile->getDiscriminatorForPrivateValue(generic);
}

XRefTypePathPieceLayout::emitRecord(Out, ScratchRecord, abbrCode,
addDeclBaseNameRef(generic->getName()),
addDeclBaseNameRef(discriminator),
false);
break;
}
Expand Down Expand Up @@ -2019,8 +2027,17 @@ void Serializer::writeCrossReference(const Decl *D) {
bool isProtocolExt = D->getDeclContext()->getAsProtocolExtensionContext();
if (auto type = dyn_cast<TypeDecl>(D)) {
abbrCode = DeclTypeAbbrCodes[XRefTypePathPieceLayout::Code];

Identifier discriminator;
if (type->isOutermostPrivateOrFilePrivateScope()) {
auto *containingFile =
cast<FileUnit>(type->getDeclContext()->getModuleScopeContext());
discriminator = containingFile->getDiscriminatorForPrivateValue(type);
}

XRefTypePathPieceLayout::emitRecord(Out, ScratchRecord, abbrCode,
addDeclBaseNameRef(type->getName()),
addDeclBaseNameRef(discriminator),
isProtocolExt);
return;
}
Expand Down
8 changes: 8 additions & 0 deletions test/Serialization/Inputs/xref-private-type/Lib.swift
@@ -0,0 +1,8 @@
import LibCore

public let lazyFoo = Foo()
public func testFoo(_: Foo = lazyFoo) {}
public let lazyBar = Bar()
public func testBar(_: Bar = lazyBar) {}
public let lazyBaz = Baz()
public func testBaz(_: Baz = lazyBaz) {}
24 changes: 24 additions & 0 deletions test/Serialization/Inputs/xref-private-type/LibCore.swift
@@ -0,0 +1,24 @@
private class TopLevelInternalClass {}
public struct Foo {
private var ref: TopLevelInternalClass

public init() { self.ref = .init() }
}

public struct Bar {
private class NestedInternalClass {}

private var ref: NestedInternalClass

public init() { self.ref = .init() }
}

public struct Baz {
fileprivate class NestedInternalClass {
fileprivate class DoublyNestedInternalClass {}
}

private var ref: NestedInternalClass.DoublyNestedInternalClass

public init() { self.ref = .init() }
}
51 changes: 51 additions & 0 deletions test/Serialization/xref-private-type.swift
@@ -0,0 +1,51 @@
// RUN: %empty-directory(%t)
// RUN: %target-build-swift -swift-version 4 -O -force-single-frontend-invocation -emit-module-path %t/LibCore.swiftmodule %S/Inputs/xref-private-type/LibCore.swift
// RUN: %target-build-swift -swift-version 4 -O -I %t -force-single-frontend-invocation -emit-module-path %t/Lib.swiftmodule %S/Inputs/xref-private-type/Lib.swift
// RUN: %target-build-swift -swift-version 4 -O -I %t -emit-sil %s | %FileCheck %s

import Lib

// CHECK: sil{{.*}} @[[TESTSR6874:[^ ]+10testSR6874[^ ]+]] :
func testSR6874() {
// The important lines in this test are the strong_retains, which refer to
// private types defined in LibCore. Normally we shouldn't have references to
// non-public declarations in inlinable code, but because SIL passes can break
// apart non-resilient structs and enums we can end up in that situation.
// Worse, this can happen *across module boundaries.*
//
// In this test, the addressor for each global defined in Lib ends up
// referencing private types defined in LibCore. Using those globals in
// default argument position leads to the addressor getting inlined into
// calling code in Swift 4 and later. This results in an attempt to not just
// reference a private type, but to *resolve a cross-reference to a private
// type.*
//
// This is the situation in SR-6874 (simplified). I'm not sure of a simpler
// way to reliably trigger the issue. But if this test breaks, please try to
// find one.
//
// (We may want to revisit this whole thing later, as it violates the model.
// But it's also useful for performance in non-resilient code.)

// CHECK: [[ADDR:%.+]] = global_addr @{{[^ ]+}}3Lib7lazyFoo
// CHECK: [[LOADED:%.+]] = load [[ADDR]] : $*Foo
// CHECK: [[REF:%.+]] = struct_extract [[LOADED]] : $Foo, #Foo.ref
// CHECK: strong_retain [[REF]] : $TopLevelInternalClass
// CHECK: apply {{%.+}}([[LOADED]])
testFoo()

// CHECK: [[ADDR:%.+]] = global_addr @{{[^ ]+}}3Lib7lazyBar
// CHECK: [[LOADED:%.+]] = load [[ADDR]] : $*Bar
// CHECK: [[REF:%.+]] = struct_extract [[LOADED]] : $Bar, #Bar.ref
// CHECK: strong_retain [[REF]] : $Bar.NestedInternalClass
// CHECK: apply {{%.+}}([[LOADED]])
testBar()

// CHECK: [[ADDR:%.+]] = global_addr @{{[^ ]+}}3Lib7lazyBaz
// CHECK: [[LOADED:%.+]] = load [[ADDR]] : $*Baz
// CHECK: [[REF:%.+]] = struct_extract [[LOADED]] : $Baz, #Baz.ref
// CHECK: strong_retain [[REF]] : $Baz.NestedInternalClass.DoublyNestedInternalClass
// CHECK: apply {{%.+}}([[LOADED]])
testBaz()
} // CHECK: end sil function '[[TESTSR6874]]'

0 comments on commit b9d3c6e

Please sign in to comment.