From 383cfbb023895949114fea1895dbc0d25d7ea9bc Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Mon, 6 Jul 2026 23:52:57 -0700 Subject: [PATCH] Fix uniform identifier generation for lengths over 64 (#7459) `GetIdentifiersImpl` unconditionally sliced the 64-entry `IdentifierLengthCounts` table even for uniform distributions, which the API documents as having no `max_length` limit. Requesting a uniform distribution with `max_length > 64` therefore tripped an out-of-bounds slice assertion. Only compute the table slice on the non-uniform path, where `max_length <= 64` is already enforced. Add `IdentifierByteSumStableAcrossSeeds`, which exercises this path (a uniform request up to length 200) and checks the core invariant that the total identifier byte count is independent of the random seed across a spread of parameters. Assisted-by: Claude Code --------- Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com> --- toolchain/benchmarking/source_gen.cpp | 12 ++- toolchain/benchmarking/source_gen_test.cpp | 85 ++++++++++++++++++++++ 2 files changed, 93 insertions(+), 4 deletions(-) diff --git a/toolchain/benchmarking/source_gen.cpp b/toolchain/benchmarking/source_gen.cpp index ca914b966a08..cc3d943dd096 100644 --- a/toolchain/benchmarking/source_gen.cpp +++ b/toolchain/benchmarking/source_gen.cpp @@ -654,11 +654,15 @@ auto SourceGen::GetIdentifiersImpl(int number, int min_length, int max_length, idents.reserve(number); // First, compute the total weight of the distribution so we know how many - // identifiers we'll get each time we collect from it. + // identifiers we'll get each time we collect from it. For a uniform + // distribution every length has weight one, so the sum is simply the number + // of lengths; this also avoids indexing the bounded `IdentifierLengthCounts` + // table, which only covers lengths up to 64 and which uniform callers are + // allowed to exceed. int num_lengths = max_length - min_length + 1; - auto length_counts = - llvm::ArrayRef(IdentifierLengthCounts).slice(min_length - 1, num_lengths); - int count_sum = uniform ? num_lengths : Sum(length_counts); + int count_sum = uniform ? num_lengths + : Sum(llvm::ArrayRef(IdentifierLengthCounts) + .slice(min_length - 1, num_lengths)); CARBON_CHECK(count_sum >= 1); int number_rem = number % count_sum; diff --git a/toolchain/benchmarking/source_gen_test.cpp b/toolchain/benchmarking/source_gen_test.cpp index 6e11a19655e0..7f8ba8a3a62f 100644 --- a/toolchain/benchmarking/source_gen_test.cpp +++ b/toolchain/benchmarking/source_gen_test.cpp @@ -7,9 +7,11 @@ #include #include +#include #include #include "common/set.h" +#include "llvm/Support/FormatVariadic.h" #include "testing/base/global_exe_path.h" #include "toolchain/base/install_paths_test_helpers.h" #include "toolchain/driver/driver.h" @@ -87,6 +89,89 @@ TEST(SourceGenTest, Identifiers) { EXPECT_THAT(idents, Each(SizeIs(AllOf(Ge(10), Le(20))))); } +// For fixed parameters, the total number of bytes across the returned +// identifiers must not depend on the random seed, even though the specific +// identifiers do. This checks that across a range of parameters and across many +// freshly-seeded generators (each `SourceGen` gets an independent random seed). +TEST(SourceGenTest, IdentifierByteSumStableAcrossSeeds) { + struct Config { + int number; + int min_length; + int max_length; + bool uniform; + bool unique; + }; + // A spread of parameters including: the default range, narrow ranges, the + // single-length extreme, uniform distributions, and a uniform range with a + // `max_length` well beyond the 64 limit that only the uniform path allows. + Config configs[] = { + {.number = 1000, .min_length = 1, .max_length = 64, .uniform = false}, + {.number = 1000, .min_length = 4, .max_length = 64, .uniform = false}, + {.number = 999, .min_length = 1, .max_length = 64, .uniform = false}, + {.number = 1000, .min_length = 10, .max_length = 20, .uniform = false}, + {.number = 1000, .min_length = 8, .max_length = 8, .uniform = false}, + {.number = 100, .min_length = 10, .max_length = 19, .uniform = true}, + {.number = 97, .min_length = 10, .max_length = 19, .uniform = true}, + {.number = 500, .min_length = 50, .max_length = 200, .uniform = true}, + {.number = 1000, + .min_length = 4, + .max_length = 64, + .uniform = false, + .unique = true}, + {.number = 1000, + .min_length = 4, + .max_length = 4, + .uniform = false, + .unique = true}, + {.number = 200, + .min_length = 30, + .max_length = 120, + .uniform = true, + .unique = true}, + }; + + for (const Config& c : configs) { + SCOPED_TRACE(llvm::formatv( + "Config: number={0} min_length={1} max_length={2} uniform={3} " + "unique={4}", + c.number, c.min_length, c.max_length, c.uniform, c.unique)); + std::optional expected_sum; + bool any_different = false; + std::optional> first; + constexpr int NumSeeds = 8; + for (int seed : llvm::seq(NumSeeds)) { + // Each iteration constructs a fresh generator with an independent random + // seed; the traced index identifies which iteration failed. + SCOPED_TRACE(llvm::formatv("Seed iteration: {0}", seed)); + SourceGen gen; + auto idents = c.unique + ? gen.GetShuffledUniqueIdentifiers( + c.number, c.min_length, c.max_length, c.uniform) + : gen.GetShuffledIdentifiers(c.number, c.min_length, + c.max_length, c.uniform); + EXPECT_THAT(idents, SizeIs(c.number)); + EXPECT_THAT(idents, + Each(SizeIs(AllOf(Ge(c.min_length), Le(c.max_length))))); + + ssize_t sum = SumSizes(idents); + if (!expected_sum) { + expected_sum = sum; + first = idents; + continue; + } + // The byte sum must be identical regardless of the seed. + EXPECT_THAT(sum, Eq(*expected_sum)); + if (idents != *first) { + any_different = true; + } + } + // Sanity check that the generators really are producing different content, + // so that the invariance check above is meaningful rather than trivially + // passing on identical output. + EXPECT_TRUE(any_different); + } +} + TEST(SourceGenTest, UniformIdentifiers) { SourceGen gen; // Check that uniform identifier length results in exact coverage of each