Reporting refactoring

David Ungar · David Ungar · commit ae64f471cd07 · 2019-11-19T16:42:21.000-08:00
diff --git a/include/swift/Driver/Compilation.h b/include/swift/Driver/Compilation.h
@@ -79,6 +79,7 @@ using CommandSet = llvm::SmallPtrSet<const Job *, 16>;
 class Compilation {
 public:
   class IncrementalSchemeComparator {
+    const bool &EnableIncrementalBuild;
     const bool EnableSourceRangeDependencies;
     const bool &UseSourceRangeDependencies;
 
@@ -87,19 +88,25 @@ class Compilation {
 
     const unsigned SwiftInputCount;
 
+  public:
+    std::string WhyIncrementalWasDisabled = "";
+
+  private:
     DiagnosticEngine &Diags;
 
     CommandSet DependencyCompileJobs;
     CommandSet SourceRangeCompileJobs;
     CommandSet SourceRangeLackingSuppJobs;
 
   public:
-    IncrementalSchemeComparator(bool EnableSourceRangeDependencies,
+    IncrementalSchemeComparator(const bool &EnableIncrementalBuild,
+                                bool EnableSourceRangeDependencies,
                                 const bool &UseSourceRangeDependencies,
                                 const StringRef CompareIncrementalSchemesPath,
                                 unsigned SwiftInputCount,
                                 DiagnosticEngine &Diags)
-        : EnableSourceRangeDependencies(EnableSourceRangeDependencies),
+        : EnableIncrementalBuild(EnableIncrementalBuild),
+          EnableSourceRangeDependencies(EnableSourceRangeDependencies),
           UseSourceRangeDependencies(UseSourceRangeDependencies),
           CompareIncrementalSchemesPath(CompareIncrementalSchemesPath),
           SwiftInputCount(SwiftInputCount), Diags(Diags) {}
@@ -374,9 +381,7 @@ class Compilation {
   bool getIncrementalBuildEnabled() const {
     return EnableIncrementalBuild;
   }
-  void disableIncrementalBuild() {
-    EnableIncrementalBuild = false;
-  }
+  void disableIncrementalBuild(Twine why);
 
   bool getEnableExperimentalDependencies() const {
     return EnableExperimentalDependencies;
diff --git a/lib/Driver/Compilation.cpp b/lib/Driver/Compilation.cpp
@@ -159,7 +159,10 @@ Compilation::Compilation(DiagnosticEngine &Diags,
     EnableSourceRangeDependencies(EnableSourceRangeDependencies) {
     if (CompareIncrementalSchemes)
       IncrementalComparator.emplace(
-      EnableSourceRangeDependencies, UseSourceRangeDependencies,
+      // Ensure the references are to inst vars, NOT arguments
+      this->EnableIncrementalBuild,
+      EnableSourceRangeDependencies,
+      this->UseSourceRangeDependencies,
       CompareIncrementalSchemesPath, countSwiftInputs(), getDiags());
 };
 // clang-format on
@@ -418,10 +421,9 @@ namespace driver {
         Comp.getDiags().diagnose(SourceLoc(),
                                  diag::warn_unable_to_load_dependencies,
                                  DependenciesFile);
-      Comp.disableIncrementalBuild();
-      if (Comp.getShowIncrementalBuildDecisions())
-         llvm::outs() << "Incremental compilation has been disabled due to "
-                      << "malformed swift dependencies file '" << DependenciesFile << "'.\n";
+      Comp.disableIncrementalBuild(
+          Twine("Malformed swift dependencies file ' ") + DependenciesFile +
+          "'");
     }
 
     /// Helper that attempts to reload a job's .swiftdeps file after the job
@@ -1955,6 +1957,15 @@ const char *Compilation::getAllSourcesPath() const {
   return AllSourceFilesPath;
 }
 
+void Compilation::disableIncrementalBuild(Twine why) {
+  if (getShowIncrementalBuildDecisions())
+    llvm::outs() << "Disabling incremental build: " << why << "\n";
+
+  EnableIncrementalBuild = false;
+  if (IncrementalComparator)
+    IncrementalComparator->WhyIncrementalWasDisabled = why.str();
+}
+
 void Compilation::IncrementalSchemeComparator::update(
     const CommandSet &depJobs,
     const CommandSet &rangeJobs,
@@ -1988,15 +1999,34 @@ void Compilation::IncrementalSchemeComparator::outputComparison() const {
 
 void Compilation::IncrementalSchemeComparator::outputComparison(
     llvm::raw_ostream &out) const {
+  if (!EnableIncrementalBuild) {
+    // No stats will have been gathered
+    assert(!WhyIncrementalWasDisabled.empty() && "Must be a reason");
+    out << "*** Incremental build disabled because "
+        << WhyIncrementalWasDisabled << ", cannot compare ***\n";
+    return;
+  }
+  unsigned additionalDependencyJobsToCreateSupps = 0;
+  for (const Job *Cmd : SourceRangeLackingSuppJobs) {
+    if (!DependencyCompileJobs.count(Cmd))
+      ++additionalDependencyJobsToCreateSupps;
+  }
+  unsigned jobsWhenEnablingDeps = DependencyCompileJobs.size();
+  unsigned jobsWhenEnablingRanges =
+      UseSourceRangeDependencies ? SourceRangeCompileJobs.size()
+                                 : DependencyCompileJobs.size() +
+                                       additionalDependencyJobsToCreateSupps;
+
   out << "*** Comparing incremental schemes: "
-      << "deps: " << DependencyCompileJobs.size() << ", "
-      << "ranges: " << SourceRangeCompileJobs.size() << ", "
-      << "supplementary output creation: " << SourceRangeLackingSuppJobs.size()
-      << ", "
+      << "deps: " << jobsWhenEnablingDeps << ", "
+      << "ranges: " << jobsWhenEnablingRanges << ", "
       << "total: " << SwiftInputCount << ", "
-      << "requested: "
-      << (EnableSourceRangeDependencies ? "ranges" : "deps") << ", "
-      << "using: " << (UseSourceRangeDependencies ? "ranges" : "deps")
+      << "requested: " << (EnableSourceRangeDependencies ? "ranges" : "deps")
+      << ", "
+      << "using: "
+      << (!EnableIncrementalBuild
+              ? "neither"
+              : UseSourceRangeDependencies ? "ranges" : "deps")
       << "\n";
 }
 
diff --git a/lib/Driver/Driver.cpp b/lib/Driver/Driver.cpp
@@ -375,18 +375,19 @@ static bool getCompilationRecordPath(std::string &buildRecordPath,
   return false;
 }
 
-static bool failedToReadOutOfDateMap(bool ShowIncrementalBuildDecisions,
-                                     StringRef buildRecordPath,
-                                     StringRef reason = "") {
+static StringRef failedToReadOutOfDateMap(bool ShowIncrementalBuildDecisions,
+                                          StringRef buildRecordPath,
+                                          StringRef reason = "") {
+  std::string why = "malformed build record file";
+  if (!reason.empty()) {
+    why += " ";
+    why += reason;
+  }
   if (ShowIncrementalBuildDecisions) {
-    llvm::outs() << "Incremental compilation has been disabled due to "
-                 << "malformed build record file '" << buildRecordPath << "'.";
-    if (!reason.empty()) {
-      llvm::outs() << " " << reason;
-    }
-    llvm::outs() << "\n";
+    llvm::outs() << "Incremental compilation has been disabled due to " << why
+                 << " '" << buildRecordPath << "'.\n";
   }
-  return true;
+  return why;
 }
 
 static SmallVector<StringRef, 8> findRemovedInputs(
@@ -396,20 +397,19 @@ static SmallVector<StringRef, 8> findRemovedInputs(
 static void dealWithRemovedInputs(ArrayRef<StringRef> removedInputs,
                                   bool ShowIncrementalBuildDecisions);
 
-/// Returns true on error.
-static bool populateOutOfDateMap(InputInfoMap &map,
-                                 llvm::sys::TimePoint<> &LastBuildTime,
-                                 StringRef argsHashStr,
-                                 const InputFileList &inputs,
-                                 StringRef buildRecordPath,
-                                 const bool EnableSourceRangeDependencies,
-                                 const bool ShowIncrementalBuildDecisions) {
+/// Returns why ignore incrementality
+static StringRef
+populateOutOfDateMap(InputInfoMap &map, llvm::sys::TimePoint<> &LastBuildTime,
+                     StringRef argsHashStr, const InputFileList &inputs,
+                     StringRef buildRecordPath,
+                     const bool EnableSourceRangeDependencies,
+                     const bool ShowIncrementalBuildDecisions) {
   // Treat a missing file as "no previous build".
   auto buffer = llvm::MemoryBuffer::getFile(buildRecordPath);
   if (!buffer) {
     if (ShowIncrementalBuildDecisions)
       llvm::outs() << "Incremental compilation could not read build record.\n";
-    return false;
+    return "could not read build record";
   }
 
   namespace yaml = llvm::yaml;
@@ -499,7 +499,7 @@ static bool populateOutOfDateMap(InputInfoMap &map,
     } else if (keyStr == compilation_record::getName(TopLevelKey::Options)) {
       auto *value = dyn_cast<yaml::ScalarNode>(i->getValue());
       if (!value)
-        return true;
+        return "no name node in build record";
       optionsMatch = (argsHashStr == value->getValue(scratch));
 
     } else if (keyStr == compilation_record::getName(TopLevelKey::BuildTime)) {
@@ -512,7 +512,7 @@ static bool populateOutOfDateMap(InputInfoMap &map,
       }
       llvm::sys::TimePoint<> timeVal;
       if (readTimeValue(i->getValue(), timeVal))
-        return true;
+        return "could not read time value in build record";
       LastBuildTime = timeVal;
 
     } else if (keyStr == compilation_record::getName(TopLevelKey::Inputs)) {
@@ -529,21 +529,21 @@ static bool populateOutOfDateMap(InputInfoMap &map,
       for (auto i = inputMap->begin(), e = inputMap->end(); i != e; ++i) {
         auto *key = dyn_cast<yaml::ScalarNode>(i->getKey());
         if (!key)
-          return true;
+          return "no input entry in build record";
 
         auto *value = dyn_cast<yaml::SequenceNode>(i->getValue());
         if (!value)
-          return true;
+          return "no sequence node for input entry in build record";
 
         using compilation_record::getInfoStatusForIdentifier;
         auto previousBuildState =
           getInfoStatusForIdentifier(value->getRawTag());
         if (!previousBuildState)
-          return true;
+          return "no previous build state in build record";
 
         llvm::sys::TimePoint<> timeValue;
         if (readTimeValue(value, timeValue))
-          return true;
+          return "could not read time value in build record";
 
         auto inputName = key->getValue(scratch);
         previousInputs[inputName] = { *previousBuildState, timeValue };
@@ -561,15 +561,15 @@ static bool populateOutOfDateMap(InputInfoMap &map,
                    << "\tPreviously compiled with: "
                    << CompilationRecordSwiftVersion << "\n";
     }
-    return true;
+    return "compiler version mismatch";
   }
 
   if (!optionsMatch) {
     if (ShowIncrementalBuildDecisions) {
       llvm::outs() << "Incremental compilation has been disabled, because "
                    << "different arguments were passed to the compiler.\n";
     }
-    return true;
+    return "different arguments passed to compiler";
   }
 
   unsigned numMatchingPreviouslyCompiledInputs = 0;
@@ -586,13 +586,14 @@ static bool populateOutOfDateMap(InputInfoMap &map,
   auto const wereAnyInputsRemoved =
       numMatchingPreviouslyCompiledInputs < previousInputs.size();
   if (!wereAnyInputsRemoved)
-    return false;
+    return "";
 
   const auto removedInputs = findRemovedInputs(inputs, previousInputs);
   assert(!removedInputs.empty());
 
   dealWithRemovedInputs(removedInputs, ShowIncrementalBuildDecisions);
-  return true; // recompile everything; could do better someday
+  return "an input was removed"; // recompile everything; could do better
+                                 // someday
 }
 
 static SmallVector<StringRef, 8> findRemovedInputs(
@@ -892,16 +893,16 @@ Driver::buildCompilation(const ToolChain &TC,
   computeArgsHash(ArgsHash, *TranslatedArgList);
   llvm::sys::TimePoint<> LastBuildTime = llvm::sys::TimePoint<>::min();
   InputInfoMap outOfDateMap;
-  bool rebuildEverything = true;
-  if (Incremental && !buildRecordPath.empty()) {
-    if (populateOutOfDateMap(outOfDateMap, LastBuildTime, ArgsHash, Inputs,
-                             buildRecordPath, EnableSourceRangeDependencies,
-                             ShowIncrementalBuildDecisions)) {
-      // FIXME: Distinguish errors from "file removed", which is benign.
-    } else {
-      rebuildEverything = false;
-    }
-  }
+  StringRef whyIgnoreIncrementallity =
+      !Incremental
+          ? ""
+          : buildRecordPath.empty()
+                ? "no build record path"
+                : populateOutOfDateMap(outOfDateMap, LastBuildTime, ArgsHash,
+                                       Inputs, buildRecordPath,
+                                       EnableSourceRangeDependencies,
+                                       ShowIncrementalBuildDecisions);
+  // FIXME: Distinguish errors from "file removed", which is benign.
 
   size_t DriverFilelistThreshold;
   if (getFilelistThreshold(*TranslatedArgList, DriverFilelistThreshold, Diags))
@@ -994,7 +995,7 @@ Driver::buildCompilation(const ToolChain &TC,
   // Construct the graph of Actions.
   SmallVector<const Action *, 8> TopLevelActions;
   buildActions(TopLevelActions, TC, OI,
-               rebuildEverything ? nullptr : &outOfDateMap, *C);
+               whyIgnoreIncrementallity.empty() ? &outOfDateMap : nullptr, *C);
 
   if (Diags.hadAnyError())
     return nullptr;
@@ -1025,8 +1026,8 @@ Driver::buildCompilation(const ToolChain &TC,
 
   // This has to happen after building jobs, because otherwise we won't even
   // emit .swiftdeps files for the next build.
-  if (rebuildEverything)
-    C->disableIncrementalBuild();
+  if (!whyIgnoreIncrementallity.empty())
+    C->disableIncrementalBuild(whyIgnoreIncrementallity.str());
 
   if (Diags.hadAnyError())
     return nullptr;
diff --git a/test/Driver/Dependencies/range-lifecycle.swift b/test/Driver/Dependencies/range-lifecycle.swift
