Merge pull request #8359 from graydon/rdar-31044067-missing-dependencies-mark-2

Author: swift-ci

include/swift/AST/ModuleLoader.h

@@ -24,38 +24,41 @@
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/TinyPtrVector.h"
 
+namespace clang {
+class DependencyCollector;
+}
+
 namespace swift {
 
 class AbstractFunctionDecl;
+class ClangImporterOptions;
 class ClassDecl;
 class ModuleDecl;
 class NominalTypeDecl;
 
 enum class KnownProtocolKind : uint8_t;
 
-/// Records dependencies on files outside of the current module.
+/// Records dependencies on files outside of the current module;
+/// implemented in terms of a wrapped clang::DependencyCollector.
 class DependencyTracker {
-  llvm::SetVector<std::string, std::vector<std::string>,
-                  llvm::SmallSet<std::string, 16>> paths;
-
+  std::shared_ptr<clang::DependencyCollector> clangCollector;
 public:
+
+  DependencyTracker();
+
   /// Adds a file as a dependency.
   ///
-  /// The contents of \p file are taken literally, and should be appropriate
+  /// The contents of \p File are taken literally, and should be appropriate
   /// for appearing in a list of dependencies suitable for tooling like Make.
   /// No path canonicalization is done.
-  void addDependency(StringRef file) {
-    paths.insert(file);
-  }
+  void addDependency(StringRef File, bool IsSystem);
 
   /// Fetches the list of dependencies.
-  ArrayRef<std::string> getDependencies() const {
-    if (paths.empty())
-      return None;
-    assert((&paths[0]) + (paths.size() - 1) == &paths.back() &&
-           "elements not stored contiguously");
-    return llvm::makeArrayRef(&paths[0], paths.size());
-  }
+  ArrayRef<std::string> getDependencies() const;
+
+  /// Return the underlying clang::DependencyCollector that this
+  /// class wraps.
+  std::shared_ptr<clang::DependencyCollector> getClangCollector();
 };
 
 /// \brief Abstract interface that loads named modules into the AST.
@@ -66,9 +69,9 @@ class ModuleLoader {
 protected:
   ModuleLoader(DependencyTracker *tracker) : dependencyTracker(tracker) {}
 
-  void addDependency(StringRef file) {
+  void addDependency(StringRef file, bool IsSystem=false) {
     if (dependencyTracker)
-      dependencyTracker->addDependency(file);
+      dependencyTracker->addDependency(file, IsSystem);
   }
 
 public:

include/swift/ClangImporter/ClangImporter.h

@@ -30,6 +30,7 @@ namespace clang {
   class ASTContext;
   class CodeGenOptions;
   class Decl;
+  class DependencyCollector;
   class EnumConstantDecl;
   class EnumDecl;
   class MacroInfo;
@@ -94,6 +95,11 @@ class ClangImporter final : public ClangModuleLoader {
 
   ~ClangImporter();
 
+  /// \brief Create a new clang::DependencyCollector customized to
+  /// ClangImporter's specific uses.
+  static std::shared_ptr<clang::DependencyCollector>
+  createDependencyCollector();
+
   /// \brief Check whether the module with a given name can be imported without
   /// importing it.
   ///

lib/AST/CMakeLists.txt

@@ -34,6 +34,7 @@ add_swift_library(swiftAST STATIC
   LayoutConstraint.cpp
   LookupVisibleDecls.cpp
   Module.cpp
+  ModuleLoader.cpp
   ModuleNameLookup.cpp
   NameLookup.cpp
   Parameter.cpp

lib/AST/ModuleLoader.cpp

@@ -0,0 +1,55 @@
+//===--- ModuleLoader.cpp - Swift Language Module Implementation ----------===//
+//
+// This source file is part of the Swift.org open source project
+//
+// Copyright (c) 2017 Apple Inc. and the Swift project authors
+// Licensed under Apache License v2.0 with Runtime Library Exception
+//
+// See https://swift.org/LICENSE.txt for license information
+// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
+//
+//===----------------------------------------------------------------------===//
+//
+//  This file implements the ModuleLoader class and/or any helpers.
+//
+//===----------------------------------------------------------------------===//
+
+#include "swift/AST/ModuleLoader.h"
+#include "clang/Frontend/Utils.h"
+#include "swift/ClangImporter/ClangImporter.h"
+
+namespace swift {
+
+DependencyTracker::DependencyTracker()
+  // NB: The ClangImporter believes it's responsible for the construction of
+  // this instance, and it static_cast<>s the instance pointer to its own
+  // subclass based on that belief. If you change this to be some other
+  // instance, you will need to change ClangImporter's code to handle the
+  // difference.
+  : clangCollector(ClangImporter::createDependencyCollector())
+{
+}
+
+void
+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, though it is presently always false, we accept it and pass it
+  // along to the clang DependencyCollector in case Swift callers start setting
+  // it to true someday.
+  clangCollector->maybeAddDependency(File, /*FromClangModule=*/false,
+                                     IsSystem, /*IsClangModuleFile=*/false,
+                                     /*IsMissing=*/false);
+}
+
+ArrayRef<std::string>
+DependencyTracker::getDependencies() const {
+  return clangCollector->getDependencies();
+}
+
+std::shared_ptr<clang::DependencyCollector>
+DependencyTracker::getClangCollector() {
+  return clangCollector;
+}
+
+}

lib/ClangImporter/ClangImporter.cpp

@@ -74,12 +74,10 @@ using clang::CompilerInvocation;
 
 namespace {
   class HeaderImportCallbacks : public clang::PPCallbacks {
-    ClangImporter &Importer;
     ClangImporter::Implementation &Impl;
   public:
-    HeaderImportCallbacks(ClangImporter &importer,
-                          ClangImporter::Implementation &impl)
-      : Importer(importer), Impl(impl) {}
+    HeaderImportCallbacks(ClangImporter::Implementation &impl)
+      : Impl(impl) {}
 
     void handleImport(const clang::Module *imported) {
       if (!imported)
@@ -97,8 +95,6 @@ namespace {
                                     StringRef RelativePath,
                                     const clang::Module *Imported) override {
       handleImport(Imported);
-      if (!Imported && File)
-        Importer.addDependency(File->getName());
     }
 
     void moduleImport(clang::SourceLocation ImportLoc,
@@ -108,21 +104,6 @@ namespace {
     }
   };
 
-  class ASTReaderCallbacks : public clang::ASTReaderListener {
-    ClangImporter &Importer;
-  public:
-    explicit ASTReaderCallbacks(ClangImporter &importer) : Importer(importer) {}
-
-    bool needsInputFileVisitation() override { return true; }
-
-    bool visitInputFile(StringRef file, bool isSystem,
-                        bool isOverridden, bool isExplicitModule) override {
-      if (!isOverridden)
-        Importer.addDependency(file);
-      return true;
-    }
-  };
-
   class PCHDeserializationCallbacks : public clang::ASTDeserializationListener {
     ClangImporter::Implementation &Impl;
   public:
@@ -313,8 +294,51 @@ class BridgingPPTracker : public clang::PPCallbacks {
     Impl.BridgeHeaderMacros.push_back(MacroNameTok.getIdentifierInfo());
   }
 };
+
+class ClangImporterDependencyCollector : public clang::DependencyCollector
+{
+  llvm::StringSet<> ExcludedPaths;
+public:
+  ClangImporterDependencyCollector() = default;
+
+  void excludePath(StringRef filename) {
+    ExcludedPaths.insert(filename);
+  }
+
+  bool isClangImporterSpecialName(StringRef Filename) {
+    using ImporterImpl = ClangImporter::Implementation;
+    return (Filename == ImporterImpl::moduleImportBufferName
+            || Filename == ImporterImpl::bridgingHeaderBufferName);
+  }
+
+  // Currently preserving older ClangImporter behaviour of ignoring system
+  // dependencies, but possibly revisit?
+  bool needSystemDependencies() override { return false; }
+
+  bool sawDependency(StringRef Filename, bool FromClangModule,
+                     bool IsSystem, bool IsClangModuleFile,
+                     bool IsMissing) override {
+    if (!clang::DependencyCollector::sawDependency(Filename, FromClangModule,
+                                                   IsSystem, IsClangModuleFile,
+                                                   IsMissing))
+      return false;
+    // Currently preserving older ClangImporter behaviour of ignoring .pcm
+    // file dependencies, but possibly revisit?
+    if (IsClangModuleFile
+        || isClangImporterSpecialName(Filename)
+        || ExcludedPaths.count(Filename))
+      return false;
+    return true;
+  }
+};
 } // end anonymous namespace
 
+std::shared_ptr<clang::DependencyCollector>
+ClangImporter::createDependencyCollector()
+{
+  return std::make_shared<ClangImporterDependencyCollector>();
+}
+
 void ClangImporter::Implementation::addBridgeHeaderTopLevelDecls(
     clang::Decl *D) {
   if (shouldIgnoreBridgeHeaderTopLevelDecl(D))
@@ -708,6 +732,14 @@ ClangImporter::create(ASTContext &ctx,
   if (llvm::sys::path::extension(importerOpts.BridgingHeader).endswith(
         PCH_EXTENSION)) {
     importer->Impl.IsReadingBridgingPCH = true;
+    if (tracker) {
+      // Currently ignoring dependency on bridging .pch files because they are
+      // temporaries; if and when they are no longer temporaries, this condition
+      // should be removed.
+      auto &coll = static_cast<ClangImporterDependencyCollector &>(
+        *tracker->getClangCollector());
+      coll.excludePath(importerOpts.BridgingHeader);
+    }
   }
 
   // FIXME: These can't be controlled from the command line.
@@ -764,6 +796,9 @@ ClangImporter::create(ASTContext &ctx,
       llvm::make_unique<clang::ObjectFilePCHContainerReader>());
   importer->Impl.Instance.reset(new CompilerInstance(PCHContainerOperations));
   auto &instance = *importer->Impl.Instance;
+  if (tracker)
+    instance.addDependencyCollector(tracker->getClangCollector());
+
   instance.setDiagnostics(&*clangDiags);
   instance.setInvocation(importer->Impl.Invocation);
 
@@ -808,9 +843,6 @@ ClangImporter::create(ASTContext &ctx,
   clangPP.addPPCallbacks(std::move(ppTracker));
 
   instance.createModuleManager();
-  instance.getModuleManager()->addListener(
-         std::unique_ptr<clang::ASTReaderListener>(
-                 new ASTReaderCallbacks(*importer)));
 
   // Manually run the action, so that the TU stays open for additional parsing.
   instance.createSema(action->getTranslationUnitKind(), nullptr);
@@ -840,7 +872,7 @@ ClangImporter::create(ASTContext &ctx,
   }
 
   // FIXME: This is missing implicit includes.
-  auto *CB = new HeaderImportCallbacks(*importer, importer->Impl);
+  auto *CB = new HeaderImportCallbacks(importer->Impl);
   clangPP.addPPCallbacks(std::unique_ptr<clang::PPCallbacks>(CB));
 
   // Create the selectors we'll be looking for.

lib/FrontendTool/FrontendTool.cpp

@@ -117,10 +117,11 @@ static bool emitMakeDependencies(DiagnosticEngine &diags,
     out << escape(targetName) << " :";
     // First include all other files in the module. Make-style dependencies
     // need to be conservative!
-    for (StringRef path : opts.InputFilenames)
+    for (auto const &path : reversePathSortedFilenames(opts.InputFilenames))
       out << ' ' << escape(path);
     // Then print dependencies we've picked up during compilation.
-    for (StringRef path : depTracker.getDependencies())
+    for (auto const &path :
+           reversePathSortedFilenames(depTracker.getDependencies()))
       out << ' ' << escape(path);
     out << '\n';
   });

lib/FrontendTool/ReferenceDependencies.cpp

@@ -102,6 +102,17 @@ static std::string mangleTypeAsContext(const NominalTypeDecl *type) {
   return Mangler.mangleTypeAsContextUSR(type);
 }
 
+std::vector<std::string>
+swift::reversePathSortedFilenames(const ArrayRef<std::string> elts) {
+  std::vector<std::string> tmp(elts.begin(), elts.end());
+  std::sort(tmp.begin(), tmp.end(), [](const std::string &a,
+                                       const std::string &b) -> bool {
+              return std::lexicographical_compare(a.rbegin(), a.rend(),
+                                                  b.rbegin(), b.rend());
+            });
+  return tmp;
+}
+
 bool swift::emitReferenceDependencies(DiagnosticEngine &diags,
                                       SourceFile *SF,
                                       DependencyTracker &depTracker,
@@ -391,7 +402,7 @@ bool swift::emitReferenceDependencies(DiagnosticEngine &diags,
   }
 
   out << "depends-external:\n";
-  for (auto &entry : depTracker.getDependencies()) {
+  for (auto &entry : reversePathSortedFilenames(depTracker.getDependencies())) {
     out << "- \"" << llvm::yaml::escape(entry) << "\"\n";
   }
 

lib/FrontendTool/ReferenceDependencies.h

@@ -13,13 +13,23 @@
 #ifndef SWIFT_FRONTENDTOOL_REFERENCEDEPENDENCIES_H
 #define SWIFT_FRONTENDTOOL_REFERENCEDEPENDENCIES_H
 
+#include "llvm/ADT/ArrayRef.h"
+#include <vector>
+#include <string>
+
 namespace swift {
 
 class DependencyTracker;
 class DiagnosticEngine;
 class FrontendOptions;
 class SourceFile;
 
+/// Sort a set of paths in an order that's (comparatively) stable against
+/// variation in filesystem prefix: lexicographic comparison of the
+/// byte-reversals of each path. Used for emitting external dependencies.
+std::vector<std::string>
+reversePathSortedFilenames(const llvm::ArrayRef<std::string> paths);
+
 /// Emit a Swift-style dependencies file for \p SF.
 bool emitReferenceDependencies(DiagnosticEngine &diags,
                                SourceFile *SF,

test/ClangImporter/pch-bridging-header-deps.swift

@@ -0,0 +1,21 @@
+// REQUIRES: objc_interop
+// RUN: rm -f %t.*
+//
+// Generate a bridging PCH, use it in a swift file, and check that the swift file's .swiftdeps
+// mention the .h the PCH was generated from, and any .h files included in it.
+//
+// RUN: %target-swift-frontend -emit-pch -o %t.pch %S/Inputs/chained-unit-test-bridging-header-to-pch.h
+// RUN: %target-swift-frontend -module-name test -c -emit-dependencies-path %t.d -emit-reference-dependencies-path %t.swiftdeps -primary-file %s %s -import-objc-header %t.pch
+// RUN: %FileCheck --check-prefix CHECK-DEPS %s < %t.d
+// RUN: %FileCheck --check-prefix CHECK-SWIFTDEPS %s < %t.swiftdeps
+// RUN: %FileCheck --check-prefix CHECK-SWIFTDEPS2 %s < %t.swiftdeps
+
+print(app_function(1))
+
+// CHECK-DEPS: pch-bridging-header-deps.o : {{.*}}SOURCE_DIR/test/ClangImporter/Inputs/app-bridging-header-to-pch.h {{.*}}SOURCE_DIR/test/ClangImporter/Inputs/chained-unit-test-bridging-header-to-pch.h
+
+// CHECK-SWIFTDEPS: depends-external:
+// CHECK-SWIFTDEPS: - "SOURCE_DIR/test/ClangImporter/Inputs/app-bridging-header-to-pch.h"
+// CHECK-SWIFTDEPS: - "SOURCE_DIR/test/ClangImporter/Inputs/chained-unit-test-bridging-header-to-pch.h"
+
+// CHECK-SWIFTDEPS2-NOT: {{.*}}.pch

test/Driver/multi-threaded.swift

@@ -58,8 +58,8 @@
 // EXEC: ld
 // EXEC: /tmp/main{{[^ ]*}}.o /tmp/multi-threaded{{[^ ]*}}.o
 
-// DEPENDENCIES-DAG: {{.*}}/multi-threaded.o : {{.*}}/Inputs/main.swift {{.*}}/multi-threaded.swift
-// DEPENDENCIES-DAG: {{.*}}/main.o : {{.*}}/Inputs/main.swift {{.*}}/multi-threaded.swift
+// DEPENDENCIES-DAG: {{.*}}/multi-threaded.o : {{.*}}/multi-threaded.swift {{.*}}/Inputs/main.swift
+// DEPENDENCIES-DAG: {{.*}}/main.o : {{.*}}/multi-threaded.swift {{.*}}/Inputs/main.swift
 
 // PARSEABLE2: "name": "compile"
 // PARSEABLE2: "outputs": [