[clang][tooling] Accept Clang invocations with multiple jobs

[jansvoboda11]

Summary

When -fno-integrated-as is passed to the Clang driver (or set by default by a specific toolchain), it will construct an assembler job in addition to the cc1 job. Similarly, the -fembed-bitcode driver flag will create additional cc1 job that reads LLVM IR file.

The Clang tooling library only cares about the job that reads a source file. Instead of relying on the fact that the client injected -fsyntax-only to the driver invocation to get a single -cc1 invocation that reads the source file, this patch filters out such jobs from Compilation automatically and ignores the rest.

This fixes a test failure in ClangScanDeps/headerwithname.cpp and ClangScanDeps/headerwithnamefollowedbyinclude.cpp on AIX reported here: https://reviews.llvm.org/D103461#2841918 and clang-scan-deps failures with -fembed-bitcode.

Depends on D106788.

[dexonsmith]

clang/lib/Tooling/Tooling.cpp

127–130

Seems like this could (unexpectedly?) let through a command with multiple -cc1s (if I'm reading the code right). Consider these two cases:

% clang -x c a.c b.c -c -fno-integrated-as -###
% clang -arch x86_64 -arch arm64 -x c /dev/null -c -fno-integrated-as -###

The first compiles two separate files; the second compiles a file with two separate architectures. But IIUC, the logic below doesn't expect this and will now let them through (ultimately, there's no reason scan-deps couldn't be updated to handle this! but that's not in scope for this patch, and maybe doesn't apply generally to clang-tooling).

Also, I'm not sure it's good that it was rejected, but I think the previous code would reject the following command-line and the new code will accept it:

% clang -x c /dev/null -###

I wonder if it'd be simpler to search for a supported action, and reject if zero or multiple are found. Something like:

const driver::Command *Cmd = nullptr;
for (const Action *A : Actions) {
  A = lookThrough(A);
  if (!shouldIgnore(*A)) // ignore some actions...
    continue;
  if (shouldReject(*A)) // if needed, error on some actions...
    return error(...);

  if (Cmd) // error if we hit a second candidate
    return error(...);
  Cmd = cast<driver::Command>(*A);
}
if (!Cmd) // error if we found no candidate
  return error(...);

[jansvoboda11]

Handle cases with multiple input files and architectures, add tests.

[dexonsmith]

LGTM!

[jansvoboda11]

Also ignore cc1 jobs that have inputs that are not source files.

[jansvoboda11]

Requesting re-review, since there are two changes:

• -cc1 commands that don't read a source file are ignored (e.g. jobs generated by -fembed-bitcode),
• test now have -target arm64-apple-macosx11.0.0 to ensure things work platforms that don't have external assembler.

[jansvoboda11]

Formatting

[dexonsmith]

Seeing the -fembed-bitcode case made me think of -save-temps. I think this will work since -x cpp-output should return false for isSrcFile()... but probably worth adding a test. LGTM assuming the test passes.

[jansvoboda11]

In D105695#2905028, @dexonsmith wrote:

Seeing the -fembed-bitcode case made me think of -save-temps. I think this will work since -x cpp-output should return false for isSrcFile()... but probably worth adding a test. LGTM assuming the test passes.

Thanks for pointing that out. Things work as expected, committing with the added test.

clang/include/clang/Tooling/Tooling.h

 class Compilation;
 
 } // namespace driver
 
 namespace tooling {
 
 class CompilationDatabase;
 
+/// Retrieves the clang CC1 specific flags out of the compilation's jobs.
+/// Returns nullptr on error.
+const llvm::opt::ArgStringList *getCC1Arguments(
+    DiagnosticsEngine *Diagnostics, driver::Compilation *Compilation);
+
 /// Interface to process a clang::CompilerInvocation.
 ///
 /// If your tool is based on FrontendAction, you should be deriving from
 /// FrontendActionFactory instead.
 class ToolAction {
 public:
   virtual ~ToolAction();
 

clang/lib/Tooling/Tooling.cpp

newDriver(DiagnosticsEngine *Diagnostics, const char *BinaryName,
           IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS) {
   driver::Driver *CompilerDriver =
       new driver::Driver(BinaryName, llvm::sys::getDefaultTargetTriple(),
                          *Diagnostics, "clang LLVM compiler", std::move(VFS));
   CompilerDriver->setTitle("clang_based_tool");
   return CompilerDriver;
 }
 
-/// Retrieves the clang CC1 specific flags out of the compilation's jobs.
-///
-/// Returns nullptr on error.
-static const llvm::opt::ArgStringList *getCC1Arguments(
-    DiagnosticsEngine *Diagnostics, driver::Compilation *Compilation) {
-  // We expect to get back exactly one Command job, if we didn't something
-  // failed. Extract that job from the Compilation.
+/// Determine whether the given command is invoking the compiler frontend.
+static bool isCC1Command(const driver::Command &Cmd) {
+  return StringRef(Cmd.getCreator().getName()) == "clang";
+}
+
+/// Decide whether extra compiler frontend commands can be ignored.
+static bool ignoreExtraCC1Commands(const driver::Compilation *Compilation) {
   const driver::JobList &Jobs = Compilation->getJobs();
   const driver::ActionList &Actions = Compilation->getActions();
+
   bool OffloadCompilation = false;
+
+  // Jobs and Actions look very different depending on whether the Clang tool
+  // injected -fsyntax-only or not. Try to handle both cases here.
+
+  for (const auto &Job : Jobs)
+    if (StringRef(Job.getExecutable()) == "clang-offload-bundler")
+      OffloadCompilation = true;
+
   if (Jobs.size() > 1) {
     for (auto A : Actions){
       // On MacOSX real actions may end up being wrapped in BindArchAction
       if (isa<driver::BindArchAction>(A))
         A = *A->input_begin();
       if (isa<driver::OffloadAction>(A)) {
         // Offload compilation has 2 top-level actions, one (at the front) is
         // the original host compilation and the other is offload action
         // composed of at least one device compilation. For such case, general
         // tooling will consider host-compilation only. For tooling on device
         // compilation, device compilation only option, such as
         // `--cuda-device-only`, needs specifying.
         assert(Actions.size() > 1);
         assert(
             isa<driver::CompileJobAction>(Actions.front()) ||
             // On MacOSX real actions may end up being wrapped in
             // BindArchAction.
             (isa<driver::BindArchAction>(Actions.front()) &&
              isa<driver::CompileJobAction>(*Actions.front()->input_begin())));
         OffloadCompilation = true;
         break;
       }
     }
   }
-  if (Jobs.size() == 0 || !isa<driver::Command>(*Jobs.begin()) ||
-      (Jobs.size() > 1 && !OffloadCompilation)) {
+
+  return OffloadCompilation;
+}
+
+namespace clang {
+namespace tooling {
+
+const llvm::opt::ArgStringList *getCC1Arguments(
+    DiagnosticsEngine *Diagnostics, driver::Compilation *Compilation) {
+  const driver::JobList &Jobs = Compilation->getJobs();
+
+  llvm::SmallVector<const driver::Command *, 1> CC1Jobs;
+  for (const driver::Command &Job : Jobs)
+    if (isCC1Command(Job))
+      CC1Jobs.push_back(&Job);
+
+  if (CC1Jobs.empty() ||
+      (CC1Jobs.size() > 1 && !ignoreExtraCC1Commands(Compilation))) {
     SmallString<256> error_msg;
     llvm::raw_svector_ostream error_stream(error_msg);
     Jobs.Print(error_stream, "; ", true);
     Diagnostics->Report(diag::err_fe_expected_compiler_job)
         << error_stream.str();
     return nullptr;
   }
 
-  // The one job we find should be to invoke clang again.
-  const auto &Cmd = cast<driver::Command>(*Jobs.begin());
-  if (StringRef(Cmd.getCreator().getName()) != "clang") {
-    Diagnostics->Report(diag::err_fe_expected_clang_command);
-    return nullptr;
-  }
-
-  return &Cmd.getArguments();
+  return &CC1Jobs[0]->getArguments();
 }
 
-namespace clang {
-namespace tooling {
-
 /// Returns a clang build invocation initialized from the CC1 flags.
 CompilerInvocation *newInvocation(DiagnosticsEngine *Diagnostics,
                                   const llvm::opt::ArgStringList &CC1Args,
                                   const char *const BinaryName) {
   assert(!CC1Args.empty() && "Must at least contain the program name!");
   CompilerInvocation *Invocation = new CompilerInvocation;
   CompilerInvocation::CreateFromArgs(*Invocation, CC1Args, *Diagnostics,
                                      BinaryName);

clang/test/Tooling/clang-check-offload.cpp

-// RUN: not clang-check "%s" -- -c -x hip -nogpulib 2>&1 | FileCheck %s
-
-// CHECK: C++ requires
-invalid;

clang/unittests/Tooling/ToolingTest.cpp

 //===- unittest/Tooling/ToolingTest.cpp - Tooling unit tests --------------===//
 //
 // 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/AST/ASTConsumer.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclGroup.h"
+#include "clang/Driver/Compilation.h"
+#include "clang/Driver/Driver.h"
 #include "clang/Frontend/ASTUnit.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Host.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/TargetSelect.h"
 #include "gtest/gtest.h"
 #include <algorithm>
 #include <string>
 #include <vector>
 
TEST(ToolInvocation, DiagConsumerExpectingSourceManager) {
 
   DiagnosticConsumerExpectingSourceManager Consumer;
   Invocation.setDiagnosticConsumer(&Consumer);
 
   EXPECT_TRUE(Invocation.run());
   EXPECT_TRUE(Consumer.SawSourceManager);
 }
 
+namespace {
+/// Overlays the real filesystem with the given VFS and returns the result.
+llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem>
+overlayRealFS(llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS) {
+  auto RFS = llvm::vfs::getRealFileSystem();
+  auto OverlayFS = llvm::makeIntrusiveRefCnt<llvm::vfs::OverlayFileSystem>(RFS);
+  OverlayFS->pushOverlay(VFS);
+  return OverlayFS;
+}
+
+struct CommandLineExtractorTest : public ::testing::Test {
+  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFS;
+  llvm::IntrusiveRefCntPtr<DiagnosticsEngine> Diags;
+  driver::Driver Driver;
+
+public:
+  CommandLineExtractorTest()
+      : InMemoryFS(new llvm::vfs::InMemoryFileSystem),
+        Diags(CompilerInstance::createDiagnostics(new DiagnosticOptions)),
+        Driver("clang", llvm::sys::getDefaultTargetTriple(), *Diags,
+               "clang LLVM compiler", overlayRealFS(InMemoryFS)) {}
+
+  void addFile(StringRef Name, StringRef Content) {
+    InMemoryFS->addFile(Name, 0, llvm::MemoryBuffer::getMemBuffer(Content));
+  }
+
+  const llvm::opt::ArgStringList *
+  extractCC1Arguments(llvm::ArrayRef<const char *> Argv) {
+    const std::unique_ptr<driver::Compilation> Compilation(
+        Driver.BuildCompilation(llvm::makeArrayRef(Argv)));
+
+    return getCC1Arguments(Diags.get(), Compilation.get());
+  }
+};
+} // namespace
+
+
+TEST_F(CommandLineExtractorTest, AcceptOffloading) {
+  addFile("test.c", "int main() {}\n");
+  const char *Args[] = {"clang",  "-x",        "hip",
+                        "test.c", "-nogpulib", "-nogpuinc"};
+  EXPECT_NE(extractCC1Arguments(Args), nullptr);
+}
+
+TEST_F(CommandLineExtractorTest, AcceptOffloadingCompile) {
+  addFile("test.c", "int main() {}\n");
+  const char *Args[] = {"clang",  "-c",        "-x",       "hip",
+                        "test.c", "-nogpulib", "-nogpuinc"};
+  EXPECT_NE(extractCC1Arguments(Args), nullptr);
+}
+
+TEST_F(CommandLineExtractorTest, AcceptOffloadingSyntaxOnly) {
+  addFile("test.c", "int main() {}\n");
+  const char *Args[] = {"clang",  "-fsyntax-only", "-x",       "hip",
+                        "test.c", "-nogpulib",     "-nogpuinc"};
+  EXPECT_NE(extractCC1Arguments(Args), nullptr);
+}
+
+TEST_F(CommandLineExtractorTest, AcceptExternalAssembler) {
+  addFile("test.c", "int main() {}\n");
+  const char *Args[] = {"clang", "-fno-integrated-as", "-c", "test.c"};
+  EXPECT_NE(extractCC1Arguments(Args), nullptr);
+}
+
+TEST_F(CommandLineExtractorTest, RejectMultipleArchitectures) {
+  addFile("test.c", "int main() {}\n");
+  const char *Args[] = {"clang", "-arch", "x86_64", "-arch",
+                        "arm64", "-c",    "test.c"};
+  EXPECT_EQ(extractCC1Arguments(Args), nullptr);
+}
+
+TEST_F(CommandLineExtractorTest, RejectMultipleInputFiles) {
+  addFile("one.c", "void one() {}\n");
+  addFile("two.c", "void two() {}\n");
+  const char *Args[] = {"clang", "-c", "one.c", "two.c"};
+  EXPECT_EQ(extractCC1Arguments(Args), nullptr);
+}
+
 struct VerifyEndCallback : public SourceFileCallbacks {
   VerifyEndCallback() : BeginCalled(0), EndCalled(0), Matched(false) {}
   bool handleBeginSource(CompilerInstance &CI) override {
     ++BeginCalled;
     return true;
   }
   void handleEndSource() override { ++EndCalled; }
   std::unique_ptr<ASTConsumer> newASTConsumer() {