Skip to content

Commit 27305d6

Browse files
artemcmartemcm
committed
[Dependency Scanning] Ensure all direct Clang module imports from Swift modules are always queried by-name to the Clang dependency scanner
This is required in order to always have computed the set of visible Clang modules for each Swift module in the graph. Otherwise when some Clang module gets cached as a transitive dependency from a query and is later looked up as a direct dependency, there will not be any computed visible modules set.
1 parent 242585d commit 27305d6

File tree

6 files changed

+177
-115
lines changed

6 files changed

+177
-115
lines changed

‎include/swift/AST/ModuleDependencies.h‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
#include "swift/AST/Import.h"
2222
#include "swift/AST/LinkLibrary.h"
23+
#include "swift/Basic/Assertions.h"
2324
#include "swift/Basic/CXXStdlibKind.h"
2425
#include "swift/Basic/LLVM.h"
2526
#include "swift/Serialization/Validation.h"
@@ -1105,9 +1106,9 @@ class ModuleDependenciesCache {
11051106
void recordDependency(StringRef moduleName,
11061107
ModuleDependencyInfo dependencies);
11071108

1108-
/// Record dependencies for the given module collection.
1109-
void recordDependencies(ModuleDependencyVector moduleDependencies,
1110-
DiagnosticEngine &diags);
1109+
/// Record dependencies for the given collection of Clang modules.
1110+
void recordClangDependencies(ModuleDependencyVector moduleDependencies,
1111+
DiagnosticEngine &diags);
11111112

11121113
/// Update stored dependencies for the given module.
11131114
void updateDependency(ModuleDependencyID moduleID,
@@ -1140,7 +1141,6 @@ class ModuleDependenciesCache {
11401141
void
11411142
setCrossImportOverlayDependencies(ModuleDependencyID moduleID,
11421143
const ArrayRef<ModuleDependencyID> dependencyIDs);
1143-
11441144
/// Add to this module's set of visible Clang modules
11451145
void
11461146
addVisibleClangModules(ModuleDependencyID moduleID,

‎lib/AST/ModuleDependencies.cpp‎

Lines changed: 50 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -778,65 +778,62 @@ void ModuleDependenciesCache::recordDependency(
778778
map.insert({moduleName, dependency});
779779
}
780780

781-
void ModuleDependenciesCache::recordDependencies(
781+
void ModuleDependenciesCache::recordClangDependencies(
782782
ModuleDependencyVector dependencies, DiagnosticEngine &diags) {
783783
for (const auto &dep : dependencies) {
784+
ASSERT(dep.first.Kind == ModuleDependencyKind::Clang);
785+
auto newClangModuleDetails = dep.second.getAsClangModule();
784786
if (hasDependency(dep.first)) {
785-
if (dep.first.Kind == ModuleDependencyKind::Clang) {
786-
auto priorClangModuleDetails =
787-
findKnownDependency(dep.first).getAsClangModule();
788-
auto newClangModuleDetails = dep.second.getAsClangModule();
789-
auto priorContextHash = priorClangModuleDetails->contextHash;
790-
auto newContextHash = newClangModuleDetails->contextHash;
791-
if (priorContextHash != newContextHash) {
792-
// This situation means that within the same scanning action, Clang
793-
// Dependency Scanner has produced two different variants of the same
794-
// module. This is not supposed to happen, but we are currently
795-
// hunting down the rare cases where it does, seemingly due to
796-
// differences in Clang Scanner direct by-name queries and transitive
797-
// header lookup queries.
798-
//
799-
// Emit a failure diagnostic here that is hopefully more actionable
800-
// for the time being.
801-
diags.diagnose(SourceLoc(), diag::dependency_scan_unexpected_variant,
802-
dep.first.ModuleName);
803-
diags.diagnose(
804-
SourceLoc(),
805-
diag::dependency_scan_unexpected_variant_context_hash_note,
806-
priorContextHash, newContextHash);
807-
diags.diagnose(
808-
SourceLoc(),
809-
diag::dependency_scan_unexpected_variant_module_map_note,
810-
priorClangModuleDetails->moduleMapFile,
811-
newClangModuleDetails->moduleMapFile);
812-
813-
auto diagnoseExtraCommandLineFlags =
814-
[&diags](const ClangModuleDependencyStorage *checkModuleDetails,
815-
const ClangModuleDependencyStorage *baseModuleDetails,
816-
bool isNewlyDiscovered) -> void {
817-
std::unordered_set<std::string> baseCommandLineSet(
818-
baseModuleDetails->buildCommandLine.begin(),
819-
baseModuleDetails->buildCommandLine.end());
820-
for (const auto &checkArg : checkModuleDetails->buildCommandLine)
821-
if (baseCommandLineSet.find(checkArg) == baseCommandLineSet.end())
822-
diags.diagnose(
823-
SourceLoc(),
824-
diag::dependency_scan_unexpected_variant_extra_arg_note,
825-
isNewlyDiscovered, checkArg);
826-
};
827-
diagnoseExtraCommandLineFlags(priorClangModuleDetails,
828-
newClangModuleDetails, true);
829-
diagnoseExtraCommandLineFlags(newClangModuleDetails,
830-
priorClangModuleDetails, false);
831-
}
787+
auto priorClangModuleDetails =
788+
findKnownDependency(dep.first).getAsClangModule();
789+
DEBUG_ASSERT(priorClangModuleDetails && newClangModuleDetails);
790+
auto priorContextHash = priorClangModuleDetails->contextHash;
791+
auto newContextHash = newClangModuleDetails->contextHash;
792+
if (priorContextHash != newContextHash) {
793+
// This situation means that within the same scanning action, Clang
794+
// Dependency Scanner has produced two different variants of the same
795+
// module. This is not supposed to happen, but we are currently
796+
// hunting down the rare cases where it does, seemingly due to
797+
// differences in Clang Scanner direct by-name queries and transitive
798+
// header lookup queries.
799+
//
800+
// Emit a failure diagnostic here that is hopefully more actionable
801+
// for the time being.
802+
diags.diagnose(SourceLoc(), diag::dependency_scan_unexpected_variant,
803+
dep.first.ModuleName);
804+
diags.diagnose(
805+
SourceLoc(),
806+
diag::dependency_scan_unexpected_variant_context_hash_note,
807+
priorContextHash, newContextHash);
808+
diags.diagnose(
809+
SourceLoc(),
810+
diag::dependency_scan_unexpected_variant_module_map_note,
811+
priorClangModuleDetails->moduleMapFile,
812+
newClangModuleDetails->moduleMapFile);
813+
814+
auto diagnoseExtraCommandLineFlags =
815+
[&diags](const ClangModuleDependencyStorage *checkModuleDetails,
816+
const ClangModuleDependencyStorage *baseModuleDetails,
817+
bool isNewlyDiscovered) -> void {
818+
std::unordered_set<std::string> baseCommandLineSet(
819+
baseModuleDetails->buildCommandLine.begin(),
820+
baseModuleDetails->buildCommandLine.end());
821+
for (const auto &checkArg : checkModuleDetails->buildCommandLine)
822+
if (baseCommandLineSet.find(checkArg) == baseCommandLineSet.end())
823+
diags.diagnose(
824+
SourceLoc(),
825+
diag::dependency_scan_unexpected_variant_extra_arg_note,
826+
isNewlyDiscovered, checkArg);
827+
};
828+
diagnoseExtraCommandLineFlags(priorClangModuleDetails,
829+
newClangModuleDetails, true);
830+
diagnoseExtraCommandLineFlags(newClangModuleDetails,
831+
priorClangModuleDetails, false);
832832
}
833-
} else
833+
} else {
834834
recordDependency(dep.first.ModuleName, dep.second);
835-
836-
if (dep.first.Kind == ModuleDependencyKind::Clang) {
837-
auto clangModuleDetails = dep.second.getAsClangModule();
838835
addSeenClangModule(clang::tooling::dependencies::ModuleID{
839-
dep.first.ModuleName, clangModuleDetails->contextHash});
836+
dep.first.ModuleName, newClangModuleDetails->contextHash});
840837
}
841838
}
842839
}

‎lib/DependencyScan/ModuleDependencyScanner.cpp‎

Lines changed: 35 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -377,7 +377,7 @@ bool ModuleDependencyScanningWorker::scanHeaderDependenciesOfSwiftModule(
377377
auto bridgedDeps = ClangImporter::bridgeClangModuleDependencies(
378378
ctx, clangScanningTool, dependencies->ModuleGraph, lookupModuleOutput,
379379
[this](StringRef path) { return remapPath(PrefixMapper, path); });
380-
cache.recordDependencies(bridgedDeps, ctx.Diags);
380+
cache.recordClangDependencies(bridgedDeps, ctx.Diags);
381381
visibleClangModules = dependencies->VisibleModules;
382382

383383
llvm::copy(dependencies->FileDeps, std::back_inserter(headerFileInputs));
@@ -1057,7 +1057,7 @@ void ModuleDependencyScanner::resolveAllClangModuleDependencies(
10571057
continue;
10581058
} else {
10591059
// We need to query the Clang dependency scanner for this module's
1060-
// unresolved imports
1060+
// non-Swift imports
10611061
llvm::StringSet<> resolvedImportIdentifiers;
10621062
for (const auto &resolvedDep :
10631063
moduleDependencyInfo.getImportedSwiftDependencies())
@@ -1095,46 +1095,23 @@ void ModuleDependencyScanner::resolveAllClangModuleDependencies(
10951095
}
10961096
}
10971097

1098-
// Prepare the module lookup result collection
1099-
llvm::StringMap<std::optional<ClangModuleScannerQueryResult>>
1100-
moduleLookupResult;
1101-
for (const auto &unresolvedIdentifier : unresolvedImportIdentifiers)
1102-
moduleLookupResult.insert(
1103-
std::make_pair(unresolvedIdentifier.getKey(), std::nullopt));
1104-
1105-
// We need a copy of the shared already-seen module set, which will be shared
1106-
// amongst all the workers. In `recordDependencies`, each worker will
1107-
// contribute its results back to the shared set for future lookups.
1098+
// Module lookup result collection
1099+
llvm::StringMap<ClangModuleScannerQueryResult> moduleLookupResult;
11081100
const llvm::DenseSet<clang::tooling::dependencies::ModuleID>
1109-
seenClangModules = cache.getAlreadySeenClangModules();
1110-
std::mutex cacheAccessLock;
1111-
auto scanForClangModuleDependency = [this, &cache, &moduleLookupResult,
1112-
&cacheAccessLock, &seenClangModules](
1101+
seenClangModules = cache.getAlreadySeenClangModules();
1102+
std::mutex resultAccessLock;
1103+
auto scanForClangModuleDependency = [this, &moduleLookupResult,
1104+
&resultAccessLock, &seenClangModules](
11131105
Identifier moduleIdentifier) {
1114-
auto moduleName = moduleIdentifier.str();
1115-
{
1116-
std::lock_guard<std::mutex> guard(cacheAccessLock);
1117-
if (cache.hasDependency(moduleName, ModuleDependencyKind::Clang))
1118-
return;
1119-
}
1120-
11211106
auto scanResult = withDependencyScanningWorker(
11221107
[&seenClangModules,
11231108
moduleIdentifier](ModuleDependencyScanningWorker *ScanningWorker) {
11241109
return ScanningWorker->scanFilesystemForClangModuleDependency(
11251110
moduleIdentifier, seenClangModules);
11261111
});
1127-
1128-
// Update the `moduleLookupResult` and cache all discovered dependencies
1129-
// so that subsequent queries do not have to call into the scanner
1130-
// if looking for a module that was discovered as a transitive
1131-
// dependency in this scan.
11321112
{
1133-
std::lock_guard<std::mutex> guard(cacheAccessLock);
1134-
moduleLookupResult.insert_or_assign(moduleName, scanResult);
1135-
if (!scanResult.foundDependencyModuleGraph.empty())
1136-
cache.recordDependencies(scanResult.foundDependencyModuleGraph,
1137-
IssueReporter.Diagnostics);
1113+
std::lock_guard<std::mutex> guard(resultAccessLock);
1114+
moduleLookupResult.insert_or_assign(moduleIdentifier.str(), scanResult);
11381115
}
11391116
};
11401117

@@ -1154,35 +1131,37 @@ void ModuleDependencyScanner::resolveAllClangModuleDependencies(
11541131
std::vector<ScannerImportStatementInfo> failedToResolveImports;
11551132
ModuleDependencyIDSetVector importedClangDependencies;
11561133
auto recordResolvedClangModuleImport =
1157-
[&moduleLookupResult, &importedClangDependencies, &cache,
1134+
[this, &moduleLookupResult, &importedClangDependencies, &cache,
11581135
&allDiscoveredClangModules, moduleID, &failedToResolveImports](
11591136
const ScannerImportStatementInfo &moduleImport,
11601137
bool optionalImport) {
1161-
auto lookupResult = moduleLookupResult[moduleImport.importIdentifier];
1162-
// The imported module was found in the cache
1163-
if (lookupResult == std::nullopt) {
1164-
importedClangDependencies.insert(
1165-
{moduleImport.importIdentifier, ModuleDependencyKind::Clang});
1166-
} else {
1167-
// Cache discovered module dependencies.
1168-
if (!lookupResult.value().foundDependencyModuleGraph.empty()) {
1169-
importedClangDependencies.insert(
1170-
{moduleImport.importIdentifier, ModuleDependencyKind::Clang});
1138+
ASSERT(moduleLookupResult.contains(moduleImport.importIdentifier));
1139+
const auto &lookupResult =
1140+
moduleLookupResult.at(moduleImport.importIdentifier);
1141+
// Cache discovered module dependencies.
1142+
if (!lookupResult.foundDependencyModuleGraph.empty() ||
1143+
!lookupResult.visibleModuleIdentifiers.empty()) {
1144+
if (!lookupResult.foundDependencyModuleGraph.empty()) {
1145+
cache.recordClangDependencies(lookupResult.foundDependencyModuleGraph,
1146+
IssueReporter.Diagnostics);
11711147
// Add the full transitive dependency set
1172-
for (const auto &dep :
1173-
lookupResult.value().foundDependencyModuleGraph)
1148+
for (const auto &dep : lookupResult.foundDependencyModuleGraph)
11741149
allDiscoveredClangModules.insert(dep.first);
1175-
// Add visible Clang modules for this query to the depending
1176-
// Swift module
1177-
cache.addVisibleClangModules(
1178-
moduleID, lookupResult->visibleModuleIdentifiers);
1179-
} else if (!optionalImport) {
1180-
// Otherwise, we failed to resolve this dependency. We will try
1181-
// again using the cache after all other imports have been
1182-
// resolved. If that fails too, a scanning failure will be
1183-
// diagnosed.
1184-
failedToResolveImports.push_back(moduleImport);
11851150
}
1151+
1152+
importedClangDependencies.insert(
1153+
{moduleImport.importIdentifier, ModuleDependencyKind::Clang});
1154+
1155+
// Add visible Clang modules for this query to the depending
1156+
// Swift module
1157+
cache.addVisibleClangModules(moduleID,
1158+
lookupResult.visibleModuleIdentifiers);
1159+
} else if (!optionalImport) {
1160+
// Otherwise, we failed to resolve this dependency. We will try
1161+
// again using the cache after all other imports have been
1162+
// resolved. If that fails too, a scanning failure will be
1163+
// diagnosed.
1164+
failedToResolveImports.push_back(moduleImport);
11861165
}
11871166
};
11881167

‎lib/DependencyScan/ScanDependencies.cpp‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1226,8 +1226,16 @@ static bool diagnoseCycle(const CompilerInstance &instance,
12261226
while (!openSet.empty()) {
12271227
auto lastOpen = openSet.back();
12281228
auto beforeSize = openSet.size();
1229+
1230+
#ifndef NDEBUG
1231+
if (!cache.findDependency(lastOpen)) {
1232+
llvm::dbgs() << "Missing Dependency Info during cycle diagnosis\n";
1233+
llvm::dbgs() << "mainID: " << mainId.ModuleName << "\n";
1234+
llvm::dbgs() << "lastOpen: " << lastOpen.ModuleName << "\n";
1235+
}
1236+
#endif
12291237
assert(cache.findDependency(lastOpen).has_value() &&
1230-
"Missing dependency info during cycle diagnosis.");
1238+
"Missing dependency info during cycle diagnosis");
12311239
for (const auto &depId : cache.getAllDependencies(lastOpen)) {
12321240
if (closeSet.count(depId))
12331241
continue;

‎test/ScanDependencies/module_deps_swift_overlay_only_visible.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
// RUN: %empty-directory(%t/clangDeps)
55
// RUN: split-file %s %t
66

7-
// RUN: %target-swift-frontend -scan-dependencies -module-cache-path %t/clang-module-cache %s -o %t/deps.json -I %t/swiftDeps -I %t/clangDeps
7+
// RUN: %target-swift-frontend -scan-dependencies -module-cache-path %t/clang-module-cache %t/client.swift -o %t/deps.json -I %t/swiftDeps -I %t/clangDeps
88
// RUN: %validate-json %t/deps.json | %FileCheck %s
99

1010
// Ensure Swift module 'E' has a Swift overlay dependency on

‎test/ScanDependencies/module_deps_swift_overlay_only_visible_cached.swift‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
// RUN: %empty-directory(%t)
2+
// RUN: %empty-directory(%t/clang-module-cache)
3+
// RUN: %empty-directory(%t/swiftDeps)
4+
// RUN: %empty-directory(%t/clangDeps)
5+
// RUN: split-file %s %t
6+
7+
// RUN: %target-swift-frontend -scan-dependencies -module-cache-path %t/clang-module-cache %t/client.swift -o %t/deps.json -I %t/swiftDeps -I %t/clangDeps
8+
// RUN: %validate-json %t/deps.json | %FileCheck %s
9+
10+
// Ensure Swift module 'E' has a Swift overlay dependency on
11+
// 'G', because Clang module 'G' is a visible dependency of Clang module 'E'
12+
//
13+
// CHECK-LABEL: "modulePath": "{{.*}}E-{{.*}}.swiftmodule"
14+
// CHECK: "swiftOverlayDependencies": [
15+
// CHECK-NEXT: {
16+
// CHECK-DAG: "swift": "G"
17+
// CHECK-DAG: "swift": "Y"
18+
// CHECK: }
19+
20+
// Ensure Swift module 'G' has a Swift overlay dependency on
21+
// 'Y', because Clang module 'Y' is a visible dependency of Clang module 'X'
22+
//
23+
// CHECK-LABEL: "modulePath": "{{.*}}G-{{.*}}.swiftmodule"
24+
25+
//--- swiftDeps/E.swiftinterface
26+
// swift-interface-format-version: 1.0
27+
// swift-module-flags: -module-name E -enable-library-evolution
28+
@_exported import E
29+
public func overlayFuncE() {}
30+
31+
//--- swiftDeps/G.swiftinterface
32+
// swift-interface-format-version: 1.0
33+
// swift-module-flags: -module-name G -enable-library-evolution
34+
@_exported import G
35+
import X
36+
public func overlayFuncG() {}
37+
38+
//--- swiftDeps/Y.swiftinterface
39+
// swift-interface-format-version: 1.0
40+
// swift-module-flags: -module-name Y -enable-library-evolution
41+
@_exported import Y
42+
public func overlayFuncX() {}
43+
44+
//--- clangDeps/module.modulemap
45+
module E {
46+
header "E.h"
47+
export *
48+
}
49+
module G {
50+
header "G.h"
51+
export *
52+
}
53+
module X {
54+
header "X.h"
55+
export *
56+
}
57+
module Y {
58+
header "Y.h"
59+
export *
60+
}
61+
62+
//--- clangDeps/E.h
63+
#include "G.h";
64+
#include "X.h";
65+
void funcE(void);
66+
67+
//--- clangDeps/G.h
68+
void funcG(void);
69+
70+
//--- clangDeps/X.h
71+
#include "Y.h";
72+
void funcX(void);
73+
74+
//--- clangDeps/Y.h
75+
void funcY(void);
76+
77+
//--- client.swift
78+
import E

0 commit comments

Comments
 (0)