[ParseableInterfaces] Improvements to forwarding module dependencies (#23968)

- Fix a typo in the debug output for finding a prebuilt module
- Don't add the prebuilt module path to the dependency tracker
- Ensure we're registering the _forwarding module_'s dependencies in
cached modules, not the prebuilt module's dependencies.
- Check for the existence of a prebuilt module before doing the
is-up-to-date check (which will have failed anyway if it didn't exist).
- Test that forwarding module dependency collection works

rdar://48659199

Author: Harlan Haskins

Diff for: lib/Frontend/ParseableInterfaceModuleLoader.cpp

@@ -408,13 +408,17 @@ class swift::ParseableInterfaceBuilder {
       StringRef DepName = Scratch.str();
 
       assert(moduleCachePath.empty() || !DepName.startswith(moduleCachePath));
-      assert(prebuiltCachePath.empty() || !DepName.startswith(prebuiltCachePath));
 
       // Serialize the paths of dependencies in the SDK relative to it.
       Optional<StringRef> SDKRelativePath = getRelativeDepPath(DepName, SDKPath);
       StringRef DepNameToStore = SDKRelativePath.getValueOr(DepName);
       bool IsSDKRelative = SDKRelativePath.hasValue();
 
+      // Forwarding modules add the underlying prebuilt module to their
+      // dependency list -- don't serialize that.
+      if (!prebuiltCachePath.empty() && DepName.startswith(prebuiltCachePath))
+        continue;
+
       if (AllDepNames.insert(DepName).second && dependencyTracker) {
         dependencyTracker->addDependency(DepName, /*isSystem*/IsSDKRelative);
       }
@@ -828,6 +832,10 @@ class ParseableInterfaceModuleLoaderImpl {
     }
     path::append(scratch, path::filename(modulePath));
 
+    // If there isn't a file at this location, skip returning a path.
+    if (!fs.exists(scratch))
+      return None;
+
     return scratch.str();
   }
 
@@ -914,7 +922,7 @@ class ParseableInterfaceModuleLoaderImpl {
           return DiscoveredModule::prebuilt(*path, std::move(moduleBuffer));
         } else {
           LLVM_DEBUG(llvm::dbgs() << "Found out-of-date prebuilt module at "
-                     << modulePath << "\n");
+                                  << path->str() << "\n");
         }
       }
     }
@@ -951,40 +959,62 @@ class ParseableInterfaceModuleLoaderImpl {
 
   /// Writes the "forwarding module" that will forward to a module in the
   /// prebuilt cache.
+  ///
   /// Since forwarding modules track dependencies separately from the module
   /// they point to, we'll need to grab the up-to-date file status while doing
-  /// this.
-  bool writeForwardingModule(const DiscoveredModule &mod,
-                             StringRef outputPath,
-                             ArrayRef<FileDependency> deps) {
+  /// this. If the write was successful, it also updates the
+  /// list of dependencies to match what was written to the forwarding file.
+  bool writeForwardingModuleAndUpdateDeps(
+      const DiscoveredModule &mod, StringRef outputPath,
+      SmallVectorImpl<FileDependency> &deps) {
     assert(mod.isPrebuilt() &&
            "cannot write forwarding file for non-prebuilt module");
     ForwardingModule fwd(mod.path);
 
+    SmallVector<FileDependency, 16> depsAdjustedToMTime;
+
     // FIXME: We need to avoid re-statting all these dependencies, otherwise
     //        we may record out-of-date information.
-    auto addDependency = [&](StringRef path) {
+    auto addDependency = [&](StringRef path) -> FileDependency {
       auto status = fs.status(path);
       uint64_t mtime =
         status->getLastModificationTime().time_since_epoch().count();
       fwd.addDependency(path, status->getSize(), mtime);
+
+      // Construct new FileDependency matching what we've added to the
+      // forwarding module. This is no longer SDK-relative because the absolute
+      // path has already been resolved.
+      return FileDependency::modTimeBased(path, /*isSDKRelative*/false,
+                                          status->getSize(), mtime);
     };
 
-    // Add the prebuilt module as a dependency of the forwarding module.
-    addDependency(fwd.underlyingModulePath);
+    // Add the prebuilt module as a dependency of the forwarding module, but
+    // don't add it to the outer dependency list.
+    (void)addDependency(fwd.underlyingModulePath);
 
-    // Add all the dependencies from the prebuilt module.
+    // Add all the dependencies from the prebuilt module, and update our list
+    // of dependencies to reflect what's recorded in the forwarding module.
     SmallString<128> SDKRelativeBuffer;
     for (auto dep : deps) {
-      addDependency(getFullDependencyPath(dep, SDKRelativeBuffer));
+      auto adjustedDep =
+          addDependency(getFullDependencyPath(dep, SDKRelativeBuffer));
+      depsAdjustedToMTime.push_back(adjustedDep);
     }
 
-    return withOutputFile(diags, outputPath,
+    auto hadError = withOutputFile(diags, outputPath,
       [&](llvm::raw_pwrite_stream &out) {
         llvm::yaml::Output yamlWriter(out);
         yamlWriter << fwd;
         return false;
       });
+
+    if (hadError)
+      return true;
+
+    // If and only if we succeeded writing the forwarding file, update the
+    // provided list of dependencies.
+    deps = depsAdjustedToMTime;
+    return false;
   }
 
   /// Looks up the best module to load for a given interface, and returns a
@@ -1033,9 +1063,11 @@ class ParseableInterfaceModuleLoaderImpl {
     if (moduleOrErr) {
       auto module = std::move(moduleOrErr.get());
 
-      // If it's prebuilt, use this time to generate a forwarding module.
+      // If it's prebuilt, use this time to generate a forwarding module and
+      // update the dependencies to use modification times.
       if (module.isPrebuilt())
-        if (writeForwardingModule(module, cachedOutputPath, allDeps))
+        if (writeForwardingModuleAndUpdateDeps(module, cachedOutputPath,
+                                               allDeps))
           return std::make_error_code(std::errc::not_supported);
 
       // Report the module's dependencies to the dependencyTracker

Diff for: test/ParseableInterface/ModuleCache/forwarding-module-dependencies.swift

@@ -0,0 +1,56 @@
+// RUN: %empty-directory(%t)
+// RUN: %empty-directory(%t/mock-sdk)
+// RUN: %empty-directory(%t/ModuleCache)
+// RUN: %empty-directory(%t/PrebuiltModuleCache)
+
+// This test makes sure that we propagate the right set of dependencies from a
+// prebuilt module to its corresponding forwarding module.
+
+// Setup. Copy the mock SDK into the tmp directory.
+// RUN: cp -r %S/Inputs/mock-sdk/* %t/mock-sdk/.
+
+// 1. Compile ExportedLib.swiftinterface, which a) is in the SDK, and b) depends
+//    on a C module with headers that should be in the dependency list.
+//    Put it in the prebuilt cache.
+
+// RUN: %target-swift-frontend -build-module-from-parseable-interface %t/mock-sdk/ExportedLib.swiftinterface -sdk %t/mock-sdk -o %t/PrebuiltModuleCache/ExportedLib.swiftmodule -serialize-parseable-module-interface-dependency-hashes -track-system-dependencies
+
+// 2. Make sure the prebuilt module we built has SomeCModule.h as a dependency.
+
+// RUN: llvm-bcanalyzer -dump %t/PrebuiltModuleCache/ExportedLib.swiftmodule | grep 'FILE_DEPENDENCY.*SomeCModule.h'
+
+// 3. Typecheck this file, which imports SdkLib, which imports ExportedLib.
+//    Because ExportedLib is prebuilt, we expect a forwarding module for
+//    ExportedLib in the module cache, and a serialized module for SdkLib in
+//    the cache.
+
+// RUN: %target-swift-frontend -typecheck %s -prebuilt-module-cache-path %t/PrebuiltModuleCache -module-cache-path %t/ModuleCache -sdk %t/mock-sdk -I %t/mock-sdk -track-system-dependencies
+
+// 4. Make sure the forwarding module is installed in the cache.
+
+// RUN: %{python} %S/Inputs/check-is-forwarding-module.py %t/ModuleCache/ExportedLib-*.swiftmodule
+
+// 5. Make sure the forwarding module depends on the prebuilt module and the C
+//    header.
+
+// RUN: grep '  *path:.*ExportedLib.swiftmodule' %t/ModuleCache/ExportedLib-*.swiftmodule
+// RUN: grep '  *path:.*SomeCModule.h' %t/ModuleCache/ExportedLib-*.swiftmodule
+
+// 6. Make sure the dependencies from the forwarding module make it into the
+//    cached module.
+
+// RUN: llvm-bcanalyzer -dump %t/ModuleCache/SdkLib-*.swiftmodule | grep 'FILE_DEPENDENCY.*SomeCModule.h'
+
+// 7. Make sure the prebuilt ExportedLib module did NOT get propagated to SdkLib.
+
+// RUN: llvm-bcanalyzer -dump %t/ModuleCache/SdkLib-*.swiftmodule | not grep 'FILE_DEPENDENCY.*PrebuiltModuleCache'
+
+// 8. Make sure we re-build the SdkLib module if one of the dependencies changes its mtime (but not its hash).
+//    This will ensure we recorded the forwarding module's dependencies, not the prebuilt.
+
+// RUN: %{python} %S/Inputs/make-old.py %t/ModuleCache/SdkLib-*.swiftmodule
+// RUN: %{python} %S/Inputs/make-old.py %t/mock-sdk/usr/include/SomeCModule.h
+// RUN: %target-swift-frontend -typecheck %s -prebuilt-module-cache-path %t/PrebuiltModuleCache -module-cache-path %t/ModuleCache -sdk %t/mock-sdk -I %t/mock-sdk -track-system-dependencies
+// RUN: %{python} %S/Inputs/check-is-new.py %t/ModuleCache/SdkLib-*.swiftmodule
+
+import SdkLib