Allow CompilerInvocations to generate .d files.

[saugustine]

Summary

Most clang tools should ignore the -M
family of options because one wouldn't want them
to generate a new dependency (.d) file. However,
some tools may want this dependency file. This
patch creates a mechanism for them to do this.

This implementation just plumbs a boolean down
several layers of calls. Each of the modified calls
has several call sites, and so a single member
variable or new API entry point won't work.

An alternative would be to write a function to filter
the -M family of arguments out of CC1Args, and have
each caller call that function by hand before calling
newInvocation, Invocation::run, or buildAstFromCodeWithArgs.
This is a more complicated and error-prone solution.
Why burden all the callers to remember to use
this function?

But I could rewrite this patch to use that method if
that is deemed more appropriate.

[echristo]

Going to let Ben review this. I'd rather not pass the bool down and he might know a way to avoid that here by knowing the code more :)

[klimek]

I think a better way might be to generally leave dependency options alone, add a default argument adapter to filter out all deps related flags, and allow users to add their own argument adapters that don't do that.

[saugustine]

In D34304#783675, @klimek wrote:

I think a better way might be to generally leave dependency options alone, add a default argument adapter to filter out all deps related flags, and allow users to add their own argument adapters that don't do that.

This argument adapter would have to be passed down in a similar way, no?

buildASTFromCodeWithArgs, toolIinvocation::Run, and newInvocation are all entry-points that would need this behavior, and all are called by themselves in one place or another.

I'm happy to do that, as it is quite a bit more flexible, but it doesn't seem any cleaner.

[klimek]

In D34304#784496, @saugustine wrote:

In D34304#783675, @klimek wrote:

I think a better way might be to generally leave dependency options alone, add a default argument adapter to filter out all deps related flags, and allow users to add their own argument adapters that don't do that.

This argument adapter would have to be passed down in a similar way, no?

buildASTFromCodeWithArgs, toolIinvocation::Run, and newInvocation are all entry-points that would need this behavior, and all are called by themselves in one place or another.

I think it's cleaner, because it uses a concept we already have (argument adapters).

I'm happy to do that, as it is quite a bit more flexible, but it doesn't seem any cleaner.

[saugustine]

In D34304#785083, @klimek wrote:

I think it's cleaner, because it uses a concept we already have (argument adapters).

Will you point me to an example of these. Google is coming up empty.

[saugustine]

In D34304#785984, @saugustine wrote:

In D34304#785083, @klimek wrote:

I think it's cleaner, because it uses a concept we already have (argument adapters).

Will you point me to an example of these. Google is coming up empty.

Actually, now that I figured out you mean ArgumentAdjusters, I am making progress.

[saugustine]

Actually, now that I figured out you mean ArgumentAdjusters, I am making progress.

Unfortunately, ArgumentAdjusters only work on vector<std::string>, and while ToolInvocation::Invocation takes its arguments in that form, tooling::newInvocation (which returns a CompilerInvocation) takes a SmallVector<const char *, 16>, so we either need to change one side's interface, or write two ArgumentAdjusters, but with similar semantics.

What is your preferred solution?

[klimek]

I mean, arguments need to be adjusted before converting to ArgStringList and calling newInvocation? I'm not sure I fully understand the problem, can you elaborate?

[saugustine]

In D34304#787699, @klimek wrote:

I mean, arguments need to be adjusted before converting to ArgStringList and calling newInvocation? I'm not sure I fully understand the problem, can you elaborate?

This gets back to why the original patch plumbed the boolean all the way down to newInvocation.

newInvocation is the function that actually discards the dependency file options--today unconditionally. But there is code that calls newInvocation directly (ClangFuzzer is one), without going through a higher-level API. So I can't adjust the arguments at a higher level and still preserve the old behavior.

Unfortunately, newInvocation's argument list type is incompatible with ArgumentAdjusters, so something else will need to be done. What do you recommend?

[klimek]

In D34304#788080, @saugustine wrote:

In D34304#787699, @klimek wrote:

I mean, arguments need to be adjusted before converting to ArgStringList and calling newInvocation? I'm not sure I fully understand the problem, can you elaborate?

This gets back to why the original patch plumbed the boolean all the way down to newInvocation.

newInvocation is the function that actually discards the dependency file options--today unconditionally.

Correct, and it shouldn't.

But there is code that calls newInvocation directly (ClangFuzzer is one), without going through a higher-level API. So I can't adjust the arguments at a higher level and still preserve the old behavior.

Can't the call sites that use it themselves fix the arguments themselves? We should be able to provide a convenience wrapper for those use cases, I'd guess they all look basically the same?

Unfortunately, newInvocation's argument list type is incompatible with ArgumentAdjusters, so something else will need to be done. What do you recommend?

I'd have expected something like:

1. create ArgumentAdjuster that filters out deps file creation args
2. make that the default in the higher level APIs that already use ArgumentAdjuster
3. for each call site that uses the lower level API, look into what data they have (probably just argv from main?) and create some nice ArgumentAdjuster wrapper so they can use the default argument adjuster with a 1 line change

[saugustine]

Rework this patch to use argument adjusters. It turns out that
the call to newInvocation from ClangFuzzer has a very limited
set of hard-coded arguments, so I don't think it is necessary
to do the hand-adjusting there. Any new callers would be on their
own.

[klimek]

LG, thx for bearing with me, this looks great.
(and sorry if I didn't send this earlier, just noticed I forgot to hit submit :( )

lib/Tooling/ArgumentsAdjusters.cpp

66 ↗

(On Diff #104307)

s/ also/, too/?

include/clang/Tooling/Tooling.h

 /// \param Code C++ code.
 /// \param Args Additional flags to pass on.
 /// \param FileName The file name which 'Code' will be mapped as.
 /// \param ToolName The name of the binary running the tool. Standard library
 ///                 header paths will be resolved relative to this.
 /// \param PCHContainerOps The PCHContainerOperations for loading and creating
 /// clang modules.
 ///
+/// \param HonorDependencyOutputOptions generate dependency files if -M or
+/// a related option appears on the command line.
+///
 /// \return The resulting AST or null if an error occurred.
 std::unique_ptr<ASTUnit> buildASTFromCodeWithArgs(
     const Twine &Code, const std::vector<std::string> &Args,
     const Twine &FileName = "input.cc", const Twine &ToolName = "clang-tool",
     std::shared_ptr<PCHContainerOperations> PCHContainerOps =
-        std::make_shared<PCHContainerOperations>());
+      std::make_shared<PCHContainerOperations>(),
+    bool HonorDependencyOutputOptions = false);
 
 /// \brief Utility to run a FrontendAction in a single clang invocation.
 class ToolInvocation {
 public:
   /// \brief Create a tool invocation.
   ///
   /// \param CommandLine The command line arguments to clang. Note that clang
   /// uses its binary name (CommandLine[0]) to locate its builtin headers.
public:
   /// \param PCHContainerOps The PCHContainerOperations for loading and creating
   /// clang modules.
   ToolInvocation(std::vector<std::string> CommandLine, ToolAction *Action,
                  FileManager *Files,
                  std::shared_ptr<PCHContainerOperations> PCHContainerOps);
 
   ~ToolInvocation();
 
+  /// \brief Remove -M, -MD, -MF, -MM from the CommandLine.
+  void RemoveDependencyFileArgs();
+
   /// \brief Set a \c DiagnosticConsumer to use during parsing.
   void setDiagnosticConsumer(DiagnosticConsumer *DiagConsumer) {
     this->DiagConsumer = DiagConsumer;
   }
 
   /// \brief Map a virtual file to be used while running the tool.
   ///
   /// \param FilePath The path at which the content will be mapped.
   /// \param Content A null terminated buffer of the file's content.
   // FIXME: remove this when all users have migrated!
   void mapVirtualFile(StringRef FilePath, StringRef Content);
 
   /// \brief Run the clang invocation.
   ///
+  /// \param HonorDependencyOutputOptions generate a .d file if
+  /// -M or a related option appears on the command line.
+  ///
   /// \returns True if there were no errors during execution.
-  bool run();
+  bool run(bool HonorDependencyOutputOptions = false);
 
  private:
   void addFileMappingsTo(SourceManager &SourceManager);
 
   bool runInvocation(const char *BinaryName,
                      clang::driver::Compilation *Compilation,
                      std::shared_ptr<clang::CompilerInvocation> Invocation,
                      std::shared_ptr<PCHContainerOperations> PCHContainerOps);

 ///
 /// \note This will not set \c CommandLine[0] to \c InvokedAs. The tooling
 /// infrastructure expects that CommandLine[0] is a tool path relative to which
 /// the builtin headers can be found.
 void addTargetAndModeForProgramName(std::vector<std::string> &CommandLine,
                                     StringRef InvokedAs);
 
 /// \brief Creates a \c CompilerInvocation.
+/// \param Diagnostics where and how to report diagnostics.
+/// \param CC1Args arguments to the compiler.
+/// \param HonorDependencyOutputOptions generate dependency files if -M or
+/// a related option appears on the command line.
 clang::CompilerInvocation *newInvocation(
     clang::DiagnosticsEngine *Diagnostics,
-    const llvm::opt::ArgStringList &CC1Args);
+    const llvm::opt::ArgStringList &CC1Args,
+    bool HonorDependencyOutputOptions = false);
 
 } // end namespace tooling
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLING_TOOLING_H

lib/Tooling/Tooling.cpp

static const llvm::opt::ArgStringList *getCC1Arguments(
   }
 
   return &Cmd.getArguments();
 }
 
 /// \brief Returns a clang build invocation initialized from the CC1 flags.
 clang::CompilerInvocation *newInvocation(
     clang::DiagnosticsEngine *Diagnostics,
-    const llvm::opt::ArgStringList &CC1Args) {
+    const llvm::opt::ArgStringList &CC1Args,
+    bool HonorDependencyOutputOptions) {
   assert(!CC1Args.empty() && "Must at least contain the program name!");
   clang::CompilerInvocation *Invocation = new clang::CompilerInvocation;
   clang::CompilerInvocation::CreateFromArgs(
       *Invocation, CC1Args.data() + 1, CC1Args.data() + CC1Args.size(),
       *Diagnostics);
   Invocation->getFrontendOpts().DisableFree = false;
   Invocation->getCodeGenOpts().DisableFree = false;
+  if (!HonorDependencyOutputOptions)
     Invocation->getDependencyOutputOpts() = DependencyOutputOptions();
   return Invocation;
 }
 
 bool runToolOnCode(clang::FrontendAction *ToolAction, const Twine &Code,
                    const Twine &FileName,
                    std::shared_ptr<PCHContainerOperations> PCHContainerOps) {
   return runToolOnCodeWithArgs(ToolAction, Code, std::vector<std::string>(),
                                FileName, "clang-tool",

 }
 
 void ToolInvocation::mapVirtualFile(StringRef FilePath, StringRef Content) {
   SmallString<1024> PathStorage;
   llvm::sys::path::native(FilePath, PathStorage);
   MappedFileContents[PathStorage] = Content;
 }
 
-bool ToolInvocation::run() {
+bool ToolInvocation::run(bool HonorDependencyOutputOptions) {
   std::vector<const char*> Argv;
   for (const std::string &Str : CommandLine)
     Argv.push_back(Str.c_str());
   const char *const BinaryName = Argv[0];
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
   unsigned MissingArgIndex, MissingArgCount;
   std::unique_ptr<llvm::opt::OptTable> Opts = driver::createDriverOptTable();
   llvm::opt::InputArgList ParsedArgs = Opts->ParseArgs(
bool ToolInvocation::run(bool HonorDependencyOutputOptions) {
   if (!Compilation)
     return false;
   const llvm::opt::ArgStringList *const CC1Args = getCC1Arguments(
       &Diagnostics, Compilation.get());
   if (!CC1Args) {
     return false;
   }
   std::unique_ptr<clang::CompilerInvocation> Invocation(
-      newInvocation(&Diagnostics, *CC1Args));
+      newInvocation(&Diagnostics, *CC1Args, HonorDependencyOutputOptions));
   // FIXME: remove this when all users have migrated!
   for (const auto &It : MappedFileContents) {
     // Inject the code as the given file name into the preprocessor options.
     std::unique_ptr<llvm::MemoryBuffer> Input =
         llvm::MemoryBuffer::getMemBuffer(It.getValue());
     Invocation->getPreprocessorOpts().addRemappedFile(It.getKey(),
                                                       Input.release());
   }
buildASTFromCode(const Twine &Code, const Twine &FileName,
                  std::shared_ptr<PCHContainerOperations> PCHContainerOps) {
   return buildASTFromCodeWithArgs(Code, std::vector<std::string>(), FileName,
                                   "clang-tool", std::move(PCHContainerOps));
 }
 
 std::unique_ptr<ASTUnit> buildASTFromCodeWithArgs(
     const Twine &Code, const std::vector<std::string> &Args,
     const Twine &FileName, const Twine &ToolName,
-    std::shared_ptr<PCHContainerOperations> PCHContainerOps) {
+    std::shared_ptr<PCHContainerOperations> PCHContainerOps,
+    bool HonorDependencyOutputOptions) {
   SmallString<16> FileNameStorage;
   StringRef FileNameRef = FileName.toNullTerminatedStringRef(FileNameStorage);
 
   std::vector<std::unique_ptr<ASTUnit>> ASTs;
   ASTBuilderAction Action(ASTs);
   llvm::IntrusiveRefCntPtr<vfs::OverlayFileSystem> OverlayFileSystem(
       new vfs::OverlayFileSystem(vfs::getRealFileSystem()));
   llvm::IntrusiveRefCntPtr<vfs::InMemoryFileSystem> InMemoryFileSystem(
       new vfs::InMemoryFileSystem);
   OverlayFileSystem->pushOverlay(InMemoryFileSystem);
   llvm::IntrusiveRefCntPtr<FileManager> Files(
       new FileManager(FileSystemOptions(), OverlayFileSystem));
   ToolInvocation Invocation(getSyntaxOnlyToolArgs(ToolName, Args, FileNameRef),
                             &Action, Files.get(), std::move(PCHContainerOps));
 
   SmallString<1024> CodeStorage;
   InMemoryFileSystem->addFile(FileNameRef, 0,
                               llvm::MemoryBuffer::getMemBuffer(
                                   Code.toNullTerminatedStringRef(CodeStorage)));
-  if (!Invocation.run())
+  if (!Invocation.run(HonorDependencyOutputOptions))
     return nullptr;
 
   assert(ASTs.size() == 1);
   return std::move(ASTs[0]);
 }
 
 } // end namespace tooling
 } // end namespace clang