[clangd] Avoid "expected one compiler job" by picking the first eligible job.

[sammccall]

Summary

This happens in createInvocationWithCommandLine but only clangd currently passes
ShouldRecoverOnErorrs (sic).

One cause of this (with correct command) is several -arch arguments for mac
multi-arch support.

Fixes https://github.com/clangd/clangd/issues/827

[kadircet]

clang-tools-extra/clangd/unittests/CompilerTests.cpp

59 ↗

(On Diff #364738)

maybe move this test to clang/unittests/Frontend/ASTUnitTest.cpp? someone might send a patch to handle -arch specifically in clangd and it would no longer be possible to test this behaviour :)

(another option would be to test this through some other flags that'll trigger multiple compiler invocations, but I don't have any on top of my head)

68 ↗

(On Diff #364738)

nit: --target=macho is enough. and AFAICT root reason to support multiple architectures is mach object type.

clang/lib/Frontend/CreateInvocationFromCommandLine.cpp

92

hmm, this looks like a subtle behaviour change. I definitely like the behaviour you proposed better, but maybe do it in a separate patch for making the revert easier if need be?

[sammccall]

clang/lib/Frontend/CreateInvocationFromCommandLine.cpp

92

Which behavior change are do you mean?

When RecoverFromErrors is false and OffloadCompilation is false, behavior is same as before: if there isn't exactly one job we diagnose that and exit, if it's not suitable we diagnose that and exit.

When RecoverFromErrors is true we search for the first suitable job, instead of complaining that the number is wrong or the one at the front isn't suitable, this is the intended change (and basically only affects clangd).

When OffloadCompilation is true, there is a behavior change: previously we'd use the first job and fail if it's not clang, now we'll use the first clang job. The old behavior seems brittle and the change only affects (previously) error cases, and I'd have to add more complexity to preserve it - is this worthwhile?

[sammccall]

address comments

[sammccall]

add test

[kadircet]

thanks, lgtm!

clang-tools-extra/clangd/Compiler.cpp

42

not sure i follow this change. is this for making sure fatal/error diagnostic counts can be checked (so that actions can signal failure) or something else?

clang/lib/Frontend/CreateInvocationFromCommandLine.cpp

92

When OffloadCompilation is true, there is a behavior change: previously we'd use the first job and fail if it's not clang, now we'll use the first clang job.

I was talking about this particular change.

The old behavior seems brittle and the change only affects (previously) error cases

As mentioned I also like the new behaviour more, I just don't know about what the offloading logic does (i suppose it is about cuda compilation) and I was anxious about it breaking when one of the Jobs satisfy the constraint but it is not the first one.

and I'd have to add more complexity to preserve it - is this worthwhile?

One option could be to just keep this part the same (i.e. only use the first Job as long as PickFirstOfMany is set, even if OffloadCompilation is false). Then send a follow-up change, that'll search for a suitable job. That way we can only revert the last one if it breaks and know the constraints. (And decide to do the search iff ShouldRecoverOnErorrs && !Offload). But it is not that important, feel free to just land as-is.

[sammccall]

clang-tools-extra/clangd/Compiler.cpp

42

Oops, this was leftover from when we were testing this in clangd. (And was for the reason you suggested, just for testing). Removed.

clang/lib/Frontend/CreateInvocationFromCommandLine.cpp

92

It makes sense. I think i'm happy enough with this patch just getting reverted in such a case though, especially since it's just "defense in depth" currently rather than "load-bearing".

clang-tools-extra/clangd/Compiler.cpp

if (Info.hasSourceManager() && Info.getLocation().isValid()) {
     OS << ":";
   }
 
   clangd::vlog("Ignored diagnostic. {0}{1}", Location, Message);
 }
 
 void IgnoreDiagnostics::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                                          const clang::Diagnostic &Info) {
+  DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
   IgnoreDiagnostics::log(DiagLevel, Info);
 }
 
 void disableUnsupportedOptions(CompilerInvocation &CI) {
   // Disable "clang -verify" diagnostics, they are rarely useful in clangd, and
   // our compiler invocation set-up doesn't seem to work with it (leading
   // assertions in VerifyDiagnosticConsumer).
   CI.getDiagnosticOpts().VerifyDiagnostics = false;

clang-tools-extra/clangd/unittests/ClangdTests.cpp

 TEST(ClangdServerTest, InvalidCompileCommand) {
   MockFS FS;
   ErrorCheckingCallbacks DiagConsumer;
   MockCompilationDatabase CDB;
 
   ClangdServer Server(CDB, FS, ClangdServer::optsForTest(), &DiagConsumer);
 
   auto FooCpp = testPath("foo.cpp");
-  // clang cannot create CompilerInvocation if we pass two files in the
-  // CompileCommand. We pass the file in ExtraFlags once and CDB adds another
-  // one in getCompileCommand().
-  CDB.ExtraClangFlags.push_back(FooCpp);
+  // clang cannot create CompilerInvocation in this case.
+  CDB.ExtraClangFlags.push_back("-###");
 
   // Clang can't parse command args in that case, but we shouldn't crash.
   runAddDocument(Server, FooCpp, "int main() {}");
 
   EXPECT_EQ(dumpAST(Server, FooCpp), "<no-ast>");
   EXPECT_ERROR(runLocateSymbolAt(Server, FooCpp, Position()));
   EXPECT_ERROR(runFindDocumentHighlights(Server, FooCpp, Position()));
   EXPECT_ERROR(runRename(Server, FooCpp, Position(), "new_name",
int main() {
        xy^
     })cpp");
   FS.Files[FooCpp] = FooCpp;
 
   auto Opts = clangd::CodeCompleteOptions();
   Opts.RunParser = CodeCompleteOptions::ParseIfReady;
 
   // This will make compile command broken and preamble absent.
-  CDB.ExtraClangFlags = {"yolo.cc"};
+  CDB.ExtraClangFlags = {"-###"};
   Server.addDocument(FooCpp, Code.code());
   ASSERT_TRUE(Server.blockUntilIdleForTest());
   auto Res = cantFail(runCodeComplete(Server, FooCpp, Code.point(), Opts));
   EXPECT_EQ(Res.Context, CodeCompletionContext::CCC_Recovery);
   // Identifier-based fallback completion doesn't know about "symbol" scope.
   EXPECT_THAT(Res.Completions,
               ElementsAre(AllOf(Field(&CodeCompletion::Name, "xyz"),
                                 Field(&CodeCompletion::Scope, ""))));

clang/lib/Frontend/CreateInvocationFromCommandLine.cpp

for (auto &A : C->getActions()){
       if (isa<driver::BindArchAction>(A))
         A = *A->input_begin();
       if (isa<driver::OffloadAction>(A)) {
         OffloadCompilation = true;
         break;
       }
     }
   }
-  if (Jobs.size() == 0 || !isa<driver::Command>(*Jobs.begin()) ||
-      (Jobs.size() > 1 && !OffloadCompilation)) {
+
+  bool PickFirstOfMany = OffloadCompilation || ShouldRecoverOnErorrs;
+  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;
   }
-
-  const driver::Command &Cmd = cast<driver::Command>(*Jobs.begin());
-  if (StringRef(Cmd.getCreator().getName()) != "clang") {
+  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();
+  const ArgStringList &CCArgs = Cmd->getArguments();
   if (CC1Args)
     *CC1Args = {CCArgs.begin(), CCArgs.end()};
   auto CI = std::make_unique<CompilerInvocation>();
   if (!CompilerInvocation::CreateFromArgs(*CI, CCArgs, *Diags, Args[0]) &&
       !ShouldRecoverOnErorrs)
     return nullptr;
   return CI;
 }

clang/unittests/Frontend/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_unittest(FrontendTests
   ASTUnitTest.cpp
   CompilerInvocationTest.cpp
   CompilerInstanceTest.cpp
   FixedPointString.cpp
   FrontendActionTest.cpp
   CodeGenActionTest.cpp
   ParsedSourceLocationTest.cpp
   PCHPreambleTest.cpp
   OutputStreamTest.cpp
   TextDiagnosticTest.cpp
+  UtilsTest.cpp
   )
 clang_target_link_libraries(FrontendTests
   PRIVATE
   clangAST
   clangBasic
   clangFrontend
   clangLex
   clangSema
   clangCodeGen
   clangFrontendTool
   clangSerialization
   )

clang/unittests/Frontend/UtilsTest.cpp

+//===- unittests/Frontend/UtilsTest.cpp -----------------------------------===//
+//
+// 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 "clang/Frontend/Utils.h"
+#include "clang/Basic/Diagnostic.h"
+#include "clang/Basic/TargetOptions.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "llvm/Support/VirtualFileSystem.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+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);
+  ASSERT_TRUE(CI);
+  EXPECT_THAT(CI->TargetOpts->Triple, testing::StartsWith("i386-"));
+}
+
+} // namespace
+} // namespace clang