Fix reporting GC refs as byrefs for crossgen2 promoted structs #66903
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
ghost
assigned 
|
Tagging subscribers to this area: @JulieLeeMSFT Issue Detailscc @dotnet/jit-contrib @Maoni0 @mangod9 @PeterSolMS @sandreenko
|
|
This adds several write barriers in the function that was concurrently executing in the crash dump that was sent around internally: ; V28 tmp20 [V28,T20] ( 2, 4 ) bool -> rdx "Inlining Arg"
; V29 tmp21 [V29,T21] ( 2, 4 ) bool -> rdx "Inlining Arg"
;* V30 tmp22 [V30 ] ( 0, 0 ) ref -> zero-ref class-hnd "Inlining Arg"
-; V31 tmp23 [V31 ] ( 12, 12 ) byref -> [rbp-28H] do-not-enreg[X] addr-exposed V05.m_task(offs=0x00) P-DEP "field V05.m_task (fldOffset=0x0)"
-; V32 tmp24 [V32 ] ( 6, 6 ) byref -> [rbp-30H] do-not-enreg[X] addr-exposed single-def V06.m_task(offs=0x00) P-DEP "field V06.m_task (fldOffset=0x0)"
+; V31 tmp23 [V31 ] ( 12, 12 ) ref -> [rbp-28H] do-not-enreg[X] addr-exposed V05.m_task(offs=0x00) P-DEP "field V05.m_task (fldOffset=0x0)"
+; V32 tmp24 [V32 ] ( 6, 6 ) ref -> [rbp-30H] do-not-enreg[X] addr-exposed single-def V06.m_task(offs=0x00) P-DEP "field V06.m_task (fldOffset=0x0)"
; V33 tmp25 [V33,T11] ( 2, 4 ) ref -> rcx single-def "argument with side effect"
; V34 tmp26 [V34 ] ( 2, 4 ) struct (16) [rbp-60H] do-not-enreg[XS] must-init addr-exposed single-def "by-value struct argument"
; V35 tmp27 [V35,T12] ( 2, 4 ) ref -> rcx single-def "argument with side effect"
@@ -96,7 +96,7 @@ G_M61588_IG04: ; gcrefRegs=00000002 {rcx}, byrefRegs=00000000 {}, byref,
; gcr arg pop 0
488BC8 mov rcx, rax
; gcrRegs +[rcx]
- 4C8D1D00000000 lea r11, [(reloc 0x40000000004218a0)]
+ 4C8D1D00000000 lea r11, [(reloc 0x40000000004218a8)]
3909 cmp dword ptr [rcx], ecx
41FF13 call [r11]System.Threading.Tasks.Task:GetAwaiter():System.Runtime.CompilerServices.TaskAwaiter:this
; gcrRegs -[rcx]
@@ -109,34 +109,41 @@ G_M61588_IG04: ; gcrefRegs=00000002 {rcx}, byrefRegs=00000000 {}, byref,
; byrRegs -[rcx]
; gcr arg pop 0
85C0 test eax, eax
- 754C jne SHORT G_M61588_IG06
- 33C9 xor ecx, ecx
+ 7556 jne SHORT G_M61588_IG06
+ 33D2 xor edx, edx
4C8B4D10 mov r9, bword ptr [rbp+10H]
; byrRegs +[r9]
- 41894920 mov dword ptr [r9+32], ecx
- 488B4DD8 mov rcx, bword ptr [rbp-28H]
+ 41895120 mov dword ptr [r9+32], edx
+ 498D4960 lea rcx, bword ptr [r9+96]
; byrRegs +[rcx]
- 49894960 mov bword ptr [r9+96], rcx
+ 488B55D8 mov rdx, gword ptr [rbp-28H]
+ ; gcrRegs +[rdx]
+ ; GC ptr vars -{V02}
+ FF1500000000 call [CORINFO_HELP_CHECKED_ASSIGN_REF]
+ ; gcrRegs -[rdx]
+ ; byrRegs -[rcx r9]
+ 4C8B4D10 mov r9, bword ptr [rbp+10H]
+ ; byrRegs +[r9]
498D4930 lea rcx, bword ptr [r9+48]
+ ; byrRegs +[rcx]
488B1500000000 mov rdx, qword ptr [(reloc 0x40000000004466f0)]
4C8D45D8 lea r8, bword ptr [rbp-28H]
; byrRegs +[r8]
- ; GC ptr vars -{V02}
FF1500000000 call [System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1:AwaitUnsafeOnCompleted(byref,byref):this]
; byrRegs -[rcx r8-r9]
; gcr arg pop 0
- E9DC020000 jmp G_M61588_IG17
- ;; bbWeight=1 PerfScore 27.75
+ E9F0020000 jmp G_M61588_IG17
+ ;; bbWeight=1 PerfScore 31.25
G_M61588_IG05: ; gcVars=0000000000000005 {V00 V02}, gcrefRegs=00000002 {rcx}, byrefRegs=00000000 {}, gcvars, byref
; gcrRegs +[rcx]
; GC ptr vars +{V02}
4C8B4D10 mov r9, bword ptr [rbp+10H]
; byrRegs +[r9]
- 4D8B5960 mov r11, bword ptr [r9+96]
- ; byrRegs +[r11]
- 4C895DD8 mov bword ptr [rbp-28H], r11
+ 4D8B5960 mov r11, gword ptr [r9+96]
+ ; gcrRegs +[r11]
+ 4C895DD8 mov gword ptr [rbp-28H], r11
4533DB xor r11d, r11d
- ; byrRegs -[r11]
+ ; gcrRegs -[r11]
4D895960 mov qword ptr [r9+96], r11
BEFFFFFFFF mov esi, -1
41C74120FFFFFFFF mov dword ptr [r9+32], -1
@@ -266,7 +273,7 @@ G_M61588_IG09: ; , isz, extend
; gcr arg pop 0
488BC8 mov rcx, rax
; gcrRegs +[rcx]
- 4C8D1D00000000 lea r11, [(reloc 0x40000000004218a0)]
+ 4C8D1D00000000 lea r11, [(reloc 0x40000000004218a8)]
3909 cmp dword ptr [rcx], ecx
41FF13 call [r11]System.Threading.Tasks.Task:GetAwaiter():System.Runtime.CompilerServices.TaskAwaiter:this
; gcrRegs -[rcx]
@@ -279,33 +286,40 @@ G_M61588_IG09: ; , isz, extend
; byrRegs -[rcx]
; gcr arg pop 0
85C0 test eax, eax
- 754E jne SHORT G_M61588_IG11
+ 7558 jne SHORT G_M61588_IG11
4C8B4D10 mov r9, bword ptr [rbp+10H]
; byrRegs +[r9]
41C7412001000000 mov dword ptr [r9+32], 1
- 488B4DD8 mov rcx, bword ptr [rbp-28H]
+ 498D4960 lea rcx, bword ptr [r9+96]
; byrRegs +[rcx]
- 49894960 mov bword ptr [r9+96], rcx
+ 488B55D8 mov rdx, gword ptr [rbp-28H]
+ ; gcrRegs +[rdx]
+ ; GC ptr vars -{V02}
+ FF1500000000 call [CORINFO_HELP_CHECKED_ASSIGN_REF]
+ ; gcrRegs -[rdx]
+ ; byrRegs -[rcx r9]
+ 4C8B4D10 mov r9, bword ptr [rbp+10H]
+ ; byrRegs +[r9]
498D4930 lea rcx, bword ptr [r9+48]
+ ; byrRegs +[rcx]
488B1500000000 mov rdx, qword ptr [(reloc 0x40000000004466f0)]
4C8D45D8 lea r8, bword ptr [rbp-28H]
; byrRegs +[r8]
- ; GC ptr vars -{V02}
FF1500000000 call [System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1:AwaitUnsafeOnCompleted(byref,byref):this]
; byrRegs -[rcx r8-r9]
; gcr arg pop 0
- E94B010000 jmp G_M61588_IG17
- ;; bbWeight=1 PerfScore 35.00
+ E955010000 jmp G_M61588_IG17
+ ;; bbWeight=1 PerfScore 38.50
G_M61588_IG10: ; gcVars=0000000000000005 {V00 V02}, gcrefRegs=00000002 {rcx}, byrefRegs=00000000 {}, gcvars, byref
; gcrRegs +[rcx]
; GC ptr vars +{V02}
4C8B4D10 mov r9, bword ptr [rbp+10H]
; byrRegs +[r9]
- 4D8B5960 mov r11, bword ptr [r9+96]
- ; byrRegs +[r11]
- 4C895DD8 mov bword ptr [rbp-28H], r11
+ 4D8B5960 mov r11, gword ptr [r9+96]
+ ; gcrRegs +[r11]
+ 4C895DD8 mov gword ptr [rbp-28H], r11
4533DB xor r11d, r11d
- ; byrRegs -[r11]
+ ; gcrRegs -[r11]
4D895960 mov qword ptr [r9+96], r11
BEFFFFFFFF mov esi, -1
41C74120FFFFFFFF mov dword ptr [r9+32], -1
@@ -322,7 +336,7 @@ G_M61588_IG11: ; gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref
;; bbWeight=1 PerfScore 3.75
G_M61588_IG12: ; gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref, isz
83FE02 cmp esi, 2
- 0F84A6000000 je G_M61588_IG13
+ 0F84B0000000 je G_M61588_IG13
488B4D98 mov rcx, gword ptr [rbp-68H]
; gcrRegs +[rcx]
488B7140 mov rsi, gword ptr [rcx+64]
@@ -379,14 +393,21 @@ G_M61588_IG12: ; gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref, isz
; byrRegs -[rcx]
; gcr arg pop 0
85C0 test eax, eax
- 7545 jne SHORT G_M61588_IG14
+ 754F jne SHORT G_M61588_IG14
4C8B4D10 mov r9, bword ptr [rbp+10H]
; byrRegs +[r9]
41C7412002000000 mov dword ptr [r9+32], 2
- 488B4DD0 mov rcx, bword ptr [rbp-30H]
+ 498D4968 lea rcx, bword ptr [r9+104]
; byrRegs +[rcx]
- 49894968 mov bword ptr [r9+104], rcx
+ 488B55D0 mov rdx, gword ptr [rbp-30H]
+ ; gcrRegs +[rdx]
+ FF1500000000 call [CORINFO_HELP_CHECKED_ASSIGN_REF]
+ ; gcrRegs -[rdx]
+ ; byrRegs -[rcx r9]
+ 4C8B4D10 mov r9, bword ptr [rbp+10H]
+ ; byrRegs +[r9]
498D4930 lea rcx, bword ptr [r9+48]
+ ; byrRegs +[rcx]
488B1500000000 mov rdx, qword ptr [(reloc 0x4000000000446798)]
4C8D45D0 lea r8, bword ptr [rbp-30H]
; byrRegs +[r8]
@@ -394,15 +415,15 @@ G_M61588_IG12: ; gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref, isz
; byrRegs -[rcx r8-r9]
; gcr arg pop 0
EB71 jmp SHORT G_M61588_IG17
- ;; bbWeight=1 PerfScore 59.25
+ ;; bbWeight=1 PerfScore 62.75
G_M61588_IG13: ; gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref
4C8B4D10 mov r9, bword ptr [rbp+10H]
; byrRegs +[r9]
- 498B4968 mov rcx, bword ptr [r9+104]
- ; byrRegs +[rcx]
- 48894DD0 mov bword ptr [rbp-30H], rcx
+ 498B4968 mov rcx, gword ptr [r9+104]
+ ; gcrRegs +[rcx]
+ 48894DD0 mov gword ptr [rbp-30H], rcx
33C9 xor ecx, ecx
- ; byrRegs -[rcx]
+ ; gcrRegs -[rcx]
49894968 mov qword ptr [r9+104], rcx
41C74120FFFFFFFF mov dword ptr [r9+32], -1
;; bbWeight=1 PerfScore 6.25
@@ -525,7 +546,7 @@ G_M61588_IG22: ; gcVars=0000000000000001 {V00}, gcrefRegs=00000004 {rdx},
; gcrRegs -[r8]
; byrRegs -[rcx r9]
; gcr arg pop 0
- 488D058C030000 lea rax, G_M61588_IG17
+ 488D05AA030000 lea rax, G_M61588_IG17
;; bbWeight=0 PerfScore 0.00
G_M61588_IG23: ; , funclet epilog, nogc, extend
4883C430 add rsp, 48
@@ -536,12 +557,12 @@ G_M61588_IG23: ; , funclet epilog, nogc, extend
5D pop rbp
C3 ret
;; bbWeight=0 PerfScore 0.00
-RWD00 dd 0000007Dh ; case G_M61588_IG05
- dd 0000020Eh ; case G_M61588_IG10
- dd 00000239h ; case G_M61588_IG12
+RWD00 dd 00000087h ; case G_M61588_IG05
+ dd 00000222h ; case G_M61588_IG10
+ dd 0000024Dh ; case G_M61588_IG12
-; Total bytes of code 1056, prolog size 51, PerfScore 375.10, instruction count 259, allocated bytes for code 1056 (MethodHash=0cd00f6b) for method <GetThrottledResponse>d__22:MoveNext():this
+; Total bytes of code 1086, prolog size 51, PerfScore 388.60, instruction count 265, allocated bytes for code 1086 (MethodHash=0cd00f6b) for method <GetThrottledResponse>d__22:MoveNext():this
; ============================================================ |
|
thanks for getting a fix out so quickly, @jakobbotsch! |
|
is there a test that goes with this fix? it slipped through runtime ci, nothing failed in sdk ci and finally blocked the installer for two weeks dotnet/installer#13376. it is a long way to rely on installer for catching major bugs |
From my understanding GC info reporting is very hard to test, we have gc stress tests but apparently this one wasn't caught - is there a pipeline with R2R-only (no TC) + GC stress? |
EgorBo
approved these changes
Mar 21, 2022
cc @dotnet/crossgen-contrib |
|
I saw a failure in the first run of Assert failure(PID 312 [0x00000138], Thread: 320 [0x0140]): fieldSize != (unsigned int)-1
File: /__w/1/s/src/coreclr/vm/methodtable.cpp Line: 2323
Image: /root/helix/work/correlation/dotnet
System.Transactions.Tests.AsyncTransactionScopeTests.AsyncTSTest(variation: 48) [FAIL]
Exit code was 134 but it should have been 42
Expected: True
Actual: False
Stack Trace:
/_/src/Microsoft.DotNet.RemoteExecutor/src/RemoteInvokeHandle.cs(239,0): at Microsoft.DotNet.RemoteExecutor.RemoteInvokeHandle.Dispose(Boolean disposing)
/_/src/libraries/System.Transactions.Local/tests/AsyncTransactionScopeTests.cs(100,0): at System.Transactions.Tests.AsyncTransactionScopeTests.AsyncTSTest(Int32 variation)
Finished: System.Transactions.Local.TestsThis is an assertion failure inside Note that we have seen a similar failure before but in another test: #64544. |
|
yeah perhaps reverting the original change to unblock the sdk merge might be better assuming it was only a perf optimization and no other changes have been made on top. |
|
Will resubmit #65682 along with this fix later. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
cc @dotnet/jit-contrib @Maoni0 @mangod9 @PeterSolMS @sandreenko