Merge pull request swiftlang#27635 from varungandhi-apple/vg-robustify-module-trace-emission

Make module trace emission more robust.

Author: varungandhi-apple

include/swift/Frontend/ModuleInterfaceLoader.h

@@ -151,6 +151,7 @@ class ModuleInterfaceLoader : public SerializedModuleLoaderBase {
     AccessPathElem ModuleID, StringRef DirPath, StringRef ModuleFilename,
     StringRef ModuleDocFilename,
     StringRef ModuleSourceInfoFilename,
+    SmallVectorImpl<char> *ModuleInterfacePath,
     std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
     std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer,
     std::unique_ptr<llvm::MemoryBuffer> *ModuleSourceInfoBuffer) override;

include/swift/Serialization/SerializedModuleLoader.h

@@ -31,8 +31,8 @@ enum class ModuleLoadingMode {
   OnlySerialized
 };
 
-/// Common functionality shared between \c SerializedModuleLoader and
-/// \c ModuleInterfaceLoader.
+/// Common functionality shared between \c SerializedModuleLoader,
+/// \c ModuleInterfaceLoader and \c MemoryBufferSerializedModuleLoader.
 class SerializedModuleLoaderBase : public ModuleLoader {
   /// A { module, generation # } pair.
   using LoadedModulePair = std::pair<std::unique_ptr<ModuleFile>, unsigned>;
@@ -53,6 +53,7 @@ class SerializedModuleLoaderBase : public ModuleLoader {
 
   using AccessPathElem = std::pair<Identifier, SourceLoc>;
   bool findModule(AccessPathElem moduleID,
+                  SmallVectorImpl<char> *moduleInterfacePath,
                   std::unique_ptr<llvm::MemoryBuffer> *moduleBuffer,
                   std::unique_ptr<llvm::MemoryBuffer> *moduleDocBuffer,
                   std::unique_ptr<llvm::MemoryBuffer> *moduleSourceInfoBuffer,
@@ -74,6 +75,7 @@ class SerializedModuleLoaderBase : public ModuleLoader {
       AccessPathElem ModuleID, StringRef DirPath, StringRef ModuleFilename,
       StringRef ModuleDocFilename,
       StringRef ModuleSourceInfoFilename,
+      SmallVectorImpl<char> *ModuleInterfacePath,
       std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
       std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer,
       std::unique_ptr<llvm::MemoryBuffer> *ModuleSourceInfoBuffer) = 0;
@@ -127,6 +129,7 @@ class SerializedModuleLoaderBase : public ModuleLoader {
   /// If the AST cannot be loaded and \p diagLoc is present, a diagnostic is
   /// printed. (Note that \p diagLoc is allowed to be invalid.)
   FileUnit *loadAST(ModuleDecl &M, Optional<SourceLoc> diagLoc,
+                    StringRef moduleInterfacePath,
                     std::unique_ptr<llvm::MemoryBuffer> moduleInputBuffer,
                     std::unique_ptr<llvm::MemoryBuffer> moduleDocInputBuffer,
                     std::unique_ptr<llvm::MemoryBuffer> moduleSourceInfoInputBuffer,
@@ -178,6 +181,7 @@ class SerializedModuleLoader : public SerializedModuleLoaderBase {
       AccessPathElem ModuleID, StringRef DirPath, StringRef ModuleFilename,
       StringRef ModuleDocFilename,
       StringRef ModuleSourceInfoFilename,
+      SmallVectorImpl<char> *ModuleInterfacePath,
       std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
       std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer,
       std::unique_ptr<llvm::MemoryBuffer> *ModuleSourceInfoBuffer) override;
@@ -223,6 +227,7 @@ class MemoryBufferSerializedModuleLoader : public SerializedModuleLoaderBase {
       AccessPathElem ModuleID, StringRef DirPath, StringRef ModuleFilename,
       StringRef ModuleDocFilename,
       StringRef ModuleSourceInfoFilename,
+      SmallVectorImpl<char> *ModuleInterfacePath,
       std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
       std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer,
       std::unique_ptr<llvm::MemoryBuffer> *ModuleSourceInfoBuffer) override;

lib/AST/ModuleLoader.cpp

@@ -39,7 +39,6 @@ DependencyTracker::addDependency(StringRef File, bool IsSystem) {
   // DependencyTracker exposes an interface that (intentionally) does not talk
   // about clang at all, nor about missing deps. It does expose an IsSystem
   // dimension, which we accept and pass along to the clang DependencyCollector.
-  // along to the clang DependencyCollector.
   clangCollector->maybeAddDependency(File, /*FromModule=*/false,
                                      IsSystem, /*IsModuleFile=*/false,
                                      /*IsMissing=*/false);

lib/Frontend/Frontend.cpp

@@ -931,8 +931,8 @@ bool CompilerInstance::parsePartialModulesAndLibraryFiles(
   // Parse all the partial modules first.
   for (auto &PM : PartialModules) {
     assert(PM.ModuleBuffer);
-    if (!SML->loadAST(*MainModule, SourceLoc(), std::move(PM.ModuleBuffer),
-                      std::move(PM.ModuleDocBuffer),
+    if (!SML->loadAST(*MainModule, SourceLoc(), /*moduleInterfacePath*/"",
+                      std::move(PM.ModuleBuffer), std::move(PM.ModuleDocBuffer),
                       std::move(PM.ModuleSourceInfoBuffer), /*isFramework*/false,
                       /*treatAsPartialModule*/true))
       hadLoadError = true;

lib/Frontend/ModuleInterfaceLoader.cpp

@@ -990,6 +990,7 @@ std::error_code ModuleInterfaceLoader::findModuleFilesInDirectory(
   AccessPathElem ModuleID, StringRef DirPath, StringRef ModuleFilename,
   StringRef ModuleDocFilename,
   StringRef ModuleSourceInfoFilename,
+  SmallVectorImpl<char> *ModuleInterfacePath,
   std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
   std::unique_ptr<llvm::MemoryBuffer> *ModuleDocBuffer,
   std::unique_ptr<llvm::MemoryBuffer> *ModuleSourceInfoBuffer) {
@@ -1036,6 +1037,8 @@ std::error_code ModuleInterfaceLoader::findModuleFilesInDirectory(
 
   if (ModuleBuffer) {
     *ModuleBuffer = std::move(*ModuleBufferOrErr);
+    if (ModuleInterfacePath)
+      *ModuleInterfacePath = InPath;
   }
   // Open .swiftsourceinfo file if it's present.
   SerializedModuleLoaderBase::openModuleSourceInfoFileIfPresent(ModuleID,

lib/FrontendTool/FrontendTool.cpp

@@ -237,12 +237,165 @@ template <> struct ObjectTraits<LoadedModuleTraceFormat> {
 }
 }
 
+/// Compute the per-module information to be recorded in the trace file.
+//
+// The most interesting/tricky thing here is _which_ paths get recorded in
+// the trace file as dependencies. It depends on how the module was synthesized.
+// The key points are:
+//
+// 1. Paths to swiftmodules in the module cache or in the prebuilt cache are not
+//    recorded - Precondition: the corresponding path to the swiftinterface must
+//    already be present as a key in pathToModuleDecl.
+// 2. swiftmodules next to a swiftinterface are saved if they are up-to-date.
+//
+// FIXME: Use the VFS instead of handling paths directly. We are particularly
+// sloppy about handling relative paths in the dependency tracker.
+static void computeSwiftModuleTraceInfo(
+    const SmallPtrSetImpl<ModuleDecl *> &importedModules,
+    const llvm::DenseMap<StringRef, ModuleDecl *> &pathToModuleDecl,
+    const DependencyTracker &depTracker,
+    StringRef prebuiltCachePath,
+    std::vector<SwiftModuleTraceInfo> &traceInfo) {
+
+  SmallString<256> buffer;
+
+  std::string errMsg;
+  llvm::raw_string_ostream err(errMsg);
+
+  // FIXME: Use PrettyStackTrace instead.
+  auto errorUnexpectedPath =
+      [&pathToModuleDecl](llvm::raw_string_ostream &errStream) {
+    errStream << "The module <-> path mapping we have is:\n";
+    for (auto &m: pathToModuleDecl)
+      errStream << m.second->getName() << " <-> " << m.first << '\n';
+    llvm::report_fatal_error(errStream.str());
+  };
+
+  using namespace llvm::sys;
+
+  auto computeAdjacentInterfacePath = [](SmallVectorImpl<char> &modPath) {
+    auto swiftInterfaceExt =
+      file_types::getExtension(file_types::TY_SwiftModuleInterfaceFile);
+    path::replace_extension(modPath, swiftInterfaceExt);
+  };
+
+  for (auto &depPath : depTracker.getDependencies()) {
+
+    // Decide if this is a swiftmodule based on the extension of the raw
+    // dependency path, as the true file may have a different one.
+    // For example, this might happen when the canonicalized path points to
+    // a Content Addressed Storage (CAS) location.
+    auto moduleFileType =
+      file_types::lookupTypeForExtension(path::extension(depPath));
+    auto isSwiftmodule =
+      moduleFileType == file_types::TY_SwiftModuleFile;
+    auto isSwiftinterface =
+      moduleFileType == file_types::TY_SwiftModuleInterfaceFile;
+
+    if (!(isSwiftmodule || isSwiftinterface))
+      continue;
+
+    auto dep = pathToModuleDecl.find(depPath);
+    if (dep != pathToModuleDecl.end()) {
+      // Great, we recognize the path! Check if the file is still around.
+
+      ModuleDecl *depMod = dep->second;
+      if(depMod->isResilient() && !isSwiftinterface) {
+        SmallString<256> moduleAdjacentInterfacePath(depPath);
+        computeAdjacentInterfacePath(moduleAdjacentInterfacePath);
+        // FIXME: The behavior of fs::exists for relative paths is undocumented.
+        // Use something else instead?
+        if (!fs::exists(moduleAdjacentInterfacePath)) {
+          err << "The module " << depMod->getName().str() << " has library"
+              << " evolution enabled but we're recording a non-adjacent"
+              << " swiftmodule at\n" << depPath << "\nin the trace.";
+          llvm::report_fatal_error(err.str());
+        }
+        buffer.clear();
+      }
+
+      // FIXME: Better error handling
+      StringRef realDepPath
+        = fs::real_path(depPath, buffer, /*expand_tile*/true)
+        ? StringRef(depPath) // Couldn't find the canonical path, assume
+                             // this is good enough.
+        : buffer.str();
+
+      traceInfo.push_back({
+        /*Name=*/
+        depMod->getName(),
+        /*Path=*/
+        realDepPath,
+        // TODO: There is an edge case which is not handled here.
+        // When we build a framework using -import-underlying-module, or an
+        // app/test using -import-objc-header, we should look at the direct
+        // imports of the bridging modules, and mark those as our direct
+        // imports.
+        /*IsImportedDirectly=*/
+        importedModules.find(depMod) != importedModules.end(),
+        /*SupportsLibraryEvolution=*/
+        depMod->isResilient()
+      });
+      buffer.clear();
+
+      continue;
+    }
+
+    // If the depTracker had an interface, that means that we must've
+    // built a swiftmodule from that interface, so we should have that
+    // filename available.
+    if (isSwiftinterface) {
+      err << "Unexpected path for swiftinterface file:\n" << depPath << "\n";
+      errorUnexpectedPath(err);
+    }
+
+    // Skip cached modules in the prebuilt cache. We will add the corresponding
+    // swiftinterface from the SDK directly, but this isn't checked. :-/
+    //
+    // FIXME: This is incorrect if both paths are not relative w.r.t. to the
+    // same root.
+    if (StringRef(depPath).startswith(prebuiltCachePath))
+      continue;
+
+    // If we have a swiftmodule next to an interface, that interface path will
+    // be saved (not checked), so don't save the path to this swiftmodule.
+    SmallString<256> moduleAdjacentInterfacePath(depPath);
+    computeAdjacentInterfacePath(moduleAdjacentInterfacePath);
+    if (pathToModuleDecl.find(moduleAdjacentInterfacePath)
+        != pathToModuleDecl.end())
+      continue;
+
+    // FIXME: The behavior of fs::exists for relative paths is undocumented.
+    // Use something else instead?
+    if (fs::exists(moduleAdjacentInterfacePath)) {
+      err << "Found swiftinterface at\n" << moduleAdjacentInterfacePath
+          << "\nbut it was not recorded\n";
+      errorUnexpectedPath(err);
+    }
+    buffer.clear();
+
+    err << "Don't know how to handle the dependency at:\n" << depPath
+        << "\nfor module trace emission.\n";
+    errorUnexpectedPath(err);
+  }
+
+  // Almost a re-implementation of reversePathSortedFilenames :(.
+  std::sort(
+    traceInfo.begin(), traceInfo.end(),
+    [](const SwiftModuleTraceInfo &m1, const SwiftModuleTraceInfo &m2) -> bool {
+      return std::lexicographical_compare(
+        m1.Path.rbegin(), m1.Path.rend(),
+        m2.Path.rbegin(), m2.Path.rend());
+  });
+}
+
 static bool emitLoadedModuleTraceIfNeeded(ModuleDecl *mainModule,
                                           DependencyTracker *depTracker,
+                                          StringRef prebuiltCachePath,
                                           StringRef loadedModuleTracePath) {
   ASTContext &ctxt = mainModule->getASTContext();
   assert(!ctxt.hadError()
-         && "We may not be able to emit a proper trace if there was an error.");
+         && "We should've already exited earlier if there was an error.");
 
   if (loadedModuleTracePath.empty())
     return false;
@@ -269,67 +422,22 @@ static bool emitLoadedModuleTraceIfNeeded(ModuleDecl *mainModule,
   llvm::DenseMap<StringRef, ModuleDecl *> pathToModuleDecl;
   for (auto &module : ctxt.LoadedModules) {
     ModuleDecl *loadedDecl = module.second;
-    assert(loadedDecl && "Expected loaded module to be non-null.");
+    if (!loadedDecl)
+      llvm::report_fatal_error("Expected loaded modules to be non-null.");
     if (loadedDecl == mainModule)
       continue;
-    assert(!loadedDecl->getModuleFilename().empty()
-           && ("Don't know how to handle modules with empty names."
-               " One potential reason for getting an empty module name might"
-               " be that the module could not be deserialized correctly."));
+    if (loadedDecl->getModuleFilename().empty())
+      llvm::report_fatal_error(
+        "Don't know how to handle modules with empty names."
+        " One potential reason for getting an empty module name might"
+        " be that the module could not be deserialized correctly.");
     pathToModuleDecl.insert(
       std::make_pair(loadedDecl->getModuleFilename(), loadedDecl));
   }
 
   std::vector<SwiftModuleTraceInfo> swiftModules;
-  SmallString<256> buffer;
-  for (auto &depPath : depTracker->getDependencies()) {
-    StringRef realDepPath;
-    // FIXME: appropriate error handling
-    if (llvm::sys::fs::real_path(depPath, buffer,/*expand_tilde=*/true))
-      // Couldn't find the canonical path, so let's just assume the old one was
-      // canonical (enough).
-      realDepPath = depPath;
-    else
-      realDepPath = buffer.str();
-
-    // Decide if this is a swiftmodule based on the extension of the raw
-    // dependency path, as the true file may have a different one.
-    // For example, this might happen when the canonicalized path points to
-    // a Content Addressed Storage (CAS) location.
-    auto moduleFileType =
-      file_types::lookupTypeForExtension(llvm::sys::path::extension(depPath));
-    if (moduleFileType == file_types::TY_SwiftModuleFile
-        || moduleFileType == file_types::TY_SwiftModuleInterfaceFile) {
-      auto dep = pathToModuleDecl.find(depPath);
-      assert(dep != pathToModuleDecl.end()
-             && "Dependency must've been loaded.");
-      ModuleDecl *depMod = dep->second;
-      swiftModules.push_back({
-        /*Name=*/
-        depMod->getName(),
-        /*Path=*/
-        realDepPath,
-        // TODO: There is an edge case which is not handled here.
-        // When we build a framework using -import-underlying-module, or an
-        // app/test using -import-objc-header, we should look at the direct
-        // imports of the bridging modules, and mark those as our direct
-        // imports.
-        /*IsImportedDirectly=*/
-        importedModules.find(depMod) != importedModules.end(),
-        /*SupportsLibraryEvolution=*/
-        depMod->isResilient()
-      });
-    }
-  }
-
-  // Almost a re-implementation of reversePathSortedFilenames :(.
-  std::sort(
-    swiftModules.begin(), swiftModules.end(),
-    [](const SwiftModuleTraceInfo &m1, const SwiftModuleTraceInfo &m2) -> bool {
-      return std::lexicographical_compare(
-        m1.Path.rbegin(), m1.Path.rend(),
-        m2.Path.rbegin(), m2.Path.rend());
-  });
+  computeSwiftModuleTraceInfo(importedModules, pathToModuleDecl, *depTracker,
+                              prebuiltCachePath, swiftModules);
 
   LoadedModuleTraceFormat trace = {
       /*version=*/LoadedModuleTraceFormat::CurrentVersion,
@@ -339,8 +447,7 @@ static bool emitLoadedModuleTraceIfNeeded(ModuleDecl *mainModule,
   };
 
   // raw_fd_ostream is unbuffered, and we may have multiple processes writing,
-  // so first write the whole thing into memory and dump out that buffer to the
-  // file.
+  // so first write to memory and then dump the buffer to the trace file.
   std::string stringBuffer;
   {
     llvm::raw_string_ostream memoryBuffer(stringBuffer);
@@ -349,7 +456,6 @@ static bool emitLoadedModuleTraceIfNeeded(ModuleDecl *mainModule,
     json::jsonize(jsonOutput, trace, /*Required=*/true);
   }
   stringBuffer += "\n";
-
   out << stringBuffer;
 
   return true;
@@ -362,7 +468,8 @@ emitLoadedModuleTraceForAllPrimariesIfNeeded(ModuleDecl *mainModule,
   return opts.InputsAndOutputs.forEachInputProducingSupplementaryOutput(
       [&](const InputFile &input) -> bool {
         return emitLoadedModuleTraceIfNeeded(
-            mainModule, depTracker, input.loadedModuleTracePath());
+          mainModule, depTracker, opts.PrebuiltModuleCachePath,
+          input.loadedModuleTracePath());
       });
 }
 

lib/Serialization/ModuleFile.h

@@ -639,7 +639,8 @@ class ModuleFile
   /// \returns Whether the module was successfully loaded, or what went wrong
   ///          if it was not.
   static serialization::ValidationInfo
-  load(std::unique_ptr<llvm::MemoryBuffer> moduleInputBuffer,
+  load(StringRef moduleInterfacePath,
+       std::unique_ptr<llvm::MemoryBuffer> moduleInputBuffer,
        std::unique_ptr<llvm::MemoryBuffer> moduleDocInputBuffer,
        std::unique_ptr<llvm::MemoryBuffer> moduleSourceInfoInputBuffer,
        bool isFramework, std::unique_ptr<ModuleFile> &theModule,
@@ -649,6 +650,8 @@ class ModuleFile
                                    std::move(moduleDocInputBuffer),
                                    std::move(moduleSourceInfoInputBuffer),
                                    isFramework, info, extInfo));
+    if (!moduleInterfacePath.empty())
+      theModule->ModuleInterfacePath = moduleInterfacePath;
     return info;
   }