Skip to content

Commit 1261f2b

Browse files
elshelsh
committed
Non-package module should load swiftinterface instead of
binary module with PackageCMO enabled. If the binary module optimized with Package CMO is loaded for a non-package client module, the client module does not understand the serialized content of the optimized binary module because it contains attributes and instructions specific to the optimization, i.e. [serialized_for_package] definition that contains operations on non-loadable types that are normally not allowed. This can cause an assert or a crash in SIL verifier. This PR checks whether the optimized binary module and the client module are in the same package, and require loading swiftinterface module if not. Resolves rdar://134584629
1 parent 07357b9 commit 1261f2b

File tree

12 files changed

+179
-70
lines changed

12 files changed

+179
-70
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -888,6 +888,9 @@ ERROR(serialization_target_incompatible,Fatal,
888888
ERROR(serialization_sdk_mismatch,Fatal,
889889
"cannot load module %0 built with SDK '%1' when using SDK '%2': %3",
890890
(Identifier, StringRef, StringRef, StringRef))
891+
REMARK(serialization_module_package_mismatch,none,
892+
"cannot load module %0 built with package '%1' when using package '%2': %3",
893+
(Identifier, StringRef, StringRef, StringRef))
891894
ERROR(serialization_target_incompatible_repl,none,
892895
"module %0 was created for incompatible target %1: %2",
893896
(Identifier, StringRef, StringRef))

‎include/swift/Serialization/Validation.h

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,10 @@ enum class Status {
8787
/// The module file was built with a different SDK than the one in use
8888
/// to build the client.
8989
SDKMismatch,
90+
91+
/// The module file was built with a different package name than
92+
/// the one in use to build the client.
93+
PackageMismatch,
9094
};
9195

9296
/// Returns the string for the Status enum.
@@ -107,6 +111,7 @@ struct ValidationInfo {
107111
StringRef sdkVersion = {};
108112
StringRef problematicRevision = {};
109113
StringRef problematicChannel = {};
114+
StringRef packageName = {};
110115
size_t bytes = 0;
111116
Status status = Status::Malformed;
112117
std::vector<StringRef> allowableClients;
@@ -275,14 +280,26 @@ struct SearchPath {
275280
/// compiled with -enable-ossa-modules.
276281
/// \param requiredSDK If not empty, only accept modules built with
277282
/// a compatible SDK. The StringRef represents the canonical SDK name.
283+
/// \param packageName Used to compare the package name between
284+
/// the module being serialized and the module loading it to determine whether
285+
/// the module being serialized should be accepted.
278286
/// \param[out] extendedInfo If present, will be populated with additional
279287
/// compilation options serialized into the AST at build time that may be
280288
/// necessary to load it properly.
281289
/// \param[out] dependencies If present, will be populated with list of
282290
/// input files the module depends on, if present in INPUT_BLOCK.
283291
ValidationInfo validateSerializedAST(
284-
StringRef data, bool requiresOSSAModules,
285-
StringRef requiredSDK,
292+
StringRef data, bool requiresOSSAModules, StringRef requiredSDK,
293+
StringRef PackageName, ExtendedValidationInfo *extendedInfo = nullptr,
294+
SmallVectorImpl<SerializationOptions::FileDependency> *dependencies =
295+
nullptr,
296+
SmallVectorImpl<SearchPath> *searchPaths = nullptr);
297+
298+
/// Calls validateSerializedAST above with an empty package name.
299+
/// FIXME: should be removed once the call site of this in
300+
/// lldb/source/Plugins/TypeSystem/Swift/SwiftASTContext.cpp is updated.
301+
ValidationInfo validateSerializedAST(
302+
StringRef data, bool requiresOSSAModules, StringRef requiredSDK,
286303
ExtendedValidationInfo *extendedInfo = nullptr,
287304
SmallVectorImpl<SerializationOptions::FileDependency> *dependencies =
288305
nullptr,

‎lib/ASTSectionImporter/ASTSectionImporter.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ swift::parseASTSection(MemoryBufferSerializedModuleLoader &Loader,
5959
while (!buf.empty()) {
6060
auto info = serialization::validateSerializedAST(
6161
buf, Loader.isRequiredOSSAModules(),
62-
/*requiredSDK*/StringRef());
62+
/*requiredSDK*/ StringRef(), /*packageName*/ StringRef());
6363

6464
assert(info.name.size() < (2 << 10) && "name failed sanity check");
6565

‎lib/Frontend/CompilerInvocation.cpp

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3739,12 +3739,9 @@ bool CompilerInvocation::parseArgs(
37393739
serialization::Status
37403740
CompilerInvocation::loadFromSerializedAST(StringRef data) {
37413741
serialization::ExtendedValidationInfo extendedInfo;
3742-
serialization::ValidationInfo info =
3743-
serialization::validateSerializedAST(
3744-
data,
3745-
getSILOptions().EnableOSSAModules,
3746-
LangOpts.SDKName,
3747-
&extendedInfo);
3742+
serialization::ValidationInfo info = serialization::validateSerializedAST(
3743+
data, getSILOptions().EnableOSSAModules, LangOpts.SDKName,
3744+
LangOpts.PackageName, &extendedInfo);
37483745

37493746
if (info.status != serialization::Status::Valid)
37503747
return info.status;
@@ -3779,10 +3776,8 @@ CompilerInvocation::setUpInputForSILTool(
37793776
InputFile(inputFilename, bePrimary, fileBufOrErr.get().get(), file_types::TY_SIL));
37803777

37813778
auto result = serialization::validateSerializedAST(
3782-
fileBufOrErr.get()->getBuffer(),
3783-
getSILOptions().EnableOSSAModules,
3784-
LangOpts.SDKName,
3785-
&extendedInfo);
3779+
fileBufOrErr.get()->getBuffer(), getSILOptions().EnableOSSAModules,
3780+
LangOpts.SDKName, LangOpts.PackageName, &extendedInfo);
37863781
bool hasSerializedAST = result.status == serialization::Status::Valid;
37873782

37883783
if (hasSerializedAST) {

‎lib/Frontend/ModuleInterfaceLoader.cpp

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -214,10 +214,10 @@ namespace path = llvm::sys::path;
214214

215215
static bool serializedASTLooksValid(const llvm::MemoryBuffer &buf,
216216
bool requiresOSSAModules,
217-
StringRef requiredSDK) {
218-
auto VI = serialization::validateSerializedAST(buf.getBuffer(),
219-
requiresOSSAModules,
220-
requiredSDK);
217+
StringRef requiredSDK,
218+
StringRef packageName) {
219+
auto VI = serialization::validateSerializedAST(
220+
buf.getBuffer(), requiresOSSAModules, requiredSDK, packageName);
221221
return VI.status == serialization::Status::Valid;
222222
}
223223

@@ -260,6 +260,14 @@ struct ModuleRebuildInfo {
260260
};
261261
SmallVector<CandidateModule, 3> candidateModules;
262262

263+
/// Allows emitting remarks when rebuilding is required in an unusual situation.
264+
/// E.g. In local builds, a package-external client can try to load a module
265+
/// that's been built with a different package name; the module should not
266+
/// be loaded for the client as it can contain package-specific optimized
267+
/// content. In such case, we rebuild the module from interface, and emit
268+
/// remarks on why rebuild is required, even without -Rmodule-interface-rebuild.
269+
bool emitRemarks;
270+
263271
CandidateModule &getOrInsertCandidateModule(StringRef path) {
264272
for (auto &mod : candidateModules) {
265273
if (mod.path == path) return mod;
@@ -285,7 +293,6 @@ struct ModuleRebuildInfo {
285293
void setSerializationStatus(StringRef path, serialization::Status status) {
286294
getOrInsertCandidateModule(path).serializationStatus = status;
287295
}
288-
289296
/// Registers an out-of-date dependency at \c depPath for the module
290297
/// at \c modulePath.
291298
void addOutOfDateDependency(StringRef modulePath, StringRef depPath) {
@@ -358,6 +365,8 @@ struct ModuleRebuildInfo {
358365
return "target platform newer than current platform";
359366
case Status::SDKMismatch:
360367
return "SDK does not match";
368+
case Status::PackageMismatch:
369+
return "Package name does not match";
361370
case Status::Valid:
362371
return nullptr;
363372
}
@@ -495,14 +504,21 @@ class UpToDateModuleCheker {
495504
// Clear the existing dependencies, because we're going to re-fill them
496505
// in validateSerializedAST.
497506
allDeps.clear();
498-
507+
serialization::ExtendedValidationInfo extendedInfo;
499508
LLVM_DEBUG(llvm::dbgs() << "Validating deps of " << path << "\n");
509+
500510
auto validationInfo = serialization::validateSerializedAST(
501-
buf.getBuffer(), requiresOSSAModules,
502-
ctx.LangOpts.SDKName, /*ExtendedValidationInfo=*/nullptr, &allDeps);
511+
buf.getBuffer(), requiresOSSAModules, ctx.LangOpts.SDKName,
512+
ctx.LangOpts.PackageName,
513+
/*ExtendedValidationInfo=*/&extendedInfo, &allDeps);
503514

504515
if (validationInfo.status != serialization::Status::Valid) {
505516
rebuildInfo.setSerializationStatus(path, validationInfo.status);
517+
// When rebuild is unexpected, e.g. a client trying to load
518+
// a module built with a different package name, emit remarks
519+
// on why rebuild is required.
520+
if (validationInfo.status == serialization::Status::PackageMismatch)
521+
rebuildInfo.emitRemarks = true;
506522
return false;
507523
}
508524

@@ -659,9 +675,9 @@ class ModuleInterfaceLoaderImpl {
659675
if (!modBuf)
660676
return false;
661677

662-
auto looksValid = serializedASTLooksValid(*modBuf.get(),
663-
requiresOSSAModules,
664-
ctx.LangOpts.SDKName);
678+
auto looksValid =
679+
serializedASTLooksValid(*modBuf.get(), requiresOSSAModules,
680+
ctx.LangOpts.SDKName, ctx.LangOpts.PackageName);
665681
if (!looksValid)
666682
return false;
667683

@@ -1190,7 +1206,8 @@ class ModuleInterfaceLoaderImpl {
11901206
diag::rebuilding_module_from_interface, moduleName,
11911207
interfacePath);
11921208
};
1193-
auto remarkRebuild = Opts.remarkOnRebuildFromInterface
1209+
auto remarkRebuild = Opts.remarkOnRebuildFromInterface ||
1210+
rebuildInfo.emitRemarks
11941211
? llvm::function_ref<void()>(remarkRebuildAll)
11951212
: nullptr;
11961213

@@ -2495,9 +2512,8 @@ bool ExplicitSwiftModuleLoader::canImportModule(
24952512
}
24962513

24972514
auto metaData = serialization::validateSerializedAST(
2498-
(*moduleBuf)->getBuffer(),
2499-
Ctx.SILOpts.EnableOSSAModules,
2500-
Ctx.LangOpts.SDKName);
2515+
(*moduleBuf)->getBuffer(), Ctx.SILOpts.EnableOSSAModules,
2516+
Ctx.LangOpts.SDKName, Ctx.LangOpts.PackageName);
25012517
versionInfo->setVersion(metaData.userModuleVersion,
25022518
ModuleVersionSourceKind::SwiftBinaryModule);
25032519
return true;
@@ -2839,7 +2855,7 @@ bool ExplicitCASModuleLoader::canImportModule(
28392855
}
28402856
auto metaData = serialization::validateSerializedAST(
28412857
(*moduleBuf)->getBuffer(), Ctx.SILOpts.EnableOSSAModules,
2842-
Ctx.LangOpts.SDKName);
2858+
Ctx.LangOpts.SDKName, Ctx.LangOpts.PackageName);
28432859
versionInfo->setVersion(metaData.userModuleVersion,
28442860
ModuleVersionSourceKind::SwiftBinaryModule);
28452861
return true;

‎lib/Serialization/ModuleFile.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -407,8 +407,8 @@ ModuleFile::getModuleName(ASTContext &Ctx, StringRef modulePath,
407407
serialization::ValidationInfo loadInfo = ModuleFileSharedCore::load(
408408
"", "", std::move(newBuf), nullptr, nullptr,
409409
/*isFramework=*/isFramework, Ctx.SILOpts.EnableOSSAModules,
410-
Ctx.LangOpts.SDKName, Ctx.SearchPathOpts.DeserializedPathRecoverer,
411-
loadedModuleFile);
410+
Ctx.LangOpts.SDKName, Ctx.LangOpts.PackageName,
411+
Ctx.SearchPathOpts.DeserializedPathRecoverer, loadedModuleFile);
412412
Name = loadedModuleFile->Name.str();
413413
return std::move(moduleBuf.get());
414414
}

‎lib/Serialization/ModuleFileSharedCore.cpp

Lines changed: 44 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -224,12 +224,9 @@ static bool readOptionsBlock(llvm::BitstreamCursor &cursor,
224224

225225
static ValidationInfo validateControlBlock(
226226
llvm::BitstreamCursor &cursor, SmallVectorImpl<uint64_t> &scratch,
227-
std::pair<uint16_t, uint16_t> expectedVersion,
228-
bool requiresOSSAModules,
229-
bool requiresRevisionMatch,
230-
StringRef requiredSDK,
231-
ExtendedValidationInfo *extendedInfo,
232-
PathObfuscator &pathRecoverer) {
227+
std::pair<uint16_t, uint16_t> expectedVersion, bool requiresOSSAModules,
228+
bool requiresRevisionMatch, StringRef requiredSDK, StringRef packageName,
229+
ExtendedValidationInfo *extendedInfo, PathObfuscator &pathRecoverer) {
233230
// The control block is malformed until we've at least read a major version
234231
// number.
235232
ValidationInfo result;
@@ -261,7 +258,23 @@ static ValidationInfo validateControlBlock(
261258
result.status = Status::Malformed;
262259
return result;
263260
}
264-
if (!readOptionsBlock(cursor, scratch, *extendedInfo, pathRecoverer)) {
261+
262+
if (readOptionsBlock(cursor, scratch, *extendedInfo, pathRecoverer)) {
263+
// The client module must be in the same package as the
264+
// (optimized) binary module being loaded.
265+
// FIXME: In the long run, we should teach an external
266+
// client to skip package-specific content that's optimized,
267+
// so that we don't require rebuild of the module from
268+
// interface, which is inefficient. `extendedInfo` lookup
269+
// should also be removed, as it's not part of the core
270+
// block that determines how modules should be loaded.
271+
if (extendedInfo && extendedInfo->serializePackageEnabled() &&
272+
extendedInfo->getModulePackageName() != packageName) {
273+
result.packageName = extendedInfo->getModulePackageName();
274+
result.status = Status::PackageMismatch;
275+
return result;
276+
}
277+
} else {
265278
result.status = Status::Malformed;
266279
return result;
267280
}
@@ -576,6 +589,7 @@ std::string serialization::StatusToString(Status S) {
576589
case Status::TargetIncompatible: return "TargetIncompatible";
577590
case Status::TargetTooNew: return "TargetTooNew";
578591
case Status::SDKMismatch: return "SDKMismatch";
592+
case Status::PackageMismatch: return "PackageMismatch";
579593
}
580594
llvm_unreachable("The switch should cover all cases");
581595
}
@@ -587,9 +601,15 @@ bool serialization::isSerializedAST(StringRef data) {
587601
}
588602

589603
ValidationInfo serialization::validateSerializedAST(
590-
StringRef data, bool requiresOSSAModules,
591-
StringRef requiredSDK,
592-
ExtendedValidationInfo *extendedInfo,
604+
StringRef data, bool requiresOSSAModules, StringRef requiredSDK, ExtendedValidationInfo *extendedInfo,
605+
SmallVectorImpl<SerializationOptions::FileDependency> *dependencies,
606+
SmallVectorImpl<SearchPath> *searchPaths) {
607+
return validateSerializedAST(data, requiresOSSAModules, requiredSDK, StringRef(), extendedInfo, dependencies, searchPaths);
608+
}
609+
610+
ValidationInfo serialization::validateSerializedAST(
611+
StringRef data, bool requiresOSSAModules, StringRef requiredSDK,
612+
StringRef packageName, ExtendedValidationInfo *extendedInfo,
593613
SmallVectorImpl<SerializationOptions::FileDependency> *dependencies,
594614
SmallVectorImpl<SearchPath> *searchPaths) {
595615
ValidationInfo result;
@@ -633,8 +653,7 @@ ValidationInfo serialization::validateSerializedAST(
633653
cursor, scratch,
634654
{SWIFTMODULE_VERSION_MAJOR, SWIFTMODULE_VERSION_MINOR},
635655
requiresOSSAModules,
636-
/*requiresRevisionMatch=*/true,
637-
requiredSDK,
656+
/*requiresRevisionMatch=*/true, requiredSDK, packageName,
638657
extendedInfo, localObfuscator);
639658
if (result.status != Status::Valid)
640659
return result;
@@ -1169,8 +1188,10 @@ bool ModuleFileSharedCore::readModuleDocIfPresent(PathObfuscator &pathRecoverer)
11691188
info = validateControlBlock(
11701189
docCursor, scratch, {SWIFTDOC_VERSION_MAJOR, SWIFTDOC_VERSION_MINOR},
11711190
RequiresOSSAModules,
1172-
/*requiresRevisionMatch*/false,
1173-
/*requiredSDK*/StringRef(), /*extendedInfo*/nullptr, pathRecoverer);
1191+
/*requiresRevisionMatch*/ false,
1192+
/*requiredSDK*/ StringRef(),
1193+
/*packageName*/ StringRef(),
1194+
/*extendedInfo*/ nullptr, pathRecoverer);
11741195
if (info.status != Status::Valid)
11751196
return false;
11761197
// Check that the swiftdoc is actually for this module.
@@ -1314,8 +1335,10 @@ bool ModuleFileSharedCore::readModuleSourceInfoIfPresent(PathObfuscator &pathRec
13141335
infoCursor, scratch,
13151336
{SWIFTSOURCEINFO_VERSION_MAJOR, SWIFTSOURCEINFO_VERSION_MINOR},
13161337
RequiresOSSAModules,
1317-
/*requiresRevisionMatch*/false,
1318-
/*requiredSDK*/StringRef(), /*extendedInfo*/nullptr, pathRecoverer);
1338+
/*requiresRevisionMatch*/ false,
1339+
/*requiredSDK*/ StringRef(),
1340+
/*packageName*/ StringRef(),
1341+
/*extendedInfo*/ nullptr, pathRecoverer);
13191342
if (info.status != Status::Valid)
13201343
return false;
13211344
// Check that the swiftsourceinfo is actually for this module.
@@ -1389,10 +1412,9 @@ ModuleFileSharedCore::ModuleFileSharedCore(
13891412
std::unique_ptr<llvm::MemoryBuffer> moduleInputBuffer,
13901413
std::unique_ptr<llvm::MemoryBuffer> moduleDocInputBuffer,
13911414
std::unique_ptr<llvm::MemoryBuffer> moduleSourceInfoInputBuffer,
1392-
bool isFramework,
1393-
bool requiresOSSAModules,
1394-
StringRef requiredSDK,
1395-
serialization::ValidationInfo &info, PathObfuscator &pathRecoverer)
1415+
bool isFramework, bool requiresOSSAModules, StringRef requiredSDK,
1416+
StringRef packageName, serialization::ValidationInfo &info,
1417+
PathObfuscator &pathRecoverer)
13961418
: ModuleInputBuffer(std::move(moduleInputBuffer)),
13971419
ModuleDocInputBuffer(std::move(moduleDocInputBuffer)),
13981420
ModuleSourceInfoInputBuffer(std::move(moduleSourceInfoInputBuffer)),
@@ -1444,8 +1466,8 @@ ModuleFileSharedCore::ModuleFileSharedCore(
14441466
cursor, scratch,
14451467
{SWIFTMODULE_VERSION_MAJOR, SWIFTMODULE_VERSION_MINOR},
14461468
RequiresOSSAModules,
1447-
/*requiresRevisionMatch=*/true, requiredSDK,
1448-
&extInfo, pathRecoverer);
1469+
/*requiresRevisionMatch=*/true, requiredSDK, packageName, &extInfo,
1470+
pathRecoverer);
14491471
if (info.status != Status::Valid) {
14501472
error(info.status);
14511473
return;

‎lib/Serialization/ModuleFileSharedCore.h

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -424,10 +424,9 @@ class ModuleFileSharedCore {
424424
std::unique_ptr<llvm::MemoryBuffer> moduleInputBuffer,
425425
std::unique_ptr<llvm::MemoryBuffer> moduleDocInputBuffer,
426426
std::unique_ptr<llvm::MemoryBuffer> moduleSourceInfoInputBuffer,
427-
bool isFramework,
428-
bool requiresOSSAModules,
429-
StringRef requiredSDK,
430-
serialization::ValidationInfo &info, PathObfuscator &pathRecoverer);
427+
bool isFramework, bool requiresOSSAModules, StringRef requiredSDK,
428+
StringRef packageName, serialization::ValidationInfo &info,
429+
PathObfuscator &pathRecoverer);
431430

432431
/// Change the status of the current module.
433432
Status error(Status issue) {
@@ -562,15 +561,14 @@ class ModuleFileSharedCore {
562561
std::unique_ptr<llvm::MemoryBuffer> moduleInputBuffer,
563562
std::unique_ptr<llvm::MemoryBuffer> moduleDocInputBuffer,
564563
std::unique_ptr<llvm::MemoryBuffer> moduleSourceInfoInputBuffer,
565-
bool isFramework, bool requiresOSSAModules,
566-
StringRef requiredSDK, PathObfuscator &pathRecoverer,
564+
bool isFramework, bool requiresOSSAModules, StringRef requiredSDK,
565+
StringRef packageName, PathObfuscator &pathRecoverer,
567566
std::shared_ptr<const ModuleFileSharedCore> &theModule) {
568567
serialization::ValidationInfo info;
569568
auto *core = new ModuleFileSharedCore(
570569
std::move(moduleInputBuffer), std::move(moduleDocInputBuffer),
571570
std::move(moduleSourceInfoInputBuffer), isFramework,
572-
requiresOSSAModules, requiredSDK, info,
573-
pathRecoverer);
571+
requiresOSSAModules, requiredSDK, packageName, info, pathRecoverer);
574572
if (!moduleInterfacePath.empty()) {
575573
ArrayRef<char> path;
576574
core->allocateBuffer(path, moduleInterfacePath);

0 commit comments

Comments
 (0)