Unconditionally skip GC reporting for non-interruptible aborted methods - #119403

Merged
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363
Sep 8, 2025
Merged

Unconditionally skip GC reporting for non-interruptible aborted methods#119403
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363

Conversation

@jkotas

Copy link
Copy Markdown
Member

Fixes#119363

CopilotAI review requested due to automatic review settings September 5, 2025 18:03
@jkotasjkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 5, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes issue #119363 by unconditionally skipping GC reporting for non-interruptible aborted methods. The change simplifies the logic for handling execution-aborted methods in the GC info decoder by removing unnecessary conditional checks and variable declarations.

Key changes:

  • Removes the noTrackedRefs variable and associated logic that was never actually used
  • Moves the countIntersections variable declaration outside the conditional block to ensure proper scoping
  • Fixes incorrect brace placement that was causing logic errors in the interruptible ranges handling

@jkotas

Copy link
Copy Markdown
MemberAuthor

This is reverting part of https://github.com/dotnet/coreclr/pull/9869/files#diff-e5013db5f0394027d19785d4928e535fb1157889556c72677037bd39e20d27ebR660 .

@dotnet/jit-contrib Could you please validate that the JIT does not depend on the behavior that's being reverted here?

@jkotas

jkotas commented Sep 5, 2025

Copy link
Copy Markdown
MemberAuthor

I see number of tests failing with:

 Starting: ComInterfaceGenerator.Unit.Tests (parallel test collections = on [2 threads], stop on fail = off)
ASSERT FAILED
Expression: executionAborted
Location: line 812 in /__w/1/s/src/coreclr/vm/gcinfodecoder.cpp
Function: EnumerateLiveSlots
Process: 34
[createdump] Gathering state for process 34 dotnet

so the JIT GC info emission depends on the code being deleted.

It seems that m_NumInterruptibleRanges == 0 meaning is overloaded. The original meaning was what its name says "no interruptible ranges".

dotnet/coreclr#9869 overloaded it to mean "or untracked slots only". It sounds like that we may want to have a separate state for "untracked slots only" to disambiguate these two cases. Does it sound right?

@AndyAyersMS

Copy link
Copy Markdown
Member

Is it that the method in the issue (and the repro) have only call site safe points and so maybe no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

@jkotas

Copy link
Copy Markdown
MemberAuthor

Is it that the method in the issue (and the repro) have only call site safe points

I do not think that the existence of call site safe points is necessary to hit the bug. We may skip reporting of the callsites in the special noTrackedGCSlots mode, so the method can have calls but no reported callsites in GC info

no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

Right, this is the problem. My latest commit is tweaking the condition to skip the reporting even with no interruptible ranges.

@AndyAyersMS

Copy link
Copy Markdown
Member

Latest version looks reasonable to me.

Do you think this has gone unnoticed for years because it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

Yes, I think it is that combined with the specific structure required to hit the bug (untracked slots + no interruptible regions, untracked slot is not yet initialized when the exception is thrown).

@jkotas

Copy link
Copy Markdown
MemberAuthor

I have added your test and validated that the test fails as expected before the fix.

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@jkotas
jkotas merged commit ad0befc into dotnet:mainSep 8, 2025
186 of 188 checks passed
@jkotas
jkotas deleted the issue-119363 branch September 8, 2025 05:00
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Oct 8, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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.

Possible GC hole detected when running System.Security.Cryptography.Tests

3 participants

@jkotas@AndyAyersMS
, '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

Unconditionally skip GC reporting for non-interruptible aborted methods - #119403

Merged
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363
Sep 8, 2025
Merged

Unconditionally skip GC reporting for non-interruptible aborted methods#119403
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363

Conversation

@jkotas

Copy link
Copy Markdown
Member

Fixes#119363

CopilotAI review requested due to automatic review settings September 5, 2025 18:03
@jkotasjkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 5, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes issue #119363 by unconditionally skipping GC reporting for non-interruptible aborted methods. The change simplifies the logic for handling execution-aborted methods in the GC info decoder by removing unnecessary conditional checks and variable declarations.

Key changes:

  • Removes the noTrackedRefs variable and associated logic that was never actually used
  • Moves the countIntersections variable declaration outside the conditional block to ensure proper scoping
  • Fixes incorrect brace placement that was causing logic errors in the interruptible ranges handling

@jkotas

Copy link
Copy Markdown
MemberAuthor

This is reverting part of https://github.com/dotnet/coreclr/pull/9869/files#diff-e5013db5f0394027d19785d4928e535fb1157889556c72677037bd39e20d27ebR660 .

@dotnet/jit-contrib Could you please validate that the JIT does not depend on the behavior that's being reverted here?

@jkotas

jkotas commented Sep 5, 2025

Copy link
Copy Markdown
MemberAuthor

I see number of tests failing with:

 Starting: ComInterfaceGenerator.Unit.Tests (parallel test collections = on [2 threads], stop on fail = off)
ASSERT FAILED
Expression: executionAborted
Location: line 812 in /__w/1/s/src/coreclr/vm/gcinfodecoder.cpp
Function: EnumerateLiveSlots
Process: 34
[createdump] Gathering state for process 34 dotnet

so the JIT GC info emission depends on the code being deleted.

It seems that m_NumInterruptibleRanges == 0 meaning is overloaded. The original meaning was what its name says "no interruptible ranges".

dotnet/coreclr#9869 overloaded it to mean "or untracked slots only". It sounds like that we may want to have a separate state for "untracked slots only" to disambiguate these two cases. Does it sound right?

@AndyAyersMS

Copy link
Copy Markdown
Member

Is it that the method in the issue (and the repro) have only call site safe points and so maybe no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

@jkotas

Copy link
Copy Markdown
MemberAuthor

Is it that the method in the issue (and the repro) have only call site safe points

I do not think that the existence of call site safe points is necessary to hit the bug. We may skip reporting of the callsites in the special noTrackedGCSlots mode, so the method can have calls but no reported callsites in GC info

no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

Right, this is the problem. My latest commit is tweaking the condition to skip the reporting even with no interruptible ranges.

@AndyAyersMS

Copy link
Copy Markdown
Member

Latest version looks reasonable to me.

Do you think this has gone unnoticed for years because it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

Yes, I think it is that combined with the specific structure required to hit the bug (untracked slots + no interruptible regions, untracked slot is not yet initialized when the exception is thrown).

@jkotas

Copy link
Copy Markdown
MemberAuthor

I have added your test and validated that the test fails as expected before the fix.

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@jkotas
jkotas merged commit ad0befc into dotnet:mainSep 8, 2025
186 of 188 checks passed
@jkotas
jkotas deleted the issue-119363 branch September 8, 2025 05:00
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Oct 8, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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.

Possible GC hole detected when running System.Security.Cryptography.Tests

3 participants

@jkotas@AndyAyersMS
, '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

Unconditionally skip GC reporting for non-interruptible aborted methods - #119403

Merged
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363
Sep 8, 2025
Merged

Unconditionally skip GC reporting for non-interruptible aborted methods#119403
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363

Conversation

@jkotas

Copy link
Copy Markdown
Member

Fixes#119363

CopilotAI review requested due to automatic review settings September 5, 2025 18:03
@jkotasjkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 5, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes issue #119363 by unconditionally skipping GC reporting for non-interruptible aborted methods. The change simplifies the logic for handling execution-aborted methods in the GC info decoder by removing unnecessary conditional checks and variable declarations.

Key changes:

  • Removes the noTrackedRefs variable and associated logic that was never actually used
  • Moves the countIntersections variable declaration outside the conditional block to ensure proper scoping
  • Fixes incorrect brace placement that was causing logic errors in the interruptible ranges handling

@jkotas

Copy link
Copy Markdown
MemberAuthor

This is reverting part of https://github.com/dotnet/coreclr/pull/9869/files#diff-e5013db5f0394027d19785d4928e535fb1157889556c72677037bd39e20d27ebR660 .

@dotnet/jit-contrib Could you please validate that the JIT does not depend on the behavior that's being reverted here?

@jkotas

jkotas commented Sep 5, 2025

Copy link
Copy Markdown
MemberAuthor

I see number of tests failing with:

 Starting: ComInterfaceGenerator.Unit.Tests (parallel test collections = on [2 threads], stop on fail = off)
ASSERT FAILED
Expression: executionAborted
Location: line 812 in /__w/1/s/src/coreclr/vm/gcinfodecoder.cpp
Function: EnumerateLiveSlots
Process: 34
[createdump] Gathering state for process 34 dotnet

so the JIT GC info emission depends on the code being deleted.

It seems that m_NumInterruptibleRanges == 0 meaning is overloaded. The original meaning was what its name says "no interruptible ranges".

dotnet/coreclr#9869 overloaded it to mean "or untracked slots only". It sounds like that we may want to have a separate state for "untracked slots only" to disambiguate these two cases. Does it sound right?

@AndyAyersMS

Copy link
Copy Markdown
Member

Is it that the method in the issue (and the repro) have only call site safe points and so maybe no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

@jkotas

Copy link
Copy Markdown
MemberAuthor

Is it that the method in the issue (and the repro) have only call site safe points

I do not think that the existence of call site safe points is necessary to hit the bug. We may skip reporting of the callsites in the special noTrackedGCSlots mode, so the method can have calls but no reported callsites in GC info

no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

Right, this is the problem. My latest commit is tweaking the condition to skip the reporting even with no interruptible ranges.

@AndyAyersMS

Copy link
Copy Markdown
Member

Latest version looks reasonable to me.

Do you think this has gone unnoticed for years because it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

Yes, I think it is that combined with the specific structure required to hit the bug (untracked slots + no interruptible regions, untracked slot is not yet initialized when the exception is thrown).

@jkotas

Copy link
Copy Markdown
MemberAuthor

I have added your test and validated that the test fails as expected before the fix.

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@jkotas
jkotas merged commit ad0befc into dotnet:mainSep 8, 2025
186 of 188 checks passed
@jkotas
jkotas deleted the issue-119363 branch September 8, 2025 05:00
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Oct 8, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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.

Possible GC hole detected when running System.Security.Cryptography.Tests

3 participants

@jkotas@AndyAyersMS
, '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

Unconditionally skip GC reporting for non-interruptible aborted methods - #119403

Merged
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363
Sep 8, 2025
Merged

Unconditionally skip GC reporting for non-interruptible aborted methods#119403
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363

Conversation

@jkotas

Copy link
Copy Markdown
Member

Fixes#119363

CopilotAI review requested due to automatic review settings September 5, 2025 18:03
@jkotasjkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 5, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes issue #119363 by unconditionally skipping GC reporting for non-interruptible aborted methods. The change simplifies the logic for handling execution-aborted methods in the GC info decoder by removing unnecessary conditional checks and variable declarations.

Key changes:

  • Removes the noTrackedRefs variable and associated logic that was never actually used
  • Moves the countIntersections variable declaration outside the conditional block to ensure proper scoping
  • Fixes incorrect brace placement that was causing logic errors in the interruptible ranges handling

@jkotas

Copy link
Copy Markdown
MemberAuthor

This is reverting part of https://github.com/dotnet/coreclr/pull/9869/files#diff-e5013db5f0394027d19785d4928e535fb1157889556c72677037bd39e20d27ebR660 .

@dotnet/jit-contrib Could you please validate that the JIT does not depend on the behavior that's being reverted here?

@jkotas

jkotas commented Sep 5, 2025

Copy link
Copy Markdown
MemberAuthor

I see number of tests failing with:

 Starting: ComInterfaceGenerator.Unit.Tests (parallel test collections = on [2 threads], stop on fail = off)
ASSERT FAILED
Expression: executionAborted
Location: line 812 in /__w/1/s/src/coreclr/vm/gcinfodecoder.cpp
Function: EnumerateLiveSlots
Process: 34
[createdump] Gathering state for process 34 dotnet

so the JIT GC info emission depends on the code being deleted.

It seems that m_NumInterruptibleRanges == 0 meaning is overloaded. The original meaning was what its name says "no interruptible ranges".

dotnet/coreclr#9869 overloaded it to mean "or untracked slots only". It sounds like that we may want to have a separate state for "untracked slots only" to disambiguate these two cases. Does it sound right?

@AndyAyersMS

Copy link
Copy Markdown
Member

Is it that the method in the issue (and the repro) have only call site safe points and so maybe no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

@jkotas

Copy link
Copy Markdown
MemberAuthor

Is it that the method in the issue (and the repro) have only call site safe points

I do not think that the existence of call site safe points is necessary to hit the bug. We may skip reporting of the callsites in the special noTrackedGCSlots mode, so the method can have calls but no reported callsites in GC info

no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

Right, this is the problem. My latest commit is tweaking the condition to skip the reporting even with no interruptible ranges.

@AndyAyersMS

Copy link
Copy Markdown
Member

Latest version looks reasonable to me.

Do you think this has gone unnoticed for years because it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

Yes, I think it is that combined with the specific structure required to hit the bug (untracked slots + no interruptible regions, untracked slot is not yet initialized when the exception is thrown).

@jkotas

Copy link
Copy Markdown
MemberAuthor

I have added your test and validated that the test fails as expected before the fix.

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@jkotas
jkotas merged commit ad0befc into dotnet:mainSep 8, 2025
186 of 188 checks passed
@jkotas
jkotas deleted the issue-119363 branch September 8, 2025 05:00
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Oct 8, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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.

Possible GC hole detected when running System.Security.Cryptography.Tests

3 participants

@jkotas@AndyAyersMS
, '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

Unconditionally skip GC reporting for non-interruptible aborted methods - #119403

Merged
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363
Sep 8, 2025
Merged

Unconditionally skip GC reporting for non-interruptible aborted methods#119403
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363

Conversation

@jkotas

Copy link
Copy Markdown
Member

Fixes#119363

CopilotAI review requested due to automatic review settings September 5, 2025 18:03
@jkotasjkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 5, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes issue #119363 by unconditionally skipping GC reporting for non-interruptible aborted methods. The change simplifies the logic for handling execution-aborted methods in the GC info decoder by removing unnecessary conditional checks and variable declarations.

Key changes:

  • Removes the noTrackedRefs variable and associated logic that was never actually used
  • Moves the countIntersections variable declaration outside the conditional block to ensure proper scoping
  • Fixes incorrect brace placement that was causing logic errors in the interruptible ranges handling

@jkotas

Copy link
Copy Markdown
MemberAuthor

This is reverting part of https://github.com/dotnet/coreclr/pull/9869/files#diff-e5013db5f0394027d19785d4928e535fb1157889556c72677037bd39e20d27ebR660 .

@dotnet/jit-contrib Could you please validate that the JIT does not depend on the behavior that's being reverted here?

@jkotas

jkotas commented Sep 5, 2025

Copy link
Copy Markdown
MemberAuthor

I see number of tests failing with:

 Starting: ComInterfaceGenerator.Unit.Tests (parallel test collections = on [2 threads], stop on fail = off)
ASSERT FAILED
Expression: executionAborted
Location: line 812 in /__w/1/s/src/coreclr/vm/gcinfodecoder.cpp
Function: EnumerateLiveSlots
Process: 34
[createdump] Gathering state for process 34 dotnet

so the JIT GC info emission depends on the code being deleted.

It seems that m_NumInterruptibleRanges == 0 meaning is overloaded. The original meaning was what its name says "no interruptible ranges".

dotnet/coreclr#9869 overloaded it to mean "or untracked slots only". It sounds like that we may want to have a separate state for "untracked slots only" to disambiguate these two cases. Does it sound right?

@AndyAyersMS

Copy link
Copy Markdown
Member

Is it that the method in the issue (and the repro) have only call site safe points and so maybe no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

@jkotas

Copy link
Copy Markdown
MemberAuthor

Is it that the method in the issue (and the repro) have only call site safe points

I do not think that the existence of call site safe points is necessary to hit the bug. We may skip reporting of the callsites in the special noTrackedGCSlots mode, so the method can have calls but no reported callsites in GC info

no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

Right, this is the problem. My latest commit is tweaking the condition to skip the reporting even with no interruptible ranges.

@AndyAyersMS

Copy link
Copy Markdown
Member

Latest version looks reasonable to me.

Do you think this has gone unnoticed for years because it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

Yes, I think it is that combined with the specific structure required to hit the bug (untracked slots + no interruptible regions, untracked slot is not yet initialized when the exception is thrown).

@jkotas

Copy link
Copy Markdown
MemberAuthor

I have added your test and validated that the test fails as expected before the fix.

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@jkotas
jkotas merged commit ad0befc into dotnet:mainSep 8, 2025
186 of 188 checks passed
@jkotas
jkotas deleted the issue-119363 branch September 8, 2025 05:00
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Oct 8, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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.

Possible GC hole detected when running System.Security.Cryptography.Tests

3 participants

@jkotas@AndyAyersMS
, '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

Unconditionally skip GC reporting for non-interruptible aborted methods - #119403

Merged
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363
Sep 8, 2025
Merged

Unconditionally skip GC reporting for non-interruptible aborted methods#119403
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363

Conversation

@jkotas

Copy link
Copy Markdown
Member

Fixes#119363

CopilotAI review requested due to automatic review settings September 5, 2025 18:03
@jkotasjkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 5, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes issue #119363 by unconditionally skipping GC reporting for non-interruptible aborted methods. The change simplifies the logic for handling execution-aborted methods in the GC info decoder by removing unnecessary conditional checks and variable declarations.

Key changes:

  • Removes the noTrackedRefs variable and associated logic that was never actually used
  • Moves the countIntersections variable declaration outside the conditional block to ensure proper scoping
  • Fixes incorrect brace placement that was causing logic errors in the interruptible ranges handling

@jkotas

Copy link
Copy Markdown
MemberAuthor

This is reverting part of https://github.com/dotnet/coreclr/pull/9869/files#diff-e5013db5f0394027d19785d4928e535fb1157889556c72677037bd39e20d27ebR660 .

@dotnet/jit-contrib Could you please validate that the JIT does not depend on the behavior that's being reverted here?

@jkotas

jkotas commented Sep 5, 2025

Copy link
Copy Markdown
MemberAuthor

I see number of tests failing with:

 Starting: ComInterfaceGenerator.Unit.Tests (parallel test collections = on [2 threads], stop on fail = off)
ASSERT FAILED
Expression: executionAborted
Location: line 812 in /__w/1/s/src/coreclr/vm/gcinfodecoder.cpp
Function: EnumerateLiveSlots
Process: 34
[createdump] Gathering state for process 34 dotnet

so the JIT GC info emission depends on the code being deleted.

It seems that m_NumInterruptibleRanges == 0 meaning is overloaded. The original meaning was what its name says "no interruptible ranges".

dotnet/coreclr#9869 overloaded it to mean "or untracked slots only". It sounds like that we may want to have a separate state for "untracked slots only" to disambiguate these two cases. Does it sound right?

@AndyAyersMS

Copy link
Copy Markdown
Member

Is it that the method in the issue (and the repro) have only call site safe points and so maybe no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

@jkotas

Copy link
Copy Markdown
MemberAuthor

Is it that the method in the issue (and the repro) have only call site safe points

I do not think that the existence of call site safe points is necessary to hit the bug. We may skip reporting of the callsites in the special noTrackedGCSlots mode, so the method can have calls but no reported callsites in GC info

no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

Right, this is the problem. My latest commit is tweaking the condition to skip the reporting even with no interruptible ranges.

@AndyAyersMS

Copy link
Copy Markdown
Member

Latest version looks reasonable to me.

Do you think this has gone unnoticed for years because it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

Yes, I think it is that combined with the specific structure required to hit the bug (untracked slots + no interruptible regions, untracked slot is not yet initialized when the exception is thrown).

@jkotas

Copy link
Copy Markdown
MemberAuthor

I have added your test and validated that the test fails as expected before the fix.

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@jkotas
jkotas merged commit ad0befc into dotnet:mainSep 8, 2025
186 of 188 checks passed
@jkotas
jkotas deleted the issue-119363 branch September 8, 2025 05:00
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Oct 8, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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.

Possible GC hole detected when running System.Security.Cryptography.Tests

3 participants

@jkotas@AndyAyersMS
, '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

Unconditionally skip GC reporting for non-interruptible aborted methods - #119403

Merged
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363
Sep 8, 2025
Merged

Unconditionally skip GC reporting for non-interruptible aborted methods#119403
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363

Conversation

@jkotas

Copy link
Copy Markdown
Member

Fixes#119363

CopilotAI review requested due to automatic review settings September 5, 2025 18:03
@jkotasjkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 5, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes issue #119363 by unconditionally skipping GC reporting for non-interruptible aborted methods. The change simplifies the logic for handling execution-aborted methods in the GC info decoder by removing unnecessary conditional checks and variable declarations.

Key changes:

  • Removes the noTrackedRefs variable and associated logic that was never actually used
  • Moves the countIntersections variable declaration outside the conditional block to ensure proper scoping
  • Fixes incorrect brace placement that was causing logic errors in the interruptible ranges handling

@jkotas

Copy link
Copy Markdown
MemberAuthor

This is reverting part of https://github.com/dotnet/coreclr/pull/9869/files#diff-e5013db5f0394027d19785d4928e535fb1157889556c72677037bd39e20d27ebR660 .

@dotnet/jit-contrib Could you please validate that the JIT does not depend on the behavior that's being reverted here?

@jkotas

jkotas commented Sep 5, 2025

Copy link
Copy Markdown
MemberAuthor

I see number of tests failing with:

 Starting: ComInterfaceGenerator.Unit.Tests (parallel test collections = on [2 threads], stop on fail = off)
ASSERT FAILED
Expression: executionAborted
Location: line 812 in /__w/1/s/src/coreclr/vm/gcinfodecoder.cpp
Function: EnumerateLiveSlots
Process: 34
[createdump] Gathering state for process 34 dotnet

so the JIT GC info emission depends on the code being deleted.

It seems that m_NumInterruptibleRanges == 0 meaning is overloaded. The original meaning was what its name says "no interruptible ranges".

dotnet/coreclr#9869 overloaded it to mean "or untracked slots only". It sounds like that we may want to have a separate state for "untracked slots only" to disambiguate these two cases. Does it sound right?

@AndyAyersMS

Copy link
Copy Markdown
Member

Is it that the method in the issue (and the repro) have only call site safe points and so maybe no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

@jkotas

Copy link
Copy Markdown
MemberAuthor

Is it that the method in the issue (and the repro) have only call site safe points

I do not think that the existence of call site safe points is necessary to hit the bug. We may skip reporting of the callsites in the special noTrackedGCSlots mode, so the method can have calls but no reported callsites in GC info

no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

Right, this is the problem. My latest commit is tweaking the condition to skip the reporting even with no interruptible ranges.

@AndyAyersMS

Copy link
Copy Markdown
Member

Latest version looks reasonable to me.

Do you think this has gone unnoticed for years because it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

Yes, I think it is that combined with the specific structure required to hit the bug (untracked slots + no interruptible regions, untracked slot is not yet initialized when the exception is thrown).

@jkotas

Copy link
Copy Markdown
MemberAuthor

I have added your test and validated that the test fails as expected before the fix.

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@jkotas
jkotas merged commit ad0befc into dotnet:mainSep 8, 2025
186 of 188 checks passed
@jkotas
jkotas deleted the issue-119363 branch September 8, 2025 05:00
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Oct 8, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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.

Possible GC hole detected when running System.Security.Cryptography.Tests

3 participants

@jkotas@AndyAyersMS
, '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

Unconditionally skip GC reporting for non-interruptible aborted methods - #119403

Merged
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363
Sep 8, 2025
Merged

Unconditionally skip GC reporting for non-interruptible aborted methods#119403
jkotas merged 2 commits into
dotnet:mainfrom
jkotas:issue-119363

Conversation

@jkotas

Copy link
Copy Markdown
Member

Fixes#119363

CopilotAI review requested due to automatic review settings September 5, 2025 18:03
@jkotasjkotas added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 5, 2025

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes issue #119363 by unconditionally skipping GC reporting for non-interruptible aborted methods. The change simplifies the logic for handling execution-aborted methods in the GC info decoder by removing unnecessary conditional checks and variable declarations.

Key changes:

  • Removes the noTrackedRefs variable and associated logic that was never actually used
  • Moves the countIntersections variable declaration outside the conditional block to ensure proper scoping
  • Fixes incorrect brace placement that was causing logic errors in the interruptible ranges handling

@jkotas

Copy link
Copy Markdown
MemberAuthor

This is reverting part of https://github.com/dotnet/coreclr/pull/9869/files#diff-e5013db5f0394027d19785d4928e535fb1157889556c72677037bd39e20d27ebR660 .

@dotnet/jit-contrib Could you please validate that the JIT does not depend on the behavior that's being reverted here?

@jkotas

jkotas commented Sep 5, 2025

Copy link
Copy Markdown
MemberAuthor

I see number of tests failing with:

 Starting: ComInterfaceGenerator.Unit.Tests (parallel test collections = on [2 threads], stop on fail = off)
ASSERT FAILED
Expression: executionAborted
Location: line 812 in /__w/1/s/src/coreclr/vm/gcinfodecoder.cpp
Function: EnumerateLiveSlots
Process: 34
[createdump] Gathering state for process 34 dotnet

so the JIT GC info emission depends on the code being deleted.

It seems that m_NumInterruptibleRanges == 0 meaning is overloaded. The original meaning was what its name says "no interruptible ranges".

dotnet/coreclr#9869 overloaded it to mean "or untracked slots only". It sounds like that we may want to have a separate state for "untracked slots only" to disambiguate these two cases. Does it sound right?

@AndyAyersMS

Copy link
Copy Markdown
Member

Is it that the method in the issue (and the repro) have only call site safe points and so maybe no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

@jkotas

Copy link
Copy Markdown
MemberAuthor

Is it that the method in the issue (and the repro) have only call site safe points

I do not think that the existence of call site safe points is necessary to hit the bug. We may skip reporting of the callsites in the special noTrackedGCSlots mode, so the method can have calls but no reported callsites in GC info

no interruptible ranges, and the skip reporting logic only kicks in for aborted frames with interruptible ranges, so we end up reporting when we shouldn't?

Right, this is the problem. My latest commit is tweaking the condition to skip the reporting even with no interruptible ranges.

@AndyAyersMS

Copy link
Copy Markdown
Member

Latest version looks reasonable to me.

Do you think this has gone unnoticed for years because it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@jkotas

Copy link
Copy Markdown
MemberAuthor

it's rare to have GC in the middle of EH handling, so there's just a small window where this can cause trouble?

Yes, I think it is that combined with the specific structure required to hit the bug (untracked slots + no interruptible regions, untracked slot is not yet initialized when the exception is thrown).

@jkotas

Copy link
Copy Markdown
MemberAuthor

I have added your test and validated that the test fails as expected before the fix.

@jkotas

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr gcstress-extra, runtime-coreclr gcstress0x3-gcstress0xc

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@jkotas
jkotas merged commit ad0befc into dotnet:mainSep 8, 2025
186 of 188 checks passed
@jkotas
jkotas deleted the issue-119363 branch September 8, 2025 05:00
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Oct 8, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR 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.

Possible GC hole detected when running System.Security.Cryptography.Tests

3 participants

@jkotas@AndyAyersMS