[clang][deps] Generate the full command-line for modules

[jansvoboda11]

Summary

This patch uses the new CompilerInvocation::generateCC1CommandLine to generate the full canonical command line for modular dependencies, instead of only appending additional arguments.

[dexonsmith]

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

78–80

Looks like this will be a deep copy, but it doesn't look like it's being modified. Can this just be a const &, taken in the ModuleDeps constructor? Or is there a lifetime reason this needs to be as it is?

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

25

Should this call any of the resetNonModularOptions() functions, or are those intentionally omitted?

26–27

Should FrontendOpts gain a resetNonModularOptions()?

63

I think guaranteed copy elision means this won't be a deep copy of the return, but it might be nice to add a move constructor for CompilerInvocation so it's more obvious.

[jansvoboda11]

Share ModuleDeps::Invocation

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

78–80

It can't be const & for two reasons:

1. The current code value-initializes ModuleDeps in two places and truly initializes it later in ModuleDepCollectorPP::handleTopLevelModule. We'd have to go with something like optional<reference_wrapper<CompilerInvocation>> to be able to delay the initialization.
2. The lifetime of the CompilerInvocation is a bit wild, but the reference most likely wouldn't outlive ModuleDeps (depends on the client).

I think the best solution would be to extract the shared_ptr from CompilerInstance, share it across ModuleDeps instances and only do a deep copy when actually generating the command line.

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

25

My intent for this patch was to simply take the work @Bigcheese has done downstream with -remove-preceeding-explicit-module-build-incompatible-options and port it to CompilerInvocation.

Calling resetNonModularOptions() here would be the next step I intend to do in a future patch.

I also think disabling implicit module maps here is not correct, as that seems to disable inferred top-level modules.

26–27

Most likely yes. I'd like to look into it in a next patch.

63

That's intentional. The deep copy is performed inside the function.

Shouldn't the move constructor of CompilerInvocation be defaulted?

[jansvoboda11]

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

63

s/defaulted/implicitly-defined/

[dexonsmith]

LGTM, just a couple of other comments inline.

clang/include/clang/Frontend/CompilerInstance.h

230

Is get*Ptr() already used in CompilerInstance? If so, matching that style sounds great. Otherwise I have a slight preference for getShared*().

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

78–80

Using shared_ptr seems right given the lifetime issues.

FWIW, I think const CompilerInvocation* is probably simpler to work with than Optional<reference_wrapper<const CompilerInvocation>>.

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

63

Explicitly declaring a copy constructor suppresses the implicit move constructor. But adding a move with = default is probably all that's needed.

[jansvoboda11]

Thanks for the review!

clang/include/clang/Frontend/CompilerInstance.h

230

Yes, this naming pattern is already used:

• std::shared_ptr<HeaderSearchOptions> getHeaderSearchOptsPtr() const
• std::shared_ptr<Preprocessor> getPreprocessorPtr()

and getShared*() is not being used.

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

63

I see, thanks for pointing that out. I explicitly defaulted move constructor/assignment in D100473.

clang/include/clang/Frontend/CompilerInstance.h

public:
   bool ExecuteAction(FrontendAction &Act);
 
   /// }
   /// @name Compiler Invocation and Options
   /// {
 
   bool hasInvocation() const { return Invocation != nullptr; }
 
-  CompilerInvocation &getInvocation() {
+  CompilerInvocation &getInvocation() { return *getInvocationPtr(); }
+
+  std::shared_ptr<CompilerInvocation> getInvocationPtr() {
     assert(Invocation && "Compiler instance has no invocation!");
-    return *Invocation;
+    return Invocation;
   }
 
   /// setInvocation - Replace the current invocation.
   void setInvocation(std::shared_ptr<CompilerInvocation> Value);
 
   /// Indicates whether we should (re)build the global module index.
   bool shouldBuildGlobalModuleIndex() const;
 

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

 //===- ModuleDepCollector.h - Callbacks to collect deps ---------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLING_DEPENDENCY_SCANNING_MODULE_DEP_COLLECTOR_H
 #define LLVM_CLANG_TOOLING_DEPENDENCY_SCANNING_MODULE_DEP_COLLECTOR_H
 
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceManager.h"
+#include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/Utils.h"
 #include "clang/Lex/HeaderSearch.h"
 #include "clang/Lex/PPCallbacks.h"
 #include "clang/Serialization/ASTReader.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringSet.h"
 #include "llvm/Support/raw_ostream.h"
 #include <string>
struct ModuleID {
   /// treated as separate modules for the purpose of a build.
   std::string ContextHash;
 };
 
 struct ModuleDeps {
   /// The identifier of the module.
   ModuleID ID;
 
+  /// Whether this is a "system" module.
+  bool IsSystem;
+
   /// The path to the modulemap file which defines this module.
   ///
   /// This can be used to explicitly build this module. This file will
   /// additionally appear in \c FileDeps as a dependency.
   std::string ClangModuleMapFile;
 
   /// The path to where an implicit build would put the PCM for this module.
   std::string ImplicitModulePCMPath;
 
   /// A collection of absolute paths to files that this module directly depends
   /// on, not including transitive dependencies.
   llvm::StringSet<> FileDeps;
 
   /// A list of module identifiers this module directly depends on, not
   /// including transitive dependencies.
   ///
   /// This may include modules with a different context hash when it can be
   /// determined that the differences are benign for this compilation.
   std::vector<ModuleID> ClangModuleDeps;
 
   // Used to track which modules that were discovered were directly imported by
   // the primary TU.
   bool ImportedByMainFile = false;
 
+  /// The compiler invocation associated with the translation unit that imports
+  /// this module.
+  std::shared_ptr<CompilerInvocation> Invocation;
+
   /// Gets the full command line suitable for passing to clang.
   ///
   /// \param LookupPCMPath This function is called to fill in `-fmodule-file=`
   ///                      flags and for the `-o` flag. It needs to return a
   ///                      path for where the PCM for the given module is to
   ///                      be located.
   /// \param LookupModuleDeps This function is called to collect the full
   ///                         transitive set of dependencies for this

clang/lib/Tooling/DependencyScanning/ModuleDepCollector.cpp

 //===- ModuleDepCollector.cpp - Callbacks to collect deps -------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/DependencyScanning/ModuleDepCollector.h"
 
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Tooling/DependencyScanning/DependencyScanningWorker.h"
+#include "llvm/Support/StringSaver.h"
 
 using namespace clang;
 using namespace tooling;
 using namespace dependencies;
 
+static CompilerInvocation
+makeInvocationForModuleBuildWithoutPaths(const ModuleDeps &Deps) {
+  // Make a deep copy of the invocation.
+  CompilerInvocation CI(*Deps.Invocation);
+
+  // 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;
+  CI.getHeaderSearchOpts().ImplicitModuleMaps = false;
+
+  return CI;
+}
+
+static std::vector<std::string>
+serializeCompilerInvocation(CompilerInvocation &CI) {
+  // Set up string allocator.
+  llvm::BumpPtrAllocator Alloc;
+  llvm::StringSaver Strings(Alloc);
+  auto SA = [&Strings](const Twine &Arg) { return Strings.save(Arg).data(); };
+  SmallVector<const char *, 32> Args;
+
+  // Synthesize full command line from the CompilerInvocation.
+  CI.generateCC1CommandLine(Args, SA);
+
+  // Convert arguments to the return type.
+  std::vector<std::string> Ret;
+  Ret.reserve(Args.size());
+  for (const char *Arg : Args)
+    Ret.emplace_back(Arg);
+
+  return Ret;
+}
+
 std::vector<std::string> ModuleDeps::getFullCommandLine(
     std::function<StringRef(ModuleID)> LookupPCMPath,
     std::function<const ModuleDeps &(ModuleID)> LookupModuleDeps) const {
-  // TODO: Build full command line. That also means capturing the original
-  //       command line into NonPathCommandLine.
+  CompilerInvocation CI(makeInvocationForModuleBuildWithoutPaths(*this));
 
-  std::vector<std::string> Ret{
-      "-fno-implicit-modules",
-      "-fno-implicit-module-maps",
-  };
-
-  std::vector<std::string> PCMPaths;
-  std::vector<std::string> ModMapPaths;
   dependencies::detail::collectPCMAndModuleMapPaths(
-      ClangModuleDeps, LookupPCMPath, LookupModuleDeps, PCMPaths, ModMapPaths);
-  for (const std::string &PCMPath : PCMPaths)
-    Ret.push_back("-fmodule-file=" + PCMPath);
-  for (const std::string &ModMapPath : ModMapPaths)
-    Ret.push_back("-fmodule-map-file=" + ModMapPath);
+      ClangModuleDeps, LookupPCMPath, LookupModuleDeps,
+      CI.getFrontendOpts().ModuleFiles, CI.getFrontendOpts().ModuleMapFiles);
 
-  return Ret;
+  return serializeCompilerInvocation(CI);
 }
 
 void dependencies::detail::collectPCMAndModuleMapPaths(
     llvm::ArrayRef<ModuleID> Modules,
     std::function<StringRef(ModuleID)> LookupPCMPath,
     std::function<const ModuleDeps &(ModuleID)> LookupModuleDeps,
     std::vector<std::string> &PCMPaths, std::vector<std::string> &ModMapPaths) {
   llvm::StringSet<> AlreadyAdded;
void ModuleDepCollectorPP::handleTopLevelModule(const Module *M) {
 
   ModuleDeps &MD = ModI.first->second;
 
   const FileEntry *ModuleMap = Instance.getPreprocessor()
                                    .getHeaderSearchInfo()
                                    .getModuleMap()
                                    .getContainingModuleMapFile(M);
 
+  MD.Invocation = Instance.getInvocationPtr();
   MD.ClangModuleMapFile = std::string(ModuleMap ? ModuleMap->getName() : "");
   MD.ID.ModuleName = M->getFullModuleName();
   MD.ImplicitModulePCMPath = std::string(M->getASTFile()->getName());
   MD.ID.ContextHash = MDC.ContextHash;
+  MD.IsSystem = M->IsSystem;
   serialization::ModuleFile *MF =
       MDC.Instance.getASTReader()->getModuleManager().lookup(M->getASTFile());
   MDC.Instance.getASTReader()->visitInputFiles(
       *MF, true, true, [&](const serialization::InputFile &IF, bool isSystem) {
         MD.FileDeps.insert(IF.getFile()->getName());
       });
 
   llvm::DenseSet<const Module *> AddedModules;

clang/test/ClangScanDeps/modules-full.cpp

 // CHECK-NEXT:       "clang-module-deps": [
 // CHECK-NEXT:         {
 // CHECK-NEXT:           "context-hash": "[[CONTEXT_HASH_H1:[A-Z0-9]+]]",
 // CHECK-NEXT:           "module-name": "header2"
 // CHECK-NEXT:         }
 // CHECK-NEXT:       ],
 // CHECK-NEXT:       "clang-modulemap-file": "[[PREFIX]]/Inputs/module.modulemap",
 // CHECK-NEXT:       "command-line": [
-// CHECK-NEXT:         "-fno-implicit-modules",
-// CHECK-NEXT:         "-fno-implicit-module-maps",
-// CHECK-NEXT:         "-fmodule-file=[[PREFIX]]/module-cache{{(_clangcl)?}}/[[CONTEXT_HASH_H1]]/header2-{{[A-Z0-9]+}}.pcm",
-// CHECK-NEXT:         "-fmodule-map-file=[[PREFIX]]/Inputs/module.modulemap"
-// CHECK-NEXT:       ],
+// CHECK:              "-fmodule-map-file=[[PREFIX]]/Inputs/module.modulemap",
+// CHECK:              "-fmodule-file=[[PREFIX]]/module-cache{{(_clangcl)?}}/[[CONTEXT_HASH_H1]]/header2-{{[A-Z0-9]+}}.pcm",
+// CHECK-NOT:          "-fimplicit-module-maps",
+// CHECK:              "-fno-implicit-modules",
+// CHECK:            ],
 // CHECK-NEXT:       "context-hash": "[[CONTEXT_HASH_H1]]",
 // CHECK-NEXT:       "file-deps": [
 // CHECK-NEXT:         "[[PREFIX]]/Inputs/header.h",
 // CHECK-NEXT:         "[[PREFIX]]/Inputs/module.modulemap"
 // CHECK-NEXT:       ],
 // CHECK-NEXT:       "name": "header1"
 // CHECK-NEXT:     },
 // CHECK-NEXT:     {
 // CHECK-NEXT:       "clang-module-deps": [],
 // CHECK-NEXT:       "clang-modulemap-file": "[[PREFIX]]/Inputs/module.modulemap",
 // CHECK-NEXT:       "command-line": [
-// CHECK-NEXT:         "-fno-implicit-modules",
-// CHECK-NEXT:         "-fno-implicit-module-maps"
-// CHECK-NEXT:       ],
+// CHECK-NOT:          "-fimplicit-module-maps",
+// CHECK:              "-fno-implicit-modules",
+// CHECK:            ],
 // CHECK-NEXT:       "context-hash": "[[CONTEXT_HASH_H2:[A-Z0-9]+]]",
 // CHECK-NEXT:       "file-deps": [
 // CHECK-NEXT:         "[[PREFIX]]/Inputs/header.h",
 // CHECK-NEXT:         "[[PREFIX]]/Inputs/module.modulemap"
 // CHECK-NEXT:       ],
 // CHECK-NEXT:       "name": "header1"
 // CHECK-NEXT:     },
 // CHECK-NEXT:     {
 // CHECK-NEXT:       "clang-module-deps": [],
 // CHECK-NEXT:       "clang-modulemap-file": "[[PREFIX]]/Inputs/module.modulemap",
 // CHECK-NEXT:       "command-line": [
-// CHECK-NEXT:         "-fno-implicit-modules",
-// CHECK-NEXT:         "-fno-implicit-module-maps"
-// CHECK-NEXT:       ],
+// CHECK-NOT:          "-fimplicit-module-maps",
+// CHECK:              "-fno-implicit-modules",
+// CHECK:            ],
 // CHECK-NEXT:       "context-hash": "[[CONTEXT_HASH_H1]]",
 // CHECK-NEXT:       "file-deps": [
 // CHECK-NEXT:         "[[PREFIX]]/Inputs/header2.h",
 // CHECK-NEXT:         "[[PREFIX]]/Inputs/module.modulemap"
 // CHECK-NEXT:       ],
 // CHECK-NEXT:       "name": "header2"
 // CHECK-NEXT:     }
 // CHECK-NEXT:   ],