[RyuJIT/ARM32] Adds code to handle SecureDelegateInvoke #13166

YongseopKim · 2017-08-02T12:17:16Z

Adds code to handle SecureDelegateInvoke on morph phase like legacy's handling.
Set R4 to proper addr.
This is like legacy's doing from Enable FEATURE_STUBS_AS_IL for ARM/Linux #6500.
Fix #12993

YongseopKim · 2017-08-02T12:29:44Z

Results

JITting DomainBoundILStubClass:IL_STUB_SecureDelegate_Invoke(ref):int:this

morph

before

fgMorphTree BB01, stmt 1 (before)
               [000006] --CXG--------             ▌  RETURN    int
               [000004] --CXG--------             └──▌  CALL      int    Delegate_TC_Int.Invoke
               [000002] ---XG-------- this in r0     ├──▌  FIELD     ref    _invocationList
               [000001] -------------                │  └──▌  LCL_VAR   ref    V00 this
               [000003] ------------- arg1           └──▌  LCL_VAR   ref    V01 arg1

fgMorphTree BB01, stmt 1 (after)
               [000006] --CXG+-------             ▌  RETURN    int
               [000004] --CXG+-------             └──▌  CALL      int    Delegate_TC_Int.Invoke
               [000002] ---XG+------- this in r0     ├──▌  IND       ref
               [000008] -----+-------                │  │  ┌──▌  CNS_INT   int    20 field offset Fseq[_invocationList]
               [000009] -----+-------                │  └──▌  ADD       byref
               [000001] -----+-------                │     └──▌  LCL_VAR   ref    V00 this
               [000003] -----+------- arg1 in r1     └──▌  LCL_VAR   ref    V01 arg1

after

fgMorphTree BB01, stmt 1 (before)
               [000006] --CXG--------             ▌  RETURN    int
               [000004] --CXG--------             └──▌  CALL      int    Delegate_TC_Int.Invoke
               [000002] ---XG-------- this in r0     ├──▌  FIELD     ref    _invocationList
               [000001] -------------                │  └──▌  LCL_VAR   ref    V00 this
               [000003] ------------- arg1           └──▌  LCL_VAR   ref    V01 arg1

fgMorphTree BB01, stmt 1 (after)
               [000006] --CXG+-------             ▌  RETURN    int
               [000004] --CXG+-------             └──▌  CALL      int    Delegate_TC_Int.Invoke
     (  7,  4) [000010] ---XG-------- arg2 in r4     ├──▌  LEA(b+16) ref
     (  6,  3) [000009] ---XG--------                │  └──▌  IND       ref
     (  1,  1) [000014] -------------                │     │  ┌──▌  CNS_INT   int    20 field offset Fseq[_invocationList]
     (  5,  4) [000015] -------N-----                │     └──▌  ADD       byref
     (  3,  2) [000008] -------------                │        └──▌  LCL_VAR   ref    V00 this
     (  6,  3) [000002] ---XG-------- this in r0     ├──▌  IND       ref
     (  1,  1) [000012] -------------                │  │  ┌──▌  CNS_INT   int    20 field offset Fseq[_invocationList]
     (  5,  4) [000013] -------N-----                │  └──▌  ADD       byref
     (  3,  2) [000001] -------------                │     └──▌  LCL_VAR   ref    V00 this
               [000003] -----+------- arg1 in r1     └──▌  LCL_VAR   ref    V01 arg1

jitted code

before

IN000a: 000000      push    {r4,r10,r11,lr}
IN000b: 000004      sub     sp, 16
IN000c: 000006      add     r11, sp, 24
IN000d: 00000A      movs    r2, 0
IN000e: 00000C      str     r2, [sp+0x04]	// [V03 tmp1]
IN000f: 00000E      str     r0, [sp+0x0c]	// [V00 this]
IN0010: 000010      str     r1, [sp+0x08]	// [V01 arg1]

IN0001: 000012      ldr     r0, [sp+0x0c]
IN0002: 000014      ldr     r0, [r0+20]
IN0003: 000016      str     r0, [sp+0x04]	// [V03 tmp1]
IN0004: 000018      ldr     r0, [sp+0x04]	// [V03 tmp1]
IN0005: 00001A      ldr     r0, [r0+4]
IN0006: 00001C      ldr     r1, [sp+0x08]	// [V01 arg1]
IN0007: 00001E      ldr     r3, [sp+0x04]	// [V03 tmp1]
IN0008: 000020      ldr     r3, [r3+12]
IN0009: 000022      blx     r3		// Delegate_TC_Int:Invoke(ref):int:this

IN0011: 000024      add     sp, 16
IN0012: 000026      pop     {r4,r10,r11,pc}

after

IN000d: 000000      push    {r4,r10,r11,lr}
IN000e: 000004      sub     sp, 16
IN000f: 000006      add     r11, sp, 24
IN0010: 00000A      movs    r2, 0
IN0011: 00000C      str     r2, [sp+0x04]	// [V03 tmp1]
IN0012: 00000E      str     r0, [sp+0x0c]	// [V00 this]
IN0013: 000010      str     r1, [sp+0x08]	// [V01 arg1]

IN0001: 000012      ldr     r0, [sp+0x0c]
IN0002: 000014      ldr     r0, [r0+20]
IN0003: 000016      add     r4, r0, 16
IN0004: 00001A      ldr     r0, [sp+0x0c]
IN0005: 00001C      ldr     r0, [r0+20]
IN0006: 00001E      str     r0, [sp+0x04]	// [V03 tmp1]
IN0007: 000020      ldr     r0, [sp+0x04]	// [V03 tmp1]
IN0008: 000022      ldr     r0, [r0+4]
IN0009: 000024      ldr     r1, [sp+0x08]	// [V01 arg1]
IN000a: 000026      ldr     r3, [sp+0x04]	// [V03 tmp1]
IN000b: 000028      ldr     r3, [r3+12]
IN000c: 00002A      blx     r3		// Delegate_TC_Int:Invoke(ref):int:this

IN0014: 00002C      add     sp, 16
IN0015: 00002E      pop     {r4,r10,r11,pc}

YongseopKim · 2017-08-02T12:33:40Z

PTAL @dotnet/jit-contrib.

I'm not sure that using gtClone & GenTreeAddrMode is general.

When I just used call->gtCallObjp for GenTreeAddrMode, I faced a kind of assert This is already morphed because the gtCallObjp was already morphed. That's why I use gtClone.
And I can't find other proper GenTree except for GenTreeAddrMode.

cc/ @dotnet/arm32-contrib

BruceForstall · 2017-08-02T20:02:01Z

It's odd that GTF_CALL_M_SECURE_DELEGATE_INV is only implemented by ARM32. I wonder why that is?

Instead of using gtClone unconditionally, perhaps you should do what the VirtualStub code just below does, and insert a comma, only using gtClone on simple lclvar:

            GenTree* arg = call->gtCallAddr;
            if (arg->OperIsLocal())
            {
                arg = gtClone(arg, true);
            }
            else
            {
                call->gtCallAddr = fgInsertCommaFormTemp(&arg);
                call->gtFlags |= GTF_ASG;
            }
            noway_assert(arg != nullptr);

(in your case, call->gtCallObjp instead of gtCallAddr)

Also, is the arg list guaranteed to have at least one arg? Or, should you follow the pattern of the indirect call cookie case below, e.g.:

            GenTreeArgList** insertionPoint = &call->gtCallArgs;
            for (; *insertionPoint != nullptr; insertionPoint = &(*insertionPoint)->Rest())
            {
            }
            *insertionPoint = gtNewListNode(arg, nullptr);

(maybe there should be a gtAppendArg(call, arg) to do this)

BruceForstall · 2017-08-02T20:02:10Z

cc @dotnet/jit-contrib

YongseopKim · 2017-08-03T01:07:44Z

@BruceForstall

It's odd that GTF_CALL_M_SECURE_DELEGATE_INV is only implemented by ARM32. I wonder why that is?

That is a custom calling convention for virtual stub dispatch only on arm32. So the secure delegate invoke needs to set R4 as an indirection cell too.

You can see this comment from https://github.com/dotnet/coreclr/issues/3804#issuecomment-198326548

The problem is that virtual stub dispatch calls have a custom calling convention - they use R4 as scratch register.

Thank you for your comment. I'll update and PR again.

YongseopKim · 2017-08-03T03:56:01Z

PTAL again, @BruceForstall.

After new patch,

fgMorphTree BB01, stmt 1 (after)
               [000006] -ACXG+-------             ▌  RETURN    int
               [000004] -ACXG+-------             └──▌  CALL      int    Delegate_TC_Int.Invoke
               [000010] -----+-------                │     ┌──▌  LCL_VAR   ref    V02 tmp0
               [000011] -A-XG+-------                │  ┌──▌  COMMA     ref
               [000002] ---XG+-------                │  │  │  ┌──▌  IND       ref
               [000015] -----+-------                │  │  │  │  │  ┌──▌  CNS_INT   int    20 field offset Fseq[_invocationList]
               [000016] -----+-------                │  │  │  │  └──▌  ADD       byref
               [000001] -----+-------                │  │  │  │     └──▌  LCL_VAR   ref    V00 this
               [000009] -A-XG+-------                │  │  └──▌  ASG       ref
               [000008] D----+-N-----                │  │     └──▌  LCL_VAR   ref    V02 tmp0
               [000018] -A-XG------L- this SETUP     ├──▌  ASG       ref
               [000017] D------N-----                │  └──▌  LCL_VAR   ref    V03 tmp1
               [000019] ------------- this in r0     ├──▌  LCL_VAR   ref    V03 tmp1
               [000013] -----+------- arg2 in r4     ├──▌  LEA(b+16) ref
               [000012] -----+-------                │  └──▌  LCL_VAR   ref    V02 tmp0
               [000003] -----+------- arg1 in r1     └──▌  LCL_VAR   ref    V01 arg1

IN0010: 000000      push    {r4,r10,r11,lr}
IN0011: 000004      sub     sp, 24
IN0012: 000006      add     r11, sp, 32
IN0013: 00000A      movs    r2, 0
IN0014: 00000C      str     r2, [sp+0x0c]   // [V02 tmp0]
IN0015: 00000E      str     r2, [sp+0x08]   // [V03 tmp1]
IN0016: 000010      str     r2, [sp+0x04]   // [V05 tmp3]
IN0017: 000012      str     r0, [sp+0x14]   // [V00 this]
IN0018: 000014      str     r1, [sp+0x10]   // [V01 arg1]


IN0001: 000016      ldr     r0, [sp+0x14]
IN0002: 000018      ldr     r0, [r0+20]
IN0003: 00001A      str     r0, [sp+0x0c]   // [V02 tmp0]
IN0004: 00001C      ldr     r0, [sp+0x0c]   // [V02 tmp0]
IN0005: 00001E      str     r0, [sp+0x08]   // [V03 tmp1]
IN0006: 000020      ldr     r0, [sp+0x08]   // [V03 tmp1]
IN0007: 000022      str     r0, [sp+0x04]   // [V05 tmp3]
IN0008: 000024      ldr     r0, [sp+0x04]   // [V05 tmp3]
IN0009: 000026      ldr     r0, [r0+4]
IN000a: 000028      ldr     r1, [sp+0x0c]   // [V02 tmp0]
IN000b: 00002A      add     r4, r1, 16
IN000c: 00002E      ldr     r1, [sp+0x10]   // [V01 arg1]
IN000d: 000030      ldr     r3, [sp+0x04]   // [V05 tmp3]
IN000e: 000032      ldr     r3, [r3+12]
IN000f: 000034      blx     r3      // Delegate_TC_Int:Invoke(ref):int:this

IN0019: 000036      add     sp, 24
IN001a: 000038      pop     {r4,r10,r11,pc}

BruceForstall · 2017-08-03T05:39:50Z

That looks good to me. What does before/after codegen look like for optimized code?

- Adds code to handle SecureDelegateInvoke on morph phase like legacy's handling. - Set R4 to proper addr for indirect call of vsd - This is like legacy's doing from #6500. - Fix #12993

YongseopKim · 2017-08-03T06:28:38Z

@dotnet-bot test Ubuntu x64 Formatting please

YongseopKim · 2017-08-03T06:29:05Z

@dotnet-bot test Windows_NT x64 Formatting please

YongseopKim · 2017-08-03T06:36:33Z

@BruceForstall, I tested d(debug)/do(debug optimized)/r(release)/ro.

d

before & after's morph tree & asm are on above comment.

do

before & after's morph tree is same to d's one

before's asm

IN0005: 000000      push    {r3,lr}

IN0001: 000002      ldr     r3, [r0+20]
IN0002: 000004      ldr     r0, [r3+4]
IN0003: 000006      ldr     r3, [r3+12]
IN0004: 000008      blx     r3      // Delegate_TC_Int:Invoke(ref):int:this

IN0006: 00000A      pop     {r3,pc}

after's asm

IN0007: 000000      push    {r4,lr}

IN0001: 000002      ldr     r3, [r0+20]
IN0002: 000004      mov     r2, r3
IN0003: 000006      ldr     r0, [r2+4]
IN0004: 000008      add     r4, r3, 16
IN0005: 00000C      ldr     r3, [r2+12]
IN0006: 00000E      blx     r3      // Delegate_TC_Int:Invoke(ref):int:this

IN0008: 000010      pop     {r4,pc}

r

before & after's morph tree & asm are the same to d's one

ro

before & after's morph tree & asm are the same to do's one

YongseopKim · 2017-08-03T06:44:47Z

Plus, I've tried finding such as gtAppendArg(call, arg), but I can't. If somebody tells me about the name of func, I'll apply it. :)

YongseopKim · 2017-08-03T09:15:46Z

On my rasp3, tests pass.

Preparing CoreMangLib/system/delegate/VSD/OpenDelegate/OpenDelegate.sh
The tests have been prepared
Starting CoreMangLib/system/delegate/VSD/OpenDelegate/OpenDelegate.sh
Preparing JIT/Regression/CLR-x86-JIT/V1-M12-Beta2/b49809/b49809/b49809.sh
The tests have been prepared
Starting JIT/Regression/CLR-x86-JIT/V1-M12-Beta2/b49809/b49809/b49809.sh
PASSED   - [   0][   2s]CoreMangLib/system/delegate/VSD/OpenDelegate/OpenDelegate.sh
               BEGIN EXECUTION
               /home/dragon/dotnet/arm32-dotnet/tmp/dragon-coreoverlay-arm-vsd-release/overlay/corerun OpenDelegate.exe
               Expected: 100
               Actual: 100
               END EXECUTION - PASSED
PASSED   - [   1][   3s]JIT/Regression/CLR-x86-JIT/V1-M12-Beta2/b49809/b49809/b49809.sh
               BEGIN EXECUTION
               /home/dragon/dotnet/arm32-dotnet/tmp/dragon-coreoverlay-arm-vsd-release/overlay/corerun b49809.exe
               Expected: 100
               Actual: 100
               END EXECUTION - PASSED

=======================
     Test Results
=======================
# CoreCLR Bin Dir  : 
# Tests Discovered : 2
# Passed           : 2
# Failed           : 0
# Skipped          : 0
=======================
0 minutes and 17 seconds taken to run CoreCLR tests.

YongseopKim · 2017-08-03T09:16:03Z

@dotnet-bot test Ubuntu arm Cross Release Build please

BruceForstall · 2017-08-03T15:05:47Z

Plus, I've tried finding such as gtAppendArg(call, arg), but I can't. If somebody tells me about the name of func, I'll apply it. :)

We should add it sometime...

dnfclas added the cla-already-signed label Aug 2, 2017

BruceForstall self-requested a review August 2, 2017 16:19

[RyuJIT/ARM32] Adds code to handle SecureDelegateInvoke

36a1deb

- Adds code to handle SecureDelegateInvoke on morph phase like legacy's handling. - Set R4 to proper addr for indirect call of vsd - This is like legacy's doing from #6500. - Fix #12993

BruceForstall approved these changes Aug 3, 2017

View reviewed changes

BruceForstall merged commit e6d00a3 into dotnet:master Aug 3, 2017

YongseopKim deleted the ryujit/arm32/add_handling_secure_delegate_inv branch August 4, 2017 00:04

karelz modified the milestone: 2.1.0 Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RyuJIT/ARM32] Adds code to handle SecureDelegateInvoke #13166

[RyuJIT/ARM32] Adds code to handle SecureDelegateInvoke #13166

YongseopKim commented Aug 2, 2017

YongseopKim commented Aug 2, 2017

YongseopKim commented Aug 2, 2017 •

edited

Loading

BruceForstall commented Aug 2, 2017

BruceForstall commented Aug 2, 2017

YongseopKim commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

BruceForstall commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

BruceForstall commented Aug 3, 2017

[RyuJIT/ARM32] Adds code to handle SecureDelegateInvoke #13166

[RyuJIT/ARM32] Adds code to handle SecureDelegateInvoke #13166

Conversation

YongseopKim commented Aug 2, 2017

YongseopKim commented Aug 2, 2017

Results

morph

jitted code

YongseopKim commented Aug 2, 2017 • edited Loading

BruceForstall commented Aug 2, 2017

BruceForstall commented Aug 2, 2017

YongseopKim commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

BruceForstall commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

YongseopKim commented Aug 3, 2017

BruceForstall commented Aug 3, 2017

YongseopKim commented Aug 2, 2017 •

edited

Loading