Conversation
Do not treat method-wide checked-bound membership as proof that a value is nonnegative on every path. Validate symbolic bound bases centrally and avoid creating no-throw assertions from unsigned comparisons without nonnegative provenance. Preserve same-length unsigned bounds-check elimination and prove span slice lengths from incoming comparisons rather than checked-bound metadata. Add a source-only regression covering signed, reversed, offset, equality, unsigned, and subtraction boundary cases. Validation: 55,271 benchmarks.run. contexts replay cleanly; asmdiffs are +61 bytes total (-50/+111), with a largest method regression of 56 bytes. The 120-test recent regression subset and normal/stress/repeated-optimization regression runs pass. Fixes dotnet#133821 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a79fe91-9a35-47c4-aa46-995ddd867f58
Keep only the shared symbolic-bound nonnegativity guard and the minimal source-only regression. Leave unsigned assertion handling and span-slice analysis for separate work. The complete branch diff now changes two files: 10 JIT lines and a 39-line regression. The test fails on the original JIT and passes with the fix. All 55,271 benchmarks.run. contexts replay cleanly; asmdiffs total +76 bytes across two methods. The 120-test recent regression subset and targeted JitStress=2 run pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3a79fe91-9a35-47c4-aa46-995ddd867f58
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, includes regression coverage, and has no blocking issues.
Pull request overview
Hardens JIT range analysis against unsafe negative checked bounds and adds regression coverage for issue #133821.
Changes:
- Requires proven nonnegative provenance for symbolic limits.
- Adds regression tests for negative-bound cases.
File summaries
| File | Summary |
|---|---|
src/tests/JIT/Regression_ro_2/Runtime_133821.cs |
Adds regression coverage. |
src/coreclr/jit/rangecheck.cpp |
Filters unsafe symbolic range limits. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
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
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.
Fixes: #133821