Merge pull request #33640 from varungandhi-apple/vg-fix-module-trace-cycle

[ModuleTrace] Handle cycle detection edge case in module trace emission (ep2).

Author: varungandhi-apple

lib/FrontendTool/FrontendTool.cpp

@@ -85,6 +85,7 @@
 #include "llvm/Support/YAMLTraits.h"
 #include "llvm/Target/TargetMachine.h"
 
+#include <algorithm>
 #include <memory>
 #include <unordered_set>
 #include <utility>
@@ -351,22 +352,25 @@ class ABIDependencyEvaluator {
 
   /// Check for cases where we have a fake cycle through an overlay.
   ///
-  /// \code
-  /// Actual stack:
-  ///    sandwichedModule -> Overlay (Swift) -> ... -> sandwichedModule
-  ///                     ^^--- wrong!
-  /// Ideal stack:
-  ///    sandwichedModule -> Underlying (Clang)
-  /// \endcode
+  /// Sometimes, we have fake cycles in the import graph due to the Clang
+  /// importer injecting overlays between Clang modules. These don't represent
+  /// an actual cycle in the build, so we should ignore them.
   ///
-  /// This happens when we have a dependency like:
-  /// \code
-  ///     Overlay (Swift) -> sandwichedModule -> Underlying (Clang)
-  /// \endcode
+  /// We check this lazily after detecting a cycle because it is difficult to
+  /// determine at the point where we see the overlay whether it was incorrectly
+  /// injected by the Clang importer or whether any of its imports will
+  /// eventually lead to a cycle.
+  ///
+  /// For more details, see [NOTE: ABIDependencyEvaluator-fake-cycle-detection]
   ///
-  /// We check this lazily because eagerly detecting if the dependency on an
-  /// overlay is correct or not is difficult.
-  bool isFakeCycleThroughOverlay(ModuleDecl **sandwichedModuleIter);
+  /// \param startOfCycle A pointer to the element of \c searchStack where
+  ///        the module \em first appeared.
+  ///
+  /// \pre The module on top of \c searchStack is the same module as
+  ///      *startOfCycle.
+  ///
+  /// \pre searchStack.begin() <= startOfCycle < searchStack.end()
+  bool isFakeCycleThroughOverlay(ModuleDecl **startOfCycle);
 
   /// Recursive step in computing ABI dependencies.
   ///
@@ -492,21 +496,87 @@ bool ABIDependencyEvaluator::isOverlayOfClangModule(ModuleDecl *swiftModule) {
   return isOverlay;
 }
 
+// [NOTE: ABIDependencyEvaluator-fake-cycle-detection]
+//
+// First, let's consider a concrete example.
+// - In Clang-land, ToyKit #imports CoreDoll.
+// - The Swift overlay for CoreDoll imports both CoreDoll and ToyKit.
+// Importing ToyKit from CoreDoll's overlay informally violates the layering
+// of frameworks, but it doesn't actually create any cycles in the build
+// dependencies.
+//                        ┌───────────────────────────┐
+//                    ┌───│    CoreDoll.swiftmodule   │
+//                    │   └───────────────────────────┘
+//                    │                 │
+//              import ToyKit     @_exported import CoreDoll
+//                    │                 │
+//                    │                 │
+//                    ▼                 │
+//      ┌──────────────────────────┐    │
+//      │ ToyKit (ToyKit/ToyKit.h) │    │
+//      └──────────────────────────┘    │
+//                    │                 │
+//       #import <CoreDoll/CoreDoll.h>  │
+//                    │                 │
+//                    ▼                 │
+//   ┌──────────────────────────────┐   │
+//   │CoreDoll (CoreDoll/CoreDoll.h)│◀──┘
+//   └──────────────────────────────┘
+//
+// Say we are trying to build a Swift module that imports ToyKit. Due to how
+// module loading works, the Clang importer inserts the CoreDoll overlay
+// between the ToyKit and CoreDoll Clang modules, creating a cycle in the
+// import graph.
+//
+//   ┌──────────────────────────┐
+//   │ ToyKit (ToyKit/ToyKit.h) │◀──────────┐
+//   └──────────────────────────┘           │
+//                 │                        │
+//    #import <CoreDoll/CoreDoll.h>    import ToyKit
+//                 │                        │
+//                 ▼                        │
+//   ┌────────────────────────────┐         │
+//   │    CoreDoll.swiftmodule    │─────────┘
+//   └────────────────────────────┘
+//                 │
+//     @_exported import CoreDoll
+//                 │
+//                 ▼
+//   ┌──────────────────────────────┐
+//   │CoreDoll (CoreDoll/CoreDoll.h)│
+//   └──────────────────────────────┘
+//
+// This means that, at some point, searchStack will look like:
+//
+//   [others] → ToyKit → CoreDoll (overlay) → ToyKit
+//
+// In the general case, there may be arbitrarily many modules in the cycle,
+// including submodules.
+//
+//   [others] → ToyKit → [others] → CoreDoll (overlay) → [others] → ToyKit
+//
+// where "[others]" indicates 0 or more modules of any kind.
+//
+// To detect this, we check that the start of the cycle is a Clang module and
+// that there is at least one overlay between it and its recurrence at the end
+// of the searchStack. If so, we assume we have detected a benign cycle which
+// can be safely ignored.
+
 bool ABIDependencyEvaluator::isFakeCycleThroughOverlay(
-    ModuleDecl **sandwichModuleIter) {
-  assert(sandwichModuleIter >= searchStack.begin()
-         && sandwichModuleIter < searchStack.end()
-         && "sandwichModuleIter points to an element in searchStack");
-  // The sandwichedModule must be a Clang module.
-  if (!(*sandwichModuleIter)->isNonSwiftModule())
-    return false;
-  auto nextModuleIter = sandwichModuleIter + 1;
-  if (nextModuleIter == searchStack.end())
-    return false;
-  // The next module must be a Swift overlay for a Clang module
-  if ((*nextModuleIter)->isNonSwiftModule())
+    ModuleDecl **startOfCycle) {
+  assert(startOfCycle >= searchStack.begin() &&
+         startOfCycle < searchStack.end() &&
+         "startOfCycleIter points to an element in searchStack");
+  // The startOfCycle module must be a Clang module.
+  if (!(*startOfCycle)->isNonSwiftModule())
     return false;
-  return isOverlayOfClangModule(*nextModuleIter);
+  // Next, we must have zero or more modules followed by a Swift overlay for a
+  // Clang module.
+  return std::any_of(startOfCycle + 1, searchStack.end(),
+                     [this](ModuleDecl *module) {
+                       return !module->isNonSwiftModule() &&
+                              isOverlayOfClangModule(module);
+                     });
 }
 
 void ABIDependencyEvaluator::computeABIDependenciesForModule(
@@ -518,7 +588,11 @@ void ABIDependencyEvaluator::computeABIDependenciesForModule(
     crashOnInvariantViolation([&](llvm::raw_string_ostream &os) {
       os << "unexpected cycle in import graph!\n";
       for (auto m: searchStack) {
-        printModule(m, os); os << "\ndepends on ";
+        printModule(m, os);
+        if (!m->isNonSwiftModule()) {
+          os << " (isOverlay = " << isOverlayOfClangModule(m) << ")";
+        }
+        os << "\ndepends on ";
       }
       printModule(module, os); os << '\n';
     });

test/Driver/Inputs/imported_modules/ComplexModuleGraph2/DaemonKit.h renamed to test/Driver/Inputs/imported_modules/ComplexModuleGraph2/DaemonKit/DaemonKit.h

@@ -1,3 +1,5 @@
+#import <DaemonKit/ViolateSecondLawOfThermodynamics.h>
+
 #import <CoreDaemon.h>
 
 struct DaemonPair {

test/Driver/Inputs/imported_modules/ComplexModuleGraph2/DaemonKit/ViolateSecondLawOfThermodynamics.h

@@ -0,0 +1,5 @@
+#import <CoreDaemon.h>
+
+struct MaxwellsDaemon {
+  Runner *doorOperation;
+};

test/Driver/Inputs/imported_modules/ComplexModuleGraph2/module.modulemap

@@ -14,7 +14,11 @@ module CoreDaemon {
 }
 
 module DaemonKit {
-  header "DaemonKit.h"
+  umbrella header "DaemonKit/DaemonKit.h"
+  explicit module ViolateSecondLawOfThermodynamics {
+    header "DaemonKit/ViolateSecondLawOfThermodynamics.h"
+    export *
+  }
   export *
 }
 

test/Driver/loaded_module_trace_directness_2.swift

@@ -114,7 +114,7 @@ public func runBoth(_ pair: DaemonKit.DaemonPair) {
   #endif
   #if V2
     import DaemonKit
-    public func noop(_: CoreDaemon.Daemon) {}
+    public func noop(_: CoreDaemon.Daemon, _: MaxwellsDaemon) {}
   #endif
   #if V3
     import CoreDaemon