Teach the Clang runner to expand response files (#6555)

This uses the existing Clang driver APIs for expanding response files
and so should be pretty carefully accurate to what is needed here.

Note that this doesn't try to generalize the expansion more widely for
the interop Clang invocation, but it would be straightforward to do so
if needed at some point.

toolchain/driver/BUILD

@@ -70,6 +70,7 @@ cc_test(
         ":runtimes_cache",
         "//common:all_llvm_targets",
         "//common:check",
+        "//common:error_test_helpers",
         "//common:ostream",
         "//common:raw_string_ostream",
         "//testing/base:capture_std_streams",

toolchain/driver/clang_runner.cpp

@@ -223,7 +223,7 @@ auto ClangRunner::Run(llvm::ArrayRef<llvm::StringRef> args,
 }
 
 auto ClangRunner::RunWithNoRuntimes(llvm::ArrayRef<llvm::StringRef> args,
-                                    bool enable_leaking) -> bool {
+                                    bool enable_leaking) -> ErrorOr<bool> {
   std::string target = ComputeClangTarget(args);
   return RunInternal(args, target, /*target_resource_dir_path=*/std::nullopt,
                      /*libunwind_path=*/std::nullopt,
@@ -236,7 +236,7 @@ auto ClangRunner::RunInternal(
 
     std::optional<std::filesystem::path> libunwind_path,
     std::optional<std::filesystem::path> libcxx_path, bool enable_leaking)
-    -> bool {
+    -> ErrorOr<bool> {
   llvm::BumpPtrAllocator alloc;
 
   // Handle special dispatch for CC1 commands as they don't use the driver and
@@ -298,6 +298,14 @@ auto ClangRunner::RunInternal(
   llvm::SmallVector<const char*, 64> cstr_args =
       BuildCStrArgs(clang_path_.native(), prefix_args, args, alloc);
 
+  // Expand any response files in the arguments.
+  bool is_clang_cl_mode = clang::driver::IsClangCL(
+      clang::driver::getDriverMode(clang_path_.native(), cstr_args));
+  if (llvm::Error error = clang::driver::expandResponseFiles(
+          cstr_args, is_clang_cl_mode, alloc, fs_.get())) {
+    return Error(llvm::toString(std::move(error)));
+  }
+
   CARBON_VLOG("Running Clang driver with the following arguments:\n");
   for (const char* cstr_arg : llvm::ArrayRef(cstr_args)) {
     CARBON_VLOG("    '{0}'\n", cstr_arg);

toolchain/driver/clang_runner.h

@@ -77,8 +77,8 @@ class ClangRunner : ToolRunnerBase {
   // both to use and incorporate into the cache.
   //
   // Returns an error only if unable to successfully run Clang with the
-  // arguments. If able to run Clang, no error is returned a bool indicating
-  // whether than Clang invocation succeeded is returned.
+  // arguments. If able to run Clang, no error is returned, and a bool
+  // indicating whether than Clang invocation succeeded is returned.
   auto Run(llvm::ArrayRef<llvm::StringRef> args,
            Runtimes::Cache& runtimes_cache,
            llvm::ThreadPoolInterface& runtimes_build_thread_pool,
@@ -94,15 +94,14 @@ class ClangRunner : ToolRunnerBase {
 
   // Run Clang with the provided arguments and without any target runtimes.
   //
+  // Similar to `Run`, but omits any target runtimes.
+  //
   // This method can be used to avoid building target-dependent resources when
   // unnecessary, but not all Clang command lines will work correctly.
   // Specifically, compile-only commands will typically work, while linking will
   // not.
-  //
-  // This function simply returns true or false depending on whether Clang runs
-  // successfully, as it should display any needed error messages.
   auto RunWithNoRuntimes(llvm::ArrayRef<llvm::StringRef> args,
-                         bool enable_leaking = false) -> bool;
+                         bool enable_leaking = false) -> ErrorOr<bool>;
 
  private:
   friend class ClangRuntimesBuilderBase;
@@ -112,7 +111,7 @@ class ClangRunner : ToolRunnerBase {
                    std::optional<llvm::StringRef> target_resource_dir_path,
                    std::optional<std::filesystem::path> libunwind_path,
                    std::optional<std::filesystem::path> libcxx_path,
-                   bool enable_leaking) -> bool;
+                   bool enable_leaking) -> ErrorOr<bool>;
 
   // Returns the target-specific source files for the builtins runtime library.
   auto CollectBuiltinsSrcFiles(const llvm::Triple& target_triple)

toolchain/driver/clang_runner_test.cpp

@@ -11,6 +11,7 @@
 #include <string>
 #include <utility>
 
+#include "common/error_test_helpers.h"
 #include "common/ostream.h"
 #include "common/raw_string_ostream.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
@@ -29,6 +30,7 @@ namespace Carbon {
 namespace {
 
 using ::testing::HasSubstr;
+using Testing::IsSuccess;
 using ::testing::StrEq;
 
 // NOLINTNEXTLINE(modernize-use-trailing-return-type): Macro based function.
@@ -71,8 +73,10 @@ TEST_F(ClangRunnerTest, Version) {
 
   std::string out;
   std::string err;
-  EXPECT_TRUE(Testing::CallWithCapturedOutput(
-      out, err, [&] { return runner.RunWithNoRuntimes({"--version"}); }));
+  EXPECT_THAT(
+      Testing::CallWithCapturedOutput(
+          out, err, [&] { return runner.RunWithNoRuntimes({"--version"}); }),
+      IsSuccess(true));
   // The arguments to Clang should be part of the verbose log.
   EXPECT_THAT(test_os.TakeStr(), HasSubstr("--version"));
 
@@ -100,12 +104,13 @@ TEST_F(ClangRunnerTest, DashC) {
   ClangRunner runner(&install_paths_, vfs_, &verbose_out);
   std::string out;
   std::string err;
-  EXPECT_TRUE(Testing::CallWithCapturedOutput(
-      out, err,
-      [&] {
-        return runner.RunWithNoRuntimes(
-            {"-c", test_file.string(), "-o", test_output.string()});
-      }))
+  EXPECT_THAT(Testing::CallWithCapturedOutput(
+                  out, err,
+                  [&] {
+                    return runner.RunWithNoRuntimes(
+                        {"-c", test_file.string(), "-o", test_output.string()});
+                  }),
+              IsSuccess(true))
       << "Verbose output from runner:\n"
       << verbose_out.TakeStr() << "\n";
   verbose_out.clear();
@@ -129,12 +134,13 @@ TEST_F(ClangRunnerTest, BuitinHeaders) {
   ClangRunner runner(&install_paths_, vfs_, &verbose_out);
   std::string out;
   std::string err;
-  EXPECT_TRUE(Testing::CallWithCapturedOutput(
-      out, err,
-      [&] {
-        return runner.RunWithNoRuntimes(
-            {"-c", test_file.string(), "-o", test_output.string()});
-      }))
+  EXPECT_THAT(Testing::CallWithCapturedOutput(
+                  out, err,
+                  [&] {
+                    return runner.RunWithNoRuntimes(
+                        {"-c", test_file.string(), "-o", test_output.string()});
+                  }),
+              IsSuccess(true))
       << "Verbose output from runner:\n"
       << verbose_out.TakeStr() << "\n";
   verbose_out.clear();
@@ -156,12 +162,13 @@ TEST_F(ClangRunnerTest, CompileMultipleFiles) {
     ClangRunner runner(&install_paths_, vfs_, &verbose_out);
     std::string out;
     std::string err;
-    EXPECT_TRUE(Testing::CallWithCapturedOutput(
-        out, err,
-        [&] {
-          return runner.RunWithNoRuntimes(
-              {"-c", file.string(), "-o", output.string()});
-        }))
+    EXPECT_THAT(Testing::CallWithCapturedOutput(
+                    out, err,
+                    [&] {
+                      return runner.RunWithNoRuntimes(
+                          {"-c", file.string(), "-o", output.string()});
+                    }),
+                IsSuccess(true))
         << "Verbose output from runner:\n"
         << verbose_out.TakeStr() << "\n";
     verbose_out.clear();
@@ -191,16 +198,18 @@ TEST_F(ClangRunnerTest, LinkCommandEcho) {
   ClangRunner runner(&install_paths_, vfs_, &verbose_out);
   std::string out;
   std::string err;
-  EXPECT_TRUE(Testing::CallWithCapturedOutput(
-      out, err,
-      [&] {
-        // Note that we use the target independent run command here because
-        // we're just getting the echo-ed output back. For this to actually
-        // link, we'd need to have the target-dependent resources, but those are
-        // expensive to build so we only want to test them once (above).
-        return runner.RunWithNoRuntimes(
-            {"-###", "-o", "binary", foo_file.string(), bar_file.string()});
-      }))
+  EXPECT_THAT(
+      Testing::CallWithCapturedOutput(
+          out, err,
+          [&] {
+            // Note that we use the target independent run command here because
+            // we're just getting the echo-ed output back. For this to actually
+            // link, we'd need to have the target-dependent resources, but those
+            // are expensive to build so we only want to test them once (above).
+            return runner.RunWithNoRuntimes(
+                {"-###", "-o", "binary", foo_file.string(), bar_file.string()});
+          }),
+      IsSuccess(true))
       << "Verbose output from runner:\n"
       << verbose_out.TakeStr() << "\n";
   verbose_out.clear();
@@ -216,5 +225,41 @@ TEST_F(ClangRunnerTest, LinkCommandEcho) {
   EXPECT_THAT(out, StrEq(""));
 }
 
+TEST_F(ClangRunnerTest, ParamsFile) {
+  // Use an overlay file system to ensure the params file expansion goes through
+  // the VFS.
+  llvm::IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> overlay_fs(
+      new llvm::vfs::OverlayFileSystem(vfs_));
+  llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> in_memory_fs(
+      new llvm::vfs::InMemoryFileSystem);
+  overlay_fs->pushOverlay(in_memory_fs);
+
+  std::filesystem::path params_path = "/params";
+  in_memory_fs->addFile(params_path.native(), 0,
+                        llvm::MemoryBuffer::getMemBuffer(R"(
+--version
+)"));
+
+  RawStringOstream verbose_out;
+  ClangRunner runner(&install_paths_, overlay_fs, &verbose_out);
+
+  std::string out;
+  std::string err;
+  EXPECT_THAT(
+      Testing::CallWithCapturedOutput(out, err,
+                                      [&] {
+                                        return runner.RunWithNoRuntimes(
+                                            {"@" + params_path.native()});
+                                      }),
+      IsSuccess(true))
+      << "Verbose output:\n"
+      << verbose_out.TakeStr();
+  verbose_out.clear();
+
+  // Check that the version is printed, as if we directly passed `--version`.
+  EXPECT_THAT(err, StrEq(""));
+  EXPECT_THAT(out, HasSubstr("clang version"));
+}
+
 }  // namespace
 }  // namespace Carbon

toolchain/driver/clang_runtimes.cpp

@@ -196,7 +196,9 @@ auto ClangRuntimesBuilderBase::ArchiveBuilder::CompileMember(
       obj_path.native(),
       src_path.native(),
   });
-  if (!builder_->clang_->RunWithNoRuntimes(args)) {
+  CARBON_ASSIGN_OR_RETURN(bool success,
+                          builder_->clang_->RunWithNoRuntimes(args));
+  if (!success) {
     return Error(
         llvm::formatv("Failed to compile runtime source file '{0}'", src_file));
   }
@@ -583,21 +585,21 @@ auto ClangResourceDirBuilder::BuildCrtFile(llvm::StringRef src_file)
       installation().runtimes_root() / std::string_view(src_file);
   CARBON_VLOG("Building `{0}' from `{1}`...\n", out_path, src_path);
 
-  bool success = clang_->RunWithNoRuntimes({
-      "-no-canonical-prefixes",
-      "-DCRT_HAS_INITFINI_ARRAY",
-      "-DEH_USE_FRAME_REGISTRY",
-      "-O3",
-      "-fPIC",
-      "-ffreestanding",
-      "-std=c11",
-      "-w",
-      "-c",
-      target_flag_,
-      "-o",
-      out_path.native(),
-      src_path.native(),
-  });
+  CARBON_ASSIGN_OR_RETURN(bool success, clang_->RunWithNoRuntimes({
+                                            "-no-canonical-prefixes",
+                                            "-DCRT_HAS_INITFINI_ARRAY",
+                                            "-DEH_USE_FRAME_REGISTRY",
+                                            "-O3",
+                                            "-fPIC",
+                                            "-ffreestanding",
+                                            "-std=c11",
+                                            "-w",
+                                            "-c",
+                                            target_flag_,
+                                            "-o",
+                                            out_path.native(),
+                                            src_path.native(),
+                                        }));
 
   if (success) {
     return Success();

toolchain/driver/lld_runner_test.cpp

@@ -89,7 +89,7 @@ static auto CompileTwoSources(const InstallPaths& install_paths,
   std::string target_arg = llvm::formatv("--target={0}", target).str();
   std::string out;
   std::string err;
-  CARBON_CHECK(Testing::CallWithCapturedOutput(
+  CARBON_CHECK(*Testing::CallWithCapturedOutput(
                    out, err,
                    [&] {
                      auto run_result = clang.RunWithNoRuntimes(
@@ -101,7 +101,7 @@ static auto CompileTwoSources(const InstallPaths& install_paths,
                verbose_out.TakeStr(), err);
   verbose_out.clear();
 
-  CARBON_CHECK(Testing::CallWithCapturedOutput(
+  CARBON_CHECK(*Testing::CallWithCapturedOutput(
                    out, err,
                    [&] {
                      auto run_result = clang.RunWithNoRuntimes(