Method loadFromCommandLine should be able to report errors

[sepavloff]

Summary

Now FixedCompilationDatabase::loadFromCommandLine has no means to report
which error occurred if it fails to create compilation object. This is
a block for implementing D33013, because after that change driver refuses
to create compilation if command line contains erroneous options.

This change adds additional argument to loadFromCommandLine, which is
assigned error message text if compilation object was not created. This is
the same way as other methods of CompilationDatabase report failure.

[joerg]

Can this use ErrorOr?

[sepavloff]

In D33272#758616, @joerg wrote:

Can this use ErrorOr?

It makes sense if constructing methods of CompilationDatabase were also changed to return ErrorOr, as there is no need to have different interfaces for the similar functionality. On the other hand, such change would not present a gain over existing implementation IMHO.

[sepavloff]

Change loadFromCommandLine so that it returns std::unique_ptr as methods of CompilationDatabase does

[alexfh]

Thank you! This looks good with one nit (see the inline comment).

lib/Tooling/CompilationDatabase.cpp

292

The lack of the arguments (or the -- below) shouldn't be treated as an error (at least an error that is worth showing to the user). The caller will just move on to trying the next kind of a compilation database. The only actual errors we can get here may be coming from stripPositionalArgs.

The caller should be modified accordingly (i.e. not output anything when both the return value is nullptr and ErrorMsg is empty).

[alexfh]

lib/Tooling/CompilationDatabase.cpp

168

Another couple of nits:

1. the comment below would probably be better moved to the body of the second if
2. enclose the next statement in braces.

[sepavloff]

Addressed reviewer's notes.

[sepavloff]

Thank you!

I put updated fix here. If it is OK, I'll commit it tomorrow.

lib/Tooling/CompilationDatabase.cpp

292

What if error message contains a prefix that indicates if this is error or warning? Errors could be printed and warnings ignored in CommonOptionsParser.cpp:122. Such solution would allow clients to investigate failures in more details.

[alexfh]

lib/Tooling/CommonOptionsParser.cpp

122

Getting into the business of encoding the type of error into the string is not what we want in this particular case ;] Let's just check whether ErrorMessage is empty (we could go further and make it an Optional<std::string>, but an empty string seems to be equally reasonable here).

lib/Tooling/CompilationDatabase.cpp

292

I don't think it's valuable in this specific code. I'd just return nullptr without touching ErrorMsg and change the caller to only print something when the ErrorMsg is not empty.

The lack of the -- in the command line is not an error, it's just a way to automatically find a compilation database from the file system (which is probably even more frequently used feature than the fixed compilation database, so we definitely should not treat this case as an error).

[sepavloff]

I see your reasons. Updated the patch accordingly.
Thank you!

[sepavloff]

Updated patch

[alexfh]

Thanks, LG!

include/clang/Tooling/CompilationDatabase.h

public:
   ///   cl::ParseCommandLineOptions(argc, argv);
   ///   ...
   /// }
   ///
   /// \param Argc The number of command line arguments - will be changed to
   /// the number of arguments before "--", if "--" was found in the argument
   /// list.
   /// \param Argv Points to the command line arguments.
+  /// \param ErrorMsg Contains error text if the function returns null pointer.
   /// \param Directory The base directory used in the FixedCompilationDatabase.
-  static FixedCompilationDatabase *loadFromCommandLine(int &Argc,
-                                                       const char *const *Argv,
-                                                       Twine Directory = ".");
+  static std::unique_ptr<FixedCompilationDatabase> loadFromCommandLine(
+      int &Argc, const char *const *Argv, std::string &ErrorMsg,
 
   /// \brief Constructs a compilation data base from a specified directory
   /// and command line.
   FixedCompilationDatabase(Twine Directory, ArrayRef<std::string> CommandLine);
 
   /// \brief Returns the given compile command.
   ///
   /// Will always return a vector with one entry that contains the directory

lib/Frontend/CreateInvocationFromCommandLine.cpp

std::unique_ptr<CompilerInvocation> clang::createInvocationFromCommandLine(
   // FIXME: We shouldn't have to pass in the path info.
   driver::Driver TheDriver(Args[0], llvm::sys::getDefaultTargetTriple(),
                            *Diags);
 
   // 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;
 
   // Just print the cc1 options if -### was present.
   if (C->getArgs().hasArg(driver::options::OPT__HASH_HASH_HASH)) {
     C->getJobs().Print(llvm::errs(), "\n", true);
     return nullptr;
   }
 
   // We expect to get back exactly one command job, if we didn't something

lib/Tooling/CommonOptionsParser.cpp

CommonOptionsParser::CommonOptionsParser(
 
   static cl::list<std::string> ArgsBefore(
       "extra-arg-before",
       cl::desc("Additional argument to prepend to the compiler command line"),
       cl::cat(Category));
 
   cl::HideUnrelatedOptions(Category);
 
-  Compilations.reset(FixedCompilationDatabase::loadFromCommandLine(argc, argv));
+  std::string ErrorMessage;
+  Compilations =
+      FixedCompilationDatabase::loadFromCommandLine(argc, argv, ErrorMessage);
+  if (!Compilations && !ErrorMessage.empty())
+    llvm::errs() << ErrorMessage;
   cl::ParseCommandLineOptions(argc, argv, Overview);
   cl::PrintOptionValues();
 
   SourcePathList = SourcePaths;
   if ((OccurrencesFlag == cl::ZeroOrMore || OccurrencesFlag == cl::Optional) &&
       SourcePathList.empty())
     return;
   if (!Compilations) {
-    std::string ErrorMessage;
     if (!BuildPath.empty()) {
       Compilations =
           CompilationDatabase::autoDetectFromDirectory(BuildPath, ErrorMessage);
     } else {
       Compilations = CompilationDatabase::autoDetectFromSource(SourcePaths[0],
                                                                ErrorMessage);
     }
     if (!Compilations) {

lib/Tooling/CompilationDatabase.cpp

 #include "clang/Driver/Job.h"
 #include "clang/Frontend/TextDiagnosticPrinter.h"
 #include "clang/Tooling/CompilationDatabasePluginRegistry.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Option/Arg.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/raw_ostream.h"
 #include <sstream>
 #include <system_error>
 using namespace clang;
 using namespace tooling;
 
 LLVM_INSTANTIATE_REGISTRY(CompilationDatabasePluginRegistry)
 
 CompilationDatabase::~CompilationDatabase() {}
private:
   }
 };
 
 // Special DiagnosticConsumer that looks for warn_drv_input_file_unused
 // diagnostics from the driver and collects the option strings for those unused
 // options.
 class UnusedInputDiagConsumer : public DiagnosticConsumer {
 public:
-  UnusedInputDiagConsumer() : Other(nullptr) {}
-
-  // Useful for debugging, chain diagnostics to another consumer after
-  // recording for our own purposes.
-  UnusedInputDiagConsumer(DiagnosticConsumer *Other) : Other(Other) {}
+  UnusedInputDiagConsumer(DiagnosticConsumer &Other) : Other(Other) {}
 
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const Diagnostic &Info) override {
     if (Info.getID() == clang::diag::warn_drv_input_file_unused) {
       // Arg 1 for this diagnostic is the option that didn't get used.
       UnusedInputs.push_back(Info.getArgStdStr(0));
+    } else if (DiagLevel >= DiagnosticsEngine::Error) {
+      // If driver failed to create compilation object, show the diagnostics
+      // to user.
+      Other.HandleDiagnostic(DiagLevel, Info);
     }
-    if (Other)
-      Other->HandleDiagnostic(DiagLevel, Info);
   }
 
-  DiagnosticConsumer *Other;
+  DiagnosticConsumer &Other;
   SmallVector<std::string, 2> UnusedInputs;
 };
 
 // Unary functor for asking "Given a StringRef S1, does there exist a string
 // S2 in Arr where S1 == S2?"
 struct MatchesAny {
   MatchesAny(ArrayRef<std::string> Arr) : Arr(Arr) {}
   bool operator() (StringRef S) {

 /// clear.
 ///
 /// \param[in] Args Args as provided by the user.
 /// \return Resulting stripped command line.
 ///          \li true if successful.
 ///          \li false if \c Args cannot be used for compilation jobs (e.g.
 ///          contains an option like -E or -version).
 static bool stripPositionalArgs(std::vector<const char *> Args,
-                                std::vector<std::string> &Result) {
+                                std::vector<std::string> &Result,
+                                std::string &ErrorMsg) {
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
-  UnusedInputDiagConsumer DiagClient;
+  llvm::raw_string_ostream Output(ErrorMsg);
+  TextDiagnosticPrinter DiagnosticPrinter(Output, &*DiagOpts);
+  UnusedInputDiagConsumer DiagClient(DiagnosticPrinter);
   DiagnosticsEngine Diagnostics(
       IntrusiveRefCntPtr<clang::DiagnosticIDs>(new DiagnosticIDs()),
       &*DiagOpts, &DiagClient, false);
 
   // The clang executable path isn't required since the jobs the driver builds
   // will not be executed.
   std::unique_ptr<driver::Driver> NewDriver(new driver::Driver(
       /* ClangExecutable= */ "", llvm::sys::getDefaultTargetTriple(),
static bool stripPositionalArgs(std::vector<const char *> Args,
   // Remove -no-integrated-as; it's not used for syntax checking,
   // and it confuses targets which don't support this option.
   Args.erase(std::remove_if(Args.begin(), Args.end(),
                             MatchesAny(std::string("-no-integrated-as"))),
              Args.end());
 
   const std::unique_ptr<driver::Compilation> Compilation(
       NewDriver->BuildCompilation(Args));
+  if (!Compilation)
+    return false;
 
   const driver::JobList &Jobs = Compilation->getJobs();
 
   CompileJobAnalyzer CompileAnalyzer;
 
   for (const auto &Cmd : Jobs) {
     // Collect only for Assemble jobs. If we do all jobs we get duplicates
     // since Link jobs point to Assemble jobs as inputs.
     if (Cmd.getSource().getKind() == driver::Action::AssembleJobClass)
       CompileAnalyzer.run(&Cmd.getSource());
   }
 
   if (CompileAnalyzer.Inputs.empty()) {
-    // No compile jobs found.
-    // FIXME: Emit a warning of some kind?
+    ErrorMsg = "warning: no compile jobs found\n";
     return false;
   }
 
   // Remove all compilation input files from the command line. This is
   // necessary so that getCompileCommands() can construct a command line for
   // each file.
   std::vector<const char *>::iterator End = std::remove_if(
       Args.begin(), Args.end(), MatchesAny(CompileAnalyzer.Inputs));
 
   // Remove all inputs deemed unused for compilation.
   End = std::remove_if(Args.begin(), End, MatchesAny(DiagClient.UnusedInputs));
 
   // Remove the -c add above as well. It will be at the end right now.
   assert(strcmp(*(End - 1), "-c") == 0);
   --End;
 
   Result = std::vector<std::string>(Args.begin() + 1, End);
   return true;
 }
 
-FixedCompilationDatabase *FixedCompilationDatabase::loadFromCommandLine(
-    int &Argc, const char *const *Argv, Twine Directory) {
+std::unique_ptr<FixedCompilationDatabase>
+FixedCompilationDatabase::loadFromCommandLine(int &Argc,
+                                              const char *const *Argv,
+                                              std::string &ErrorMsg,
+                                              Twine Directory) {
+  ErrorMsg.clear();
+  if (Argc == 0)
+    return nullptr;
   const char *const *DoubleDash = std::find(Argv, Argv + Argc, StringRef("--"));
   if (DoubleDash == Argv + Argc)
     return nullptr;
   std::vector<const char *> CommandLine(DoubleDash + 1, Argv + Argc);
   Argc = DoubleDash - Argv;
 
   std::vector<std::string> StrippedArgs;
-  if (!stripPositionalArgs(CommandLine, StrippedArgs))
+  if (!stripPositionalArgs(CommandLine, StrippedArgs, ErrorMsg))
     return nullptr;
-  return new FixedCompilationDatabase(Directory, StrippedArgs);
+  return std::unique_ptr<FixedCompilationDatabase>(
+      new FixedCompilationDatabase(Directory, StrippedArgs));
 }
 
 FixedCompilationDatabase::
 FixedCompilationDatabase(Twine Directory, ArrayRef<std::string> CommandLine) {
   std::vector<std::string> ToolCommandLine(1, "clang-tool");
   ToolCommandLine.insert(ToolCommandLine.end(),
                          CommandLine.begin(), CommandLine.end());
   CompileCommands.emplace_back(Directory, StringRef(),

lib/Tooling/Tooling.cpp

DiagnosticsEngine Diagnostics(
       DiagConsumer ? DiagConsumer : &DiagnosticPrinter, false);
 
   const std::unique_ptr<clang::driver::Driver> Driver(
       newDriver(&Diagnostics, BinaryName, Files->getVirtualFileSystem()));
   // Since the input might only be virtual, don't check whether it exists.
   Driver->setCheckInputsExist(false);
   const std::unique_ptr<clang::driver::Compilation> Compilation(
       Driver->BuildCompilation(llvm::makeArrayRef(Argv)));
+  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));
   // FIXME: remove this when all users have migrated!

unittests/Tooling/CompilationDatabaseTest.cpp

TEST(FixedCompilationDatabase, GetAllCompileCommands) {
   CommandLine.push_back("two");
   FixedCompilationDatabase Database(".", CommandLine);
 
   EXPECT_EQ(0ul, Database.getAllCompileCommands().size());
 }
 
 TEST(ParseFixedCompilationDatabase, ReturnsNullOnEmptyArgumentList) {
   int Argc = 0;
-  std::unique_ptr<FixedCompilationDatabase> Database(
-      FixedCompilationDatabase::loadFromCommandLine(Argc, nullptr));
+  std::string ErrorMsg;
+  std::unique_ptr<FixedCompilationDatabase> Database =
+      FixedCompilationDatabase::loadFromCommandLine(Argc, nullptr, ErrorMsg);
   EXPECT_FALSE(Database);
+  EXPECT_TRUE(ErrorMsg.empty());
   EXPECT_EQ(0, Argc);
 }
 
 TEST(ParseFixedCompilationDatabase, ReturnsNullWithoutDoubleDash) {
   int Argc = 2;
   const char *Argv[] = { "1", "2" };
+  std::string ErrorMsg;
   std::unique_ptr<FixedCompilationDatabase> Database(
-      FixedCompilationDatabase::loadFromCommandLine(Argc, Argv));
+      FixedCompilationDatabase::loadFromCommandLine(Argc, Argv, ErrorMsg));
   EXPECT_FALSE(Database);
+  EXPECT_TRUE(ErrorMsg.empty());
   EXPECT_EQ(2, Argc);
 }
 
 TEST(ParseFixedCompilationDatabase, ReturnsArgumentsAfterDoubleDash) {
   int Argc = 5;
   const char *Argv[] = {
     "1", "2", "--\0no-constant-folding", "-DDEF3", "-DDEF4"
   };
+  std::string ErrorMsg;
   std::unique_ptr<FixedCompilationDatabase> Database(
-      FixedCompilationDatabase::loadFromCommandLine(Argc, Argv));
+      FixedCompilationDatabase::loadFromCommandLine(Argc, Argv, ErrorMsg));
   ASSERT_TRUE((bool)Database);
+  ASSERT_TRUE(ErrorMsg.empty());
   std::vector<CompileCommand> Result =
     Database->getCompileCommands("source");
   ASSERT_EQ(1ul, Result.size());
   ASSERT_EQ(".", Result[0].Directory);
   std::vector<std::string> CommandLine;
   CommandLine.push_back("clang-tool");
   CommandLine.push_back("-DDEF3");
   CommandLine.push_back("-DDEF4");
   CommandLine.push_back("source");
   ASSERT_EQ(CommandLine, Result[0].CommandLine);
   EXPECT_EQ(2, Argc);
 }
 
 TEST(ParseFixedCompilationDatabase, ReturnsEmptyCommandLine) {
   int Argc = 3;
   const char *Argv[] = { "1", "2", "--\0no-constant-folding" };
-  std::unique_ptr<FixedCompilationDatabase> Database(
-      FixedCompilationDatabase::loadFromCommandLine(Argc, Argv));
+  std::string ErrorMsg;
+  std::unique_ptr<FixedCompilationDatabase> Database =
+      FixedCompilationDatabase::loadFromCommandLine(Argc, Argv, ErrorMsg);
   ASSERT_TRUE((bool)Database);
+  ASSERT_TRUE(ErrorMsg.empty());
   std::vector<CompileCommand> Result =
     Database->getCompileCommands("source");
   ASSERT_EQ(1ul, Result.size());
   ASSERT_EQ(".", Result[0].Directory);
   std::vector<std::string> CommandLine;
   CommandLine.push_back("clang-tool");
   CommandLine.push_back("source");
   ASSERT_EQ(CommandLine, Result[0].CommandLine);
   EXPECT_EQ(2, Argc);
 }
 
 TEST(ParseFixedCompilationDatabase, HandlesPositionalArgs) {
   const char *Argv[] = {"1", "2", "--", "-c", "somefile.cpp", "-DDEF3"};
   int Argc = sizeof(Argv) / sizeof(char*);
-  std::unique_ptr<FixedCompilationDatabase> Database(
-      FixedCompilationDatabase::loadFromCommandLine(Argc, Argv));
+  std::string ErrorMsg;
+  std::unique_ptr<FixedCompilationDatabase> Database =
+      FixedCompilationDatabase::loadFromCommandLine(Argc, Argv, ErrorMsg);
   ASSERT_TRUE((bool)Database);
+  ASSERT_TRUE(ErrorMsg.empty());
   std::vector<CompileCommand> Result =
     Database->getCompileCommands("source");
   ASSERT_EQ(1ul, Result.size());
   ASSERT_EQ(".", Result[0].Directory);
   std::vector<std::string> Expected;
   Expected.push_back("clang-tool");
   Expected.push_back("-c");
   Expected.push_back("-DDEF3");
   Expected.push_back("source");
   ASSERT_EQ(Expected, Result[0].CommandLine);
   EXPECT_EQ(2, Argc);
 }
 
 TEST(ParseFixedCompilationDatabase, HandlesArgv0) {
   const char *Argv[] = {"1", "2", "--", "mytool", "somefile.cpp"};
   int Argc = sizeof(Argv) / sizeof(char*);
-  std::unique_ptr<FixedCompilationDatabase> Database(
-      FixedCompilationDatabase::loadFromCommandLine(Argc, Argv));
+  std::string ErrorMsg;
+  std::unique_ptr<FixedCompilationDatabase> Database =
+      FixedCompilationDatabase::loadFromCommandLine(Argc, Argv, ErrorMsg);
   ASSERT_TRUE((bool)Database);
+  ASSERT_TRUE(ErrorMsg.empty());
   std::vector<CompileCommand> Result =
     Database->getCompileCommands("source");
   ASSERT_EQ(1ul, Result.size());
   ASSERT_EQ(".", Result[0].Directory);
   std::vector<std::string> Expected;
   Expected.push_back("clang-tool");
   Expected.push_back("source");
   ASSERT_EQ(Expected, Result[0].CommandLine);
   EXPECT_EQ(2, Argc);
 }
 
 } // end namespace tooling
 } // end namespace clang