[Frontend] give createInvocationFromCommandLine an options struct

[sammccall]

Summary

It's accumulating way too many optional params (see D124970)

While here, improve the name and the documentation.

[kadircet]

Thanks this LGTM. Just two questions:

• Why not migrate rest of the usages and drop the old function completely in this patch
• What about moving the function from clang/Frontend to Tooling, Support or FrontendTool? this is clearly only inteded to be used by tools and not part of the Frontend. There's actually Tooling/Tooling.h which has a newInvocation that takes in cc1 args, rather than driver args. so these two seem like siblings.

clang/include/clang/Frontend/Utils.h

232

what's the reason for not deleting this all together from open source. there isn't very many references.
is it just to keep the patch small so that relevant changes are more visible ?

[sammccall]

In D124971#3493657, @kadircet wrote:

Thanks this LGTM. Just two questions:

• Why not migrate rest of the usages and drop the old function completely in this patch

In order to keep the patch small. I can do this as a followup.

• What about moving the function from clang/Frontend to Tooling, Support or FrontendTool? this is clearly only inteded to be used by tools and not part of the Frontend. There's actually Tooling/Tooling.h which has a newInvocation that takes in cc1 args, rather than driver args. so these two seem like siblings.

Sounds reasonable to me, but I'm not personally planning to work on it.
Note that ASTUnit depends on this, so there's some layering mess to clean up.

clang-tools-extra/clangd/Compiler.cpp

 buildCompilerInvocation(const ParseInputs &Inputs, clang::DiagnosticConsumer &D,
                         std::vector<std::string> *CC1Args) {
   if (Inputs.CompileCommand.CommandLine.empty())
     return nullptr;
   std::vector<const char *> ArgStrs;
   for (const auto &S : Inputs.CompileCommand.CommandLine)
     ArgStrs.push_back(S.c_str());
 
-  auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
-  llvm::IntrusiveRefCntPtr<DiagnosticsEngine> CommandLineDiagsEngine =
+  CreateInvocationOptions CIOpts;
+  CIOpts.VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
+  CIOpts.CC1Args = CC1Args;
+  CIOpts.RecoverOnError = true;
+  CIOpts.Diags =
       CompilerInstance::createDiagnostics(new DiagnosticOptions, &D, false);
-  std::unique_ptr<CompilerInvocation> CI = createInvocationFromCommandLine(
-      ArgStrs, CommandLineDiagsEngine, std::move(VFS),
-      /*ShouldRecoverOnErrors=*/true, CC1Args);
+  std::unique_ptr<CompilerInvocation> CI = createInvocation(ArgStrs, CIOpts);
   if (!CI)
     return nullptr;
   // createInvocationFromCommandLine sets DisableFree.
   CI->getFrontendOpts().DisableFree = false;
   CI->getLangOpts()->CommentOpts.ParseAllComments = true;
   CI->getLangOpts()->RetainCommentsFromSystemHeaders = true;
 
   disableUnsupportedOptions(*CI);

clang/include/clang/Frontend/Utils.h

 #include "clang/Basic/LLVM.h"
 #include "clang/Driver/OptionUtils.h"
 #include "clang/Frontend/DependencyOutputOptions.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSet.h"
-#include "llvm/Option/OptSpecifier.h"
 #include "llvm/Support/FileCollector.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include <cstdint>
 #include <memory>
 #include <string>
 #include <system_error>
 #include <utility>
 #include <vector>
void AttachHeaderIncludeGen(Preprocessor &PP,
                             bool ShowDepth = true, bool MSStyle = false);
 
 /// The ChainedIncludesSource class converts headers to chained PCHs in
 /// memory, mainly for testing.
 IntrusiveRefCntPtr<ExternalSemaSource>
 createChainedIncludesSource(CompilerInstance &CI,
                             IntrusiveRefCntPtr<ExternalSemaSource> &Reader);
 
-/// createInvocationFromCommandLine - Construct a compiler invocation object for
-/// a command line argument vector.
+/// Optional inputs to createInvocation.
+struct CreateInvocationOptions {
+  /// Receives diagnostics encountered while parsing command-line flags.
+  /// If not provided, these are printed to stderr.
+  IntrusiveRefCntPtr<DiagnosticsEngine> Diags = nullptr;
+  /// Used e.g. to probe for system headers locations.
+  /// If not provided, the real filesystem is used.
+  /// FIXME: the driver does perform some non-virtualized IO.
+  IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = nullptr;
+  /// Whether to attempt to produce a non-null (possibly incorrect) invocation
+  /// if any errors were encountered.
+  /// By default, always return null on errors.
+  bool RecoverOnError = false;
+  /// If set, the target is populated with the cc1 args produced by the driver.
+  /// This may be populated even if createInvocation returns nullptr.
+  std::vector<std::string> *CC1Args = nullptr;
+};
+
+/// Interpret clang arguments in preparation to parse a file.
+///
+/// This simulates a number of steps Clang takes when its driver is invoked:
+/// - choosing actions (e.g compile + link) to run
+/// - probing the system for settings like standard library locations
+/// - spawning a cc1 subprocess to compile code, with more explicit arguments
+/// - in the cc1 process, assembling those arguments into a CompilerInvocation
+///   which is used to configure the parser
 ///
-/// \param ShouldRecoverOnErrors - whether we should attempt to return a
-/// non-null (and possibly incorrect) CompilerInvocation if any errors were
-/// encountered. When this flag is false, always return null on errors.
+/// This simulation is lossy, e.g. in some situations one driver run would
+/// result in multiple parses. (Multi-arch, CUDA, ...).
+/// This function tries to select a reasonable invocation that tools should use.
 ///
-/// \param CC1Args - if non-null, will be populated with the args to cc1
-/// expanded from \p Args. May be set even if nullptr is returned.
+/// Args[0] should be the driver name, such as "clang" or "/usr/bin/g++".
+/// Absolute path is preferred - this affects searching for system headers.
 ///
-/// \return A CompilerInvocation, or nullptr if none was built for the given
-/// argument vector.
+/// May return nullptr if an invocation could not be determined.
+/// See CreateInvocationOptions::ShouldRecoverOnErrors to try harder!
+std::unique_ptr<CompilerInvocation>
+createInvocation(ArrayRef<const char *> Args,
+                 CreateInvocationOptions Opts = {});
+
+/// Deprecated version of createInvocation with individual optional args.
 std::unique_ptr<CompilerInvocation> createInvocationFromCommandLine(
     ArrayRef<const char *> Args,
     IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
         IntrusiveRefCntPtr<DiagnosticsEngine>(),
     IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = nullptr,
     bool ShouldRecoverOnErrors = false,
     std::vector<std::string> *CC1Args = nullptr);
 
-// Frontend timing utils
-
 } // namespace clang
 
 #endif // LLVM_CLANG_FRONTEND_UTILS_H

clang/lib/Frontend/CreateInvocationFromCommandLine.cpp

 #include "clang/Frontend/Utils.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/Host.h"
 using namespace clang;
 using namespace llvm::opt;
 
-std::unique_ptr<CompilerInvocation> clang::createInvocationFromCommandLine(
-    ArrayRef<const char *> ArgList, IntrusiveRefCntPtr<DiagnosticsEngine> Diags,
-    IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS, bool ShouldRecoverOnErorrs,
-    std::vector<std::string> *CC1Args) {
+std::unique_ptr<CompilerInvocation>
+clang::createInvocation(ArrayRef<const char *> ArgList,
+                        CreateInvocationOptions Opts) {
   assert(!ArgList.empty());
-  if (!Diags.get()) {
-    // No diagnostics engine was provided, so create our own diagnostics object
-    // with the default options.
-    Diags = CompilerInstance::createDiagnostics(new DiagnosticOptions);
-  }
+  auto Diags = Opts.Diags
+                   ? std::move(Opts.Diags)
+                   : CompilerInstance::createDiagnostics(new DiagnosticOptions);
 
   SmallVector<const char *, 16> Args(ArgList.begin(), ArgList.end());
 
   // FIXME: Find a cleaner way to force the driver into restricted modes.
   Args.insert(
       llvm::find_if(
           Args, [](const char *Elem) { return llvm::StringRef(Elem) == "--"; }),
       "-fsyntax-only");
 
   // FIXME: We shouldn't have to pass in the path info.
   driver::Driver TheDriver(Args[0], llvm::sys::getDefaultTargetTriple(), *Diags,
-                           "clang LLVM compiler", VFS);
+                           "clang LLVM compiler", Opts.VFS);
 
   // Don't check that inputs exist, they may have been remapped.
   TheDriver.setCheckInputsExist(false);
 
   std::unique_ptr<driver::Compilation> C(TheDriver.BuildCompilation(Args));
   if (!C)
     return nullptr;
 
for (auto &A : C->getActions()){
         A = *A->input_begin();
       if (isa<driver::OffloadAction>(A)) {
         OffloadCompilation = true;
         break;
       }
     }
   }
 
-  bool PickFirstOfMany = OffloadCompilation || ShouldRecoverOnErorrs;
+  bool PickFirstOfMany = OffloadCompilation || Opts.RecoverOnError;
   if (Jobs.size() == 0 || (Jobs.size() > 1 && !PickFirstOfMany)) {
     SmallString<256> Msg;
     llvm::raw_svector_ostream OS(Msg);
     Jobs.Print(OS, "; ", true);
     Diags->Report(diag::err_fe_expected_compiler_job) << OS.str();
     return nullptr;
   }
   auto Cmd = llvm::find_if(Jobs, [](const driver::Command &Cmd) {
     return StringRef(Cmd.getCreator().getName()) == "clang";
   });
   if (Cmd == Jobs.end()) {
     Diags->Report(diag::err_fe_expected_clang_command);
     return nullptr;
   }
 
   const ArgStringList &CCArgs = Cmd->getArguments();
-  if (CC1Args)
-    *CC1Args = {CCArgs.begin(), CCArgs.end()};
+  if (Opts.CC1Args)
+    *Opts.CC1Args = {CCArgs.begin(), CCArgs.end()};
   auto CI = std::make_unique<CompilerInvocation>();
   if (!CompilerInvocation::CreateFromArgs(*CI, CCArgs, *Diags, Args[0]) &&
-      !ShouldRecoverOnErorrs)
+      !Opts.RecoverOnError)
     return nullptr;
   return CI;
 }
+
+std::unique_ptr<CompilerInvocation> clang::createInvocationFromCommandLine(
+    ArrayRef<const char *> Args, IntrusiveRefCntPtr<DiagnosticsEngine> Diags,
+    IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS, bool ShouldRecoverOnErrors,
+    std::vector<std::string> *CC1Args) {
+  return createInvocation(
+      Args,
+      CreateInvocationOptions{Diags, VFS, ShouldRecoverOnErrors, CC1Args});
+}

clang/unittests/Frontend/UtilsTest.cpp

 namespace clang {
 namespace {
 
 TEST(BuildCompilerInvocationTest, RecoverMultipleJobs) {
   // This generates multiple jobs and we recover by using the first.
   std::vector<const char *> Args = {"clang", "--target=macho", "-arch",  "i386",
                                     "-arch", "x86_64",         "foo.cpp"};
   clang::IgnoringDiagConsumer D;
-  llvm::IntrusiveRefCntPtr<DiagnosticsEngine> CommandLineDiagsEngine =
-      clang::CompilerInstance::createDiagnostics(new DiagnosticOptions, &D,
-                                                 false);
-  std::unique_ptr<CompilerInvocation> CI = createInvocationFromCommandLine(
-      Args, CommandLineDiagsEngine, new llvm::vfs::InMemoryFileSystem(),
-      /*ShouldRecoverOnErrors=*/true);
+  CreateInvocationOptions Opts;
+  Opts.RecoverOnError = true;
+  Opts.Diags = clang::CompilerInstance::createDiagnostics(new DiagnosticOptions,
+                                                          &D, false);
+  Opts.VFS = new llvm::vfs::InMemoryFileSystem();
+  std::unique_ptr<CompilerInvocation> CI = createInvocation(Args, Opts);
   ASSERT_TRUE(CI);
   EXPECT_THAT(CI->TargetOpts->Triple, testing::StartsWith("i386-"));
 }
 
 } // namespace
 } // namespace clang