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

[dwblaikie]

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

[danakj]

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

Do you think we can print negative values as negative instead of as a large positive number?

[danakj]

Looking at #6310, I think we need to do the same in relational_value_store.h too, as GetRawIndex starts with Remove()?

[danakj]

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

Do you think we can print negative values as negative instead of as a large positive number?

I think we need to do something for a bunch of id types, I have a PR for this in the works.

[danakj]

This GetRawIndex calls Remove() which makes the confusing error in the negative case right? Do we want the DCHECK in here and TryGetId?

[dwblaikie]

Looking at #6310, I think we need to do the same in relational_value_store.h too, as GetRawIndex starts with Remove()?

Made the same transformation for RelationalValueStore::Get (moving the CHECK up above the Remove call) - though for the GetRawIndex I added CHECKs down in the IdTag to check its pre/post conditions. In some ways this would remove the need for the checks up in the ValueStores - but given your preferences about having checks higher up (& I can appreciate it - it is a precondition of these higher functions too - should we check at every level? I'd worry about performance, but they probably get inlined and optimized away relatively easily)

[danakj]

Oh I see, this LGTM

Re performance, the checks are debug only and cheap so I don't think it's really a concern but this is okay as is.