Switch to BumpPtrAllocator for C-string storage (#6550)

This keeps the allocations cheap and simplifies the code. It was
inspired by the need to expand param files, but
no functionality changed yet.

common/string_helpers.cpp

@@ -18,6 +18,7 @@
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/ConvertUTF.h"
 
 namespace Carbon {
@@ -211,33 +212,19 @@ auto StringRefContainsPointer(llvm::StringRef ref, const char* ptr) -> bool {
 
 auto BuildCStrArgs(llvm::StringRef tool_path,
                    llvm::ArrayRef<llvm::StringRef> args,
-                   llvm::OwningArrayRef<char>& cstr_arg_storage)
+                   llvm::BumpPtrAllocator& alloc)
     -> llvm::SmallVector<const char*, 64> {
-  return BuildCStrArgs(tool_path, /*prefix_args=*/{}, args, cstr_arg_storage);
+  return BuildCStrArgs(tool_path, /*prefix_args=*/{}, args, alloc);
 }
 
 auto BuildCStrArgs(llvm::StringRef tool_path,
                    llvm::ArrayRef<std::string> prefix_args,
                    llvm::ArrayRef<llvm::StringRef> args,
-                   llvm::OwningArrayRef<char>& cstr_arg_storage)
+                   llvm::BumpPtrAllocator& alloc)
     -> llvm::SmallVector<const char*, 64> {
-  // 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<char>(total_size);
   ssize_t i = 0;
   auto make_cstr = [&](llvm::StringRef arg) {
-    char* cstr = &cstr_arg_storage[i];
+    char* cstr = alloc.Allocate<char>(arg.size() + 1);
     memcpy(cstr, arg.data(), arg.size());
     cstr[arg.size()] = '\0';
     i += arg.size() + 1;

common/string_helpers.h

@@ -12,6 +12,7 @@
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
 
 namespace Carbon {
 
@@ -44,7 +45,7 @@ auto StringRefContainsPointer(llvm::StringRef ref, const char* ptr) -> bool;
 // lines to avoid extra allocations and growth passes.
 auto BuildCStrArgs(llvm::StringRef tool_path,
                    llvm::ArrayRef<llvm::StringRef> args,
-                   llvm::OwningArrayRef<char>& cstr_arg_storage)
+                   llvm::BumpPtrAllocator& alloc)
     -> llvm::SmallVector<const char*, 64>;
 
 // An overload of `BuildCStrArgs` with the same core behavior as the above, but
@@ -61,7 +62,7 @@ auto BuildCStrArgs(llvm::StringRef tool_path,
 auto BuildCStrArgs(llvm::StringRef tool_path,
                    llvm::ArrayRef<std::string> prefix_args,
                    llvm::ArrayRef<llvm::StringRef> args,
-                   llvm::OwningArrayRef<char>& cstr_arg_storage)
+                   llvm::BumpPtrAllocator& alloc)
     -> llvm::SmallVector<const char*, 64>;
 
 }  // namespace Carbon

common/string_helpers_test.cpp

@@ -12,6 +12,7 @@
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
 
 using ::testing::Eq;
 using ::testing::Optional;
@@ -233,23 +234,23 @@ TEST(ParseBlockStringLiteral, OkMultipleSlashes) {
 }
 
 TEST(BuildCStrArgs, NoArgs) {
-  llvm::OwningArrayRef<char> storage;
-  auto result = BuildCStrArgs("tool", {}, storage);
+  llvm::BumpPtrAllocator alloc;
+  auto result = BuildCStrArgs("tool", {}, alloc);
   ASSERT_THAT(result.size(), Eq(1));
   EXPECT_THAT(result[0], StrEq("tool"));
 }
 
 TEST(BuildCStrArgs, OneArg) {
-  llvm::OwningArrayRef<char> storage;
-  auto result = BuildCStrArgs("tool", {"arg1"}, storage);
+  llvm::BumpPtrAllocator alloc;
+  auto result = BuildCStrArgs("tool", {"arg1"}, alloc);
   ASSERT_THAT(result.size(), Eq(2));
   EXPECT_THAT(result[0], StrEq("tool"));
   EXPECT_THAT(result[1], StrEq("arg1"));
 }
 
 TEST(BuildCStrArgs, MultipleArgs) {
-  llvm::OwningArrayRef<char> storage;
-  auto result = BuildCStrArgs("tool", {"arg1", "arg2"}, storage);
+  llvm::BumpPtrAllocator alloc;
+  auto result = BuildCStrArgs("tool", {"arg1", "arg2"}, alloc);
   ASSERT_THAT(result.size(), Eq(3));
   EXPECT_THAT(result[0], StrEq("tool"));
   EXPECT_THAT(result[1], StrEq("arg1"));
@@ -257,16 +258,16 @@ TEST(BuildCStrArgs, MultipleArgs) {
 }
 
 TEST(BuildCStrArgsWithPrefix, NoArgs) {
-  llvm::OwningArrayRef<char> storage;
-  auto result = BuildCStrArgs("tool", {}, {}, storage);
+  llvm::BumpPtrAllocator alloc;
+  auto result = BuildCStrArgs("tool", {}, {}, alloc);
   ASSERT_THAT(result.size(), Eq(1));
   EXPECT_THAT(result[0], StrEq("tool"));
 }
 
 TEST(BuildCStrArgsWithPrefix, PrefixOnly) {
-  llvm::OwningArrayRef<char> storage;
+  llvm::BumpPtrAllocator alloc;
   std::string prefix_args[] = {"p_arg1", "p_arg2"};
-  auto result = BuildCStrArgs("tool", prefix_args, {}, storage);
+  auto result = BuildCStrArgs("tool", prefix_args, {}, alloc);
   ASSERT_THAT(result.size(), Eq(3));
   EXPECT_THAT(result[0], StrEq("tool"));
   EXPECT_THAT(result[1], Eq(prefix_args[0].c_str()));
@@ -274,8 +275,8 @@ TEST(BuildCStrArgsWithPrefix, PrefixOnly) {
 }
 
 TEST(BuildCStrArgsWithPrefix, ArgsOnly) {
-  llvm::OwningArrayRef<char> storage;
-  auto result = BuildCStrArgs("tool", {}, {"arg1", "arg2"}, storage);
+  llvm::BumpPtrAllocator alloc;
+  auto result = BuildCStrArgs("tool", {}, {"arg1", "arg2"}, alloc);
   ASSERT_THAT(result.size(), Eq(3));
   EXPECT_THAT(result[0], StrEq("tool"));
   EXPECT_THAT(result[1], StrEq("arg1"));
@@ -283,9 +284,9 @@ TEST(BuildCStrArgsWithPrefix, ArgsOnly) {
 }
 
 TEST(BuildCStrArgsWithPrefix, BothPrefixAndArgs) {
-  llvm::OwningArrayRef<char> storage;
+  llvm::BumpPtrAllocator alloc;
   std::string prefix_args[] = {"p_arg1", "p_arg2"};
-  auto result = BuildCStrArgs("tool", prefix_args, {"arg1", "arg2"}, storage);
+  auto result = BuildCStrArgs("tool", prefix_args, {"arg1", "arg2"}, alloc);
   ASSERT_THAT(result.size(), Eq(5));
   EXPECT_THAT(result[0], StrEq("tool"));
   EXPECT_THAT(result[1], Eq(prefix_args[0].c_str()));

toolchain/base/clang_invocation.cpp

@@ -95,9 +95,9 @@ auto BuildClangInvocation(Diagnostics::Consumer& consumer,
 
   // The clang driver inconveniently wants an array of `const char*`, so convert
   // the arguments.
-  llvm::OwningArrayRef<char> cstr_arg_storage;
+  llvm::BumpPtrAllocator alloc;
   llvm::SmallVector<const char*> cstr_args = BuildCStrArgs(
-      install_paths.clang_path().native(), args, extra_args, cstr_arg_storage);
+      install_paths.clang_path().native(), args, extra_args, alloc);
 
   // Build a diagnostics engine. Note that we don't have any diagnostic options
   // yet; they're produced by running the driver.

toolchain/driver/clang_runner.cpp

@@ -237,14 +237,13 @@ auto ClangRunner::RunInternal(
     std::optional<std::filesystem::path> libunwind_path,
     std::optional<std::filesystem::path> libcxx_path, bool enable_leaking)
     -> bool {
-  // Rebuild the args as C-string args.
-  llvm::OwningArrayRef<char> cstr_arg_storage;
+  llvm::BumpPtrAllocator alloc;
 
   // 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")) {
     llvm::SmallVector<const char*, 64> cstr_args =
-        BuildCStrArgs(clang_path_.native(), args, cstr_arg_storage);
+        BuildCStrArgs(clang_path_.native(), args, alloc);
     if (args[0] == "-cc1") {
       CARBON_VLOG("Dispatching `-cc1` command line...");
       int exit_code =
@@ -297,7 +296,7 @@ auto ClangRunner::RunInternal(
 
   // Rebuild the args as C-string args.
   llvm::SmallVector<const char*, 64> cstr_args =
-      BuildCStrArgs(clang_path_.native(), prefix_args, args, cstr_arg_storage);
+      BuildCStrArgs(clang_path_.native(), prefix_args, args, alloc);
 
   CARBON_VLOG("Running Clang driver with the following arguments:\n");
   for (const char* cstr_arg : llvm::ArrayRef(cstr_args)) {

toolchain/driver/lld_runner.cpp

@@ -14,6 +14,7 @@
 #include "common/vlog.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
 
 // Declare the supported driver flavor entry points.
 //
@@ -33,9 +34,9 @@ auto LldRunner::LinkHelper(llvm::StringLiteral label,
     -> bool {
   // Allocate one chunk of storage for the actual C-strings and a vector of
   // pointers into the storage.
-  llvm::OwningArrayRef<char> cstr_arg_storage;
+  llvm::BumpPtrAllocator alloc;
   llvm::SmallVector<const char*, 64> cstr_args =
-      BuildCStrArgs(path, args, cstr_arg_storage);
+      BuildCStrArgs(path, args, alloc);
 
   CARBON_VLOG("Running LLD {0}-platform link with args:\n", label);
   for (const char* cstr_arg : cstr_args) {

toolchain/driver/llvm_runner.cpp

@@ -15,6 +15,7 @@
 #include "lld/Common/Driver.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Allocator.h"
 
 namespace Carbon {
 
@@ -24,9 +25,9 @@ auto LLVMRunner::Run(LLVMTool tool, llvm::ArrayRef<llvm::StringRef> args)
 
   // Allocate one chunk of storage for the actual C-strings and a vector of
   // pointers into the storage.
-  llvm::OwningArrayRef<char> cstr_arg_storage;
+  llvm::BumpPtrAllocator alloc;
   llvm::SmallVector<const char*, 64> cstr_args =
-      BuildCStrArgs(path, args, cstr_arg_storage);
+      BuildCStrArgs(path, args, alloc);
 
   CARBON_VLOG("Running LLVM's {0} tool with args:\n", tool.name());
   for (const char* cstr_arg : cstr_args) {