[clang][deps] Prevent unintended modifications of the original TU command-line

[jansvoboda11]

Summary

One of the goals of the dependency scanner is to generate command-lines that can be used to explicitly build modular dependencies of a translation unit. The only modifications to these command-lines should be for the purposes of explicit modular build.

However, the current version of dependency scanner leaks its implementation details into the command-lines.

The first problem is that the clang-scan-deps tool adjusts the original textual command-line (adding -Eonly -M -MT <target> -sys-header-deps -Wno-error -o /dev/null , removing --serialize-diagnostics) in order to set up the DependencyScanning library. This has been addressed in D103461, D104012, D104030, D104031, D104033. With these patches, the DependencyScanning library receives the unmodified CompilerInvocation, sets it up and uses it for the implicit modular build.

Finally, to prevent leaking the implementation details to the resulting command-lines, this patch generates them from the original unmodified CompilerInvocation rather than from the one that drives the implicit build.

[dexonsmith]

LGTM, with one suggestion inline.

clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h

194

I wonder if it'd be better to take CompilerInvocation&& here. Then the caller is required to either pass std::move or make a deep copy at the call site, and it's perhaps more clear that there's a deep copy being made.

[jansvoboda11]

clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h

194

That's much clearer indeed. Thanks for the suggestion!

[thakis]

Either this or your concurrent commit broke check-clang: http://45.33.8.238/linux/48839/step_7.txt

Please take a look and revert for now if it takes a while to fix.

[jansvoboda11]

In D104036#2816602, @thakis wrote:

Either this or your concurrent commit broke check-clang: http://45.33.8.238/linux/48839/step_7.txt

Please take a look and revert for now if it takes a while to fix.

This test has been intermittently failing since yesterday: https://lab.llvm.org/buildbot/#/builders/109/builds/16653
I've notified the author here: https://reviews.llvm.org/rG673c5ba58497298a684f8b8dfddbfb11cd89950e

clang/include/clang/Tooling/DependencyScanning/ModuleDepCollector.h

 };
 
 /// Collects modular and non-modular dependencies of the main file by attaching
 /// \c ModuleDepCollectorPP to the preprocessor.
 class ModuleDepCollector final : public DependencyCollector {
 public:
   ModuleDepCollector(std::unique_ptr<DependencyOutputOptions> Opts,
                      CompilerInstance &I, DependencyConsumer &C,
-                     std::map<std::string, std::string, std::less<>>
-                         OriginalPrebuiltModuleFiles);
+                     CompilerInvocation &&OriginalCI);
 
   void attachToPreprocessor(Preprocessor &PP) override;
   void attachToASTReader(ASTReader &R) override;
 
 private:
   friend ModuleDepCollectorPP;
 
   /// The compiler instance for the current translation unit.
   CompilerInstance &Instance;
   /// The consumer of collected dependency information.
   DependencyConsumer &Consumer;
   /// Path to the main source file.
   std::string MainFile;
   /// Hash identifying the compilation conditions of the current TU.
   std::string ContextHash;
   /// Non-modular file dependencies. This includes the main source file and
   /// textually included header files.
   std::vector<std::string> FileDeps;
   /// Direct and transitive modular dependencies of the main source file.
   std::unordered_map<const Module *, ModuleDeps> ModularDeps;
   /// Options that control the dependency output generation.
   std::unique_ptr<DependencyOutputOptions> Opts;
-  /// The mapping between prebuilt module names and module files that were
-  /// present in the original CompilerInvocation.
-  std::map<std::string, std::string, std::less<>> OriginalPrebuiltModuleFiles;
+  /// The original Clang invocation passed to dependency scanner.
+  CompilerInvocation OriginalInvocation;
 
   /// Checks whether the module is known as being prebuilt.
   bool isPrebuiltModule(const Module *M);
 
   /// Constructs a CompilerInvocation that can be used to build the given
   /// module, excluding paths to discovered modular dependencies that are yet to
   /// be built.
   CompilerInvocation
   makeInvocationForModuleBuildWithoutPaths(const ModuleDeps &Deps) const;
 };
 
 } // end namespace dependencies
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_DEPENDENCY_SCANNING_MODULE_DEP_COLLECTOR_H

clang/lib/Tooling/DependencyScanning/DependencyScanningWorker.cpp

 
 private:
   std::unique_ptr<DependencyOutputOptions> Opts;
   DependencyConsumer &C;
 };
 
 /// A listener that collects the names and paths to imported modules.
 class ImportCollectingListener : public ASTReaderListener {
+  using PrebuiltModuleFilesT =
+      decltype(HeaderSearchOptions::PrebuiltModuleFiles);
+
 public:
-  ImportCollectingListener(
-      std::map<std::string, std::string> &PrebuiltModuleFiles)
+  ImportCollectingListener(PrebuiltModuleFilesT &PrebuiltModuleFiles)
       : PrebuiltModuleFiles(PrebuiltModuleFiles) {}
 
   bool needsImportVisitation() const override { return true; }
 
   void visitImport(StringRef ModuleName, StringRef Filename) override {
     PrebuiltModuleFiles[std::string(ModuleName)] = std::string(Filename);
   }
 
 private:
-  std::map<std::string, std::string> &PrebuiltModuleFiles;
+  PrebuiltModuleFilesT &PrebuiltModuleFiles;
 };
 
 /// Transform arbitrary file name into an object-like file name.
 static std::string makeObjFileName(StringRef FileName) {
   SmallString<128> ObjFileName(FileName);
   llvm::sys::path::replace_extension(ObjFileName, "o");
   return std::string(ObjFileName.str());
 }
DependencyScanningAction(
       : WorkingDirectory(WorkingDirectory), Consumer(Consumer),
         DepFS(std::move(DepFS)), PPSkipMappings(PPSkipMappings),
         Format(Format) {}
 
   bool runInvocation(std::shared_ptr<CompilerInvocation> Invocation,
                      FileManager *FileMgr,
                      std::shared_ptr<PCHContainerOperations> PCHContainerOps,
                      DiagnosticConsumer *DiagConsumer) override {
+    // Make a deep copy of the original Clang invocation.
+    CompilerInvocation OriginalInvocation(*Invocation);
+
     // Create a compiler instance to handle the actual work.
     CompilerInstance Compiler(std::move(PCHContainerOps));
     Compiler.setInvocation(std::move(Invocation));
 
     // Don't print 'X warnings and Y errors generated'.
     Compiler.getDiagnosticOpts().ShowCarets = false;
     // Don't write out diagnostic file.
     Compiler.getDiagnosticOpts().DiagnosticSerializationFile.clear();
if (DepFS) {
         Compiler.getPreprocessorOpts()
             .ExcludedConditionalDirectiveSkipMappings = PPSkipMappings;
     }
 
     FileMgr->getFileSystemOpts().WorkingDir = std::string(WorkingDirectory);
     Compiler.setFileManager(FileMgr);
     Compiler.createSourceManager(*FileMgr);
 
-    std::map<std::string, std::string> PrebuiltModuleFiles;
     if (!Compiler.getPreprocessorOpts().ImplicitPCHInclude.empty()) {
-      /// Collect the modules that were prebuilt as part of the PCH.
-      ImportCollectingListener Listener(PrebuiltModuleFiles);
+      // Collect the modules that were prebuilt as part of the PCH and pass them
+      // to the compiler. This will prevent the implicit build to create
+      // duplicate modules and force reuse of existing prebuilt module files
+      // instead.
+      ImportCollectingListener Listener(
+          Compiler.getHeaderSearchOpts().PrebuiltModuleFiles);
       ASTReader::readASTFileControlBlock(
           Compiler.getPreprocessorOpts().ImplicitPCHInclude,
           Compiler.getFileManager(), Compiler.getPCHContainerReader(),
           /*FindModuleFileExtensions=*/false, Listener,
           /*ValidateDiagnosticOptions=*/false);
     }
-    /// Make a backup of the original prebuilt module file arguments.
-    std::map<std::string, std::string, std::less<>> OrigPrebuiltModuleFiles =
-        Compiler.getHeaderSearchOpts().PrebuiltModuleFiles;
-    /// Configure the compiler with discovered prebuilt modules. This will
-    /// prevent the implicit build of duplicate modules and force reuse of
-    /// existing prebuilt module files instead.
-    Compiler.getHeaderSearchOpts().PrebuiltModuleFiles.insert(
-        PrebuiltModuleFiles.begin(), PrebuiltModuleFiles.end());
 
     // Create the dependency collector that will collect the produced
     // dependencies.
     //
     // This also moves the existing dependency output options from the
     // invocation to the collector. The options in the invocation are reset,
     // which ensures that the compiler won't create new dependency collectors,
     // and thus won't write out the extra '.d' files to disk.
bool runInvocation(std::shared_ptr<CompilerInvocation> Invocation,
     switch (Format) {
     case ScanningOutputFormat::Make:
       Compiler.addDependencyCollector(
           std::make_shared<DependencyConsumerForwarder>(std::move(Opts),
                                                         Consumer));
       break;
     case ScanningOutputFormat::Full:
       Compiler.addDependencyCollector(std::make_shared<ModuleDepCollector>(
-          std::move(Opts), Compiler, Consumer,
-          std::move(OrigPrebuiltModuleFiles)));
+          std::move(Opts), Compiler, Consumer, std::move(OriginalInvocation)));
       break;
     }
 
     // Consider different header search and diagnostic options to create
     // different modules. This avoids the unsound aliasing of module PCMs.
     //
     // TODO: Implement diagnostic bucketing and header search pruning to reduce
     // the impact of strict context hashing.

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

 #include "llvm/Support/StringSaver.h"
 
 using namespace clang;
 using namespace tooling;
 using namespace dependencies;
 
 CompilerInvocation ModuleDepCollector::makeInvocationForModuleBuildWithoutPaths(
     const ModuleDeps &Deps) const {
-  // Make a deep copy of the invocation.
-  CompilerInvocation CI(Instance.getInvocation());
+  // Make a deep copy of the original Clang invocation.
+  CompilerInvocation CI(OriginalInvocation);
 
   // Remove options incompatible with explicit module build.
   CI.getFrontendOpts().Inputs.clear();
   CI.getFrontendOpts().OutputFile.clear();
 
   CI.getFrontendOpts().ProgramAction = frontend::GenerateModule;
   CI.getLangOpts()->ModuleName = Deps.ID.ModuleName;
   CI.getFrontendOpts().IsSystemModule = Deps.IsSystem;
 
   CI.getLangOpts()->ImplicitModules = false;
 
   // Report the prebuilt modules this module uses.
   for (const auto &PrebuiltModule : Deps.PrebuiltModuleDeps) {
     CI.getFrontendOpts().ModuleFiles.push_back(PrebuiltModule.PCMFile);
     CI.getFrontendOpts().ModuleMapFiles.push_back(PrebuiltModule.ModuleMapFile);
   }
 
-  // Restore the original set of prebuilt module files.
-  CI.getHeaderSearchOpts().PrebuiltModuleFiles = OriginalPrebuiltModuleFiles;
-
   CI.getPreprocessorOpts().ImplicitPCHInclude.clear();
 
   return CI;
 }
 
 static std::vector<std::string>
 serializeCompilerInvocation(const CompilerInvocation &CI) {
   // Set up string allocator.
if (Import->getTopLevelModule() != M->getTopLevelModule() &&
       if (AddedModules.insert(Import->getTopLevelModule()).second)
         MD.ClangModuleDeps.push_back(ImportID);
     }
   }
 }
 
 ModuleDepCollector::ModuleDepCollector(
     std::unique_ptr<DependencyOutputOptions> Opts, CompilerInstance &I,
-    DependencyConsumer &C,
-    std::map<std::string, std::string, std::less<>> OriginalPrebuiltModuleFiles)
+    DependencyConsumer &C, CompilerInvocation &&OriginalCI)
     : Instance(I), Consumer(C), Opts(std::move(Opts)),
-      OriginalPrebuiltModuleFiles(std::move(OriginalPrebuiltModuleFiles)) {}
+      OriginalInvocation(std::move(OriginalCI)) {}
 
 void ModuleDepCollector::attachToPreprocessor(Preprocessor &PP) {
   PP.addPPCallbacks(std::make_unique<ModuleDepCollectorPP>(Instance, *this));
 }
 
 void ModuleDepCollector::attachToASTReader(ASTReader &R) {}
 
 bool ModuleDepCollector::isPrebuiltModule(const Module *M) {

clang/test/ClangScanDeps/Inputs/preserved-args/cdb.json.template

+[
+  {
+    "directory": "DIR",
+    "command": "clang -MD -MT my_target -serialize-diagnostics DIR/tu.dia -fsyntax-only DIR/tu.c -fmodules -fimplicit-module-maps -fmodules-cache-path=DIR/cache -fmodule-file=Foo=DIR/foo.pcm -o DIR/tu.o",
+    "file": "DIR/tu.c"
+  }
+]

clang/test/ClangScanDeps/Inputs/preserved-args/mod.h

+// mod.h

clang/test/ClangScanDeps/Inputs/preserved-args/module.modulemap

+module Mod {
+  header "mod.h"
+}

clang/test/ClangScanDeps/Inputs/preserved-args/tu.c

+#include "mod.h"

clang/test/ClangScanDeps/preserved-args.c

+// RUN: rm -rf %t && mkdir %t
+// RUN: cp -r %S/Inputs/preserved-args/* %t
+// RUN: sed -e "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+
+// RUN: echo -%t > %t/result.json
+// RUN: clang-scan-deps -compilation-database %t/cdb.json -format experimental-full >> %t/result.json
+// RUN: cat %t/result.json | sed 's:\\\\\?:/:g' | FileCheck %s
+
+// CHECK:      -[[PREFIX:.*]]
+// CHECK-NEXT: {
+// CHECK-NEXT:   "modules": [
+// CHECK-NEXT:     {
+// CHECK:            "command-line": [
+// CHECK-NEXT:         "-cc1"
+// CHECK:              "-serialize-diagnostic-file"
+// CHECK-NEXT:         "[[PREFIX]]/tu.dia"
+// CHECK:              "-fmodule-file=Foo=[[PREFIX]]/foo.pcm"
+// CHECK:              "-MT"
+// CHECK-NEXT:         "my_target"
+// CHECK:              "-dependency-file"
+// CHECK-NEXT:         "[[PREFIX]]/tu.d"
+// CHECK:            ],
+// CHECK:            "name": "Mod"
+// CHECK-NEXT:     }
+// CHECK-NEXT:   ]
+// CHECK:      }