Skip to content

Commit 99cefe7

Browse files
varungandhi-applevarungandhi-apple
committed
[ModuleTrace] Handle cycle detection edge case in module trace emission.
Fixes rdar://66512316.
1 parent 6773d7e commit 99cefe7

File tree

2 files changed

+79
-14
lines changed

2 files changed

+79
-14
lines changed

‎lib/FrontendTool/FrontendTool.cpp

Lines changed: 73 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -328,6 +328,9 @@ class ABIDependencyEvaluator {
328328

329329
llvm::DenseSet<ModuleDecl *> visited;
330330

331+
/// Cache to avoid recomputing public imports of Swift modules repeatedly.
332+
llvm::DenseMap<ModuleDecl *, bool> isOverlayCache;
333+
331334
/// Helper function to handle invariant violations as crashes in debug mode.
332335
void crashOnInvariantViolation(
333336
llvm::function_ref<void (llvm::raw_string_ostream &)> f) const;
@@ -342,6 +345,31 @@ class ABIDependencyEvaluator {
342345
void reexposeImportedABI(ModuleDecl *module, ModuleDecl *importedModule,
343346
bool includeImportedModule = true);
344347

348+
/// Check if a Swift module is an overlay for some Clang module.
349+
///
350+
/// FIXME: Delete this hack once SR-13363 is fixed and ModuleDecl has the
351+
/// right API which we can use directly.
352+
bool isOverlayOfClangModule(ModuleDecl *swiftModule);
353+
354+
/// Check for cases where we have a fake cycle through an overlay.
355+
///
356+
/// \code
357+
/// Actual stack:
358+
/// sandwichedModule -> Overlay (Swift) -> ... -> sandwichedModule
359+
/// ^^--- wrong!
360+
/// Ideal stack:
361+
/// sandwichedModule -> Underlying (Clang)
362+
/// \endcode
363+
///
364+
/// This happens when we have a dependency like:
365+
/// \code
366+
/// Overlay (Swift) -> sandwichedModule -> Underlying (Clang)
367+
/// \endcode
368+
///
369+
/// We check this lazily because eagerly detecting if the dependency on an
370+
/// overlay is correct or not is difficult.
371+
bool isFakeCycleThroughOverlay(ModuleDecl **sandwichedModuleIter);
372+
345373
/// Recursive step in computing ABI dependencies.
346374
///
347375
/// Use this method instead of using the \c forClangModule/\c forSwiftModule
@@ -453,9 +481,47 @@ void ABIDependencyEvaluator::reexposeImportedABI(
453481
addToABIExportMap(module, reexportedModule);
454482
}
455483

484+
bool ABIDependencyEvaluator::isOverlayOfClangModule(ModuleDecl *swiftModule) {
485+
assert(!swiftModule->isNonSwiftModule());
486+
487+
auto cacheEntry = isOverlayCache.find(swiftModule);
488+
if (cacheEntry != isOverlayCache.end())
489+
return cacheEntry->second;
490+
491+
llvm::SmallPtrSet<ModuleDecl *, 8> importList;
492+
::getImmediateImports(swiftModule, importList,
493+
{ModuleDecl::ImportFilterKind::Public});
494+
bool isOverlay =
495+
llvm::any_of(importList, [&](ModuleDecl *importedModule) -> bool {
496+
return isClangOverlayOf(swiftModule, importedModule);
497+
});
498+
isOverlayCache[swiftModule] = isOverlay;
499+
return isOverlay;
500+
}
501+
502+
bool ABIDependencyEvaluator::isFakeCycleThroughOverlay(
503+
ModuleDecl **sandwichModuleIter) {
504+
assert(sandwichModuleIter >= searchStack.begin()
505+
&& sandwichModuleIter < searchStack.end()
506+
&& "sandwichModuleIter points to an element in searchStack");
507+
// The sandwichedModule must be a Clang module.
508+
if (!(*sandwichModuleIter)->isNonSwiftModule())
509+
return false;
510+
auto nextModuleIter = sandwichModuleIter + 1;
511+
if (nextModuleIter == searchStack.end())
512+
return false;
513+
// The next module must be a Swift overlay for a Clang module
514+
if ((*nextModuleIter)->isNonSwiftModule())
515+
return false;
516+
return isOverlayOfClangModule(*nextModuleIter);
517+
}
518+
456519
void ABIDependencyEvaluator::computeABIDependenciesForModule(
457520
ModuleDecl *module) {
458-
if (llvm::find(searchStack, module) != searchStack.end()) {
521+
auto moduleIter = llvm::find(searchStack, module);
522+
if (moduleIter != searchStack.end()) {
523+
if (isFakeCycleThroughOverlay(moduleIter))
524+
return;
459525
crashOnInvariantViolation([&](llvm::raw_string_ostream &os) {
460526
os << "unexpected cycle in import graph!\n";
461527
for (auto m: searchStack) {
@@ -510,6 +576,10 @@ void ABIDependencyEvaluator::computeABIDependenciesForClangModule(
510576
// C' imports S. This creates a cycle: S -> C' -> ... -> S.
511577
// In practice, this case is hit for
512578
// Darwin (Swift) -> SwiftOverlayShims (Clang) -> Darwin (Swift).
579+
// We may also hit this in a slightly different direction, in case
580+
// the module directly imports SwiftOverlayShims:
581+
// SwiftOverlayShims -> Darwin (Swift) -> SwiftOverlayShims
582+
// The latter is handled later by isFakeCycleThroughOverlay.
513583
// 3. [NOTE: Intra-module-leafwards-traversal]
514584
// Cycles within the same top-level module.
515585
// These don't matter for us, since we only care about the dependency
@@ -519,9 +589,8 @@ void ABIDependencyEvaluator::computeABIDependenciesForClangModule(
519589
if (import->isStdlibModule()) {
520590
continue;
521591
}
522-
if (!import->isNonSwiftModule()
523-
&& import->findUnderlyingClangModule() != nullptr
524-
&& llvm::find(searchStack, import) != searchStack.end()) {
592+
if (!import->isNonSwiftModule() && isOverlayOfClangModule(import) &&
593+
llvm::find(searchStack, import) != searchStack.end()) {
525594
continue;
526595
}
527596
if (import->isNonSwiftModule()

‎test/Driver/loaded_module_trace_directness_2.swift

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,8 @@ public class MyNode {
6969

7070
// 4. CoreDaemon - Build.
7171

72-
// FIXME: Trace emission shouldn't crash.
73-
// RUN: %target-swift-frontend %s -emit-module -o %t/include/CoreDaemon.swiftmodule -DCoreDaemon -module-name CoreDaemon -I %t/include
74-
// RUN: not --crash %target-swift-frontend %s -typecheck -DCoreDaemon -module-name CoreDaemon -emit-loaded-module-trace-path %t/CoreDaemon.trace.json -I %t/include 2>/dev/null
75-
// SKIP: %FileCheck %s --check-prefix=COREDAEMON < %t/CoreDaemon.trace.json
72+
// RUN: %target-swift-frontend %s -emit-module -o %t/include/CoreDaemon.swiftmodule -DCoreDaemon -module-name CoreDaemon -emit-loaded-module-trace-path %t/CoreDaemon.trace.json -I %t/include
73+
// RUN: %FileCheck %s --check-prefix=COREDAEMON < %t/CoreDaemon.trace.json
7674

7775
// * CoreDaemon - ObjC module with overlay, the overlay has import DaemonKit
7876
// * DaemonKit - ObjC module without overlay, has #import <CoreDaemon.h>
@@ -101,16 +99,14 @@ public func runBoth(_ pair: DaemonKit.DaemonPair) {
10199
// RUN: %target-swift-frontend %s -typecheck -DTestDaemon -DV1 -module-name TestDaemonV1 -emit-loaded-module-trace-path %t/TestDaemonV1.trace.json -I %t/include
102100
// RUN: %FileCheck %s --check-prefix=TESTDAEMON < %t/TestDaemonV1.trace.json
103101

104-
// FIXME: Trace emission shouldn't crash.
105-
// RUN: not --crash %target-swift-frontend %s -typecheck -DTestDaemon -DV2 -module-name TestDaemonV2 -emit-loaded-module-trace-path %t/TestDaemonV2.trace.json -I %t/include 2>/dev/null
106-
// SKIP: %FileCheck %s --check-prefix=TESTDAEMON < %t/TestDaemonV2.trace.json
102+
// RUN: %target-swift-frontend %s -typecheck -DTestDaemon -DV2 -module-name TestDaemonV2 -emit-loaded-module-trace-path %t/TestDaemonV2.trace.json -I %t/include
103+
// RUN: %FileCheck %s --check-prefix=TESTDAEMON < %t/TestDaemonV2.trace.json
107104

108105
// RUN: %target-swift-frontend %s -typecheck -DTestDaemon -DV3 -module-name TestDaemonV3 -emit-loaded-module-trace-path %t/TestDaemonV3.trace.json -I %t/include
109106
// RUN: %FileCheck %s --check-prefix=TESTDAEMON < %t/TestDaemonV3.trace.json
110107

111-
// FIXME: Trace emission shouldn't crash.
112-
// RUN: not --crash %target-swift-frontend %s -typecheck -DTestDaemon -DV4 -module-name TestDaemonV4 -emit-loaded-module-trace-path %t/TestDaemonV4.trace.json -I %t/include 2>/dev/null
113-
// SKIP: %FileCheck %s --check-prefix=TESTDAEMON < %t/TestDaemonV4.trace.json
108+
// RUN: %target-swift-frontend %s -typecheck -DTestDaemon -DV4 -module-name TestDaemonV4 -emit-loaded-module-trace-path %t/TestDaemonV4.trace.json -I %t/include
109+
// RUN: %FileCheck %s --check-prefix=TESTDAEMON < %t/TestDaemonV4.trace.json
114110

115111
#if TestDaemon
116112
#if V1

0 commit comments

Comments
 (0)