JIT: Track sideness of arrOp in GetCheckedBoundArithInfo - #100848

Merged
EgorBo merged 5 commits into
dotnet:mainfrom
EgorBo:fix-GetCompareCheckedBoundArithInfo
Apr 10, 2024
Merged

JIT: Track sideness of arrOp in GetCheckedBoundArithInfo#100848
EgorBo merged 5 commits into
dotnet:mainfrom
EgorBo:fix-GetCompareCheckedBoundArithInfo

Conversation

@EgorBo

@EgorBoEgorBo commented Apr 10, 2024•
edited
Loading

Copy link
Copy Markdown
Member

Fixes#100809

Minimal repro:

usingSystem.Runtime.CompilerServices;AlwaysFalse(10);[MethodImpl(MethodImplOptions.NoInlining|MethodImplOptions.AggressiveOptimization)]boolAlwaysFalse(intx){varresult=newbyte[x];intcount=result.Length-2;return(x<0||result.Length-count<0);}

It prints False in Debug and True in Release.

Here we have ((arr_len - (arr_len + -2)) >= 0) expression which should evaluate into 2 >= 0 -> true. But instead, we evaluate it as -2 >= 0 -> false. It happens because GetCheckedBoundArithInfo looses track where it took arrOp in cmpOp - from op1 or op2? the order is important since we don't guarantee that arrOper is commutative.

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 10, 2024
@dotnet-policy-service

dotnet-policy-serviceBot commented Apr 10, 2024

Copy link
Copy Markdown
Contributor

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

@build-analysisbuild-analysisBot mentioned this pull request Apr 10, 2024
Closed
@EgorBo
EgorBo requested a review from AndyAyersMSApril 10, 2024 10:20
@EgorBo

EgorBo commented Apr 10, 2024

Copy link
Copy Markdown
MemberAuthor

PTAL @AndyAyersMS, @dotnet/jit-contrib

Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..

@AndyAyersMS

AndyAyersMS commented Apr 10, 2024

Copy link
Copy Markdown
Member

Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..

I think we should make a case for it, since it was found by one of our users.

@AndyAyersMS

AndyAyersMS commented Apr 10, 2024

Copy link
Copy Markdown
Member

Maybe another consideration is whether this is a regression. Original issue said this does not happen in .NET 6, but I think that code pattern is fairly old. So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.

@Ziuan

Ziuan commented Apr 10, 2024

Copy link
Copy Markdown

If this is not backported to NET8, could you provide the alternative pattern that solves the issue - given the code in the original issue. The original issue uses framework calls to Array.Copy, inside which the above problem arises if I'm not mistaken.

Asking only since @AndyAyersMS mentions the pattern as being fairly old. Thanks!

@EgorBo

EgorBo commented Apr 10, 2024

Copy link
Copy Markdown
MemberAuthor

So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.

The bug was there for quite a while, it was exposed it .NET 8.0 in #81998 that introduced a VN optimization that kicks in here for (x + cns) - x. Thanks to @jakobbotsch who traced it down to that commit.
So technically, the regression is .NET 8.0 only

@EgorBo

EgorBo commented Apr 10, 2024

Copy link
Copy Markdown
MemberAuthor

I'll try to backport it.

If this is not backported to NET8, could you provide the alternative pattern that solves the issue

It seems like the pattern itself is located in the standard library so it's hard to workaround it. You may try to disable <TieredPGO>false</TieredPGO> (or env var DOTNET_TieredPGO=1) that typically triggers inlining that leads to that issue.

@EgorBo
EgorBo merged commit 674ba3f into dotnet:mainApr 10, 2024
@EgorBo
EgorBo deleted the fix-GetCompareCheckedBoundArithInfo branch April 10, 2024 17:32
@EgorBo
EgorBo restored the fix-GetCompareCheckedBoundArithInfo branch April 10, 2024 17:32
@EgorBo

EgorBo commented Apr 11, 2024

Copy link
Copy Markdown
MemberAuthor

/backport to release/8.0-staging

@github-actions

github-actionsBot commented Apr 11, 2024

Copy link
Copy Markdown
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8653024932

@github-actions

github-actionsBot commented Apr 11, 2024

Copy link
Copy Markdown
Contributor

@EgorBo backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Add ArrOpLHS to CompareCheckedBoundArithInfo
Using index info to reconstruct a base tree...
M	src/coreclr/jit/optcse.cpp
M	src/coreclr/jit/valuenum.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/valuenum.h
Auto-merging src/coreclr/jit/optcse.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/optcse.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 Add ArrOpLHS to CompareCheckedBoundArithInfo
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

github-actionsBot commented Apr 11, 2024

Copy link
Copy Markdown
Contributor

@EgorBo an error occurred while backporting to release/8.0-staging, please check the run log for details!

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

@EgorBoEgorBo mentioned this pull request Apr 12, 2024
Merged
4 tasks
matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
@EgorBo@matouskozak
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo (dotnet#100848)
7ca1498
@EgorBoEgorBo mentioned this pull request May 3, 2024
Closed
@github-actionsgithub-actionsBot locked and limited conversation to collaborators May 12, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@EgorBoEgorBo

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

HMACSHA256 randomly throws: System.ArgumentException

3 participants

@EgorBo@AndyAyersMS@Ziuan
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo - #100848
Merged
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Apr 10, 2024
Merged
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo#100848
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Conversation
@EgorBo
@EgorBoEgorBo
commented
Apr 10, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#100809
Minimal repro:
usingSystem.Runtime.CompilerServices;AlwaysFalse(10);[MethodImpl(MethodImplOptions.NoInlining|MethodImplOptions.AggressiveOptimization)]boolAlwaysFalse(intx){varresult=newbyte[x];intcount=result.Length-2;return(x<0||result.Length-count<0);}
It prints False in Debug and True in Release.
Here we have ((arr_len - (arr_len + -2)) >= 0) expression which should evaluate into 2 >= 0 -> true. But instead, we evaluate it as -2 >= 0 -> false. It happens because GetCheckedBoundArithInfo looses track where it took arrOp in cmpOp - from op1 or op2? the order is important since we don't guarantee that arrOper is commutative.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Apr 10, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 10, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
@build-analysisbuild-analysisBot
mentioned this pull request
Apr 10, 2024

Closed

@EgorBo
EgorBo
requested a review
from AndyAyersMSApril 10, 2024 10:20
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
PTAL @AndyAyersMS, @dotnet/jit-contrib
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
I think we should make a case for it, since it was found by one of our users.
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Maybe another consideration is whether this is a regression. Original issue said this does not happen in .NET 6, but I think that code pattern is fairly old. So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
@Ziuan
Ziuan
commented
Apr 10, 2024

Copy link


Copy Markdown

If this is not backported to NET8, could you provide the alternative pattern that solves the issue - given the code in the original issue. The original issue uses framework calls to Array.Copy, inside which the above problem arises if I'm not mistaken.
Asking only since @AndyAyersMS mentions the pattern as being fairly old. Thanks!
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
The bug was there for quite a while, it was exposed it .NET 8.0 in #81998 that introduced a VN optimization that kicks in here for (x + cns) - x. Thanks to @jakobbotsch who traced it down to that commit.

So technically, the regression is .NET 8.0 only
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
I'll try to backport it.
If this is not backported to NET8, could you provide the alternative pattern that solves the issue
It seems like the pattern itself is located in the standard library so it's hard to workaround it. You may try to disable <TieredPGO>false</TieredPGO> (or env var DOTNET_TieredPGO=1) that typically triggers inlining that leads to that issue.
@EgorBo
EgorBo
merged commit 674ba3f
into
dotnet:mainApr 10, 2024
@EgorBo
EgorBo
deleted the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
restored the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
commented
Apr 11, 2024

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8653024932
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:
$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Add ArrOpLHS to CompareCheckedBoundArithInfo
Using index info to reconstruct a base tree...
M	src/coreclr/jit/optcse.cpp
M	src/coreclr/jit/valuenum.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/valuenum.h
Auto-merging src/coreclr/jit/optcse.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/optcse.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 Add ArrOpLHS to CompareCheckedBoundArithInfo
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
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo an error occurred while backporting to release/8.0-staging, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
@EgorBoEgorBo
mentioned this pull request
Apr 12, 2024
 Merged

4 tasks
matouskozak pushed a commit
to matouskozak/runtime
that referenced
this pull request
Apr 30, 2024
@EgorBo@matouskozak
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo (dotnet#100848)
7ca1498
@EgorBoEgorBo
mentioned this pull request
May 3, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@EgorBoEgorBo

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

HMACSHA256 randomly throws: System.ArgumentException


3 participants

@EgorBo@AndyAyersMS@Ziuan

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo - #100848
Merged
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Apr 10, 2024
Merged
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo#100848
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Conversation
@EgorBo
@EgorBoEgorBo
commented
Apr 10, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#100809
Minimal repro:
usingSystem.Runtime.CompilerServices;AlwaysFalse(10);[MethodImpl(MethodImplOptions.NoInlining|MethodImplOptions.AggressiveOptimization)]boolAlwaysFalse(intx){varresult=newbyte[x];intcount=result.Length-2;return(x<0||result.Length-count<0);}
It prints False in Debug and True in Release.
Here we have ((arr_len - (arr_len + -2)) >= 0) expression which should evaluate into 2 >= 0 -> true. But instead, we evaluate it as -2 >= 0 -> false. It happens because GetCheckedBoundArithInfo looses track where it took arrOp in cmpOp - from op1 or op2? the order is important since we don't guarantee that arrOper is commutative.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Apr 10, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 10, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
@build-analysisbuild-analysisBot
mentioned this pull request
Apr 10, 2024

Closed

@EgorBo
EgorBo
requested a review
from AndyAyersMSApril 10, 2024 10:20
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
PTAL @AndyAyersMS, @dotnet/jit-contrib
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
I think we should make a case for it, since it was found by one of our users.
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Maybe another consideration is whether this is a regression. Original issue said this does not happen in .NET 6, but I think that code pattern is fairly old. So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
@Ziuan
Ziuan
commented
Apr 10, 2024

Copy link


Copy Markdown

If this is not backported to NET8, could you provide the alternative pattern that solves the issue - given the code in the original issue. The original issue uses framework calls to Array.Copy, inside which the above problem arises if I'm not mistaken.
Asking only since @AndyAyersMS mentions the pattern as being fairly old. Thanks!
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
The bug was there for quite a while, it was exposed it .NET 8.0 in #81998 that introduced a VN optimization that kicks in here for (x + cns) - x. Thanks to @jakobbotsch who traced it down to that commit.

So technically, the regression is .NET 8.0 only
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
I'll try to backport it.
If this is not backported to NET8, could you provide the alternative pattern that solves the issue
It seems like the pattern itself is located in the standard library so it's hard to workaround it. You may try to disable <TieredPGO>false</TieredPGO> (or env var DOTNET_TieredPGO=1) that typically triggers inlining that leads to that issue.
@EgorBo
EgorBo
merged commit 674ba3f
into
dotnet:mainApr 10, 2024
@EgorBo
EgorBo
deleted the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
restored the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
commented
Apr 11, 2024

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8653024932
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:
$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Add ArrOpLHS to CompareCheckedBoundArithInfo
Using index info to reconstruct a base tree...
M	src/coreclr/jit/optcse.cpp
M	src/coreclr/jit/valuenum.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/valuenum.h
Auto-merging src/coreclr/jit/optcse.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/optcse.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 Add ArrOpLHS to CompareCheckedBoundArithInfo
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
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo an error occurred while backporting to release/8.0-staging, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
@EgorBoEgorBo
mentioned this pull request
Apr 12, 2024
 Merged

4 tasks
matouskozak pushed a commit
to matouskozak/runtime
that referenced
this pull request
Apr 30, 2024
@EgorBo@matouskozak
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo (dotnet#100848)
7ca1498
@EgorBoEgorBo
mentioned this pull request
May 3, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@EgorBoEgorBo

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

HMACSHA256 randomly throws: System.ArgumentException


3 participants

@EgorBo@AndyAyersMS@Ziuan

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo - #100848
Merged
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Apr 10, 2024
Merged
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo#100848
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Conversation
@EgorBo
@EgorBoEgorBo
commented
Apr 10, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#100809
Minimal repro:
usingSystem.Runtime.CompilerServices;AlwaysFalse(10);[MethodImpl(MethodImplOptions.NoInlining|MethodImplOptions.AggressiveOptimization)]boolAlwaysFalse(intx){varresult=newbyte[x];intcount=result.Length-2;return(x<0||result.Length-count<0);}
It prints False in Debug and True in Release.
Here we have ((arr_len - (arr_len + -2)) >= 0) expression which should evaluate into 2 >= 0 -> true. But instead, we evaluate it as -2 >= 0 -> false. It happens because GetCheckedBoundArithInfo looses track where it took arrOp in cmpOp - from op1 or op2? the order is important since we don't guarantee that arrOper is commutative.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Apr 10, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 10, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
@build-analysisbuild-analysisBot
mentioned this pull request
Apr 10, 2024

Closed

@EgorBo
EgorBo
requested a review
from AndyAyersMSApril 10, 2024 10:20
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
PTAL @AndyAyersMS, @dotnet/jit-contrib
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
I think we should make a case for it, since it was found by one of our users.
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Maybe another consideration is whether this is a regression. Original issue said this does not happen in .NET 6, but I think that code pattern is fairly old. So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
@Ziuan
Ziuan
commented
Apr 10, 2024

Copy link


Copy Markdown

If this is not backported to NET8, could you provide the alternative pattern that solves the issue - given the code in the original issue. The original issue uses framework calls to Array.Copy, inside which the above problem arises if I'm not mistaken.
Asking only since @AndyAyersMS mentions the pattern as being fairly old. Thanks!
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
The bug was there for quite a while, it was exposed it .NET 8.0 in #81998 that introduced a VN optimization that kicks in here for (x + cns) - x. Thanks to @jakobbotsch who traced it down to that commit.

So technically, the regression is .NET 8.0 only
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
I'll try to backport it.
If this is not backported to NET8, could you provide the alternative pattern that solves the issue
It seems like the pattern itself is located in the standard library so it's hard to workaround it. You may try to disable <TieredPGO>false</TieredPGO> (or env var DOTNET_TieredPGO=1) that typically triggers inlining that leads to that issue.
@EgorBo
EgorBo
merged commit 674ba3f
into
dotnet:mainApr 10, 2024
@EgorBo
EgorBo
deleted the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
restored the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
commented
Apr 11, 2024

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8653024932
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:
$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Add ArrOpLHS to CompareCheckedBoundArithInfo
Using index info to reconstruct a base tree...
M	src/coreclr/jit/optcse.cpp
M	src/coreclr/jit/valuenum.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/valuenum.h
Auto-merging src/coreclr/jit/optcse.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/optcse.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 Add ArrOpLHS to CompareCheckedBoundArithInfo
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
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo an error occurred while backporting to release/8.0-staging, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
@EgorBoEgorBo
mentioned this pull request
Apr 12, 2024
 Merged

4 tasks
matouskozak pushed a commit
to matouskozak/runtime
that referenced
this pull request
Apr 30, 2024
@EgorBo@matouskozak
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo (dotnet#100848)
7ca1498
@EgorBoEgorBo
mentioned this pull request
May 3, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@EgorBoEgorBo

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

HMACSHA256 randomly throws: System.ArgumentException


3 participants

@EgorBo@AndyAyersMS@Ziuan

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo - #100848
Merged
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Apr 10, 2024
Merged
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo#100848
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Conversation
@EgorBo
@EgorBoEgorBo
commented
Apr 10, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#100809
Minimal repro:
usingSystem.Runtime.CompilerServices;AlwaysFalse(10);[MethodImpl(MethodImplOptions.NoInlining|MethodImplOptions.AggressiveOptimization)]boolAlwaysFalse(intx){varresult=newbyte[x];intcount=result.Length-2;return(x<0||result.Length-count<0);}
It prints False in Debug and True in Release.
Here we have ((arr_len - (arr_len + -2)) >= 0) expression which should evaluate into 2 >= 0 -> true. But instead, we evaluate it as -2 >= 0 -> false. It happens because GetCheckedBoundArithInfo looses track where it took arrOp in cmpOp - from op1 or op2? the order is important since we don't guarantee that arrOper is commutative.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Apr 10, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 10, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
@build-analysisbuild-analysisBot
mentioned this pull request
Apr 10, 2024

Closed

@EgorBo
EgorBo
requested a review
from AndyAyersMSApril 10, 2024 10:20
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
PTAL @AndyAyersMS, @dotnet/jit-contrib
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
I think we should make a case for it, since it was found by one of our users.
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Maybe another consideration is whether this is a regression. Original issue said this does not happen in .NET 6, but I think that code pattern is fairly old. So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
@Ziuan
Ziuan
commented
Apr 10, 2024

Copy link


Copy Markdown

If this is not backported to NET8, could you provide the alternative pattern that solves the issue - given the code in the original issue. The original issue uses framework calls to Array.Copy, inside which the above problem arises if I'm not mistaken.
Asking only since @AndyAyersMS mentions the pattern as being fairly old. Thanks!
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
The bug was there for quite a while, it was exposed it .NET 8.0 in #81998 that introduced a VN optimization that kicks in here for (x + cns) - x. Thanks to @jakobbotsch who traced it down to that commit.

So technically, the regression is .NET 8.0 only
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
I'll try to backport it.
If this is not backported to NET8, could you provide the alternative pattern that solves the issue
It seems like the pattern itself is located in the standard library so it's hard to workaround it. You may try to disable <TieredPGO>false</TieredPGO> (or env var DOTNET_TieredPGO=1) that typically triggers inlining that leads to that issue.
@EgorBo
EgorBo
merged commit 674ba3f
into
dotnet:mainApr 10, 2024
@EgorBo
EgorBo
deleted the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
restored the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
commented
Apr 11, 2024

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8653024932
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:
$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Add ArrOpLHS to CompareCheckedBoundArithInfo
Using index info to reconstruct a base tree...
M	src/coreclr/jit/optcse.cpp
M	src/coreclr/jit/valuenum.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/valuenum.h
Auto-merging src/coreclr/jit/optcse.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/optcse.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 Add ArrOpLHS to CompareCheckedBoundArithInfo
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
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo an error occurred while backporting to release/8.0-staging, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
@EgorBoEgorBo
mentioned this pull request
Apr 12, 2024
 Merged

4 tasks
matouskozak pushed a commit
to matouskozak/runtime
that referenced
this pull request
Apr 30, 2024
@EgorBo@matouskozak
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo (dotnet#100848)
7ca1498
@EgorBoEgorBo
mentioned this pull request
May 3, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@EgorBoEgorBo

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

HMACSHA256 randomly throws: System.ArgumentException


3 participants

@EgorBo@AndyAyersMS@Ziuan

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo - #100848
Merged
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Apr 10, 2024
Merged
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo#100848
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Conversation
@EgorBo
@EgorBoEgorBo
commented
Apr 10, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#100809
Minimal repro:
usingSystem.Runtime.CompilerServices;AlwaysFalse(10);[MethodImpl(MethodImplOptions.NoInlining|MethodImplOptions.AggressiveOptimization)]boolAlwaysFalse(intx){varresult=newbyte[x];intcount=result.Length-2;return(x<0||result.Length-count<0);}
It prints False in Debug and True in Release.
Here we have ((arr_len - (arr_len + -2)) >= 0) expression which should evaluate into 2 >= 0 -> true. But instead, we evaluate it as -2 >= 0 -> false. It happens because GetCheckedBoundArithInfo looses track where it took arrOp in cmpOp - from op1 or op2? the order is important since we don't guarantee that arrOper is commutative.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Apr 10, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 10, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
@build-analysisbuild-analysisBot
mentioned this pull request
Apr 10, 2024

Closed

@EgorBo
EgorBo
requested a review
from AndyAyersMSApril 10, 2024 10:20
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
PTAL @AndyAyersMS, @dotnet/jit-contrib
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
I think we should make a case for it, since it was found by one of our users.
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Maybe another consideration is whether this is a regression. Original issue said this does not happen in .NET 6, but I think that code pattern is fairly old. So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
@Ziuan
Ziuan
commented
Apr 10, 2024

Copy link


Copy Markdown

If this is not backported to NET8, could you provide the alternative pattern that solves the issue - given the code in the original issue. The original issue uses framework calls to Array.Copy, inside which the above problem arises if I'm not mistaken.
Asking only since @AndyAyersMS mentions the pattern as being fairly old. Thanks!
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
The bug was there for quite a while, it was exposed it .NET 8.0 in #81998 that introduced a VN optimization that kicks in here for (x + cns) - x. Thanks to @jakobbotsch who traced it down to that commit.

So technically, the regression is .NET 8.0 only
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
I'll try to backport it.
If this is not backported to NET8, could you provide the alternative pattern that solves the issue
It seems like the pattern itself is located in the standard library so it's hard to workaround it. You may try to disable <TieredPGO>false</TieredPGO> (or env var DOTNET_TieredPGO=1) that typically triggers inlining that leads to that issue.
@EgorBo
EgorBo
merged commit 674ba3f
into
dotnet:mainApr 10, 2024
@EgorBo
EgorBo
deleted the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
restored the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
commented
Apr 11, 2024

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8653024932
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:
$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Add ArrOpLHS to CompareCheckedBoundArithInfo
Using index info to reconstruct a base tree...
M	src/coreclr/jit/optcse.cpp
M	src/coreclr/jit/valuenum.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/valuenum.h
Auto-merging src/coreclr/jit/optcse.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/optcse.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 Add ArrOpLHS to CompareCheckedBoundArithInfo
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
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo an error occurred while backporting to release/8.0-staging, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
@EgorBoEgorBo
mentioned this pull request
Apr 12, 2024
 Merged

4 tasks
matouskozak pushed a commit
to matouskozak/runtime
that referenced
this pull request
Apr 30, 2024
@EgorBo@matouskozak
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo (dotnet#100848)
7ca1498
@EgorBoEgorBo
mentioned this pull request
May 3, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@EgorBoEgorBo

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

HMACSHA256 randomly throws: System.ArgumentException


3 participants

@EgorBo@AndyAyersMS@Ziuan

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo - #100848
Merged
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Apr 10, 2024
Merged
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo#100848
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Conversation
@EgorBo
@EgorBoEgorBo
commented
Apr 10, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#100809
Minimal repro:
usingSystem.Runtime.CompilerServices;AlwaysFalse(10);[MethodImpl(MethodImplOptions.NoInlining|MethodImplOptions.AggressiveOptimization)]boolAlwaysFalse(intx){varresult=newbyte[x];intcount=result.Length-2;return(x<0||result.Length-count<0);}
It prints False in Debug and True in Release.
Here we have ((arr_len - (arr_len + -2)) >= 0) expression which should evaluate into 2 >= 0 -> true. But instead, we evaluate it as -2 >= 0 -> false. It happens because GetCheckedBoundArithInfo looses track where it took arrOp in cmpOp - from op1 or op2? the order is important since we don't guarantee that arrOper is commutative.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Apr 10, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 10, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
@build-analysisbuild-analysisBot
mentioned this pull request
Apr 10, 2024

Closed

@EgorBo
EgorBo
requested a review
from AndyAyersMSApril 10, 2024 10:20
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
PTAL @AndyAyersMS, @dotnet/jit-contrib
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
I think we should make a case for it, since it was found by one of our users.
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Maybe another consideration is whether this is a regression. Original issue said this does not happen in .NET 6, but I think that code pattern is fairly old. So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
@Ziuan
Ziuan
commented
Apr 10, 2024

Copy link


Copy Markdown

If this is not backported to NET8, could you provide the alternative pattern that solves the issue - given the code in the original issue. The original issue uses framework calls to Array.Copy, inside which the above problem arises if I'm not mistaken.
Asking only since @AndyAyersMS mentions the pattern as being fairly old. Thanks!
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
The bug was there for quite a while, it was exposed it .NET 8.0 in #81998 that introduced a VN optimization that kicks in here for (x + cns) - x. Thanks to @jakobbotsch who traced it down to that commit.

So technically, the regression is .NET 8.0 only
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
I'll try to backport it.
If this is not backported to NET8, could you provide the alternative pattern that solves the issue
It seems like the pattern itself is located in the standard library so it's hard to workaround it. You may try to disable <TieredPGO>false</TieredPGO> (or env var DOTNET_TieredPGO=1) that typically triggers inlining that leads to that issue.
@EgorBo
EgorBo
merged commit 674ba3f
into
dotnet:mainApr 10, 2024
@EgorBo
EgorBo
deleted the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
restored the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
commented
Apr 11, 2024

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8653024932
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:
$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Add ArrOpLHS to CompareCheckedBoundArithInfo
Using index info to reconstruct a base tree...
M	src/coreclr/jit/optcse.cpp
M	src/coreclr/jit/valuenum.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/valuenum.h
Auto-merging src/coreclr/jit/optcse.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/optcse.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 Add ArrOpLHS to CompareCheckedBoundArithInfo
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
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo an error occurred while backporting to release/8.0-staging, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
@EgorBoEgorBo
mentioned this pull request
Apr 12, 2024
 Merged

4 tasks
matouskozak pushed a commit
to matouskozak/runtime
that referenced
this pull request
Apr 30, 2024
@EgorBo@matouskozak
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo (dotnet#100848)
7ca1498
@EgorBoEgorBo
mentioned this pull request
May 3, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@EgorBoEgorBo

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

HMACSHA256 randomly throws: System.ArgumentException


3 participants

@EgorBo@AndyAyersMS@Ziuan

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo - #100848
Merged
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Apr 10, 2024
Merged
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo#100848
EgorBo merged 5 commits into
dotnet:mainfrom 
EgorBo:fix-GetCompareCheckedBoundArithInfo
Conversation
@EgorBo
@EgorBoEgorBo
commented
Apr 10, 2024•

edited

Loading

Copy link


Copy Markdown

Member
Fixes#100809
Minimal repro:
usingSystem.Runtime.CompilerServices;AlwaysFalse(10);[MethodImpl(MethodImplOptions.NoInlining|MethodImplOptions.AggressiveOptimization)]boolAlwaysFalse(intx){varresult=newbyte[x];intcount=result.Length-2;return(x<0||result.Length-count<0);}
It prints False in Debug and True in Release.
Here we have ((arr_len - (arr_len + -2)) >= 0) expression which should evaluate into 2 >= 0 -> true. But instead, we evaluate it as -2 >= 0 -> false. It happens because GetCheckedBoundArithInfo looses track where it took arrOp in cmpOp - from op1 or op2? the order is important since we don't guarantee that arrOper is commutative.
@ghostghost
added
the 
area-CodeGen-coreclr
CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
label
Apr 10, 2024
@dotnet-policy-service
dotnet-policy-serviceBot
commented
Apr 10, 2024

Copy link


Copy Markdown

Contributor
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch

See info in area-owners.md if you want to be subscribed.
@build-analysisbuild-analysisBot
mentioned this pull request
Apr 10, 2024

Closed

@EgorBo
EgorBo
requested a review
from AndyAyersMSApril 10, 2024 10:20
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
PTAL @AndyAyersMS, @dotnet/jit-contrib
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Do we need to backport this to net8? The repro seems to be small, but it has to be a very specific pattern to kick in..
I think we should make a case for it, since it was found by one of our users.
@AndyAyersMS
AndyAyersMS
commented
Apr 10, 2024

Copy link


Copy Markdown

Member
Maybe another consideration is whether this is a regression. Original issue said this does not happen in .NET 6, but I think that code pattern is fairly old. So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
@Ziuan
Ziuan
commented
Apr 10, 2024

Copy link


Copy Markdown

If this is not backported to NET8, could you provide the alternative pattern that solves the issue - given the code in the original issue. The original issue uses framework calls to Array.Copy, inside which the above problem arises if I'm not mistaken.
Asking only since @AndyAyersMS mentions the pattern as being fairly old. Thanks!
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
So it's worth trying your repro on .NET 7 or .NET 6 to understand what happens there.
The bug was there for quite a while, it was exposed it .NET 8.0 in #81998 that introduced a VN optimization that kicks in here for (x + cns) - x. Thanks to @jakobbotsch who traced it down to that commit.

So technically, the regression is .NET 8.0 only
@EgorBo
EgorBo
commented
Apr 10, 2024

Copy link


Copy Markdown

MemberAuthor
I'll try to backport it.
If this is not backported to NET8, could you provide the alternative pattern that solves the issue
It seems like the pattern itself is located in the standard library so it's hard to workaround it. You may try to disable <TieredPGO>false</TieredPGO> (or env var DOTNET_TieredPGO=1) that typically triggers inlining that leads to that issue.
@EgorBo
EgorBo
merged commit 674ba3f
into
dotnet:mainApr 10, 2024
@EgorBo
EgorBo
deleted the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
restored the

fix-GetCompareCheckedBoundArithInfo

branch
April 10, 2024 17:32
@EgorBo
EgorBo
commented
Apr 11, 2024

Copy link


Copy Markdown

MemberAuthor
/backport to release/8.0-staging
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8653024932
@github-actions
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:
$ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Add ArrOpLHS to CompareCheckedBoundArithInfo
Using index info to reconstruct a base tree...
M	src/coreclr/jit/optcse.cpp
M	src/coreclr/jit/valuenum.h
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/jit/valuenum.h
Auto-merging src/coreclr/jit/optcse.cpp
CONFLICT (content): Merge conflict in src/coreclr/jit/optcse.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 Add ArrOpLHS to CompareCheckedBoundArithInfo
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
github-actionsBot
commented
Apr 11, 2024

Copy link


Copy Markdown

Contributor
@EgorBo an error occurred while backporting to release/8.0-staging, please check the run log for details!
Error: git am failed, most likely due to a merge conflict.
@EgorBoEgorBo
mentioned this pull request
Apr 12, 2024
 Merged

4 tasks
matouskozak pushed a commit
to matouskozak/runtime
that referenced
this pull request
Apr 30, 2024
@EgorBo@matouskozak
JIT: Track sideness of arrOp in GetCheckedBoundArithInfo (dotnet#100848)
7ca1498
@EgorBoEgorBo
mentioned this pull request
May 3, 2024

Closed

@github-actionsgithub-actionsBot
locked and limited conversation to collaborators
May 12, 2024
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@AndyAyersMSAndyAyersMSAndyAyersMS approved these changes

Assignees

@EgorBoEgorBo

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

HMACSHA256 randomly throws: System.ArgumentException


3 participants

@EgorBo@AndyAyersMS@Ziuan