Check for use of InstIds from the wrong SemIR::File
#5997
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dwblaikie
added a commit
to dwblaikie/carbon-lang
that referenced
this pull request
Aug 28, 2025
Found by WIP validation for this type of issue ongoing in carbon-language#5997 I'm not entirely sure how the one test update falls out of this change - but it is from the same test that I originally reduced the problem from, which is reassuring. The reduced test case I investigated the issue with was this: a.carbon: library "lib"; interface I1(Other:! type) { let Result:! type; } b.carbon: import library "lib"; class T1 { } impl T1 as I1(Self) where .Result = Self { } The SemIR dump diff looked like this: 89c89 < %Main.import_ref.b6f = import_ref Main//lib, inst28 [no loc], unloaded --- > %Main.import_ref.b6f = import_ref Main//lib, inst27 [no loc], unloaded 96c96 < %Main.import_ref.f7b: @i1.%I1.type (%I1.type.e87) = import_ref Main//lib, inst28 [no loc], loaded [symbolic = @i1.%Self (constants.%Self.c47)] --- > %Main.import_ref.f7b: @i1.%I1.type (%I1.type.e87) = import_ref Main//lib, inst27 [no loc], loaded [symbolic = @i1.%Self (constants.%Self.c47)] Which is a difference, but given the `inst28`/`inst27` don't appear anywhere else than these two lines, it doesn't give a terribly meaningful diff/story about what changed - but perhaps it's sufficient... Not sure if this test ^ is sufficiently more interesting than the diff update already in this patch. If so, happy to add the above as a new test case. Open to ideas.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 28, 2025
…#5998) Found by WIP validation for this type of issue ongoing in #5997 I'm not entirely sure how the one test update falls out of this change - but it is from the same test that I originally reduced the problem from, which is reassuring. The reduced test case I investigated the issue with was this: `a.carbon`: ``` library "lib"; interface I1(Other:! type) { let Result:! type; } ``` `b.carbon`: ``` import library "lib"; class T1 { } impl T1 as I1(Self) where .Result = Self { } ``` The SemIR dump diff looked like this: ``` 89c89 < %Main.import_ref.b6f = import_ref Main//lib, inst28 [no loc], unloaded --- > %Main.import_ref.b6f = import_ref Main//lib, inst27 [no loc], unloaded 96c96 < %Main.import_ref.f7b: @i1.%I1.type (%I1.type.e87) = import_ref Main//lib, inst28 [no loc], loaded [symbolic = @i1.%Self (constants.%Self.c47)] --- > %Main.import_ref.f7b: @i1.%I1.type (%I1.type.e87) = import_ref Main//lib, inst27 [no loc], loaded [symbolic = @i1.%Self (constants.%Self.c47)] ``` Which is a difference, but given the `inst28`/`inst27` don't appear anywhere else than these two lines, it doesn't give a terribly meaningful diff/story about what changed - but perhaps it's sufficient... Not sure if this test ^ is sufficiently more interesting than the diff update already in this patch. If so, happy to add the above as a new test case. Open to ideas.
dwblaikie
force-pushed
the
debug_checks_checkirid_matching_value_store
branch
from
e89bd87 to
9c3465d
Compare
dwblaikie
force-pushed
the
debug_checks_checkirid_matching_value_store
branch
from
9c3465d to
d0734b3
Compare
Otherwise invalid InstIds passed to canstants would cause arbitrary/large memory allocation as the constant storage would be grown to fit the giant invalid index.
… of any context useful for dumping given only an id without the container or SemIR::File it might've come from
The more-informative check failure (reproducing the original bug that motivated this extra checking) looks like: CHECK failure at ./toolchain/base/value_store.h:295: index < size_: Untagged index was outside of container range. Possibly tagged index 2113929262. Best-effort decomposition: CheckIRId: 30, index: 46. The CheckIRIdTag for this container is: 29 Not sure how this should be phrased - it all feels a bit awkward. Maybe something like "CheckIRId was 30, should be 29. Index is 46"? Maybe print the tagged index as hex?
dwblaikie
force-pushed
the
debug_checks_checkirid_matching_value_store
branch
from
d0734b3 to
ea8fb53
Compare
zygoloid
reviewed
Sep 11, 2025
…urate Add back in the original `IdToChunkIndices` but as a non-static member (since it's needed to un-swizzle the CheckIRIdTag back out of the Id to get to the raw index)
|
Ping @zygoloid - further thoughts? |
zygoloid
reviewed
Sep 24, 2025
There was a problem hiding this comment.
Thanks, a couple of comments but basically LGTM.
(Looks like it's worth making some updates to the first comment on the PR before that becomes the commit message.)
I forget -- have you done any performance testing here? I'm not really expecting much difference but value stores are pretty hot so it seems worth checking.
dwblaikie
commented
Sep 25, 2025
There was a problem hiding this comment.
Thanks, a couple of comments but basically LGTM.
(Looks like it's worth making some updates to the first comment on the PR before that becomes the commit message.)
Ah, yep - updated.
I forget -- have you done any performance testing here? I'm not really expecting much difference but value stores are pretty hot so it seems worth checking.
I have not - any recommended benchmarks/mechanisms for performance testing changes?
dwblaikie
changed the title
InstIds from the wrong SemIR::File
zygoloid
approved these changes
Sep 25, 2025
|
yeah, got some performance regressions I'll need to look into :/ |
|
FWIW, those are pretty small regressions, and there were up to 5% swings in the parse benchmarks so might be some noise in them too. Can look for anything obvious, but may not be worth stressing too much about this. IT's not like a 25% or 50% regression. |
Fair - yeah, better understood the perf report and how for higher numbers of iterations the difference went down, and the noise. Ran some perf diffs and didn't find anything really standout. So going ahead with this as-is for now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use the
CheckIRIdas a unique identifier for the scope of anInstId- if anInstIdis created within the scope of oneCheckIRIdit must not be used in the scope of a differentCheckIRId.This is achieved without extra storage, but with false negatives for large inputs.
When an
InstIdis created, the original index of theInstis XORed with a tag derived from theCheckIRIdto produce the finalInstId. When theInstIdis used, the expected tag is XORed with theInstIdto get back to the original index - if the tags don't match, the resulting index will be corrupted, likely too large - resulting in an out of bounds index CHECK-failure.(the tag value is derived as such:
InstIdhas a tag combined into it)In this way, the tag is unlikely to overlap with the index for small test cases - making it possible to separate out the
CheckIRIdfrom the index in these cases to provide more meaningful debugging/CHECK messages, and more informativeSemIRtextual dumping that can now include theCheckIRIdalong with theInst's index in the name of aninst)The test churn here is improved printing as tagged
InstIds can now, with best effort (more likely for small test cases where theCheckIRIdand theInstindex aren't at risk of overlapping from the high and low bits), render theCheckIRIdas part of the inst's name. Going frominstNNtoirMM.instNN.