Skip to content

Enable multiple nested "no GC" region requests - #68105

Merged
BruceForstall merged 1 commit into
dotnet:mainfrom
BruceForstall:FixNestedNoGcRegionRequests
Apr 18, 2022
Merged

Enable multiple nested "no GC" region requests#68105
BruceForstall merged 1 commit into
dotnet:mainfrom
BruceForstall:FixNestedNoGcRegionRequests

Conversation

@BruceForstall

@BruceForstall BruceForstall commented Apr 16, 2022

Copy link
Copy Markdown
Contributor

When doing fast tail call on arm32, we disable GC reporting.
But some IR nodes, like unrolled STORE_BLK, also disable GC
reporting. If one of those is within a fast tail call argument
setup region, we'll attempt to disable GC reporting twice. Since
we didn't keep track of nesting, we end up marking some of the
tail call region after the STORE_BLK as interruptible, leading
to be GC info in the argument area.

Change the enable/disable GC calls to keep a nesting count,
and only re-enable GC reporting when the count reaches zero.

Fixes #67046

Also, don't remove GT_NO_OP "non-removable" NOP. These were being
removed by liveness. They were being inserted in at least some cases
by fast tail call code to ensure there was at least one instruction in a
function that was GC interruptible. All the diffs are due to this.
@BruceForstall
Enable multiple nested "no GC" region requests
fd9d787 
When doing fast tail call on arm32, we disable GC reporting.
But some IR nodes, like unrolled STORE_BLK, also disable GC
reporting. If one of those is within a fast tail call argument
setup region, we'll attempt to disable GC reporting twice. Since
we didn't keep track of nesting, we end up marking some of the
tail call region after the STORE_BLK as interruptible, leading
to be GC info in the argument area.

Change the enable/disable GC calls to keep a nesting count,
and only re-enable GC reporting when the count reaches zero.

Fixes dotnet#67046
@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 16, 2022
@ghost ghost assigned BruceForstall Apr 16, 2022
@ghost

ghost commented Apr 16, 2022

Copy link
Copy Markdown

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

Issue Details

When doing fast tail call on arm32, we disable GC reporting.
But some IR nodes, like unrolled STORE_BLK, also disable GC
reporting. If one of those is within a fast tail call argument
setup region, we'll attempt to disable GC reporting twice. Since
we didn't keep track of nesting, we end up marking some of the
tail call region after the STORE_BLK as interruptible, leading
to be GC info in the argument area.

Change the enable/disable GC calls to keep a nesting count,
and only re-enable GC reporting when the count reaches zero.

Fixes #67046

Also, don't remove GT_NO_OP "non-removable" NOP. These were being
removed by liveness. They were being inserted in at least some cases
by fast tail call code to ensure there was at least one instruction in a
function that was GC interruptible. All the diffs are due to this.

Author: BruceForstall
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -
@BruceForstall

BruceForstall commented Apr 16, 2022

Copy link
Copy Markdown
Contributor Author

@jakobbotsch @dotnet/jit-contrib PTAL
@BruceForstall

BruceForstall commented Apr 16, 2022

Copy link
Copy Markdown
Contributor Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines

azure-pipelines Bot commented Apr 16, 2022

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).
jakobbotsch
jakobbotsch approved these changes Apr 16, 2022
View reviewed changes

@jakobbotsch jakobbotsch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks for tracking this down.
Do you know why this only happens on ARM32? Do we not use STORE_BLK for args on other platforms as well?
@BruceForstall

BruceForstall commented Apr 16, 2022

Copy link
Copy Markdown
Contributor Author

Do you know why this only happens on ARM32? Do we not use STORE_BLK for args on other platforms as well?

It should happen on any non-x86 platform. It's possible that there are reasons why this particular case didn't get optimized quite right. It also requires a GC to actually happen in exactly the right place to be noticed. I'm not sure why GCStress doesn't make it happen reliably, and in fact in my testing, setting GCStress made the failure not happen.
@BruceForstall

BruceForstall commented Apr 16, 2022

Copy link
Copy Markdown
Contributor Author

If I remove the "NO_OP" change and run asm diffs, I don't see any diffs on win-x64 or win-arm64, but I do see about a dozen on linux-x64 where we extend the no-GC range (as would be expected).

(examples for libraries.crossgen2.Linux.x64:

Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE.PEParameterSymbol:Create(Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE.PEModuleSymbol,Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE.PEPropertySymbol,int,System.Reflection.Metadata.ParameterHandle,Microsoft.CodeAnalysis.ParamInfo`1[Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol],byref):Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE.PEParameterSymbol
ArraySet:GetType(Microsoft.CodeAnalysis.Emit.EmitContext):Microsoft.Cci.ITypeReference:this
System.TupleExtensions:ToTuple(System.ValueTuple`7[System.__Canon, System.__Canon, System.__Canon, System.__Canon, System.__Canon, System.__Canon, System.__Canon]):System.Tuple`7[System.__Canon, System.__Canon, System.__Canon, System.__Canon, System.__Canon, System.__Canon, System.__Canon]

and libraries_tests:

ProjectAccessor:TryGetDatabaseId(Microsoft.CodeAnalysis.SQLite.v2.Interop.SqlConnection,System.ValueTuple`2[[Microsoft.CodeAnalysis.Storage.ProjectKey, Microsoft.CodeAnalysis.Workspaces, Version=4.3.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35],[System.String, System.Private.CoreLib, Version=7.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]],bool,byref):bool:this

)

(looks like there's a problem running spmi replay on linux-arm: we run out of memory and crash)
@BruceForstall

BruceForstall commented Apr 16, 2022

Copy link
Copy Markdown
Contributor Author

Most gcstress failures are #68112
@BruceForstall

BruceForstall commented Apr 16, 2022

Copy link
Copy Markdown
Contributor Author

The NOP change is triggering an x86 GCStress=C failure in baseservices\threading\generics\Monitor\EnterExit12\EnterExit12.dll:

Assert failure(PID 107252 [0x0001a2f4], Thread: 108464 [0x1a7b0]): CheckInstrBytePattern(epilogBase[offset] & X86_INSTR_RET, X86_INSTR_RET, epilogBase[offset]) || CheckInstrBytePattern(epilogBase[offset], X86_INSTR_JMP_NEAR_REL32, epilogBase[offset]) || CheckInstrWord(*PTR_WORD(epilogBase + offset), X86_INSTR_w_JMP_FAR_IND_IMM)

CORECLR! UnwindEspFrameEpilog + 0x1DC (0x5a8eab4c)
CORECLR! UnwindStackFrame + 0x230 (0x5a8eaf50)
CORECLR! DoGcStress + 0x263 (0x5aad21fb)
CORECLR! OnGcCoverageInterrupt + 0x160 (0x5aad284e)
CORECLR! IsGcMarker + 0x93 (0x5a8ff26f)
CORECLR! CLRVectoredExceptionHandlerShim + 0xC5 (0x5a8f46f5)
NTDLL! RtlIpv4AddressToStringA + 0x132 (0x778ae5a2)
NTDLL! RtlUnwind + 0x1D1 (0x778a9261)
NTDLL! KiUserExceptionDispatcher + 0x26 (0x778b7026)
SYSTEM.PRIVATE.CORELIB! <no symbol> + 0x0 (0x59c0de2c)
    File: C:\gh\runtime\src\coreclr\vm\eetwain.cpp Line: 3531
    Image: c:\gh\runtime\artifacts\tests\coreclr\windows.x86.Checked\Tests\Core_Root\corerun.exe
@BruceForstall
BruceForstall force-pushed the FixNestedNoGcRegionRequests branch from cc176b6 to fd9d787 Compare April 16, 2022 17:47
@BruceForstall

BruceForstall commented Apr 16, 2022

Copy link
Copy Markdown
Contributor Author

I'm going to split out the NOP change to a separate PR.
@BruceForstall

BruceForstall commented Apr 16, 2022

Copy link
Copy Markdown
Contributor Author

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr gcstress0x3-gcstress0xc
@azure-pipelines

azure-pipelines Bot commented Apr 16, 2022

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).
@BruceForstall
BruceForstall merged commit 54e82d3 into dotnet:main Apr 18, 2022
@BruceForstall
BruceForstall deleted the FixNestedNoGcRegionRequests branch April 18, 2022 16:12
directhex pushed a commit to directhex/runtime that referenced this pull request Apr 21, 2022
@BruceForstall Jo Shields
Enable multiple nested "no GC" region requests (dotnet#68105)
d8fcf0e 
When doing fast tail call on arm32, we disable GC reporting.
But some IR nodes, like unrolled STORE_BLK, also disable GC
reporting. If one of those is within a fast tail call argument
setup region, we'll attempt to disable GC reporting twice. Since
we didn't keep track of nesting, we end up marking some of the
tail call region after the STORE_BLK as interruptible, leading
to be GC info in the argument area.

Change the enable/disable GC calls to keep a nesting count,
and only re-enable GC reporting when the count reaches zero.

Fixes dotnet#67046
@JulieLeeMSFT JulieLeeMSFT mentioned this pull request Apr 29, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators May 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

2 participants

@BruceForstall @jakobbotsch