From be884a8be0c23c2877d82bac8fbe81f10a011f97 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Tue, 6 Jan 2026 22:53:28 -0800 Subject: [PATCH] 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 | 1 + toolchain/driver/clang_runner.cpp | 12 ++- toolchain/driver/clang_runner.h | 13 ++- toolchain/driver/clang_runner_test.cpp | 105 ++++++++++++++++++------- toolchain/driver/clang_runtimes.cpp | 34 ++++---- toolchain/driver/lld_runner_test.cpp | 4 +- 6 files changed, 112 insertions(+), 57 deletions(-) diff --git a/toolchain/driver/BUILD b/toolchain/driver/BUILD index dd9129aff16e..d006700fb7a0 100644 --- a/toolchain/driver/BUILD +++ b/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", diff --git a/toolchain/driver/clang_runner.cpp b/toolchain/driver/clang_runner.cpp index 34900de007d3..16bb4b64ec27 100644 --- a/toolchain/driver/clang_runner.cpp +++ b/toolchain/driver/clang_runner.cpp @@ -223,7 +223,7 @@ auto ClangRunner::Run(llvm::ArrayRef args, } auto ClangRunner::RunWithNoRuntimes(llvm::ArrayRef args, - bool enable_leaking) -> bool { + bool enable_leaking) -> ErrorOr { 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 libunwind_path, std::optional libcxx_path, bool enable_leaking) - -> bool { + -> ErrorOr { 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 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); diff --git a/toolchain/driver/clang_runner.h b/toolchain/driver/clang_runner.h index 3d9a820fa47e..a59a02917d5c 100644 --- a/toolchain/driver/clang_runner.h +++ b/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 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 args, - bool enable_leaking = false) -> bool; + bool enable_leaking = false) -> ErrorOr; private: friend class ClangRuntimesBuilderBase; @@ -112,7 +111,7 @@ class ClangRunner : ToolRunnerBase { std::optional target_resource_dir_path, std::optional libunwind_path, std::optional libcxx_path, - bool enable_leaking) -> bool; + bool enable_leaking) -> ErrorOr; // Returns the target-specific source files for the builtins runtime library. auto CollectBuiltinsSrcFiles(const llvm::Triple& target_triple) diff --git a/toolchain/driver/clang_runner_test.cpp b/toolchain/driver/clang_runner_test.cpp index 03daed08479f..8567c2752e9a 100644 --- a/toolchain/driver/clang_runner_test.cpp +++ b/toolchain/driver/clang_runner_test.cpp @@ -11,6 +11,7 @@ #include #include +#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 overlay_fs( + new llvm::vfs::OverlayFileSystem(vfs_)); + llvm::IntrusiveRefCntPtr 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 diff --git a/toolchain/driver/clang_runtimes.cpp b/toolchain/driver/clang_runtimes.cpp index 3bf52f40a89c..b555ea86e383 100644 --- a/toolchain/driver/clang_runtimes.cpp +++ b/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(); diff --git a/toolchain/driver/lld_runner_test.cpp b/toolchain/driver/lld_runner_test.cpp index ad07ba1cbde7..c576a70723e7 100644 --- a/toolchain/driver/lld_runner_test.cpp +++ b/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(