Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[release/10.0-preview3] JIT: Don't use checked bounds in assertprop #113656

Merged
merged 1 commit into from
Mar 18, 2025
Merged

[release/10.0-preview3] JIT: Don't use checked bounds in assertprop #113656

merged 1 commit into from
Mar 18, 2025

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Mar 18, 2025 •
edited by EgorBo
Loading

Backport of #113638 to release/10.0-preview3

Please accept a bug-fix for a bug that sneaked into Preview3 - nobody hit it yet (except for jitstress pipeline in Main), but I suspect it might show up (some conditions folded as true/false when they shouldn't).

/cc @EgorBo

Customer Impact

  • Customer reported
  • Found internally

A new optimization in 10.0 P3 could fold conditions based on the mechanism that is used by Bounds Check Elimination (BCE). That mechanism contained a pre-existing "potential" bug (we're not sure) that in the worst case could remove a bounds check where it shouldn't (but we suspect it doesn't happen today). The bug manifested itself as real test failures #113049 and #113052 when we started to use this mechanism outside of BCE recently. It may result in taking conditional branches that should not be taken (e.g. unexpected exception is thrown).

Regression

  • Yes
  • No

The regression was introduced in 10.0 P3

Testing

jitstress. The fix basically disables some recently introduced optimizations.

Risk

Low

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 18, 2025
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@EgorBo
Copy link
Member

EgorBo commented Mar 18, 2025

PTAL @jakobbotsch cc @dotnet/jit-contrib @jeffschwMSFT

jeffschwMSFT
jeffschwMSFT approved these changes Mar 18, 2025
View reviewed changes
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. please add details to the customer impact that highlight was code patterns may run into an issue. we will take for consideration in 10 P3

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Mar 18, 2025
@jeffschwMSFT jeffschwMSFT added this to the 10.0.0 milestone Mar 18, 2025
@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 18, 2025
@carlossanlop
Copy link
Member

Please ping me when it's ready to merge, preview branch has restricted merge permissions.

@carlossanlop
Copy link
Member

/ba-g Failure is a test timeout in Collections. does not seem related to the issue being fixed here:

===========================================================================================================
/private/tmp/helix/working/A9D8097B/w/A7DC090B/e /private/tmp/helix/working/A9D8097B/w/A7DC090B/e
  Discovering: System.Collections.Concurrent.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Collections.Concurrent.Tests (found 901 of 909 test cases)
  Starting:    System.Collections.Concurrent.Tests (parallel test collections = on [12 threads], stop on fail = off)
['System.Collections.Concurrent.Tests' END OF WORK ITEM LOG: Command timed out, and was killed]

@carlossanlop carlossanlop merged commit fb44ef8 into release/10.0-preview3 Mar 18, 2025
99 of 103 checks passed
@carlossanlop carlossanlop deleted the backport/pr-113638-to-release/10.0-preview3 branch March 18, 2025 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffschwMSFT jeffschwMSFT jeffschwMSFT approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI Servicing-approved Approved for servicing release
Projects
None yet
Milestone
10.0.0
Development

Successfully merging this pull request may close these issues.
4 participants
@EgorBo @carlossanlop @jeffschwMSFT @leecow