Skip to content

Commit 1c73538

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 15593e5 commit 1c73538

File tree

5 files changed

+126
-55
lines changed

5 files changed

+126
-55
lines changed

‎include/swift/AST/ModuleDependencies.h

Lines changed: 10 additions & 0 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"
@@ -1048,6 +1049,15 @@ class ModuleDependenciesCache {
10481049
void addSeenClangModule(clang::tooling::dependencies::ModuleID newModule) {
10491050
alreadySeenClangModules.insert(newModule);
10501051
}
1052+
void addSeenClangModules(ModuleDependencyVector &modules) {
1053+
for (const auto &m : modules) {
1054+
auto clangDetails = m.second.getAsClangModule();
1055+
ASSERT(m.first.Kind == ModuleDependencyKind::Clang);
1056+
ASSERT(clangDetails);
1057+
alreadySeenClangModules.insert(clang::tooling::dependencies::ModuleID{
1058+
m.first.ModuleName, clangDetails->contextHash});
1059+
}
1060+
}
10511061

10521062
/// Query all dependencies
10531063
ModuleDependencyIDSetVector

‎lib/AST/ModuleDependencies.cpp

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -832,12 +832,6 @@ void ModuleDependenciesCache::recordDependencies(
832832
}
833833
} else
834834
recordDependency(dep.first.ModuleName, dep.second);
835-
836-
if (dep.first.Kind == ModuleDependencyKind::Clang) {
837-
auto clangModuleDetails = dep.second.getAsClangModule();
838-
addSeenClangModule(clang::tooling::dependencies::ModuleID{
839-
dep.first.ModuleName, clangModuleDetails->contextHash});
840-
}
841835
}
842836
}
843837

‎lib/DependencyScan/ModuleDependencyScanner.cpp

Lines changed: 37 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -1056,7 +1056,7 @@ void ModuleDependencyScanner::resolveAllClangModuleDependencies(
10561056
continue;
10571057
} else {
10581058
// We need to query the Clang dependency scanner for this module's
1059-
// unresolved imports
1059+
// non-Swift imports
10601060
llvm::StringSet<> resolvedImportIdentifiers;
10611061
for (const auto &resolvedDep :
10621062
moduleDependencyInfo.getImportedSwiftDependencies())
@@ -1094,29 +1094,18 @@ void ModuleDependencyScanner::resolveAllClangModuleDependencies(
10941094
}
10951095
}
10961096

1097-
// Prepare the module lookup result collection
1098-
llvm::StringMap<std::optional<ClangModuleScannerQueryResult>>
1099-
moduleLookupResult;
1100-
for (const auto &unresolvedIdentifier : unresolvedImportIdentifiers)
1101-
moduleLookupResult.insert(
1102-
std::make_pair(unresolvedIdentifier.getKey(), std::nullopt));
1103-
1104-
// We need a copy of the shared already-seen module set, which will be shared
1105-
// amongst all the workers. In `recordDependencies`, each worker will
1106-
// contribute its results back to the shared set for future lookups.
1107-
const llvm::DenseSet<clang::tooling::dependencies::ModuleID>
1108-
seenClangModules = cache.getAlreadySeenClangModules();
1109-
std::mutex cacheAccessLock;
1097+
// Module lookup result collection
1098+
llvm::StringMap<ClangModuleScannerQueryResult> moduleLookupResult;
1099+
std::mutex resultAccessLock;
11101100
auto scanForClangModuleDependency = [this, &cache, &moduleLookupResult,
1111-
&cacheAccessLock, &seenClangModules](
1101+
&resultAccessLock](
11121102
Identifier moduleIdentifier) {
1113-
auto moduleName = moduleIdentifier.str();
1103+
llvm::DenseSet<clang::tooling::dependencies::ModuleID> seenClangModules;
11141104
{
1115-
std::lock_guard<std::mutex> guard(cacheAccessLock);
1116-
if (cache.hasDependency(moduleName, ModuleDependencyKind::Clang))
1117-
return;
1105+
std::lock_guard<std::mutex> guard(resultAccessLock);
1106+
seenClangModules = cache.getAlreadySeenClangModules();
11181107
}
1119-
1108+
11201109
auto scanResult = withDependencyScanningWorker(
11211110
[&seenClangModules,
11221111
moduleIdentifier](ModuleDependencyScanningWorker *ScanningWorker) {
@@ -1129,11 +1118,9 @@ void ModuleDependencyScanner::resolveAllClangModuleDependencies(
11291118
// if looking for a module that was discovered as a transitive
11301119
// dependency in this scan.
11311120
{
1132-
std::lock_guard<std::mutex> guard(cacheAccessLock);
1133-
moduleLookupResult.insert_or_assign(moduleName, scanResult);
1134-
if (!scanResult.foundDependencyModuleGraph.empty())
1135-
cache.recordDependencies(scanResult.foundDependencyModuleGraph,
1136-
IssueReporter.Diagnostics);
1121+
std::lock_guard<std::mutex> guard(resultAccessLock);
1122+
moduleLookupResult.insert_or_assign(moduleIdentifier.str(), scanResult);
1123+
cache.addSeenClangModules(scanResult.foundDependencyModuleGraph);
11371124
}
11381125
};
11391126

@@ -1153,35 +1140,37 @@ void ModuleDependencyScanner::resolveAllClangModuleDependencies(
11531140
std::vector<ScannerImportStatementInfo> failedToResolveImports;
11541141
ModuleDependencyIDSetVector importedClangDependencies;
11551142
auto recordResolvedClangModuleImport =
1156-
[&moduleLookupResult, &importedClangDependencies, &cache,
1143+
[this, &moduleLookupResult, &importedClangDependencies, &cache,
11571144
&allDiscoveredClangModules, moduleID, &failedToResolveImports](
11581145
const ScannerImportStatementInfo &moduleImport,
11591146
bool optionalImport) {
1160-
auto lookupResult = moduleLookupResult[moduleImport.importIdentifier];
1161-
// The imported module was found in the cache
1162-
if (lookupResult == std::nullopt) {
1163-
importedClangDependencies.insert(
1164-
{moduleImport.importIdentifier, ModuleDependencyKind::Clang});
1165-
} else {
1166-
// Cache discovered module dependencies.
1167-
if (!lookupResult.value().foundDependencyModuleGraph.empty()) {
1168-
importedClangDependencies.insert(
1169-
{moduleImport.importIdentifier, ModuleDependencyKind::Clang});
1147+
ASSERT(moduleLookupResult.contains(moduleImport.importIdentifier));
1148+
const auto &lookupResult =
1149+
moduleLookupResult.at(moduleImport.importIdentifier);
1150+
// Cache discovered module dependencies.
1151+
if (!lookupResult.foundDependencyModuleGraph.empty() ||
1152+
!lookupResult.visibleModuleIdentifiers.empty()) {
1153+
if (!lookupResult.foundDependencyModuleGraph.empty()) {
1154+
cache.recordDependencies(lookupResult.foundDependencyModuleGraph,
1155+
IssueReporter.Diagnostics);
11701156
// Add the full transitive dependency set
1171-
for (const auto &dep :
1172-
lookupResult.value().foundDependencyModuleGraph)
1157+
for (const auto &dep : lookupResult.foundDependencyModuleGraph)
11731158
allDiscoveredClangModules.insert(dep.first);
1174-
// Add visible Clang modules for this query to the depending
1175-
// Swift module
1176-
cache.addVisibleClangModules(
1177-
moduleID, lookupResult->visibleModuleIdentifiers);
1178-
} else if (!optionalImport) {
1179-
// Otherwise, we failed to resolve this dependency. We will try
1180-
// again using the cache after all other imports have been
1181-
// resolved. If that fails too, a scanning failure will be
1182-
// diagnosed.
1183-
failedToResolveImports.push_back(moduleImport);
11841159
}
1160+
1161+
importedClangDependencies.insert(
1162+
{moduleImport.importIdentifier, ModuleDependencyKind::Clang});
1163+
1164+
// Add visible Clang modules for this query to the depending
1165+
// Swift module
1166+
cache.addVisibleClangModules(moduleID,
1167+
lookupResult.visibleModuleIdentifiers);
1168+
} else if (!optionalImport) {
1169+
// Otherwise, we failed to resolve this dependency. We will try
1170+
// again using the cache after all other imports have been
1171+
// resolved. If that fails too, a scanning failure will be
1172+
// diagnosed.
1173+
failedToResolveImports.push_back(moduleImport);
11851174
}
11861175
};
11871176

‎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)