From ff8ce31e1be9a1eb64b42b889fb1f8d0527da366 Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Wed, 10 Dec 2025 14:07:01 -0800 Subject: [PATCH] Factor out C-string argv building and simplify vlogs (#6478) This extracts the C-string `argv`-like building routine to a more broadly reusable location. It also sinks the verbose logging logic out of it and into the relevant runners. In turn, it simplifies the verbose logging logic significantly. The biggest functional change is removing the implicit synthesis of a tool's `-v` verbose flag from the presence of a `vlog` stream. I thought this would be helpful, but in practice of debugging these layers it has been more of a hindrance than a help -- I pretty often only want verbose logging on one side or the other, and we have ways of explicitly passing a `-v` flag to the underlying tools already. I think my instinct to do this was just wrong, so rip it out and simplify. This does add an unused feature -- prepending a prefix of arguments while building the C-string variant. This isn't used in this PR but will be used in subsequent PRs and it seemed more disruptive to undo that logic and then re-do it in a later PR. Let me know if it's too confusing here. Assisted-by: Gemini Code Assist --------- Co-authored-by: Geoff Romer --- common/string_helpers.cpp | 55 ++++++++++++++++++++++ common/string_helpers.h | 32 +++++++++++++ common/string_helpers_test.cpp | 66 +++++++++++++++++++++++++++ toolchain/driver/BUILD | 3 ++ toolchain/driver/clang_runner.cpp | 13 ++++-- toolchain/driver/lld_runner.cpp | 9 +++- toolchain/driver/llvm_runner.cpp | 11 +++-- toolchain/driver/tool_runner_base.cpp | 54 ---------------------- toolchain/driver/tool_runner_base.h | 19 -------- 9 files changed, 180 insertions(+), 82 deletions(-) diff --git a/common/string_helpers.cpp b/common/string_helpers.cpp index b6497a6e13d3..4ca000cc46c9 100644 --- a/common/string_helpers.cpp +++ b/common/string_helpers.cpp @@ -4,11 +4,18 @@ #include "common/string_helpers.h" +#include +#include + #include +#include #include #include #include "common/check.h" +#include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringExtras.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/ConvertUTF.h" @@ -202,4 +209,52 @@ auto StringRefContainsPointer(llvm::StringRef ref, const char* ptr) -> bool { return le(ref.begin(), ptr) && le(ptr, ref.end()); } +auto BuildCStrArgs(llvm::StringRef tool_path, + llvm::ArrayRef args, + llvm::OwningArrayRef& cstr_arg_storage) + -> llvm::SmallVector { + return BuildCStrArgs(tool_path, /*prefix_args=*/{}, args, cstr_arg_storage); +} + +auto BuildCStrArgs(llvm::StringRef tool_path, + llvm::ArrayRef prefix_args, + llvm::ArrayRef args, + llvm::OwningArrayRef& cstr_arg_storage) + -> llvm::SmallVector { + // Render the arguments into null-terminated C-strings. Command lines can get + // quite long in build systems so this tries to minimize the memory allocation + // overhead. + + // Precompute the total C-string data size needed. + int total_size = tool_path.size() + 1; + for (llvm::StringRef arg : args) { + // Accumulate both the string size and a null terminator byte. + total_size += arg.size() + 1; + } + + // Allocate one chunk of storage for the actual C-strings, and reserve a + // vector of pointers into the storage. + cstr_arg_storage = llvm::OwningArrayRef(total_size); + ssize_t i = 0; + auto make_cstr = [&](llvm::StringRef arg) { + char* cstr = &cstr_arg_storage[i]; + memcpy(cstr, arg.data(), arg.size()); + cstr[arg.size()] = '\0'; + i += arg.size() + 1; + return cstr; + }; + + llvm::SmallVector cstr_args; + cstr_args.reserve(1 + prefix_args.size() + args.size()); + cstr_args.push_back(make_cstr(tool_path)); + for (const std::string& prefix_arg : prefix_args) { + cstr_args.push_back(prefix_arg.c_str()); + } + for (llvm::StringRef arg : args) { + cstr_args.push_back(make_cstr(arg)); + } + + return cstr_args; +} + } // namespace Carbon diff --git a/common/string_helpers.h b/common/string_helpers.h index cbe9e2f3b724..793d836c70cb 100644 --- a/common/string_helpers.h +++ b/common/string_helpers.h @@ -9,6 +9,8 @@ #include #include "common/error.h" +#include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" namespace Carbon { @@ -32,6 +34,36 @@ auto ParseBlockStringLiteral(llvm::StringRef source, int hashtag_num = 0) // correctness. auto StringRefContainsPointer(llvm::StringRef ref, const char* ptr) -> bool; +// Converts `tool_path` and each of the `args` into C-strings and returns the +// results. This is intended for use with APIs that expect `argv`-like command +// line argument lists. +// +// Accepts a `cstr_arg_storage` that will provide the underlying storage for +// the C-strings, and returns a small vector of the C-string pointers. The +// returned small vector uses a large small size to allow most common command +// lines to avoid extra allocations and growth passes. +auto BuildCStrArgs(llvm::StringRef tool_path, + llvm::ArrayRef args, + llvm::OwningArrayRef& cstr_arg_storage) + -> llvm::SmallVector; + +// An overload of `BuildCStrArgs` with the same core behavior as the above, but +// with an extra series of `prefix_args` that are placed between the `tool_path` +// and the `args` in the resulting list. +// +// Unlike the tool path and the main `args`, the `prefix_args` are accepted as +// an array of `std::string`s and those string object's `c_str()` method is used +// to get the underlying C-strings to include in the result. This is because +// callers with prefix arguments regularly need to provide dedicated storage for +// these arguments anyways and we can efficiently reuse that. In contrast, the +// `args` are often pulled from an existing `llvm::StringRef` that may never +// exist as a valid C-string and so we need to rebuild those using the storage. +auto BuildCStrArgs(llvm::StringRef tool_path, + llvm::ArrayRef prefix_args, + llvm::ArrayRef args, + llvm::OwningArrayRef& cstr_arg_storage) + -> llvm::SmallVector; + } // namespace Carbon #endif // CARBON_COMMON_STRING_HELPERS_H_ diff --git a/common/string_helpers_test.cpp b/common/string_helpers_test.cpp index fefd494152fb..120d0ba01ca5 100644 --- a/common/string_helpers_test.cpp +++ b/common/string_helpers_test.cpp @@ -10,8 +10,12 @@ #include #include +#include "llvm/ADT/ArrayRef.h" +#include "llvm/ADT/StringRef.h" + using ::testing::Eq; using ::testing::Optional; +using ::testing::StrEq; namespace Carbon { namespace { @@ -228,5 +232,67 @@ TEST(ParseBlockStringLiteral, OkMultipleSlashes) { EXPECT_THAT(*ParseBlockStringLiteral(Input), Eq(Expected)); } +TEST(BuildCStrArgs, NoArgs) { + llvm::OwningArrayRef storage; + auto result = BuildCStrArgs("tool", {}, storage); + ASSERT_THAT(result.size(), Eq(1)); + EXPECT_THAT(result[0], StrEq("tool")); +} + +TEST(BuildCStrArgs, OneArg) { + llvm::OwningArrayRef storage; + auto result = BuildCStrArgs("tool", {"arg1"}, storage); + ASSERT_THAT(result.size(), Eq(2)); + EXPECT_THAT(result[0], StrEq("tool")); + EXPECT_THAT(result[1], StrEq("arg1")); +} + +TEST(BuildCStrArgs, MultipleArgs) { + llvm::OwningArrayRef storage; + auto result = BuildCStrArgs("tool", {"arg1", "arg2"}, storage); + ASSERT_THAT(result.size(), Eq(3)); + EXPECT_THAT(result[0], StrEq("tool")); + EXPECT_THAT(result[1], StrEq("arg1")); + EXPECT_THAT(result[2], StrEq("arg2")); +} + +TEST(BuildCStrArgsWithPrefix, NoArgs) { + llvm::OwningArrayRef storage; + auto result = BuildCStrArgs("tool", {}, {}, storage); + ASSERT_THAT(result.size(), Eq(1)); + EXPECT_THAT(result[0], StrEq("tool")); +} + +TEST(BuildCStrArgsWithPrefix, PrefixOnly) { + llvm::OwningArrayRef storage; + std::string prefix_args[] = {"p_arg1", "p_arg2"}; + auto result = BuildCStrArgs("tool", prefix_args, {}, storage); + ASSERT_THAT(result.size(), Eq(3)); + EXPECT_THAT(result[0], StrEq("tool")); + EXPECT_THAT(result[1], Eq(prefix_args[0].c_str())); + EXPECT_THAT(result[2], Eq(prefix_args[1].c_str())); +} + +TEST(BuildCStrArgsWithPrefix, ArgsOnly) { + llvm::OwningArrayRef storage; + auto result = BuildCStrArgs("tool", {}, {"arg1", "arg2"}, storage); + ASSERT_THAT(result.size(), Eq(3)); + EXPECT_THAT(result[0], StrEq("tool")); + EXPECT_THAT(result[1], StrEq("arg1")); + EXPECT_THAT(result[2], StrEq("arg2")); +} + +TEST(BuildCStrArgsWithPrefix, BothPrefixAndArgs) { + llvm::OwningArrayRef storage; + std::string prefix_args[] = {"p_arg1", "p_arg2"}; + auto result = BuildCStrArgs("tool", prefix_args, {"arg1", "arg2"}, storage); + ASSERT_THAT(result.size(), Eq(5)); + EXPECT_THAT(result[0], StrEq("tool")); + EXPECT_THAT(result[1], Eq(prefix_args[0].c_str())); + EXPECT_THAT(result[2], Eq(prefix_args[1].c_str())); + EXPECT_THAT(result[3], StrEq("arg1")); + EXPECT_THAT(result[4], StrEq("arg2")); +} + } // namespace } // namespace Carbon diff --git a/toolchain/driver/BUILD b/toolchain/driver/BUILD index f47755bacc40..eac5afdd2b96 100644 --- a/toolchain/driver/BUILD +++ b/toolchain/driver/BUILD @@ -36,6 +36,7 @@ cc_library( "//common:filesystem", "//common:latch", "//common:ostream", + "//common:string_helpers", "//common:vlog", "//third_party/llvm:clang_cc1", "//toolchain/base:install_paths", @@ -250,6 +251,7 @@ cc_library( deps = [ ":tool_runner_base", "//common:ostream", + "//common:string_helpers", "//common:vlog", "//toolchain/base:install_paths", "@llvm-project//lld:Common", @@ -288,6 +290,7 @@ cc_library( deps = [ ":tool_runner_base", "//common:ostream", + "//common:string_helpers", "//common:vlog", "//toolchain/base:install_paths", "//toolchain/base:llvm_tools", diff --git a/toolchain/driver/clang_runner.cpp b/toolchain/driver/clang_runner.cpp index 7a3574962a2d..5ac2163d6ba0 100644 --- a/toolchain/driver/clang_runner.cpp +++ b/toolchain/driver/clang_runner.cpp @@ -30,6 +30,7 @@ #include "clang/Serialization/PCHContainerOperations.h" #include "common/check.h" #include "common/error.h" +#include "common/string_helpers.h" #include "common/vlog.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/IntrusiveRefCntPtr.h" @@ -214,12 +215,13 @@ auto ClangRunner::RunInternal( // Rebuild the args as C-string args. llvm::OwningArrayRef cstr_arg_storage; llvm::SmallVector cstr_args = - BuildCStrArgs("Clang", clang_path, "-v", args, cstr_arg_storage); + BuildCStrArgs(clang_path, args, cstr_arg_storage); - // Handle special dispatch for CC1 commands as they don't use the driver. + // Handle special dispatch for CC1 commands as they don't use the driver and + // we don't synthesize any default arguments there. if (!args.empty() && args[0].starts_with("-cc1")) { if (args[0] == "-cc1") { - CARBON_VLOG("Dispatching `-cc1` command line"); + CARBON_VLOG("Dispatching `-cc1` command line..."); int exit_code = RunClangCC1(*installation_, fs_, cstr_args, enable_leaking); // TODO: Should this be forwarding the full exit code? @@ -244,7 +246,10 @@ auto ClangRunner::RunInternal( return exit_code == 0; } - CARBON_VLOG("Preparing Clang driver...\n"); + 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); + } // Create the diagnostic options and parse arguments controlling them out of // our arguments. diff --git a/toolchain/driver/lld_runner.cpp b/toolchain/driver/lld_runner.cpp index 528bdb24fb88..5bd30026e73f 100644 --- a/toolchain/driver/lld_runner.cpp +++ b/toolchain/driver/lld_runner.cpp @@ -10,6 +10,7 @@ #include #include +#include "common/string_helpers.h" #include "common/vlog.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/StringRef.h" @@ -34,9 +35,13 @@ auto LldRunner::LinkHelper(llvm::StringLiteral label, // pointers into the storage. llvm::OwningArrayRef cstr_arg_storage; llvm::SmallVector cstr_args = - BuildCStrArgs("LLD", path, "-v", args, cstr_arg_storage); + BuildCStrArgs(path, args, cstr_arg_storage); + + CARBON_VLOG("Running LLD {0}-platform link with args:\n", label); + for (const char* cstr_arg : cstr_args) { + CARBON_VLOG(" '{0}'\n", cstr_arg); + } - CARBON_VLOG("Running LLD {0}-platform link...\n", label); lld::Result result = lld::lldMain(cstr_args, llvm::outs(), llvm::errs(), {driver_def}); diff --git a/toolchain/driver/llvm_runner.cpp b/toolchain/driver/llvm_runner.cpp index c3465c61b917..d6da036b9607 100644 --- a/toolchain/driver/llvm_runner.cpp +++ b/toolchain/driver/llvm_runner.cpp @@ -10,6 +10,7 @@ #include #include +#include "common/string_helpers.h" #include "common/vlog.h" #include "lld/Common/Driver.h" #include "llvm/ADT/ArrayRef.h" @@ -24,10 +25,14 @@ auto LLVMRunner::Run(LLVMTool tool, llvm::ArrayRef args) // Allocate one chunk of storage for the actual C-strings and a vector of // pointers into the storage. llvm::OwningArrayRef cstr_arg_storage; - llvm::SmallVector cstr_args = BuildCStrArgs( - tool.name(), path, /*verbose_flag=*/std::nullopt, args, cstr_arg_storage); + llvm::SmallVector cstr_args = + BuildCStrArgs(path, args, cstr_arg_storage); + + CARBON_VLOG("Running LLVM's {0} tool with args:\n", tool.name()); + for (const char* cstr_arg : cstr_args) { + CARBON_VLOG(" '{0}'\n", cstr_arg); + } - CARBON_VLOG("Running LLVM's {0} tool...\n", tool.name()); int exit_code = tool.main_fn()( cstr_args.size(), const_cast(cstr_args.data()), {.Path = path.c_str(), .PrependArg = nullptr, .NeedsPrependArg = false}); diff --git a/toolchain/driver/tool_runner_base.cpp b/toolchain/driver/tool_runner_base.cpp index 3b9eec88ba63..88af4e9ebdf7 100644 --- a/toolchain/driver/tool_runner_base.cpp +++ b/toolchain/driver/tool_runner_base.cpp @@ -18,58 +18,4 @@ ToolRunnerBase::ToolRunnerBase(const InstallPaths* install_paths, llvm::raw_ostream* vlog_stream) : installation_(install_paths), vlog_stream_(vlog_stream) {} -auto ToolRunnerBase::BuildCStrArgs(llvm::StringRef tool_name, - llvm::StringRef tool_path, - std::optional verbose_flag, - llvm::ArrayRef args, - llvm::OwningArrayRef& cstr_arg_storage) - -> llvm::SmallVector { - // TODO: Maybe handle response file expansion similar to the Clang CLI? - - // If we have a verbose logging stream, and that stream is the same as - // `llvm::errs`, then add the `-v` flag so that the driver also prints verbose - // information. - bool inject_v_arg = verbose_flag.has_value() && vlog_stream_ == &llvm::errs(); - std::array v_arg_storage; - llvm::ArrayRef maybe_v_arg; - if (inject_v_arg) { - v_arg_storage[0] = *verbose_flag; - maybe_v_arg = v_arg_storage; - } - - CARBON_VLOG("Running {} driver with arguments:\n", tool_name); - - // Render the arguments into null-terminated C-strings. Command lines can get - // quite long in build systems so this tries to minimize the memory allocation - // overhead. - - // Provide the wrapped tool path as the synthetic `argv[0]`. - std::array exe_arg = {tool_path}; - auto args_range = - llvm::concat(exe_arg, maybe_v_arg, args); - int total_size = 0; - for (llvm::StringRef arg : args_range) { - // Accumulate both the string size and a null terminator byte. - total_size += arg.size() + 1; - } - - // Allocate one chunk of storage for the actual C-strings and a vector of - // pointers into the storage. - cstr_arg_storage = llvm::OwningArrayRef(total_size); - llvm::SmallVector cstr_args; - cstr_args.reserve(args.size() + inject_v_arg + 1); - for (ssize_t i = 0; llvm::StringRef arg : args_range) { - cstr_args.push_back(&cstr_arg_storage[i]); - memcpy(&cstr_arg_storage[i], arg.data(), arg.size()); - i += arg.size(); - cstr_arg_storage[i] = '\0'; - ++i; - } - for (const char* cstr_arg : llvm::ArrayRef(cstr_args)) { - CARBON_VLOG(" '{0}'\n", cstr_arg); - } - - return cstr_args; -} - } // namespace Carbon diff --git a/toolchain/driver/tool_runner_base.h b/toolchain/driver/tool_runner_base.h index ef741ee6c318..988789d0055f 100644 --- a/toolchain/driver/tool_runner_base.h +++ b/toolchain/driver/tool_runner_base.h @@ -32,25 +32,6 @@ class ToolRunnerBase { llvm::raw_ostream* vlog_stream = nullptr); protected: - // Translates `args` into C-string arguments for tool APIs based on `main`. - // - // Accepts a `tool_name` for logging, and a `tool_path` that will be used as - // the first C-string argument to simulate and `argv[0]` entry. - // - // Accepts a `cstr_arg_storage` that will provide the underlying storage for - // the C-strings, and returns a small vector of the C-string pointers. The - // returned small vector uses a large small size to allow most common command - // lines to avoid extra allocations and growth passes. - // - // Lastly accepts an optional `verbose_flag`. If provided, and if - // `vlog_stream_` is bound to stderr for this instance, the verbose flag will - // be injected at the start of the argument list. - auto BuildCStrArgs(llvm::StringRef tool_name, llvm::StringRef tool_path, - std::optional verbose_flag, - llvm::ArrayRef args, - llvm::OwningArrayRef& cstr_arg_storage) - -> llvm::SmallVector; - // We use protected members as this base is just factoring out common // implementation details of other runners. //