Improve CHECK-failure when passing a negative id to a ValueStore #6370 (#6392)

Otherwise the value fails in confusing ways while untagging:

  CHECK failure at ./toolchain/base/value_store.h:71:
  index >= initial_reserved_ids_: When removing tagging bits,
  found an index that shouldn't've been tagged in the first place.

With this change:

  CHECK failure at ./toolchain/base/fixed_size_value_store.h:112:
  id.index >= 0: instFFFFFFFFFFFFFFFD

Author: dwblaikie

toolchain/base/fixed_size_value_store.h

@@ -102,22 +102,22 @@ class FixedSizeValueStore {
 
   // Sets the value for an ID.
   auto Set(IdT id, ValueType value) -> void {
+    CARBON_DCHECK(id.index >= 0, "{0}", id);
     auto index = tag_.Remove(id.index);
-    CARBON_DCHECK(index >= 0, "{0}", id);
     values_[index] = value;
   }
 
   // Returns a mutable value for an ID.
   auto Get(IdT id) -> RefType {
+    CARBON_DCHECK(id.index >= 0, "{0}", id);
     auto index = tag_.Remove(id.index);
-    CARBON_DCHECK(index >= 0, "{0}", id);
     return values_[index];
   }
 
   // Returns the value for an ID.
   auto Get(IdT id) const -> ConstRefType {
+    CARBON_DCHECK(id.index >= 0, "{0}", id);
     auto index = tag_.Remove(id.index);
-    CARBON_DCHECK(index >= 0, "{0}", id);
     return values_[index];
   }
 

toolchain/base/relational_value_store.h

@@ -72,8 +72,8 @@ class RelationalValueStore {
 
   // Returns a value for an ID.
   auto Get(IdT id) const -> ConstRefType {
+    CARBON_DCHECK(id.index >= 0, "{0}", id);
     auto index = related_store_->GetIdTag().Remove(id.index);
-    CARBON_DCHECK(index >= 0, "{0}", id);
     return *values_[index];
   }
 

toolchain/base/value_store.h

@@ -51,14 +51,18 @@ struct IdTag {
   }
 
   auto Apply(int32_t index) const -> int32_t {
+    CARBON_DCHECK(index >= 0, "{0}", index);
     if (index < initial_reserved_ids_) {
       return index;
     }
     // TODO: Assert that id_tag_ doesn't have the second highest bit set.
-    return index ^ id_tag_;
+    auto tagged_index = index ^ id_tag_;
+    CARBON_DCHECK(tagged_index >= 0, "{0}", tagged_index);
+    return tagged_index;
   }
 
   auto Remove(int32_t tagged_index) const -> int32_t {
+    CARBON_DCHECK(tagged_index >= 0, "{0}", tagged_index);
     if (!HasTag(tagged_index)) {
       CARBON_DCHECK(tagged_index < initial_reserved_ids_,
                     "This untagged index is outside the initial reserved ids "
@@ -226,11 +230,11 @@ class ValueStore
   auto GetWithDefault(IdType id,  //
                       ConstRefType default_value [[clang::lifetimebound]]) const
       -> ConstRefType {
+    CARBON_DCHECK(id.index >= 0, "{0}", id);
     auto index = tag_.Remove(id.index);
     if (index >= size_) {
       return default_value;
     }
-    CARBON_DCHECK(index >= 0, "{0}", id);
     auto [chunk_index, pos] = RawIndexToChunkIndices(index);
     return chunks_[chunk_index].Get(pos);
   }
@@ -325,8 +329,8 @@ class ValueStore
 
   auto GetIdTag() const -> IdTag { return tag_; }
   auto GetRawIndex(IdT id) const -> int32_t {
+    CARBON_DCHECK(id.index >= 0, "{0}", index);
     auto index = tag_.Remove(id.index);
-    CARBON_DCHECK(index >= 0, "{0}", index);
 #ifndef NDEBUG
     if (index >= size_) {
       // Attempt to decompose id.index to include extra detail in the check