[Driver] Add an option for createInvocationFromCommandLine to recover on errors

[ilya-biryukov]

Summary

Previously, it would always return nullptr on any error.
This change adds a parameter, controlling whether the function should
attempt to return a non-null result even if unknown arguments (or other
errors were encountered).

The new behavior is only used in clangd.

Considered an alternative of changing the return value instead of adding
a new parameter, but that would require updating all callsites. Settled
with the parameter to minimize the code changes.

[gribozavr]

It does seem reasonable for Clangd to recover when it sees unknown command line flags (after all, in compiler_commands.json we can see command line flags from past and future versions of Clang, GCC and other compilers -- whatever the build system decided to write there). However I have two requests:

• In the doc comment for CompilerInvocation::CreateFromArgs, could you add a note that it does a best effort to provide a CompilerInvocation even if it returns false? Right now the doc comment reads like there are no guarantees at all if it returns false.

• (can be a separate patch) Could we produce a diagnostic in the affected TU notifying the user that the compiler commands could not be parsed and we did our best to recover, but it may be completely wrong?

[ilya-biryukov]

In D66731#1644840, @gribozavr wrote:

• In the doc comment for CompilerInvocation::CreateFromArgs, could you add a note that it does a best effort to provide a CompilerInvocation even if it returns false? Right now the doc comment reads like there are no guarantees at all if it returns false.

Will do and update this patch.

• (can be a separate patch) Could we produce a diagnostic in the affected TU notifying the user that the compiler commands could not be parsed and we did our best to recover, but it may be completely wrong?

You are right, this is long overdue. Although not specific to this particular patch, which aims at options for better recovery rather than fixing this limitation of clangd.
Nevertheless, D66759 surfaces the errors from command-line arguments in clangd and should address this point as well.

[ilya-biryukov]

• Add a comment to CreateFromArgs

[ilya-biryukov]

• Fix english grammar in the comment

[gribozavr]

clang/include/clang/Frontend/CompilerInvocation.h

150 ↗

(On Diff #217320)

\returns false if an error...

(\returns is a paragraph command. It starts a new paragraph.)

[ilya-biryukov]

• Start a paragrah with \returns instead of putting it in the middle

[ilya-biryukov]

clang/include/clang/Frontend/CompilerInvocation.h

150 ↗

(On Diff #217320)

TIL something about doxygen :-)

cfe/trunk/include/clang/Frontend/CompilerInvocation.h

public:
   CompilerInvocation() : AnalyzerOpts(new AnalyzerOptions()) {}
 
   /// @name Utility Methods
   /// @{
 
   /// Create a compiler invocation from a list of input options.
   /// \returns true on success.
   ///
+  /// \returns false if an error was encountered while parsing the arguments
+  /// and attempts to recover and continue parsing the rest of the arguments.
+  /// The recovery is best-effort and only guarantees that \p Res will end up in
+  /// one of the vaild-to-access (albeit arbitrary) states.
+  ///
   /// \param [out] Res - The resulting invocation.
   /// \param ArgBegin - The first element in the argument vector.
   /// \param ArgEnd - The last element in the argument vector.
   /// \param Diags - The diagnostic engine to use for errors.
   static bool CreateFromArgs(CompilerInvocation &Res,
                              const char* const *ArgBegin,
                              const char* const *ArgEnd,
                              DiagnosticsEngine &Diags);

cfe/trunk/include/clang/Frontend/Utils.h

 /// memory, mainly for testing.
 IntrusiveRefCntPtr<ExternalSemaSource>
 createChainedIncludesSource(CompilerInstance &CI,
                             IntrusiveRefCntPtr<ExternalSemaSource> &Reader);
 
 /// createInvocationFromCommandLine - Construct a compiler invocation object for
 /// a command line argument vector.
 ///
+/// \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.
+///
 /// \return A CompilerInvocation, or 0 if none was built for the given
 /// argument vector.
 std::unique_ptr<CompilerInvocation> createInvocationFromCommandLine(
     ArrayRef<const char *> Args,
     IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
         IntrusiveRefCntPtr<DiagnosticsEngine>(),
-    IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = nullptr);
+    IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = nullptr,
+    bool ShouldRecoverOnErrors = false);
 
 /// Return the value of the last argument as an integer, or a default. If Diags
 /// is non-null, emits an error if the argument is given, but non-integral.
 int getLastArgIntValue(const llvm::opt::ArgList &Args,
                        llvm::opt::OptSpecifier Id, int Default,
                        DiagnosticsEngine *Diags = nullptr);
 
 inline int getLastArgIntValue(const llvm::opt::ArgList &Args,

cfe/trunk/lib/Frontend/CreateInvocationFromCommandLine.cpp

 #include "clang/Driver/Tool.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "llvm/Option/ArgList.h"
 #include "llvm/Support/Host.h"
 using namespace clang;
 using namespace llvm::opt;
 
-/// createInvocationFromCommandLine - Construct a compiler invocation object for
-/// a command line argument vector.
-///
-/// \return A CompilerInvocation, or 0 if none was built for the given
-/// argument vector.
 std::unique_ptr<CompilerInvocation> clang::createInvocationFromCommandLine(
     ArrayRef<const char *> ArgList, IntrusiveRefCntPtr<DiagnosticsEngine> Diags,
-    IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS) {
+    IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS, bool ShouldRecoverOnErorrs) {
   if (!Diags.get()) {
     // No diagnostics engine was provided, so create our own diagnostics object
     // with the default options.
     Diags = CompilerInstance::createDiagnostics(new DiagnosticOptions);
   }
 
   SmallVector<const char *, 16> Args(ArgList.begin(), ArgList.end());
 
std::unique_ptr<CompilerInvocation> clang::createInvocationFromCommandLine(
   const driver::Command &Cmd = cast<driver::Command>(*Jobs.begin());
   if (StringRef(Cmd.getCreator().getName()) != "clang") {
     Diags->Report(diag::err_fe_expected_clang_command);
     return nullptr;
   }
 
   const ArgStringList &CCArgs = Cmd.getArguments();
   auto CI = std::make_unique<CompilerInvocation>();
-  if (!CompilerInvocation::CreateFromArgs(*CI,
-                                     const_cast<const char **>(CCArgs.data()),
-                                     const_cast<const char **>(CCArgs.data()) +
-                                     CCArgs.size(),
-                                     *Diags))
+  if (!CompilerInvocation::CreateFromArgs(
+          *CI, const_cast<const char **>(CCArgs.data()),
+          const_cast<const char **>(CCArgs.data()) + CCArgs.size(), *Diags) &&
+      !ShouldRecoverOnErorrs)
     return nullptr;
   return CI;
 }

clang-tools-extra/trunk/clangd/Compiler.cpp

buildCompilerInvocation(const ParseInputs &Inputs) {
 
   // FIXME(ibiryukov): store diagnostics from CommandLine when we start
   // reporting them.
   IgnoreDiagnostics IgnoreDiagnostics;
   llvm::IntrusiveRefCntPtr<DiagnosticsEngine> CommandLineDiagsEngine =
       CompilerInstance::createDiagnostics(new DiagnosticOptions,
                                           &IgnoreDiagnostics, false);
   std::unique_ptr<CompilerInvocation> CI = createInvocationFromCommandLine(
-      ArgStrs, CommandLineDiagsEngine, Inputs.FS);
+      ArgStrs, CommandLineDiagsEngine, Inputs.FS,
+      /*ShouldRecoverOnErrors=*/true);
   if (!CI)
     return nullptr;
   // createInvocationFromCommandLine sets DisableFree.
   CI->getFrontendOpts().DisableFree = false;
   CI->getLangOpts()->CommentOpts.ParseAllComments = true;
   return CI;
 }
 

clang-tools-extra/trunk/clangd/unittests/ClangdUnitTests.cpp

 //===-- ClangdUnitTests.cpp - ClangdUnit tests ------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "AST.h"
 #include "Annotations.h"
 #include "ClangdUnit.h"
+#include "Compiler.h"
 #include "SourceCode.h"
+#include "TestFS.h"
 #include "TestTU.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/TokenKinds.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/Support/ScopedPrinter.h"
 #include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
TEST(ClangdUnitTest, NoCrashOnTokensWithTidyCheck) {
   // Check first token after the preamble.
   EXPECT_EQ(T.expandedTokens().front().text(SM), "inline");
   // Last token is always 'eof'.
   EXPECT_EQ(T.expandedTokens().back().kind(), tok::eof);
   // Check the token before 'eof'.
   EXPECT_EQ(T.expandedTokens().drop_back().back().text(SM), "}");
 }
 
+TEST(ClangdUnitTest, CanBuildInvocationWithUnknownArgs) {
+  // Unknown flags should not prevent a build of compiler invocation.
+  ParseInputs Inputs;
+  Inputs.FS = buildTestFS({{testPath("foo.cpp"), "void test() {}"}});
+  Inputs.CompileCommand.CommandLine = {"clang", "-fsome-unknown-flag",
+                                       testPath("foo.cpp")};
+  EXPECT_NE(buildCompilerInvocation(Inputs), nullptr);
+
+  // Unknown forwarded to -cc1 should not a failure either.
+  Inputs.CompileCommand.CommandLine = {
+      "clang", "-Xclang", "-fsome-unknown-flag", testPath("foo.cpp")};
+  EXPECT_NE(buildCompilerInvocation(Inputs), nullptr);
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang