Skip to content

JIT: Mark GT_LCLHEAP with GTF_CALL and GTF_GLOB_REF - #132536

Merged
EgorBo merged 2 commits into
dotnet:mainfrom
EgorBo:localloc-order-sideeff
Aug 21, 2026
Merged

JIT: Mark GT_LCLHEAP with GTF_CALL and GTF_GLOB_REF#132536
EgorBo merged 2 commits into
dotnet:mainfrom
EgorBo:localloc-order-sideeff

Conversation

@EgorBo

@EgorBoEgorBo commented Aug 19, 2026

Copy link
Copy Markdown
Member

Fixes#132243.

GT_LCLHEAP stopped carrying any effect flags in #125362, so if-conversion started converting the conditional localloc in the interop string marshalling stubs into an unconditional one feeding a SELECT:

imuledi, dword ptr [SystemMaxDBCSCharSize] ; bufSize (~11MB for a 5.5MB string)... ; probe + rsp adjust: stack overflow herecmpedi,261 ; check happens after the damagecmovg r8, qword ptr [rbp+0x38] ; only discards the pointer

The guarding branch is what bounds the allocation size, so the localloc must not be reordered with, or made to execute under a different condition than, the surrounding code. This marks GT_LCLHEAP with GTF_CALL | GTF_GLOB_REF, the same way GT_KEEPALIVE is handled, and adds it to OperRequiresCallFlag/OperRequiresGlobRefFlag so the flags are not scrubbed by gtUpdateNodeOperSideEffects.

Repros in pure managed code too:

byte*buffer=null;if(size<=261){byte*tmp=stackallocbyte[size];buffer=tmp;}return(nint)buffer;// AllocateGuarded(int.MaxValue / 2) => stack overflow

Local diffs: benchmarks.run +11, aspnet2 +141, libraries.pmi -71, realworld -8 bytes; linux-arm64 aspnet2 +72 bytes. SPMI replay clean.

CopilotAI lite review requested due to automatic review settings August 19, 2026 21:05
@github-actionsgithub-actionsBot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 19, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@EgorBo
EgorBoforce-pushed the localloc-order-sideeff branch 2 times, most recently from a5d0f4c to 75fe03bCompareAugust 19, 2026 21:16
If-conversion turned a conditional localloc into an unconditional one feeding
a SELECT, since GT_LCLHEAP no longer carries any effect flags. The guarding
branch is what bounds the allocation size (as the interop string marshalling
stubs do), so the speculated localloc overflowed the stack.
Fixesdotnet#132243
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@EgorBo
EgorBoforce-pushed the localloc-order-sideeff branch from 75fe03b to 8f1ac90CompareAugust 19, 2026 21:19

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

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Fixes a JIT miscompile where conditional localloc (GT_LCLHEAP) could be speculated across its guarding check due to missing effect flags, potentially causing stack overflow in guarded allocation patterns.

Changes:

  • Mark GT_LCLHEAP with GTF_ORDER_SIDEEFF during import to preserve ordering below dominating bounds checks.
  • Teach GenTree::OperSupportsOrderingSideEffect that GT_LCLHEAP supports ordering side effects.
  • Add a regression test validating conditional stackalloc is not speculated into an unconditional allocation.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

FileDescription
src/tests/JIT/Regression/Regression_ro_2.csprojRegisters the new regression test source file in the test project.
src/tests/JIT/Regression/JitBlue/Runtime_132243/Runtime_132243.csAdds a repro/regression validating conditional stackalloc remains guarded.
src/coreclr/jit/importer.cppApplies GTF_ORDER_SIDEEFF to GT_LCLHEAP to prevent unsafe reordering/speculation.
src/coreclr/jit/gentree.cppAllows GT_LCLHEAP to participate in ordering-side-effect handling.

CopilotAI review requested due to automatic review settings August 19, 2026 21:21
Copilot stopped reviewing on behalf of EgorBo due to an error August 19, 2026 21:26

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@jakobbotsch

Copy link
Copy Markdown
Member

GTF_ORDER_SIDEEFF only interferes with other GTF_ORDER_SIDEEFF. It can be reordered with nodes or control flow that does not have any side effects.

I suspect the node should instead get GTF_CALL and GTF_GLOB_REF flags (similar to GT_KEEPALIVE)

GTF_ORDER_SIDEEFF only interferes with other GTF_ORDER_SIDEEFF nodes, so it
does not prevent the localloc from being reordered with, or predicated on,
side-effect-free code. Mark it as a call and global reference, as GT_KEEPALIVE
does, and register the flags as required so they are not scrubbed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CopilotAI review requested due to automatic review settings August 20, 2026 13:16
@EgorBoEgorBo changed the title JIT: Mark GT_LCLHEAP with GTF_ORDER_SIDEEFFJIT: Mark GT_LCLHEAP with GTF_CALL and GTF_GLOB_REFAug 20, 2026
@EgorBo

Copy link
Copy Markdown
MemberAuthor

Thanks, switched to GTF_CALL | GTF_GLOB_REF in 90a2370.

Also added GT_LCLHEAP to OperRequiresCallFlag and OperRequiresGlobRefFlag - without the former, gtUpdateNodeOperSideEffects scrubs GTF_CALL right back off, and without the latter the default: assert in OperRequiresGlobRefFlag fires.

SPMI replay is clean; diffs are small and mixed (benchmarks.run +11, aspnet2 +141, libraries.pmi -71, realworld -8 bytes; linux-arm64 aspnet2 +72).

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.

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

Note

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

@EgorBo

Copy link
Copy Markdown
MemberAuthor

/ba-g deadletter

@EgorBo
EgorBo enabled auto-merge (squash) August 21, 2026 11:36
@EgorBo
EgorBo merged commit 3ab25f0 into dotnet:mainAug 21, 2026
131 of 136 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

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.

[NET 11 Preview 7 Regression] P/Invoke DllImport string marshaling causes StackOverflowException on large string payloads

3 participants

@EgorBo@jakobbotsch