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

Reword the wrap-around comment with a worked example, name the intermediate values (remainder, lastValue), and make lastValue always reflect the true lowest IV value visited (accounting for the exclusive-test/zero-remainder case), collapsing the safety check to a single comparison. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

🔵 Needs a closer look

GT_NE handling and an unsafe literal-GT regression test remain unresolved.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/coreclr/jit/loopcloning.cpp:1379

GT_NE is also an exclusive decreasing test, and IsDecreasingLoop only admits it for unit stride. For a constant loop such as for (uint i = 7; i != 0; i--), the body visits 7..1, then updates to 0 and exits; this proof currently treats it as inclusive, computes lastValue == 0, and rejects an otherwise safe clone. Include GT_NE in the exclusive case so this existing optimization is not unnecessarily disabled.

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

The literal-bound tests cover the safe GT case and safe/unsafe GE cases, but they do not cover an unsafe literal GT case. The new proof has a distinct exclusive-GT path, while the parameterized UnsignedArrayGT calls never set HasConstInit/HasConstLimit; add a helper such as for (uint i = 7; i > 0; i -= 3) and assert IndexOutOfRangeException so a regression in that constant path cannot silently re-enable the out-of-bounds clone.

        Assert.Equal(11, UnsignedArrayGTConstSafe(a));
        Assert.Equal(18, UnsignedArrayGEConstSafe(a));
        Assert.Throws<IndexOutOfRangeException>(() => UnsignedArrayGEConstUnsafe(a));

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

…unsafe GT const test GT_NE is an exclusive decreasing test (only valid with unit stride), so it should get the same "limit not visited when remainder is 0" exemption as GT_GT; otherwise safe GT_NE constant loops were rejected. Also add a literal-bound GT_GT test case that must not be proven safe, to cover the exclusive-test/nonzero-remainder path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

AndyAyersMS · 2026-09-16T22:49:44Z

@dhartglassMSFT PTAL
fyi @dotnet/jit-contrib

I was going to try and handle invariant but not-known limits and strides, but that started getting complicated.

dhartglassMSFT · 2026-09-21T20:35:32Z

+    // i = 7, 4, 1, then wraps: 1 - 3 underflows to UINT_MAX, and UINT_MAX >= 1
+    // is true, so the loop keeps running with i far out of range. This applies
+    // even for unit stride: "i >= 0" is never false for unsigned i.
+    if (!isIncreasingLoop && iterInfo->TestTree->IsUnsigned())


Does this reject loops with unit stride like for(uint i=7; i>limit; i--)?

Id think a GT decrementing loop with unit stride and unsigned IV might be able to be exempt from this guard always. I'm assuming that this would be the most common kind of decrementing unsigned loop anyways.

if lucky might recover some of those diffs

dhartglassMSFT · 2026-09-21T21:04:22Z

+        // the loop visits "limit" (1) via GE and then underflows on the
+        // next decrement -- must not be proven safe.
+        int sum = 0;
+        for (uint i = 7; i >= 1; i -= 3)


the loop here and in UnsignedArrayGTConstUnsafe might be rejected from cloning anyway, since morph can rewrite something like i>=1 to i!=0. changing these loops to instead start at 8 and compare i >= 2 and i > 1 i think should get around it and exercise the new guard

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

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

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

Runtime_133587 SIGSEGV in mono #133910

Open

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

Copilot started reviewing on behalf of AndyAyersMS September 15, 2026 18:29 View session

Copilot AI reviewed Sep 15, 2026

View reviewed changes

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

Copilot AI reviewed Sep 15, 2026

AndyAyersMS requested a review from dhartglassMSFT September 16, 2026 22:48

AndyAyersMS mentioned this pull request Sep 17, 2026

[Build Failure] Crossgen2 AccessViolationException in Dictionary<Utf8String,_>.FindValue during maccatalyst-arm64 R2R build #134157

Open

dhartglassMSFT reviewed Sep 21, 2026

View reviewed changes

review feedback: adjust tests

399fd40

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!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS commented Sep 16, 2026

Uh oh!

dhartglassMSFT Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhartglassMSFT Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhartglassMSFT Sep 21, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dhartglassMSFT Sep 21, 2026 •

edited

Loading

dhartglassMSFT Sep 21, 2026 •

edited

Loading