Use destType for size instead of srcType when writing a variable from a register to the stack. by jkoritzinsky · Pull Request #46176 · dotnet/runtime

jkoritzinsky · 2020-12-17T01:07:50Z

If we use srcType, we'll in some cases do a 64-bit store instead of a 32-bit store or vice versa. This is causing random failures as described in #46172.

Alternative fix to #46172

I've validated that this PR has no diffs outside of P/Invoke stubs from the JIT in 35fbaef (the commit before #45625).

cc: @safern

jkoritzinsky · 2020-12-17T01:08:15Z

cc @dotnet/jit-contrib

jkoritzinsky · 2020-12-17T01:13:16Z

The P/Invoke stub diffs are due to introducing a P/Invoke Frame instead of omitting it. As a result, I am fully confident that this is a complete fix for the random crashes.

Base

; Assembly listing for method ILStubClass:IL_STUB_PInvoke(int):int
; Emitting BLENDED_CODE for X64 CPU with SSE2 - Unix
; ReadyToRun compilation
; optimized code
; rbp based frame
; partially interruptible
; Final local variable assignments
;
; V00 arg0 [V00,T00] ( 3, 3 ) int -> rdi ;* V01 loc0 [V01 ] ( 0, 0 ) int -> zero-ref ; V02 loc1 [V02,T02] ( 2, 2 ) long -> rdi ;* V03 loc2 [V03 ] ( 0, 0 ) int -> zero-ref ; V04 loc3 [V04,T03] ( 2, 2 ) int -> rbx ;# V05 OutArgs [V05 ] ( 1, 1 ) lclBlk ( 0) [rsp+0x00] "OutgoingArgSpace"
; V06 tmp1 [V06 ] ( 2, 2 ) long -> [rbp-0x10] do-not-enreg[X] addr-exposed "stub argument"
; V07 tmp2 [V07,T01] ( 2, 4 ) long -> rax "impImportIndirectCall"
;
; Lcl frame size = 8
G_M63189_IG01:
push rbp
push rbx
push rax
lea rbp, [rsp+10H]
mov qword ptr [rbp-10H], r10
;; bbWeight=1 PerfScore 4.50
G_M63189_IG02:
movsxd rdi, edi
mov rax, qword ptr [rbp-10H]
mov rax, qword ptr [rax+72]
mov rax, qword ptr [rax]
;; bbWeight=1 PerfScore 5.25
G_M63189_IG03:
call rax
mov ebx, eax
mov rax, qword ptr [(reloc)]
cmp dword ptr [rax], 0
jne SHORT G_M63189_IG06
;; bbWeight=1 PerfScore 8.25
G_M63189_IG04:
mov eax, ebx
;; bbWeight=1 PerfScore 0.25
G_M63189_IG05:
lea rsp, [rbp-08H]
pop rbx
pop rbp
ret ;; bbWeight=1 PerfScore 2.50
G_M63189_IG06:
call [CORINFO_HELP_POLL_GC]
jmp SHORT G_M63189_IG04
;; bbWeight=0 PerfScore 0.00
; Total bytes of code 59, prolog size 12, PerfScore 26.65, instruction count 21 (MethodHash=87cd092a) for method ILStubClass:IL_STUB_PInvoke(int):int
; ============================================================
Unwind Info:
>> Start offset : 0x000000 (not in unwind data)
>> End offset : 0xd1ffab1e (not in unwind data)
Version : 1
Flags : 0x00
SizeOfProlog : 0x03
CountOfUnwindCodes: 3
FrameRegister : none (0)
FrameOffset : N/A (no FrameRegister) (Value=0)
UnwindCodes :
CodeOffset: 0x03 UnwindOp: UWOP_ALLOC_SMALL (2) OpInfo: 0 * 8 + 8 = 8 = 0x08
CodeOffset: 0x02 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: rbx (3)
CodeOffset: 0x01 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: rbp (5)
; Assembly listing for method ILStubClass:IL_STUB_PInvoke(int):int
; Emitting BLENDED_CODE for X64 CPU with SSE2 - Unix
; ReadyToRun compilation
; optimized code
; rbp based frame
; partially interruptible
; Final local variable assignments
;
; V00 arg0 [V00,T00] ( 3, 3 ) int -> rdi ;* V01 loc0 [V01 ] ( 0, 0 ) int -> zero-ref ; V02 loc1 [V02,T02] ( 2, 2 ) long -> rdi ;* V03 loc2 [V03 ] ( 0, 0 ) int -> zero-ref ; V04 loc3 [V04,T03] ( 2, 2 ) int -> rbx ;# V05 OutArgs [V05 ] ( 1, 1 ) lclBlk ( 0) [rsp+0x00] "OutgoingArgSpace"
; V06 tmp1 [V06 ] ( 2, 2 ) long -> [rbp-0x10] do-not-enreg[X] addr-exposed "stub argument"
; V07 tmp2 [V07,T01] ( 2, 4 ) long -> rax "impImportIndirectCall"
;
; Lcl frame size = 8
G_M63189_IG01:
push rbp
push rbx
push rax
lea rbp, [rsp+10H]
mov qword ptr [rbp-10H], r10
;; bbWeight=1 PerfScore 4.50
G_M63189_IG02:
movsxd rdi, edi
mov rax, qword ptr [rbp-10H]
mov rax, qword ptr [rax+72]
mov rax, qword ptr [rax]
;; bbWeight=1 PerfScore 5.25
G_M63189_IG03:
call rax
mov ebx, eax
mov rax, qword ptr [(reloc)]
cmp dword ptr [rax], 0
jne SHORT G_M63189_IG06
;; bbWeight=1 PerfScore 8.25
G_M63189_IG04:
mov eax, ebx
;; bbWeight=1 PerfScore 0.25
G_M63189_IG05:
lea rsp, [rbp-08H]
pop rbx
pop rbp
ret ;; bbWeight=1 PerfScore 2.50
G_M63189_IG06:
call [CORINFO_HELP_POLL_GC]
jmp SHORT G_M63189_IG04
;; bbWeight=0 PerfScore 0.00
; Total bytes of code 59, prolog size 12, PerfScore 26.65, instruction count 21 (MethodHash=87cd092a) for method ILStubClass:IL_STUB_PInvoke(int):int
; ============================================================
Unwind Info:
>> Start offset : 0x000000 (not in unwind data)
>> End offset : 0xd1ffab1e (not in unwind data)
Version : 1
Flags : 0x00
SizeOfProlog : 0x03
CountOfUnwindCodes: 3
FrameRegister : none (0)
FrameOffset : N/A (no FrameRegister) (Value=0)
UnwindCodes :
CodeOffset: 0x03 UnwindOp: UWOP_ALLOC_SMALL (2) OpInfo: 0 * 8 + 8 = 8 = 0x08
CodeOffset: 0x02 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: rbx (3)
CodeOffset: 0x01 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: rbp (5)

Updated with this PR

; Assembly listing for method ILStubClass:IL_STUB_PInvoke(int):int
; Emitting BLENDED_CODE for X64 CPU with SSE2 - Unix
; ReadyToRun compilation
; optimized code
; rbp based frame
; partially interruptible
; Final local variable assignments
;
; V00 arg0 [V00,T00] ( 3, 3 ) int -> rdi ;* V01 loc0 [V01 ] ( 0, 0 ) int -> zero-ref ; V02 loc1 [V02,T03] ( 2, 2 ) long -> rdi ;* V03 loc2 [V03 ] ( 0, 0 ) int -> zero-ref ; V04 loc3 [V04,T04] ( 2, 2 ) int -> rax ;# V05 OutArgs [V05 ] ( 1, 1 ) lclBlk ( 0) [rsp+0x00] "OutgoingArgSpace"
; V06 tmp1 [V06 ] ( 2, 2 ) long -> [rbp-0x38] do-not-enreg[X] addr-exposed "stub argument"
; V07 tmp2 [V07,T01] ( 2, 4 ) long -> rbx "impImportIndirectCall"
; V08 tmp3 [V08,T02] ( 2, 4 ) int -> rax "Single return block return value"
; V09 PInvokeFrame [V09 ] ( 3, 3 ) blk (88) [rbp-0x90] do-not-enreg[X] addr-exposed "Pinvoke FrameVar"
; TEMP_01 long -> [rbp-0x30]
;
; Lcl frame size = 104
G_M63189_IG01:
push rbp
push r15
push r14
push r13
push r12
push rbx
sub rsp, 104
lea rbp, [rsp+90H]
mov qword ptr [rbp-38H], r10
;; bbWeight=1 PerfScore 7.75
G_M63189_IG02:
movsxd rdi, edi
mov rax, qword ptr [rbp-38H]
mov rax, qword ptr [rax+72]
mov rbx, qword ptr [rax]
mov qword ptr [rbp-30H], rdi
lea rdi, [rbp-90H]
;; bbWeight=1 PerfScore 6.75
G_M63189_IG03:
call [CORINFO_HELP_JIT_PINVOKE_BEGIN]
mov rdi, qword ptr [rbp-30H]
;; bbWeight=1 PerfScore 4.00
G_M63189_IG04:
call rbx
mov ebx, eax
lea rdi, [rbp-90H]
call [CORINFO_HELP_JIT_PINVOKE_END]
mov eax, ebx
;; bbWeight=1 PerfScore 7.00
G_M63189_IG05:
lea rsp, [rbp-28H]
pop rbx
pop r12
pop r13
pop r14
pop r15
pop rbp
ret ;; bbWeight=1 PerfScore 4.50
; Total bytes of code 95, prolog size 26, PerfScore 39.50, instruction count 30 (MethodHash=87cd092a) for method ILStubClass:IL_STUB_PInvoke(int):int
; ============================================================
Unwind Info:
>> Start offset : 0x000000 (not in unwind data)
>> End offset : 0xd1ffab1e (not in unwind data)
Version : 1
Flags : 0x00
SizeOfProlog : 0x0E
CountOfUnwindCodes: 7
FrameRegister : none (0)
FrameOffset : N/A (no FrameRegister) (Value=0)
UnwindCodes :
CodeOffset: 0x0E UnwindOp: UWOP_ALLOC_SMALL (2) OpInfo: 12 * 8 + 8 = 104 = 0x68
CodeOffset: 0x0A UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: rbx (3)
CodeOffset: 0x09 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: r12 (12)
CodeOffset: 0x07 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: r13 (13)
CodeOffset: 0x05 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: r14 (14)
CodeOffset: 0x03 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: r15 (15)
CodeOffset: 0x01 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: rbp (5)
; Assembly listing for method ILStubClass:IL_STUB_PInvoke(int):int
; Emitting BLENDED_CODE for X64 CPU with SSE2 - Unix
; ReadyToRun compilation
; optimized code
; rbp based frame
; partially interruptible
; Final local variable assignments
;
; V00 arg0 [V00,T00] ( 3, 3 ) int -> rdi ;* V01 loc0 [V01 ] ( 0, 0 ) int -> zero-ref ; V02 loc1 [V02,T03] ( 2, 2 ) long -> rdi ;* V03 loc2 [V03 ] ( 0, 0 ) int -> zero-ref ; V04 loc3 [V04,T04] ( 2, 2 ) int -> rax ;# V05 OutArgs [V05 ] ( 1, 1 ) lclBlk ( 0) [rsp+0x00] "OutgoingArgSpace"
; V06 tmp1 [V06 ] ( 2, 2 ) long -> [rbp-0x38] do-not-enreg[X] addr-exposed "stub argument"
; V07 tmp2 [V07,T01] ( 2, 4 ) long -> rbx "impImportIndirectCall"
; V08 tmp3 [V08,T02] ( 2, 4 ) int -> rax "Single return block return value"
; V09 PInvokeFrame [V09 ] ( 3, 3 ) blk (88) [rbp-0x90] do-not-enreg[X] addr-exposed "Pinvoke FrameVar"
; TEMP_01 long -> [rbp-0x30]
;
; Lcl frame size = 104
G_M63189_IG01:
push rbp
push r15
push r14
push r13
push r12
push rbx
sub rsp, 104
lea rbp, [rsp+90H]
mov qword ptr [rbp-38H], r10
;; bbWeight=1 PerfScore 7.75
G_M63189_IG02:
movsxd rdi, edi
mov rax, qword ptr [rbp-38H]
mov rax, qword ptr [rax+72]
mov rbx, qword ptr [rax]
mov qword ptr [rbp-30H], rdi
lea rdi, [rbp-90H]
;; bbWeight=1 PerfScore 6.75
G_M63189_IG03:
call [CORINFO_HELP_JIT_PINVOKE_BEGIN]
mov rdi, qword ptr [rbp-30H]
;; bbWeight=1 PerfScore 4.00
G_M63189_IG04:
call rbx
mov ebx, eax
lea rdi, [rbp-90H]
call [CORINFO_HELP_JIT_PINVOKE_END]
mov eax, ebx
;; bbWeight=1 PerfScore 7.00
G_M63189_IG05:
lea rsp, [rbp-28H]
pop rbx
pop r12
pop r13
pop r14
pop r15
pop rbp
ret ;; bbWeight=1 PerfScore 4.50
; Total bytes of code 95, prolog size 26, PerfScore 39.50, instruction count 30 (MethodHash=87cd092a) for method ILStubClass:IL_STUB_PInvoke(int):int
; ============================================================
Unwind Info:
>> Start offset : 0x000000 (not in unwind data)
>> End offset : 0xd1ffab1e (not in unwind data)
Version : 1
Flags : 0x00
SizeOfProlog : 0x0E
CountOfUnwindCodes: 7
FrameRegister : none (0)
FrameOffset : N/A (no FrameRegister) (Value=0)
UnwindCodes :
CodeOffset: 0x0E UnwindOp: UWOP_ALLOC_SMALL (2) OpInfo: 12 * 8 + 8 = 104 = 0x68
CodeOffset: 0x0A UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: rbx (3)
CodeOffset: 0x09 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: r12 (12)
CodeOffset: 0x07 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: r13 (13)
CodeOffset: 0x05 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: r14 (14)
CodeOffset: 0x03 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: r15 (15)
CodeOffset: 0x01 UnwindOp: UWOP_PUSH_NONVOL (0) OpInfo: rbp (5)

AndyAyersMS · 2020-12-17T01:38:35Z

I'm not sure what the codegen diffs above are supposed to show.

jkoritzinsky · 2020-12-17T01:43:52Z

The diffs above are the diffs from the IL stubs between 35fbeaf and this PR. I was posting them as they're the only diffs and I believe that they're benign and not the cause of any of the failures we were seeing.

… a register to the stack.

AndyAyersMS · 2020-12-17T02:04:03Z

Would be nice to see a method where there are diffs, but the change looks reasonable.

ghost · 2020-12-17T02:33:58Z

Hello @jkoritzinsky!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

sandreenko · 2020-12-18T00:23:33Z

I've validated that this PR has no diffs outside of P/Invoke stubs from the JIT in 35fbaef (the commit before #45625).

I am confused here, do you mean that this PR + your merged change #45625 has no diffs against 35fbaef or just this change on top of 35fbaef has no diffs?

sandreenko · 2020-12-18T00:25:28Z

 if (!lclNode->AsLclVar()->IsLastUse(i))
 {
- GetEmitter()->emitIns_S_R(ins_Store(srcType), emitTypeSize(srcType), reg, fieldLclNum, 0);
+ GetEmitter()->emitIns_S_R(ins_Store(srcType), emitTypeSize(destType), reg, fieldLclNum, 0);


is not it strange that ins_Store and emitTypeSize have different arguments here?
Also, ins_Store is usually taking destType, not source, it does not matter on x64 but I expect arm.

Yeah, that’s how I got confused and did the wrong thing earlier.
in this case, ins_Store needs to take srcType because we might have a float variable in an int register. For example we hit this case on x86 when calling an unmanaged method that returns a struct of floats and we want to store the floats on the stack, which is what prompted my fix in the first place.

jkoritzinsky · 2020-12-18T01:12:35Z

I mean my PR + this change has no diffs barring the one I posted (which looks to be related to SuppressGCTransition)

sandreenko · 2020-12-18T16:58:48Z

Ok, so on which platforms did you see the asm changes? How can I repro them?
Can srcType and dstType have different sizes? What happens in this case with such code ins_Store(srcType), emitTypeSize(destType) on arm64?

jkoritzinsky · 2020-12-18T17:23:39Z

I saw the failures (that this pr fixed) on Linux x64, arm, and ARM64 IIRC.

jkoritzinsky added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 17, 2020

jkoritzinsky requested a review from jkotas December 17, 2020 01:07

Use destType for size instead of srcType when writing a variable from…
380d7a2
… a register to the stack.

jkoritzinsky force-pushed the reg-to-stack-move-fix branch from f4f1a3b to 380d7a2Compare December 17, 2020 01:44

AndyAyersMS approved these changes Dec 17, 2020
View reviewed changes

jkoritzinsky added the auto-merge label Dec 17, 2020

jkotas merged commit be27e29 into dotnet:masterDec 17, 2020

jkotas mentioned this pull request Dec 17, 2020
Revert "Enable non-blittable struct returns on UnmanagedCallersOnly (#45625)" #46172
Closed

jkoritzinsky deleted the reg-to-stack-move-fix branch December 17, 2020 05:27

akoeplinger mentioned this pull request Dec 17, 2020
[master] Update dependencies from dotnet/runtime dotnet/sdk#14574
Merged

kunalspathak mentioned this pull request Dec 17, 2020
Fix crash in SuperPMI getUnmanagedCallConv #46202
Merged

sandreenko reviewed Dec 18, 2020
View reviewed changes

sandreenko mentioned this pull request Dec 19, 2020
wrong multireg copies from reg to small type field in memory #46240
Closed

ghost locked as resolved and limited conversation to collaborators Jan 17, 2021

Navigation Menu

Uh oh!

Use destType for size instead of srcType when writing a variable from a register to the stack. - #46176

Use destType for size instead of srcType when writing a variable from a register to the stack.#46176
jkotas merged 1 commit into
dotnet:masterfrom
jkoritzinsky:reg-to-stack-move-fix

jkoritzinsky commented Dec 17, 2020•
edited
Loading

Uh oh!

jkoritzinsky commented Dec 17, 2020

Uh oh!

jkoritzinsky commented Dec 17, 2020•
edited by AndyAyersMS
Loading

Uh oh!

AndyAyersMS commented Dec 17, 2020

Uh oh!

jkoritzinsky commented Dec 17, 2020

Uh oh!

AndyAyersMS commented Dec 17, 2020

Uh oh!

ghost commented Dec 17, 2020

Uh oh!

sandreenko commented Dec 18, 2020

Uh oh!

sandreenkoDec 18, 2020

Uh oh!

jkoritzinskyDec 18, 2020

Uh oh!

jkoritzinsky commented Dec 18, 2020

Uh oh!

sandreenko commented Dec 18, 2020

Uh oh!

jkoritzinsky commented Dec 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

jkoritzinsky commented Dec 17, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkoritzinsky commented Dec 17, 2020

Uh oh!

jkoritzinsky commented Dec 17, 2020• edited by AndyAyersMS Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndyAyersMS commented Dec 17, 2020

Uh oh!

jkoritzinsky commented Dec 17, 2020

Uh oh!

AndyAyersMS commented Dec 17, 2020

Uh oh!

ghost commented Dec 17, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

sandreenko commented Dec 18, 2020

Uh oh!

sandreenkoDec 18, 2020

Choose a reason for hiding this comment

Uh oh!

jkoritzinskyDec 18, 2020

Choose a reason for hiding this comment

Uh oh!

jkoritzinsky commented Dec 18, 2020

Uh oh!

sandreenko commented Dec 18, 2020

Uh oh!

jkoritzinsky commented Dec 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jkoritzinsky commented Dec 17, 2020•
edited
Loading

jkoritzinsky commented Dec 17, 2020•
edited by AndyAyersMS
Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.