Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Source generator tests failing with AV in Roslyn #102919

Closed
kunalspathak opened this issue May 31, 2024 · 28 comments · Fixed by #103301
Closed

Source generator tests failing with AV in Roslyn #102919

kunalspathak opened this issue May 31, 2024 · 28 comments · Fixed by #103301
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' Known Build Error Use this to report build issues in the .NET Helix tab Priority:1 Work that is critical for the release, but we could probably ship without
Milestone

Comments

@kunalspathak
Copy link
Member

kunalspathak commented May 31, 2024

Build Information

Build: https://dev.azure.com/dnceng-public/cbb18261-c48f-4abb-8651-8cdcb5474649/_build/results?buildId=693513
Build error leg or test failing: System.Text.RegularExpressions.Tests.RegexPcre2Tests.IsMatchTests
Pull request: #102903

Error Message

Fill the error message using step by step known issues guidance.

{
  "ErrorMessage": [ "System.AccessViolationException", "Microsoft.CodeAnalysis.CSharp.CSharpCompilation.CompileMethods" ],
  "BuildRetry": false,
  "ExcludeConsoleLog": false
}

Known issue validation

Build: 🔎 https://dev.azure.com/dnceng-public/public/_build/results?buildId=693513
Error message validated: [System.AccessViolationException Microsoft.CodeAnalysis.CSharp.CSharpCompilation.CompileMethods]
Result validation: ✅ Known issue matched with the provided build.
Validation performed at: 5/31/2024 5:13:05 PM UTC

Report

Build Definition Test Pull Request
713808 dotnet/runtime System.Text.RegularExpressions.Tests.WorkItemExecution
713038 dotnet/runtime System.Text.RegularExpressions.Tests.WorkItemExecution #103706
709265 dotnet/runtime System.Text.RegularExpressions.Tests.WorkItemExecution
705911 dotnet/runtime System.Text.RegularExpressions.Tests.WorkItemExecution #103381
693513 dotnet/runtime System.Text.RegularExpressions.Tests.RegexPcre2Tests.IsMatchTests #102903

Summary

24-Hour Hit Count 7-Day Hit Count 1-Month Count
0 1 5
@kunalspathak kunalspathak added blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' Known Build Error Use this to report build issues in the .NET Helix tab labels May 31, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label May 31, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions
See info in area-owners.md if you want to be subscribed.

@stephentoub stephentoub added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed area-System.Text.RegularExpressions labels May 31, 2024
Copy link
Contributor

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

@jkotas jkotas changed the title RegexPcre2Tests.IsMatchTests failing with AV Source generator tests failing with AV in Roslyn May 31, 2024
@jkotas
Copy link
Member

jkotas commented May 31, 2024

Stacktrace of the failure:

      System.AccessViolationException : Attempted to read or write protected memory. This is often an indication that other memory is corrupt.
      Stack Trace:
           at Microsoft.CodeAnalysis.CSharp.Binder.BindConstructorInitializerCoreContinued(Boolean found, ArgumentListSyntax initializerArgumentListOpt, MethodSymbol constructor, AnalyzedArguments analyzedArguments, TypeSymbol constructorReturnType, NamedTypeSymbol initializerType, Boolean isBaseConstructorInitializer, CSharpSyntaxNode nonNullSyntax, Location errorLocation, Boolean enableCallerInfo, MemberResolutionResult`1 memberResolutionResult, ImmutableArray`1 candidateConstructors, CompoundUseSiteInfo`1& overloadResolutionUseSiteInfo, BindingDiagnosticBag diagnostics)
           at Microsoft.CodeAnalysis.CSharp.Binder.BindConstructorInitializerCore(ArgumentListSyntax initializerArgumentListOpt, MethodSymbol constructor, BindingDiagnosticBag diagnostics)
           at Microsoft.CodeAnalysis.CSharp.Binder.BindImplicitConstructorInitializer(MethodSymbol constructor, BindingDiagnosticBag diagnostics, CSharpCompilation compilation)
           at Microsoft.CodeAnalysis.CSharp.MethodCompiler.BindMethodBody(MethodSymbol method, TypeCompilationState compilationState, BindingDiagnosticBag diagnostics, Boolean includeInitializersInBody, BoundNode initializersBody, Boolean reportNullableDiagnostics, ImportChain& importChain, Boolean& originalBodyNested, Boolean& prependedDefaultValueTypeConstructorInitializer, InitialState& forSemanticModel)
           at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileMethod(MethodSymbol methodSymbol, Int32 methodOrdinal, ProcessedFieldInitializers& processedInitializers, SynthesizedSubmissionFields previousSubmissionFields, TypeCompilationState compilationState)
           at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileNamedType(NamedTypeSymbol containingType)
           at Microsoft.CodeAnalysis.CSharp.MethodCompiler.<>c__DisplayClass25_0.<CompileNamedTypeAsync>b__0()
        /_/src/libraries/System.Private.CoreLib/src/System/Threading/ExecutionContext.cs(264,0): at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
        --- End of stack trace from previous location ---
        /_/src/libraries/System.Private.CoreLib/src/System/Threading/ExecutionContext.cs(289,0): at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
        /_/src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/Task.cs(2346,0): at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
        --- End of stack trace from previous location ---
           at Microsoft.CodeAnalysis.CSharp.MethodCompiler.WaitForWorkers()
           at Microsoft.CodeAnalysis.CSharp.MethodCompiler.CompileMethodBodies(CSharpCompilation compilation, PEModuleBuilder moduleBeingBuiltOpt, Boolean emittingPdb, Boolean hasDeclarationErrors, Boolean emitMethodBodies, BindingDiagnosticBag diagnostics, Predicate`1 filterOpt, CancellationToken cancellationToken)
           at Microsoft.CodeAnalysis.CSharp.CSharpCompilation.CompileMethods(CommonPEModuleBuilder moduleBuilder, Boolean emittingPdb, DiagnosticBag diagnostics, Predicate`1 filterOpt, CancellationToken cancellationToken)
           at Microsoft.CodeAnalysis.Compilation.Emit(Stream peStream, Stream metadataPEStream, Stream pdbStream, Stream xmlDocumentationStream, Stream win32Resources, IEnumerable`1 manifestResources, EmitOptions options, IMethodSymbol debugEntryPoint, Stream sourceLinkStream, IEnumerable`1 embeddedTexts, RebuildData rebuildData, CompilationTestData testData, CancellationToken cancellationToken)
           at Microsoft.CodeAnalysis.Compilation.Emit(Stream peStream, Stream pdbStream, Stream xmlDocumentationStream, Stream win32Resources, IEnumerable`1 manifestResources, EmitOptions options, IMethodSymbol debugEntryPoint, Stream sourceLinkStream, IEnumerable`1 embeddedTexts, Stream metadataPEStream, RebuildData rebuildData, CancellationToken cancellationToken)
           at Microsoft.CodeAnalysis.Compilation.Emit(Stream peStream, Stream pdbStream, Stream xmlDocumentationStream, Stream win32Resources, IEnumerable`1 manifestResources, EmitOptions options, IMethodSymbol debugEntryPoint, Stream sourceLinkStream, IEnumerable`1 embeddedTexts, Stream metadataPEStream, CancellationToken cancellationToken)
        /_/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/RegexGeneratorHelper.netcoreapp.cs(227,0): at System.Text.RegularExpressions.Tests.RegexGeneratorHelper.SourceGenRegexAsync(ValueTuple`4[] regexes, CancellationToken cancellationToken)
        /_/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/Regex.Tests.Common.cs(147,0): at System.Text.RegularExpressions.Tests.RegexHelpers.GetRegexesAsync(RegexEngine engine, ValueTuple`4[] regexes)
        /_/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/Regex.Tests.Common.cs(135,0): at System.Text.RegularExpressions.Tests.RegexHelpers.GetRegexes(RegexEngine engine, ValueTuple`4[] regexes)
        /_/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/RegexPcreTests.cs(22,0): at System.Text.RegularExpressions.Tests.RegexPcre2Tests.PcreTestData()+MoveNext()
        /_/src/libraries/System.Linq/src/System/Linq/Select.cs(131,0): at System.Linq.Enumerable.IEnumerableSelectIterator`2.MoveNext()

@jkotas
Copy link
Member

jkotas commented May 31, 2024

The CI failure is missing crash dump. It is not actionable. cc @JulieLeeMSFT

@JulieLeeMSFT
Copy link
Member

@jkotas, Juan looked into it. AccessViolationException shows that xunit is suppressing the exception making the process not crash. So, we won't get a crash dump in this case.

The CI failure is missing crash dump. It is not actionable. cc @JulieLeeMSFT

@JulieLeeMSFT JulieLeeMSFT added Priority:1 Work that is critical for the release, but we could probably ship without Priority:2 Work that is important, but not critical for the release and removed untriaged New issue has not been triaged by the area owner Priority:2 Work that is important, but not critical for the release labels Jun 3, 2024
@JulieLeeMSFT JulieLeeMSFT added this to the 9.0.0 milestone Jun 3, 2024
@jkotas
Copy link
Member

jkotas commented Jun 3, 2024

AccessViolationException is not a catchable exception. xunit cannot suppress it. AccessViolationException results into immediate process termination that produces crash dump (when the runtime is configured to produce crash dumps).

@jkotas
Copy link
Member

jkotas commented Jun 4, 2024

Ok, there is a regression in EH that makes us to not produce crash dumps on access violations. Opened: #103018

@AndyAyersMS
Copy link
Member

Trying to get a local repro, no luck so far. Suspect it will require another hit in CI.

@jakobbotsch
Copy link
Member

There is a failure from a couple of days ago now that has a crash dump it looks like.

@jkotas
Copy link
Member

jkotas commented Jun 19, 2024

Analysis for build 709265:

  • No GC heap corruptions detected by VerifyHeap
  • The crash is caused by bogus GC pointer in AnalyzedArguments arg at this callstack:
Microsoft.CodeAnalysis.CSharp.Binder.CheckAndCoerceArguments[[System.__Canon, System.Private.CoreLib]](Microsoft.CodeAnalysis.SyntaxNode, Microsoft.CodeAnalysis.CSharp.MemberResolutionResult`1, Microsoft.CodeAnalysis.CSharp.AnalyzedArguments, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Microsoft.CodeAnalysis.CSharp.BoundExpression, Boolean, System.Collections.Immutable.ImmutableArray`1 ByRef) [/_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs @ 3359]
Microsoft.CodeAnalysis.CSharp.Binder.BindClassCreationExpressionContinued(Microsoft.CodeAnalysis.SyntaxNode, Microsoft.CodeAnalysis.SyntaxNode, Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol, Microsoft.CodeAnalysis.CSharp.AnalyzedArguments, Microsoft.CodeAnalysis.CSharp.Syntax.InitializerExpressionSyntax, Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol, Boolean, Microsoft.CodeAnalysis.CSharp.MemberResolutionResult`1, System.Collections.Immutable.ImmutableArray`1, Microsoft.CodeAnalysis.CompoundUseSiteInfo`1 ByRef, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs @ 6672]
Microsoft.CodeAnalysis.CSharp.Binder.BindClassCreationExpression(Microsoft.CodeAnalysis.SyntaxNode, System.String, Microsoft.CodeAnalysis.SyntaxNode, Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol, Microsoft.CodeAnalysis.CSharp.AnalyzedArguments, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Microsoft.CodeAnalysis.CSharp.Syntax.InitializerExpressionSyntax, Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol, Boolean) [/_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs @ 6638]
Microsoft.CodeAnalysis.CSharp.Binder.BindClassCreationExpression(Microsoft.CodeAnalysis.CSharp.Syntax.ObjectCreationExpressionSyntax, Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol, System.String, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol) [/_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs @ 5476]
Microsoft.CodeAnalysis.CSharp.Binder.g__bindObjectCreationExpression|430_0(Microsoft.CodeAnalysis.CSharp.Syntax.ObjectCreationExpressionSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs @ 5161]
Microsoft.CodeAnalysis.CSharp.Binder.g__bindExpressionInternal|334_0(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Boolean, Boolean) [/_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs @ 614]
Microsoft.CodeAnalysis.CSharp.Binder.BindExpressionInternal(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Boolean, Boolean) [/_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs @ 579]

Actual AnalyzedArguments value:

0:018> !do 0000018fa4da5c80
<Note: this object has an invalid CLASS field>

Expected AnalyzedArguments value from !dso:

0:018> !do 0000018fa6277c80
Name:        Microsoft.CodeAnalysis.CSharp.AnalyzedArguments

@jkotas
Copy link
Member

jkotas commented Jun 19, 2024

Analysis for build 705911:

  • No GC heap corruptions detected by VerifyHeap
  • The crash is caused by bogus GC pointer in AnalyzedArguments arg at this callstack:
Microsoft.CodeAnalysis.CSharp.Binder.BindClassCreationExpressionContinued(Microsoft.CodeAnalysis.SyntaxNode, Microsoft.CodeAnalysis.SyntaxNode, Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol, Microsoft.CodeAnalysis.CSharp.AnalyzedArguments, Microsoft.CodeAnalysis.CSharp.Syntax.InitializerExpressionSyntax, Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol, Boolean, Microsoft.CodeAnalysis.CSharp.MemberResolutionResult`1, System.Collections.Immutable.ImmutableArray`1, Microsoft.CodeAnalysis.CompoundUseSiteInfo`1 ByRef, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs @ 6672]
Microsoft.CodeAnalysis.CSharp.Binder.BindClassCreationExpression(Microsoft.CodeAnalysis.CSharp.Syntax.ObjectCreationExpressionSyntax, Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol, System.String, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol)
Microsoft.CodeAnalysis.CSharp.Binder.g__bindObjectCreationExpression|430_0(Microsoft.CodeAnalysis.CSharp.Syntax.ObjectCreationExpressionSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag)
Microsoft.CodeAnalysis.CSharp.Binder.g__bindExpressionInternal|334_0(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Boolean, Boolean) [/_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs @ 614]
Microsoft.CodeAnalysis.CSharp.Binder.BindExpressionInternal(Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag, Boolean, Boolean) [/_/src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs @ 579]
Microsoft.CodeAnalysis.CSharp.Binder.BindExpressionBodyAsBlock(Microsoft.CodeAnalysis.CSharp.Syntax.ArrowExpressionClauseSyntax, Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag) [/_/src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs @ 3492]

Actual AnalyzedArguments value:

0:014> !do 0000032d3b90500
<Note: this object has an invalid CLASS field>

Expected AnalyzedArguments value from !dso:

0:014> !do 0000032d33a09f8
Name:        Microsoft.CodeAnalysis.CSharp.AnalyzedArguments

@jkotas
Copy link
Member

jkotas commented Jun 19, 2024

@VSadov Could you please take a look? The symptoms suggest that we are missing GC reporting for AnalyzedArguments argument or local variable somewhere near BindClassCreationExpression. It may be related to the GC reporting changes that you have been working on recently.

@VSadov
Copy link
Member

VSadov commented Jun 19, 2024

I will take a look.
2 hits in a week - is it that rare or just not reported reliably?

@jkotas
Copy link
Member

jkotas commented Jun 19, 2024

This particular crash signature is that rare. It is likely that there are other failures caused by the same underlying bug.

@VSadov
Copy link
Member

VSadov commented Jun 19, 2024

Also - from stacks it looks like it is the compiler crashing (so bug would be in the toolset runtime), or is it in the actual tests?

@jkotas
Copy link
Member

jkotas commented Jun 19, 2024

The crash is in source generator tests. It is running on live built runtime.

@VSadov
Copy link
Member

VSadov commented Jun 20, 2024

I am able to repro (to get a GC stress failure) on these tests. Not sure what the reasons for it yet.

@VSadov
Copy link
Member

VSadov commented Jun 22, 2024

It looks like something is happening when BindConstructorInitializerCoreContinued is called from BindConstructorInitializerCore.

The failure happens when we call Relocate and the argument #4 is corrupted.
The failure is nondeterministic. Fails only sometimes. (and does not want to fail when debugger is attached)

Right now it does not look like a problem with root reporting. We seem to report roots correctly and arg #4 is correctly repointed, when there is no failure,...
Problems with GC info are typically more deterministic.

@VSadov
Copy link
Member

VSadov commented Jun 23, 2024

Suppressing TryLowerBlockStoreAsGcBulkCopyCall optimization fixes the issue

@VSadov
Copy link
Member

VSadov commented Jun 23, 2024

The suspicious part is that we can have CORINFO_HELP_BULK_WRITEBARRIER in the middle of argument set up. Where before it would be a sequence of arg copies, we now have a method call to a managed helper.

Perhaps this is another variant of #102577 - we have replaced uninterrupted sequence with one that has a call in it and something is not ready for that.

		bool found = TryPerformConstructorOverloadResolution(namedTypeSymbol, instance, ".ctor", location, suppressResultDiagnostics: false, diagnostics, out memberResolutionResult, out candidateConstructors, allowProtectedConstructorsOfBaseType: true, out useSiteInfo2);
		return BindConstructorInitializerCoreContinued(found, initializerArgumentListOpt, constructor, instance, returnType, namedTypeSymbol, flag, cSharpSyntaxNode2, location, enableCallerInfo, memberResolutionResult, candidateConstructors, in useSiteInfo2, diagnostics);
       call     [Microsoft.CodeAnalysis.CSharp.Binder:TryPerformConstructorOverloadResolution(Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol,Microsoft.CodeAnalysis.CSharp.AnalyzedArguments,System.String,Microsoft.CodeAnalysis.Location,ubyte,Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag,byref,byref,ubyte,byref,ubyte):ubyte:this]
                            ; gcrRegs -[rcx rdx r8-r9]
                            ; gcr arg pop 0
       mov      dword ptr [rbp-0x30C], eax
       mov      r9, gword ptr [rbp-0x318]
                            ; gcrRegs +[r9]
       mov      gword ptr [rsp+0x20], r9
                            ; gcr arg write
       mov      r10, gword ptr [rbp-0x320]
                            ; gcrRegs +[r10]
       mov      gword ptr [rsp+0x28], r10
                            ; gcr arg write
       mov      r11, gword ptr [rbp-0x328]
                            ; gcrRegs +[r11]
       mov      gword ptr [rsp+0x30], r11
                            ; gcr arg write
       mov      r8d, dword ptr [rbp-0x3C]
       mov      dword ptr [rsp+0x38], r8d
       mov      gword ptr [rsp+0x40], r15
                            ; gcr arg write
       mov      gword ptr [rsp+0x48], r13
                            ; gcr arg write
       mov      r15d, dword ptr [rbp-0x40]
                            ; gcrRegs -[r15]
       mov      dword ptr [rsp+0x50], r15d
       lea      rcx, bword ptr [rbp-0x2E8]
                            ; byrRegs +[rcx]
       lea      rdx, [rbp-0xC0]
       mov      r8d, 128
                            ; GC ptr vars -{V06 V07}
       call     CORINFO_HELP_BULK_WRITEBARRIER                              <============
                            ; gcrRegs -[r9-r11 r13]
                            ; byrRegs -[rcx]
                            ; gcr arg pop 0
       mov      rdx, gword ptr [rbp-0xC8]
                            ; gcrRegs +[rdx]
       mov      gword ptr [rsp+0x60], rdx
                            ; gcr arg write
       lea      rdx, [rbp-0xE8]
                            ; gcrRegs -[rdx]
       mov      qword ptr [rsp+0x68], rdx
       mov      gword ptr [rsp+0x70], rsi
                            ; gcr arg write
       mov      edx, dword ptr [rbp-0x30C]
       lea      r8, [rbp-0x2E8]
       mov      qword ptr [rsp+0x58], r8
       mov      r8, rbx
                            ; gcrRegs +[r8]
       mov      r9, r14
                            ; gcrRegs +[r9]
       mov      rcx, rdi
                            ; gcrRegs +[rcx]
       call     [Microsoft.CodeAnalysis.CSharp.Binder:BindConstructorInitializerCoreContinued(ubyte,Microsoft.CodeAnalysis.CSharp.Syntax.ArgumentListSyntax,Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol,Microsoft.CodeAnalysis.CSharp.AnalyzedArguments,Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol,Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol,ubyte,Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode,Microsoft.CodeAnalysis.Location,ubyte,Microsoft.CodeAnalysis.CSharp.MemberResolutionResult`1[Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol],System.Collections.Immutable.ImmutableArray`1[Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol],byref,Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag):Microsoft.CodeAnalysis.CSharp.BoundExpression:this]
                            ; gcrRegs -[rcx rbx rsi rdi r8-r9 r14] +[rax]
                            ; gcr arg pop 0

Two questions:

  • Is this a block-copy to a stack location? Perhaps using a helper is not an optimization in this case?
  • What if GC happens when we are in the helper? Will we know about already pushed arguments?

@jkotas
Copy link
Member

jkotas commented Jun 23, 2024

cc @EgorBo

@VSadov
Copy link
Member

VSadov commented Jun 23, 2024

Without TryLowerBlockStoreAsGcBulkCopyCall the codegen equivalent looks roughly as compact as above.

       call     [Microsoft.CodeAnalysis.CSharp.Binder:TryPerformConstructorOverloadResolution(Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol,Microsoft.CodeAnalysis.CSharp.AnalyzedArguments,System.String,Microsoft.CodeAnalysis.Location,ubyte,Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag,byref,byref,ubyte,byref,ubyte):ubyte:this]
                            ; gcrRegs -[rcx rdx r8-r9]
                            ; gcr arg pop 0
       mov      edx, eax
       mov      r9, gword ptr [rbp-0x318]
                            ; gcrRegs +[r9]
       mov      gword ptr [rsp+0x20], r9
                            ; gcr arg write
       mov      r10, gword ptr [rbp-0x320]
                            ; gcrRegs +[r10]
       mov      gword ptr [rsp+0x28], r10
                            ; gcr arg write
       mov      r11, gword ptr [rbp-0x328]
                            ; gcrRegs +[r11]
       mov      gword ptr [rsp+0x30], r11
                            ; gcr arg write
       mov      r8d, dword ptr [rbp-0x3C]
       mov      dword ptr [rsp+0x38], r8d
       mov      gword ptr [rsp+0x40], rsi
                            ; gcr arg write
       mov      gword ptr [rsp+0x48], rdi
                            ; gcr arg write
       mov      esi, dword ptr [rbp-0x40]
                            ; gcrRegs -[rsi]
       mov      dword ptr [rsp+0x50], esi
       lea      rdi, bword ptr [rbp-0x2E8]
                            ; gcrRegs -[rdi]
                            ; byrRegs +[rdi]
       lea      rsi, bword ptr [rbp-0xC0]
                            ; byrRegs +[rsi]
       mov      ecx, 16
       rep movsq
       mov      r8, gword ptr [rbp-0xC8]
                            ; gcrRegs +[r8]
       mov      gword ptr [rsp+0x60], r8
                            ; gcr arg write
       lea      r8, [rbp-0xE8]
                            ; gcrRegs -[r8]
       mov      qword ptr [rsp+0x68], r8
       mov      gword ptr [rsp+0x70], r14
                            ; gcr arg write
       lea      r8, [rbp-0x2E8]
       mov      qword ptr [rsp+0x58], r8
       mov      r8, rbx
                            ; gcrRegs +[r8]
       mov      r9, r13
       mov      rcx, r15
                            ; gcrRegs +[rcx]
                            ; GC ptr vars -{V06 V07}
       call     [Microsoft.CodeAnalysis.CSharp.Binder:BindConstructorInitializerCoreContinued(ubyte,Microsoft.CodeAnalysis.CSharp.Syntax.ArgumentListSyntax,Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol,Microsoft.CodeAnalysis.CSharp.AnalyzedArguments,Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol,Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol,ubyte,Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode,Microsoft.CodeAnalysis.Location,ubyte,Microsoft.CodeAnalysis.CSharp.MemberResolutionResult`1[Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol],System.Collections.Immutable.ImmutableArray`1[Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol],byref,Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag):Microsoft.CodeAnalysis.CSharp.BoundExpression:this]
                            ; gcrRegs -[rcx rbx r8-r11 r13-r15] +[rax]
                            ; byrRegs -[rsi rdi]
                            ; gcr arg pop 0

The issue of suspension starvation is mitigated for stack pushes by the fact that stacks sizes are in order of megabytes and that is very limiting to how long it could take to make these copies.

If we ever want to support much larger stacks, methods that push a lot could be made fully interruptible and use the helper. I think that would work, since fully interruptible code tracks argument pushes.

@EgorBo
Copy link
Member

EgorBo commented Jun 24, 2024

cc @EgorBo

Thanks! I'll take a look, presumably it's not this helper specific as there are other managed helpers

@jakobbotsch
Copy link
Member

jakobbotsch commented Jun 24, 2024

The fundamental problem here is most likely #103300 (#103300 (comment) is basically this issue I'd expect). Can you check if #103301 fixes the issue?

@VSadov
Copy link
Member

VSadov commented Jun 24, 2024

@jakobbotsch I think #103301 should help. I will check.

@VSadov
Copy link
Member

VSadov commented Jun 24, 2024

#103301 puts CORINFO_HELP_BULK_WRITEBARRIER before all putargs and fixes the issue.

I have run the repro in a loop, to be sure. It has run through more than enough iterations to typically see failures, but so far it keeps running.

       call     [Microsoft.CodeAnalysis.CSharp.Binder:TryPerformConstructorOverloadResolution(Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol,Microsoft.CodeAnalysis.CSharp.AnalyzedArguments,System.String,Microsoft.CodeAnalysis.Location,ubyte,Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag,byref,byref,ubyte,byref,ubyte):ubyte:this]
                            ; gcrRegs -[rcx rdx r8-r9]
                            ; gcr arg pop 0
       mov      dword ptr [rbp-0x30C], eax
       lea      rcx, bword ptr [rbp-0x2E8]
                            ; byrRegs +[rcx]
       lea      rdx, [rbp-0xC0]
       mov      r8d, 128
       call     CORINFO_HELP_BULK_WRITEBARRIER        <--  now before all putargs
                            ; byrRegs -[rcx]
                            ; gcr arg pop 0
       mov      rdx, gword ptr [rbp-0xC8]
                            ; gcrRegs +[rdx]
       mov      gword ptr [rsp+0x60], rdx
                            ; gcr arg write
       lea      rdx, [rbp-0xE8]
                            ; gcrRegs -[rdx]
       mov      qword ptr [rsp+0x68], rdx
       mov      gword ptr [rsp+0x70], rsi
                            ; gcr arg write
       mov      edx, dword ptr [rbp-0x30C]
       lea      r8, [rbp-0x2E8]
       mov      qword ptr [rsp+0x58], r8
       mov      r8, rbx
                            ; gcrRegs +[r8]
       mov      r9, r14
                            ; gcrRegs +[r9]
       mov      rcx, rdi
                            ; gcrRegs +[rcx]
       mov      r10, gword ptr [rbp-0x318]
                            ; gcrRegs +[r10]
       mov      gword ptr [rsp+0x20], r10
                            ; gcr arg write
       mov      rbx, gword ptr [rbp-0x320]
       mov      gword ptr [rsp+0x28], rbx
                            ; gcr arg write
       mov      rbx, gword ptr [rbp-0x328]
       mov      gword ptr [rsp+0x30], rbx
                            ; gcr arg write
       mov      edi, dword ptr [rbp-0x3C]
                            ; gcrRegs -[rdi]
       mov      dword ptr [rsp+0x38], edi
       mov      gword ptr [rsp+0x40], r15
                            ; gcr arg write
       mov      gword ptr [rsp+0x48], r13
                            ; gcr arg write
       mov      ebx, dword ptr [rbp-0x40]
                            ; gcrRegs -[rbx]
       mov      dword ptr [rsp+0x50], ebx
                            ; GC ptr vars -{V06 V07}
       call     [Microsoft.CodeAnalysis.CSharp.Binder:BindConstructorInitializerCoreContinued(ubyte,Microsoft.CodeAnalysis.CSharp.Syntax.ArgumentListSyntax,Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol,Microsoft.CodeAnalysis.CSharp.AnalyzedArguments,Microsoft.CodeAnalysis.CSharp.Symbols.TypeSymbol,Microsoft.CodeAnalysis.CSharp.Symbols.NamedTypeSymbol,ubyte,Microsoft.CodeAnalysis.CSharp.CSharpSyntaxNode,Microsoft.CodeAnalysis.Location,ubyte,Microsoft.CodeAnalysis.CSharp.MemberResolutionResult`1[Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol],System.Collections.Immutable.ImmutableArray`1[Microsoft.CodeAnalysis.CSharp.Symbols.MethodSymbol],byref,Microsoft.CodeAnalysis.CSharp.BindingDiagnosticBag):Microsoft.CodeAnalysis.CSharp.BoundExpression:this]
                            ; gcrRegs -[rcx rsi r8-r10 r13-r15] +[rax]
                            ; gcr arg pop 0

@jakobbotsch
Copy link
Member

Thanks for confirming. I can grab this one.

@VSadov
Copy link
Member

VSadov commented Jun 24, 2024

@EgorBo I still think that if we can know statically that the dest is on stack, we might not want to involve CORINFO_HELP_BULK_WRITEBARRIER.

We use the number of object fields > 4 as a threshold to use the helper. I assume that is the number of write barriers that we do not need to emit. But when the target is on stack, the object fields do not need to do barriers, so technically the stack-targeted copy would not meet the optimization threshold.

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 blocking-clean-ci Blocking PR or rolling runs of 'runtime' or 'runtime-extra-platforms' Known Build Error Use this to report build issues in the .NET Helix tab Priority:1 Work that is critical for the release, but we could probably ship without
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants