Skip to content

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

Merged
carlossanlop merged 1 commit into
release/10.0-preview3from
backport/pr-113638-to-release/10.0-preview3
Mar 18, 2025
Merged

[release/10.0-preview3] JIT: Don't use checked bounds in assertprop#113656
carlossanlop merged 1 commit into
release/10.0-preview3from
backport/pr-113638-to-release/10.0-preview3

Conversation

@github-actions

@github-actions github-actions Bot commented Mar 18, 2025

Copy link
Copy Markdown
Contributor

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

@ghost ghost 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

Copy link
Copy Markdown
Contributor

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

@EgorBo

EgorBo commented Mar 18, 2025

Copy link
Copy Markdown
Member

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

@jeffschwMSFT jeffschwMSFT left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor

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

@carlossanlop

Copy link
Copy Markdown
Contributor

/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
@carlossanlop carlossanlop deleted the backport/pr-113638-to-release/10.0-preview3 branch March 18, 2025 20:06
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 18, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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

Development

Successfully merging this pull request may close these issues.

4 participants