From 02fc484f23957f339ce36b9bf7d6e35da423634a Mon Sep 17 00:00:00 2001 From: Dana Jansens Date: Mon, 2 Jun 2025 15:16:31 -0400 Subject: [PATCH] Make pointers in ValueStore stable across insertions (#5576) This avoids reallocating the backing buffer in ValueStore so that references into the ValueStore are never invalidated when adding new values. This works especially well since we never delete values from a ValueStore. The strategy used is to allocate chunks of a fixed size, and inserting into each chunk until it is full before allocating the next. The ValueStore starts with an initial allocated chunk in all cases, so that there is only a single indirection for adding and accessing values from this chunk. After it's full, additional chunks are allocated in a vector, so two indirections are required to add or access values in these chunks. This obviates the need for https://github.com/carbon-language/carbon-lang/pull/5529 as we no longer need to worry about holding pointers into a ValueStore. We introduce a Flatten operation for ranges. It flattens a "range over ranges over Ts" down to a "range over Ts". This allows us to make an range over the values in the ValueStore from a range over the chunks in the ValueStore. See https://doc.rust-lang.org/stable/std/iter/trait.Iterator.html#method.flatten for inspiration for this name choice. Flatten is used in one other case where we were writing two levels of for loops to do the same thing. The `array_ref()` accessor is changed to `values()` and its now a range (typed as a `ValueStoreRange`) over all values as references (like ArrayRef was, but without random access). As pointers to a ValueStore can no longer be invalidated, we remove the ASAN poisoning feature and support from ValueStore. This may cause a regression in our compile benchmark of up to 5%, though that is close to or within the noise of the benchmark. We can look at ways to optimize things further in the future. Perhaps by tuning the chunk size further, or by making later chunks larger than earlier chunks, or other strategies. --- .../clang_cc_toolchain_config.bzl | 17 -- toolchain/base/BUILD | 5 +- toolchain/base/int.h | 4 +- toolchain/base/value_store.h | 239 ++++++++---------- toolchain/base/value_store_chunk.h | 164 ++++++++++++ toolchain/check/check_unit.cpp | 4 +- toolchain/check/handle_index.cpp | 2 +- toolchain/check/impl_validation.cpp | 2 +- toolchain/check/import_ref.cpp | 14 - toolchain/lower/file_context.cpp | 2 +- toolchain/sem_ir/file.cpp | 2 +- toolchain/sem_ir/formatter.cpp | 2 +- toolchain/sem_ir/generic.cpp | 10 +- toolchain/sem_ir/generic.h | 12 +- toolchain/sem_ir/impl.h | 12 +- toolchain/sem_ir/inst.h | 8 +- 16 files changed, 303 insertions(+), 196 deletions(-) create mode 100644 toolchain/base/value_store_chunk.h diff --git a/bazel/cc_toolchains/clang_cc_toolchain_config.bzl b/bazel/cc_toolchains/clang_cc_toolchain_config.bzl index 8b63fdeded43..3aeea1593a9f 100644 --- a/bazel/cc_toolchains/clang_cc_toolchain_config.bzl +++ b/bazel/cc_toolchains/clang_cc_toolchain_config.bzl @@ -643,22 +643,6 @@ def _impl(ctx): ], ) - # A feature that enables poisoning of value stores to detect use after - # potential reallocation bugs. - # - # TODO: Remove this and leave poisoning always on once these bugs are - # fixed. - poison_value_stores = feature( - name = "poison_value_stores", - requires = [feature_set(["asan"])], - flag_sets = [flag_set( - actions = all_compile_actions, - flag_groups = [flag_group(flags = [ - "-DCARBON_POISON_VALUE_STORES=1", - ])], - )], - ) - fuzzer = feature( name = "fuzzer", flag_sets = [flag_set( @@ -1153,7 +1137,6 @@ def _impl(ctx): asan, asan_min_size, enable_in_fastbuild, - poison_value_stores, fuzzer, layering_check, module_maps, diff --git a/toolchain/base/BUILD b/toolchain/base/BUILD index 5255190b16b9..c9b1c86667fd 100644 --- a/toolchain/base/BUILD +++ b/toolchain/base/BUILD @@ -86,7 +86,10 @@ cc_library( cc_library( name = "value_store", - hdrs = ["value_store.h"], + hdrs = [ + "value_store.h", + "value_store_chunk.h", + ], deps = [ ":mem_usage", ":yaml", diff --git a/toolchain/base/int.h b/toolchain/base/int.h index 8bf572a9af92..a8c8e2090c76 100644 --- a/toolchain/base/int.h +++ b/toolchain/base/int.h @@ -346,8 +346,8 @@ class IntStore { // into the ID space. auto OutputYaml() const -> Yaml::OutputMapping; - auto array_ref() const -> llvm::ArrayRef { - return values_.array_ref(); + auto values() const [[clang::lifetimebound]] -> auto { + return values_.values(); } auto size() const -> size_t { return values_.size(); } diff --git a/toolchain/base/value_store.h b/toolchain/base/value_store.h index 015bb9bb3229..f721e9ecc42a 100644 --- a/toolchain/base/value_store.h +++ b/toolchain/base/value_store.h @@ -7,6 +7,7 @@ #include #include +#include #include "common/check.h" #include "common/hashtable_key_context.h" @@ -15,9 +16,11 @@ #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/Sequence.h" #include "llvm/ADT/SmallVector.h" +#include "llvm/ADT/StringRef.h" #include "llvm/ADT/iterator_range.h" #include "llvm/Support/Compiler.h" #include "toolchain/base/mem_usage.h" +#include "toolchain/base/value_store_chunk.h" #include "toolchain/base/yaml.h" namespace Carbon { @@ -29,29 +32,22 @@ namespace Internal { class ValueStoreNotPrintable {}; } // namespace Internal -// Setup our compile time condition controlling poisoning of value stores. This -// is set to one by the Bazel flag `--features=poison_value_stores`. -// -// TODO: Eventually, this will always enabled when ASan is enabled, but we can't -// do that until we clean up all of the latent bugs. -#ifndef CARBON_POISON_VALUE_STORES -#define CARBON_POISON_VALUE_STORES 0 -#elif !LLVM_ADDRESS_SANITIZER_BUILD -#error "CARBON_POISON_VALUE_STORES requires address sanitizer" -#endif +template +class ValueStoreRange; // A simple wrapper for accumulating values, providing IDs to later retrieve the // value. This does not do deduplication. // // IdT::ValueType must represent the type being indexed. template + requires(Internal::IdHasValueType) class ValueStore : public std::conditional< std::is_base_of_v, typename IdT::ValueType>, Yaml::Printable>, Internal::ValueStoreNotPrintable> { public: - using ValueType = typename IdT::ValueType; + using ValueType = std::decay_t; // Typically we want to use `ValueType&` and `const ValueType& to avoid // copies, but when the value type is a `StringRef`, we assume external @@ -64,83 +60,61 @@ class ValueStore llvm::StringRef, const ValueType&>; ValueStore() = default; - ValueStore(ValueStore&& other) noexcept - : values_((other.UnpoisonAll(), std::move(other.values_))) -#if CARBON_POISON_VALUE_STORES - , - all_poisoned_(false) -#endif - { - PoisonAll(); - } - auto operator=(ValueStore&& other) noexcept -> ValueStore& { - UnpoisonAll(); - other.UnpoisonAll(); - values_ = std::move(other.values_); -#if CARBON_POISON_VALUE_STORES - all_poisoned_ = false; -#endif - PoisonAll(); - return *this; - } - ~ValueStore() { UnpoisonAll(); } // Stores the value and returns an ID to reference it. auto Add(ValueType value) -> IdT { - IdT id(values_.size()); // This routine is especially hot and the check here relatively expensive - // for the value provided, so only do this in debug builds to make tracking - // down issues easier. - CARBON_DCHECK(id.index >= 0, "Id overflow"); + // for the value provided, so only do this in non-optimized builds to make + // tracking down issues easier. + CARBON_DCHECK(size_ < std::numeric_limits::max(), "Id overflow"); - bool realloc = values_.capacity() == values_.size(); - if (realloc) { - // Unpoison everything if the push will reallocate, in order to allow the - // vector to make a copy of the elements. - UnpoisonAll(); - } else { - PoisonAll(); - } + IdT id(size_); + auto [chunk_index, pos] = Internal::IdToChunkIndices(id); + ++size_; - values_.push_back(std::move(value)); - - if (realloc) { - PoisonAll(); - } else { - PoisonElement(id.index); + CARBON_DCHECK(static_cast(chunk_index) <= chunks_.size(), + "{0} <= {1}", chunk_index, chunks_.size()); + if (static_cast(chunk_index) == chunks_.size()) { + chunks_.emplace_back(); } + CARBON_DCHECK(pos == chunks_[chunk_index].size()); + chunks_[chunk_index].push(std::move(value)); return id; } // Returns a mutable value for an ID. auto Get(IdT id) -> RefType { CARBON_DCHECK(id.index >= 0, "{0}", id); - UnpoisonElement(id.index); - return values_[id.index]; + CARBON_DCHECK(id.index < size_, "{0}", id); + auto [chunk_index, pos] = Internal::IdToChunkIndices(id); + return chunks_[chunk_index].at(pos); } // Returns the value for an ID. auto Get(IdT id) const -> ConstRefType { CARBON_DCHECK(id.index >= 0, "{0}", id); - UnpoisonElement(id.index); - return values_[id.index]; + CARBON_DCHECK(id.index < size_, "{0}", id); + auto [chunk_index, pos] = Internal::IdToChunkIndices(id); + return chunks_[chunk_index].at(pos); } // Reserves space. auto Reserve(size_t size) -> void { - UnpoisonAll(); - values_.reserve(size); - PoisonAll(); + // We get the number of chunks needed to satisfy `size` by rounding any + // partial result up. + size_t num_more_chunks = + (size + ChunkType::Capacity - 1) / ChunkType::Capacity; + if (chunks_.size() < num_more_chunks) { + // We resize() rather than reserve() here to create the new `ChunkType` + // objects, which will in turn allocate space for values in those chunks + // (but not initialize them). + chunks_.resize(num_more_chunks); + } } - // Invalidates all current pointers and references into the value store. Used - // in debug builds to trigger use-after-invalidation bugs. - auto Invalidate() -> void { PoisonAll(); } - // These are to support printable structures, and are not guaranteed. auto OutputYaml() const -> Yaml::OutputMapping { - UnpoisonAll(); return Yaml::OutputMapping([&](Yaml::OutputMapping::Map map) { for (auto [id, value] : enumerate()) { map.Add(PrintToString(id), Yaml::OutputScalar(value)); @@ -151,15 +125,16 @@ class ValueStore // Collects memory usage of the values. auto CollectMemUsage(MemUsage& mem_usage, llvm::StringRef label) const -> void { - UnpoisonAll(); - mem_usage.Collect(label.str(), values_); + mem_usage.Add(label.str(), size_ * sizeof(ValueType), + ChunkType::CapacityBytes * chunks_.size()); } - auto array_ref() const -> llvm::ArrayRef { - UnpoisonAll(); - return values_; + auto size() const -> size_t { return size_; } + + // Makes an iterable range over references to all values in the ValueStore. + auto values() const [[clang::lifetimebound]] -> ValueStoreRange { + return ValueStoreRange(*this); } - auto size() const -> size_t { return values_.size(); } // Makes an iterable range over pairs of the index and a reference to the // value for each value in the store. @@ -170,63 +145,63 @@ class ValueStore // ``` // for (auto [id, value] : store.enumerate()) { ... } // ``` - auto enumerate() const -> auto { - UnpoisonAll(); - auto index_to_id = [](auto pair) -> std::pair { - auto [index, value] = pair; - return std::pair(IdT(index), value); + auto enumerate() const [[clang::lifetimebound]] -> auto { + auto index_to_id = [&](int32_t i) -> std::pair { + return std::pair(IdT(i), Get(IdT(i))); }; - return llvm::map_range(llvm::enumerate(values_), index_to_id); + // Because indices into `ValueStore` are all sequential values from 0, we + // can use llvm::seq to walk all indices in the store. + return llvm::map_range(llvm::seq(size_), index_to_id); } private: - // Poison the entire contents of the value store. This is used to detect cases - // where references to elements in a value store are used across calls that - // might modify the store. - auto PoisonAll() const -> void { -#if CARBON_POISON_VALUE_STORES - if (!all_poisoned_) { - __asan_poison_memory_region(values_.data(), - values_.size() * sizeof(values_[0])); - all_poisoned_ = true; - } -#endif - } - // Unpoison the entire contents of the value store. - auto UnpoisonAll() const -> void { -#if CARBON_POISON_VALUE_STORES - __asan_unpoison_memory_region(values_.data(), - values_.size() * sizeof(values_[0])); - all_poisoned_ = false; -#endif - } - // Poison a single element. - auto PoisonElement([[maybe_unused]] int element) const -> void { -#if CARBON_POISON_VALUE_STORES - __asan_unpoison_memory_region(values_.data() + element, sizeof(values_[0])); -#endif - } - // Unpoison a single element. - auto UnpoisonElement([[maybe_unused]] int element) const -> void { -#if CARBON_POISON_VALUE_STORES - __asan_unpoison_memory_region(values_.data() + element, sizeof(values_[0])); - all_poisoned_ = false; -#endif + friend class ValueStoreRange; + + using ChunkType = Internal::ValueStoreChunk; + + // Number of elements added to the store. The number should never exceed what + // fits in an `int32_t`, which is checked in non-optimized builds in Add(). + int32_t size_ = 0; + + // Storage for the `ValueType` objects, indexed by the id. We use a vector of + // chunks of `ValueType` instead of just a vector of `ValueType` so that + // addresses of `ValueType` objects are stable. This allows the rest of the + // toolchain to hold references into `ValueStore` without having to worry + // about invalidation and use-after-free. We ensure at least one Chunk is held + // inline so that in the case where there is only a single Chunk (i.e. small + // files) we can avoid one indirection. + llvm::SmallVector chunks_; +}; + +// A range over references to the values in a ValueStore, returned from +// `ValueStore::values()`. Hides the complex type name of the iterator +// internally to provide a type name (`ValueStoreRange`) that can be +// referred to without auto and templates. +template +class ValueStoreRange { + public: + explicit ValueStoreRange(const ValueStore& store + [[clang::lifetimebound]]) + : flattened_range_(MakeFlattenedRange(store)) {} + + auto begin() const -> auto { return flattened_range_.begin(); } + auto end() const -> auto { return flattened_range_.end(); } + + private: + // Flattens the range of `ValueStoreChunk`s of `ValueType`s into a single + // range of `ValueType`s. + static auto MakeFlattenedRange(const ValueStore& store) -> auto { + // Because indices into `ValueStore` are all sequential values from 0, we + // can use llvm::seq to walk all indices in the store. + return llvm::map_range(llvm::seq(store.size_), + [&](int32_t i) -> ValueStore::ConstRefType { + return store.Get(IdT(i)); + }); } - // Set inline size to 0 because these will typically be too large for the - // stack, while this does make File smaller. - llvm::SmallVector, 0> values_; - -#if CARBON_POISON_VALUE_STORES - // Whether the vector is currently fully poisoned. - // - // We use this to avoid repeated re-poisoning of the entire store. Doing so is - // linear in the size of the store, and we trigger re-poisoning frequently, - // for example on each import. Tracking that here allows us to coalesce these - // into a single linear operation. - mutable bool all_poisoned_ = true; -#endif + using FlattenedRangeType = + decltype(MakeFlattenedRange(std::declval&>())); + FlattenedRangeType flattened_range_; }; // A wrapper for accumulating immutable values with deduplication, providing IDs @@ -253,17 +228,13 @@ class CanonicalValueStore { // Reserves space. auto Reserve(size_t size) -> void; - // Invalidates all current pointers and references into the value store. Used - // in debug builds to trigger use-after-invalidation bugs. - auto Invalidate() -> void { values_.Invalidate(); } - // These are to support printable structures, and are not guaranteed. auto OutputYaml() const -> Yaml::OutputMapping { return values_.OutputYaml(); } - auto array_ref() const -> llvm::ArrayRef { - return values_.array_ref(); + auto values() const [[clang::lifetimebound]] -> ValueStoreRange { + return values_.values(); } auto size() const -> size_t { return values_.size(); } @@ -271,8 +242,7 @@ class CanonicalValueStore { auto CollectMemUsage(MemUsage& mem_usage, llvm::StringRef label) const -> void { mem_usage.Collect(MemUsage::ConcatLabel(label, "values_"), values_); - auto bytes = - set_.ComputeMetrics(KeyContext(values_.array_ref())).storage_bytes; + auto bytes = set_.ComputeMetrics(KeyContext(&values_)).storage_bytes; mem_usage.Add(MemUsage::ConcatLabel(label, "set_"), bytes, bytes); } @@ -287,27 +257,27 @@ template class CanonicalValueStore::KeyContext : public TranslatingKeyContext { public: - explicit KeyContext(llvm::ArrayRef values) : values_(values) {} + explicit KeyContext(const ValueStore* values) : values_(values) {} // Note that it is safe to return a `const` reference here as the underlying - // object's lifetime is provided by the `store_`. - auto TranslateKey(IdT id) const -> const ValueType& { - return values_[id.index]; + // object's lifetime is provided by the `ValueStore`. + auto TranslateKey(IdT id) const -> ValueStore::ConstRefType { + return values_->Get(id); } private: - llvm::ArrayRef values_; + const ValueStore* values_; }; template auto CanonicalValueStore::Add(ValueType value) -> IdT { auto make_key = [&] { return IdT(values_.Add(std::move(value))); }; - return set_.Insert(value, make_key, KeyContext(values_.array_ref())).key(); + return set_.Insert(value, make_key, KeyContext(&values_)).key(); } template auto CanonicalValueStore::Lookup(ValueType value) const -> IdT { - if (auto result = set_.Lookup(value, KeyContext(values_.array_ref()))) { + if (auto result = set_.Lookup(value, KeyContext(&values_))) { return result.key(); } return IdT::None; @@ -318,8 +288,7 @@ auto CanonicalValueStore::Reserve(size_t size) -> void { // Compute the resulting new insert count using the size of values -- the // set doesn't have a fast to compute current size. if (size > values_.size()) { - set_.GrowForInsertCount(size - values_.size(), - KeyContext(values_.array_ref())); + set_.GrowForInsertCount(size - values_.size(), KeyContext(&values_)); } values_.Reserve(size); } diff --git a/toolchain/base/value_store_chunk.h b/toolchain/base/value_store_chunk.h new file mode 100644 index 000000000000..e5ec30525061 --- /dev/null +++ b/toolchain/base/value_store_chunk.h @@ -0,0 +1,164 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#ifndef CARBON_TOOLCHAIN_BASE_VALUE_STORE_CHUNK_H_ +#define CARBON_TOOLCHAIN_BASE_VALUE_STORE_CHUNK_H_ + +#include +#include +#include +#include +#include +#include + +#include "common/check.h" +#include "llvm/ADT/StringRef.h" +#include "llvm/Support/MemAlloc.h" +#include "toolchain/base/mem_usage.h" + +namespace Carbon::Internal { + +// Ids which are stored in a `ValueStore` have a `ValueType` which indicates the +// type of value held in the `ValueStore`. +template +concept IdHasValueType = requires { typename IdT::ValueType; }; + +// The max size of each chunk allocation for `ValueStore`. This is based on TLB +// page sizes for the target platform. +// +// See https://docs.kernel.org/admin-guide/mm/hugetlbpage.html +// +// A 4K chunk size outperforms a 1M chunk size on Linux-x64 and MacOS-arm64 in +// benchmarks and when running file_test. +// +// Linux-x64: x64 CPUs support 4K and 2M page sizes, but we see 1M is slower +// than 4K with tcmalloc in opt builds for our tests. +// +// Mac-arm64: arm64 CPUs support 4K, 8K, 64K, 256K, 1M, 4M and up. Like for +// Linux-x64, 4K outperformed 1M. We didn't try other sizes yet. +// +// TODO: Is there a more optimize size for Mac-arm64? What should Linux-arm64 +// and Mac-x64 use? What should Windows use? +// +// TODO: The previous SmallVector seems to outperform 4K chunks (they +// may be slower by up to 5%) in benchmarks. Find ways to make chunking faster. +// Should successive chunks get larger in size? That will greatly complicate +// math for choosing a chunk though. +template + requires(IdHasValueType) +static constexpr auto PlatformChunkMaxAllocationBytes() -> int32_t { +#if !defined(NDEBUG) || LLVM_ADDRESS_SANITIZER_BUILD + // Use a small size in unoptimized builds to ensure multiple chunks get used. + // And do the same in ASAN builds to reduce bookkeeping overheads. Using large + // allocations (e.g. 1M+) incurs a 10x runtime cost for our tests under ASAN. + return sizeof(typename IdT::ValueType) * 5; +#else + return 4 * 1024; +#endif +} + +// The number of elements stored in each chunk allocation. +// +// The number must be a power of two so that that there are no unused values in +// bits indexing into the allocation. +template + requires(IdHasValueType) +constexpr auto PlatformChunkCapacity() -> int32_t { + constexpr auto MaxElements = + PlatformChunkMaxAllocationBytes() / sizeof(typename IdT::ValueType); + return std::bit_floor(MaxElements); +} + +// The number of bits needed to index each element in a chunk allocation. +template + requires(IdHasValueType) +constexpr auto PlatformChunkIndexBits() -> int32_t { + static_assert(PlatformChunkCapacity() > 0); + return std::bit_width(uint32_t{PlatformChunkCapacity() - 1}); +} + +// Converts an id into an index into the set of chunks, and an offset into that +// specific chunk. Looks for index overflow in non-optimized builds. +template + requires(IdHasValueType) +inline auto IdToChunkIndices(IdT id) -> std::pair { + constexpr auto LowBits = PlatformChunkIndexBits(); + + // Verify there are no unused bits when indexing up to the + // PlatformChunkCapacity(). This ensures that ids are contiguous values + // from 0, as if the values were all stored in a single array, and allows + // using the ids to index into other arrays. + static_assert((1 << LowBits) == PlatformChunkCapacity()); + // Simple check to make sure nothing went wildly wrong with the + // PlatformChunkCapacity, and we have some room for a chunk index, and + // that shifting by the number of bits won't be UB in an int32_t. + static_assert(LowBits < 30); + + // The index of the chunk is the high bits. + auto chunk = id.index >> LowBits; + // The index into the chunk is the low bits. + auto pos = id.index & ((1 << LowBits) - 1); + return {chunk, pos}; +} + +// A chunk of `ValueType`s which has a fixed capacity, but variable size. Tracks +// the size internally for verifying bounds. +template +struct ValueStoreChunk { + public: + static constexpr auto Capacity = Internal::PlatformChunkCapacity(); + static constexpr auto CapacityBytes = Capacity * sizeof(ValueType); + + explicit ValueStoreChunk() + : buf_(reinterpret_cast( + llvm::allocate_buffer(CapacityBytes, alignof(ValueType)))) {} + + // Moving leaves nullptr behind in the moved-from object so that the + // destructor is a no-op (preventing double free). + ValueStoreChunk(ValueStoreChunk&& rhs) noexcept + : buf_(std::exchange(rhs.buf_, nullptr)), num_(rhs.num_) {} + + auto operator=(ValueStoreChunk&& rhs) noexcept -> ValueStoreChunk& { + buf_ = std::exchange(rhs.buf_, nullptr); + num_ = rhs.num_; + return *this; + } + + ~ValueStoreChunk() { + if (buf_) { + if constexpr (!std::is_trivially_destructible_v) { + std::destroy_n(buf_, num_); + } + llvm::deallocate_buffer(buf_, CapacityBytes, alignof(ValueType)); + } + } + + auto at(int32_t i) -> ValueType& { + CARBON_CHECK(i < num_, "{0}", i); + return buf_[i]; + } + auto at(int32_t i) const -> const ValueType& { + CARBON_CHECK(i < num_, "{0}", i); + return buf_[i]; + } + + auto push(ValueType&& value) -> void { + CARBON_CHECK(num_ < Capacity); + std::construct_at(buf_ + num_, std::move(value)); + ++num_; + } + + auto size() const -> int32_t { return num_; } + + private: + // Verify using an `int32_t` for `num_` is sound. + static_assert(Capacity <= std::numeric_limits::max()); + + ValueType* buf_; + int32_t num_ = 0; +}; + +} // namespace Carbon::Internal + +#endif // CARBON_TOOLCHAIN_BASE_VALUE_STORE_CHUNK_H_ diff --git a/toolchain/check/check_unit.cpp b/toolchain/check/check_unit.cpp index 7ba395fec7c7..0c79ea4ea538 100644 --- a/toolchain/check/check_unit.cpp +++ b/toolchain/check/check_unit.cpp @@ -213,7 +213,7 @@ auto CheckUnit::CollectTransitiveImports(SemIR::InstId import_decl_id, bool is_export = results[direct_index].is_export; for (const auto& indirect_ir : - results[direct_index].sem_ir->import_irs().array_ref()) { + results[direct_index].sem_ir->import_irs().values()) { if (!indirect_ir.is_export) { continue; } @@ -416,7 +416,7 @@ auto CheckUnit::ProcessNodeIds() -> bool { } auto CheckUnit::CheckRequiredDeclarations() -> void { - for (const auto& function : context_.functions().array_ref()) { + for (const auto& function : context_.functions().values()) { if (!function.first_owning_decl_id.has_value() && function.extern_library_id == context_.sem_ir().library_id()) { auto function_import_id = diff --git a/toolchain/check/handle_index.cpp b/toolchain/check/handle_index.cpp index a642dda0146a..a477e0ba59e4 100644 --- a/toolchain/check/handle_index.cpp +++ b/toolchain/check/handle_index.cpp @@ -55,7 +55,7 @@ static auto GetIndexWithArgs(Context& context, Parse::NodeId node_id, return std::nullopt; } - for (const auto& impl : context.impls().array_ref()) { + for (const auto& impl : context.impls().values()) { auto impl_self_type_id = context.types().GetTypeIdForTypeInstId(impl.self_id); auto impl_constraint_type_id = diff --git a/toolchain/check/impl_validation.cpp b/toolchain/check/impl_validation.cpp index 5eb8a1792e38..f0192937bcfd 100644 --- a/toolchain/check/impl_validation.cpp +++ b/toolchain/check/impl_validation.cpp @@ -367,7 +367,7 @@ static auto ImportFinalImplsWithImplInFile(Context& context) -> void { }; llvm::SmallVector interfaces_to_import; - for (const auto& impl : context.impls().array_ref()) { + for (const auto& impl : context.impls().values()) { if (impl.witness_id == SemIR::ErrorInst::InstId) { continue; } diff --git a/toolchain/check/import_ref.cpp b/toolchain/check/import_ref.cpp index 072824773806..535a4a7aac38 100644 --- a/toolchain/check/import_ref.cpp +++ b/toolchain/check/import_ref.cpp @@ -3324,20 +3324,6 @@ static auto GetInstForLoad(Context& context, // NOLINTNEXTLINE(misc-no-recursion) auto LoadImportRef(Context& context, SemIR::InstId inst_id) -> void { -#if LLVM_ADDRESS_SANITIZER_BUILD - // Under ASan, invalidate all of our value stores on any import in order to - // flush out bugs where pointers and references to entities are held across - // imports. - context.classes().Invalidate(); - context.entity_names().Invalidate(); - context.facet_types().Invalidate(); - context.functions().Invalidate(); - context.generics().Invalidate(); - context.impls().Invalidate(); - context.interfaces().Invalidate(); - context.specifics().Invalidate(); -#endif - auto inst = context.insts().TryGetAs(inst_id); if (!inst) { return; diff --git a/toolchain/lower/file_context.cpp b/toolchain/lower/file_context.cpp index beb7812db976..b341204393b9 100644 --- a/toolchain/lower/file_context.cpp +++ b/toolchain/lower/file_context.cpp @@ -90,7 +90,7 @@ auto FileContext::PrepareToLower() -> void { // TODO: Move this to lower.cpp. auto FileContext::LowerDefinitions() -> void { - for (const auto& class_info : sem_ir_->classes().array_ref()) { + for (const auto& class_info : sem_ir_->classes().values()) { if (auto* llvm_vtable = BuildVtable(class_info)) { global_variables_.Insert(class_info.vtable_id, llvm_vtable); } diff --git a/toolchain/sem_ir/file.cpp b/toolchain/sem_ir/file.cpp index c75578c7209a..129fc95915d2 100644 --- a/toolchain/sem_ir/file.cpp +++ b/toolchain/sem_ir/file.cpp @@ -62,7 +62,7 @@ auto File::Verify() const -> ErrorOr { // Check that every code block has a terminator sequence that appears at the // end of the block. - for (const Function& function : functions_.array_ref()) { + for (const Function& function : functions_.values()) { for (InstBlockId block_id : function.body_block_ids) { TerminatorKind prior_kind = TerminatorKind::NotTerminator; for (InstId inst_id : inst_blocks().Get(block_id)) { diff --git a/toolchain/sem_ir/formatter.cpp b/toolchain/sem_ir/formatter.cpp index 4e87aa56b670..0851c66750e1 100644 --- a/toolchain/sem_ir/formatter.cpp +++ b/toolchain/sem_ir/formatter.cpp @@ -1158,7 +1158,7 @@ auto Formatter::FormatCallRhs(Call inst) -> void { auto Formatter::FormatImportCppDeclRhs() -> void { out_ << " "; OpenBrace(); - for (ImportCpp import_cpp : sem_ir_->import_cpps().array_ref()) { + for (ImportCpp import_cpp : sem_ir_->import_cpps().values()) { Indent(); out_ << "import Cpp \"" << FormatEscaped( diff --git a/toolchain/sem_ir/generic.cpp b/toolchain/sem_ir/generic.cpp index 6bf7f77fd576..c918eadefed3 100644 --- a/toolchain/sem_ir/generic.cpp +++ b/toolchain/sem_ir/generic.cpp @@ -19,16 +19,16 @@ class SpecificStore::KeyContext : public TranslatingKeyContext { friend auto operator==(const Key&, const Key&) -> bool = default; }; - explicit KeyContext(llvm::ArrayRef specifics) + explicit KeyContext(const ValueStore* specifics) : specifics_(specifics) {} auto TranslateKey(SpecificId id) const -> Key { - const auto& specific = specifics_[id.index]; + const auto& specific = specifics_->Get(id); return {.generic_id = specific.generic_id, .args_id = specific.args_id}; } private: - llvm::ArrayRef specifics_; + const ValueStore* specifics_; }; auto SpecificStore::GetOrAdd(GenericId generic_id, InstBlockId args_id) @@ -41,7 +41,7 @@ auto SpecificStore::GetOrAdd(GenericId generic_id, InstBlockId args_id) return specifics_.Add( {.generic_id = generic_id, .args_id = args_id}); }, - KeyContext(specifics_.array_ref())) + KeyContext(&specifics_)) .key(); } @@ -49,7 +49,7 @@ auto SpecificStore::CollectMemUsage(MemUsage& mem_usage, llvm::StringRef label) const -> void { mem_usage.Collect(MemUsage::ConcatLabel(label, "specifics_"), specifics_); mem_usage.Collect(MemUsage::ConcatLabel(label, "lookup_table_"), - lookup_table_, KeyContext(specifics_.array_ref())); + lookup_table_, KeyContext(&specifics_)); } static auto GetConstantInSpecific(const File& sem_ir, SpecificId specific_id, diff --git a/toolchain/sem_ir/generic.h b/toolchain/sem_ir/generic.h index 40fea3f6c27e..387bd1079b10 100644 --- a/toolchain/sem_ir/generic.h +++ b/toolchain/sem_ir/generic.h @@ -127,10 +127,6 @@ class SpecificStore : public Yaml::Printable { : InstBlockId::Empty; } - // Invalidates all current pointers and references into the value store. Used - // in debug builds to trigger use-after-invalidation bugs. - auto Invalidate() -> void { specifics_.Invalidate(); } - // These are to support printable structures, and are not guaranteed. auto OutputYaml() const -> Yaml::OutputMapping { return specifics_.OutputYaml(); @@ -140,11 +136,13 @@ class SpecificStore : public Yaml::Printable { auto CollectMemUsage(MemUsage& mem_usage, llvm::StringRef label) const -> void; - auto array_ref() const -> llvm::ArrayRef { - return specifics_.array_ref(); + auto values() const [[clang::lifetimebound]] -> ValueStoreRange { + return specifics_.values(); } auto size() const -> size_t { return specifics_.size(); } - auto enumerate() const -> auto { return specifics_.enumerate(); } + auto enumerate() const [[clang::lifetimebound]] -> auto { + return specifics_.enumerate(); + } private: // Context for hashing keys. diff --git a/toolchain/sem_ir/impl.h b/toolchain/sem_ir/impl.h index 53741389f213..d18fa86a0022 100644 --- a/toolchain/sem_ir/impl.h +++ b/toolchain/sem_ir/impl.h @@ -181,10 +181,6 @@ class ImplStore { // Returns the value for an ID. auto Get(ImplId id) const -> const Impl& { return values_.Get(id); } - // Invalidates all current pointers and references into the value store. Used - // in debug builds to trigger use-after-invalidation bugs. - auto Invalidate() -> void { values_.Invalidate(); } - auto OutputYaml() const -> Yaml::OutputMapping { return values_.OutputYaml(); } @@ -196,9 +192,13 @@ class ImplStore { mem_usage.Collect(MemUsage::ConcatLabel(label, "lookup_"), lookup_); } - auto array_ref() const -> llvm::ArrayRef { return values_.array_ref(); } + auto values() const [[clang::lifetimebound]] -> ValueStoreRange { + return values_.values(); + } auto size() const -> size_t { return values_.size(); } - auto enumerate() const -> auto { return values_.enumerate(); } + auto enumerate() const [[clang::lifetimebound]] -> auto { + return values_.enumerate(); + } private: File& sem_ir_; diff --git a/toolchain/sem_ir/inst.h b/toolchain/sem_ir/inst.h index 1c452c59f5d5..cea01925fef3 100644 --- a/toolchain/sem_ir/inst.h +++ b/toolchain/sem_ir/inst.h @@ -530,9 +530,13 @@ class InstStore { mem_usage.Collect(MemUsage::ConcatLabel(label, "values_"), values_); } - auto array_ref() const -> llvm::ArrayRef { return values_.array_ref(); } + auto values() const [[clang::lifetimebound]] -> ValueStoreRange { + return values_.values(); + } auto size() const -> int { return values_.size(); } - auto enumerate() const -> auto { return values_.enumerate(); } + auto enumerate() const [[clang::lifetimebound]] -> auto { + return values_.enumerate(); + } private: // Given a symbolic type, get the corresponding unattached type.