JIT: Guard loop cloning against unsigned decreasing IV wraparound by AndyAyersMS · Pull Request #133915 · dotnet/runtime

AndyAyersMS · 2026-09-14T22:02:23Z

Loop cloning was not doing proper checks for cloning a decreasing loop
with a non-unit stride and an unsigned control variable. The IV might
wrap around leading to out of bounds accesses.

Only clone such loops when the init and limit are jit-time constants
that provably can't lead to wraparound.

Fixes #133824.

Note

This pull request description was created with GitHub Copilot.

Loop cloning was not doing propre checks for cloning a decreasing loop with a non-unit stride and an unsigned control variable. The IV might wrap around leading to out of bounds accesses. Only clone such loops when the init and limit are jit-time constants that provably can't lead to wraparound. Fixes dotnet#133824. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

azure-pipelines · 2026-09-14T22:03:12Z

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.

dotnet-policy-service · 2026-09-14T22:03:33Z

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

Copilot

🟡 Changes recommended

A critical wraparound issue remains, and the tests do not exercise the constant-proof path.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Guards JIT loop cloning against unsigned decreasing-IV wraparound and adds array/span regression coverage.

Changes:

Adds constant-bound safety checks for unsigned decreasing loops.
Adds underflow and safe-iteration tests.

File summaries

File	Summary	Review findings
`src/tests/JIT/opt/Cloning/DownCounted.cs`	Adds array and span regression tests.	Nit (2 votes): Tests do not exercise the constant-proof path; add literal-bound no-inlining helpers.
`src/coreclr/jit/loopcloning.cpp`	Adds unsigned countdown safety checks.	Critical (3 votes): `GT_GE` is treated like `GT_GT`, allowing inclusive loops to wrap around.

Review details

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Lite

Constant-bound proof allowed cloning for unsigned decreasing loops using >= tests when the const limit was below the stride, since it only checked stride divisibility, not the actual last visited value. Compare that value against the stride instead. Also add regression tests with literal bounds to exercise the constant-proof path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

AndyAyersMS · 2026-09-14T22:59:38Z

Thanks for catching this. Fixed both points:

The critical issue: the constant-bound proof only checked stride
divisibility, so an inclusive (>=) test with a const limit below
the stride could still be proven "safe" incorrectly. Now the check
compares the actual last-visited value (\limit + ((init - limit) %
stride)) against the stride, with the one exception where an
exclusive (>) test with zero remainder never visits \limit\ at
all.
Added \UnsignedArrayGTConstSafe/\GEConstSafe/\GEConstUnsafe
regression tests using literal loop bounds so the constant-proof
path is actually exercised (the earlier tests all used parameters,
so \HasConstInit/\HasConstLimit\ were never true).

Rebuilt the JIT and re-validated with the standalone repro harness
(all cases pass) plus \jitformat.py\ (clean, no diffs).

Note

This comment was generated with the help of GitHub Copilot.

Copilot

🟡 Changes recommended

A critical cloning guard still permits unsafe unsigned countdown loops that can wrap around.

Get a fresh assessment by requesting another Copilot review.

Review details

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

Unit-stride countdown loops using an inclusive >= test were exempt from the wraparound check, but >= 0 still underflows on the next decrement. Drop the stride!=1 exemption so the same const-bound proof covers unit stride too. Adds a regression test for this case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

AndyAyersMS · 2026-09-15T00:00:59Z

Good catch, thanks. The stride != 1 exemption assumed unit-stride
loops can't skip past the limit, but that only protects the
exclusive > test; an inclusive >= test still visits limit
itself, so limit == 0 underflows on the next decrement regardless
of stride.

Dropped the stride != 1 exemption so the existing const-bound proof
(which already reduces to limit != 0 when stride is 1) covers unit
stride too, and added a regression test
(UnsignedArrayGEUnitStride/UnsignedArrayGEUnitStrideConstSafe)
that exercises it.

Rebuilt the JIT and re-validated with the standalone harness (14
cases pass) and jitformat.py (clean). Also ran SPMI asmdiffs
(pre/post this change) on libraries.pmi and realworld.run: only
19/331K contexts diff, all in decreasing-loop patterns (e.g. F#'s
ScanBack) where bounds checks are now correctly retained.

Note

This comment was generated with the help of GitHub Copilot.

Copilot

🟡 Changes recommended

Address the GT_NE handling issue and expand arithmetic-proof test coverage.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

src/tests/JIT/opt/Cloning/DownCounted.cs:118

The new literal tests cover only zero-remainder cases ((init - limit) % 3 == 0 for every stride-3 helper), so they never execute the r != 0 branch of the proof at loopcloning.cpp:1362. The parameterized helpers cannot cover it because HasConstInit/HasConstLimit are false there; add representative constant safe and unsafe non-zero-remainder cases so this arithmetic is validated.

        // init=7, limit=1, stride=3: remainder r=0, so a GT test never
        // visits "limit" itself -- always safe to clone.

Files reviewed: 2/2 changed files
Comments generated: 1
Review effort level: Lite

+                // actually visits "constLimit" (the smallest in-range value
+                // is "constLimit + stride"), so it's safe regardless.
+                const unsigned r = ((unsigned)constInit - (unsigned)constLimit) % (unsigned)stride;
+                if (((r == 0) && (iterInfo->TestOper() == GT_GT)) || ((unsigned)constLimit + r >= (unsigned)stride))


Copilot AI lite review requested due to automatic review settings September 14, 2026 22:02

github-actions Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 14, 2026

dotnet-policy-service Bot assigned AndyAyersMS Sep 14, 2026

Copilot started reviewing on behalf of AndyAyersMS September 14, 2026 22:02 View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

Comment thread src/coreclr/jit/loopcloning.cpp Outdated

Comment thread src/tests/JIT/opt/Cloning/DownCounted.cs

Copilot AI review requested due to automatic review settings September 14, 2026 22:59

Copilot started reviewing on behalf of AndyAyersMS September 14, 2026 23:00 View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

Comment thread src/coreclr/jit/loopcloning.cpp Outdated

Copilot AI review requested due to automatic review settings September 15, 2026 00:00

Copilot started reviewing on behalf of AndyAyersMS September 15, 2026 00:01 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

build-analysis Bot mentioned this pull request Sep 15, 2026

Runtime_133587 SIGSEGV in mono #133910

Open

Navigation Menu

Uh oh!

JIT: Guard loop cloning against unsigned decreasing IV wraparound - #133915

JIT: Guard loop cloning against unsigned decreasing IV wraparound#133915
AndyAyersMS wants to merge 3 commits into
dotnet:mainfrom
AndyAyersMS:fix-loop-cloning-unsigned-decr-wraparound

AndyAyersMS commented Sep 14, 2026

Uh oh!

azure-pipelines Bot commented Sep 14, 2026

Uh oh!

dotnet-policy-service Bot commented Sep 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

AndyAyersMS commented Sep 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

AndyAyersMS commented Sep 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

AndyAyersMS commented Sep 14, 2026

Uh oh!

azure-pipelines Bot commented Sep 14, 2026

Uh oh!

dotnet-policy-service Bot commented Sep 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

AndyAyersMS commented Sep 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

AndyAyersMS commented Sep 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants