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.

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

JIT: fix bug in cloning conditions for jagged array #83414

Merged
merged 2 commits into from
Mar 15, 2023

Conversation

AndyAyersMS
Copy link
Member

When checking that an inner array access is in bounds, we must ensure any outer access is fully in bounds too. We were checking that idx < array.Len but not that idx >= 0.

Use an unsigned compare for this check so we can do both sides with a single instruction.

Fixes #83242.

When checking that an inner array access is in bounds, we must ensure any outer
access is fully in bounds too. We were checking that `idx < array.Len` but not
that `idx >= 0`.

Use an unsigned compare for this check so we can do both sides with a single
instruction.

Fixes dotnet#83242.
@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 14, 2023
@ghost ghost assigned AndyAyersMS Mar 14, 2023
@ghost
Copy link

ghost commented Mar 14, 2023

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

Issue Details

When checking that an inner array access is in bounds, we must ensure any outer access is fully in bounds too. We were checking that idx < array.Len but not that idx >= 0.

Use an unsigned compare for this check so we can do both sides with a single instruction.

Fixes #83242.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

@BruceForstall PTAL
cc @dotnet/jit-contrib

We will want to backport this to .NET 7. Expecting some minor diffs (maybe 20 cases across SPMI).

@AndyAyersMS
Copy link
Member Author

Opened #83459 for the osx installer test failure (quite likely unrelated).

Diffs

@AndyAyersMS AndyAyersMS merged commit c9173f2 into dotnet:main Mar 15, 2023
@AndyAyersMS
Copy link
Member Author

/backport to release/7.0

@github-actions
Copy link
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/4427678261

@github-actions
Copy link
Contributor

@AndyAyersMS backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: JIT: fix bug in cloning conditions for jagged array
.git/rebase-apply/patch:126: trailing whitespace.
        
.git/rebase-apply/patch:133: trailing whitespace.
            
warning: 2 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	src/coreclr/jit/loopcloning.cpp
M	src/coreclr/jit/loopcloning.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/loopcloning.h
Auto-merging src/coreclr/jit/loopcloning.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/loopcloning.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 JIT: fix bug in cloning conditions for jagged array
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@github-actions
Copy link
Contributor

@AndyAyersMS an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

AndyAyersMS added a commit that referenced this pull request Mar 15, 2023
…3414)

Backport of #83414 to release/7.0, fixes #83242

When checking that an inner array access is in bounds, we must ensure any outer
access is fully in bounds too. We were checking that `idx < array.Len` but not
that `idx >= 0`.

Use an unsigned compare for this check so we can do both sides with a single
instruction.

Fixes #83242.
AndyAyersMS added a commit that referenced this pull request Mar 31, 2023
…3414) (#83462)

Backport of #83414 to release/7.0, fixes #83242

When checking that an inner array access is in bounds, we must ensure any outer
access is fully in bounds too. We were checking that `idx < array.Len` but not
that `idx >= 0`.

Use an unsigned compare for this check so we can do both sides with a single
instruction.

Fixes #83242.
@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.AccessViolationException occurring in managed code on dotnet 7
2 participants