Enable FEATURE_STUBS_AS_IL for ARM/Linux #6500

parjong · 2016-07-28T02:38:22Z

This commit re-enables FEATURE_STUBS_AS_IL for ARM/Linux in order to fix #6452.

Currently, CoreCLR in ARM/Linux uses the stub function emitted by ThumbEmitCallWithGenericInstantiationParameter in vm/arm/stubs.cpp for some generic instantiation.

As those stubs emitted by ThumbEmitCallWithGenericInstantiationParameter do not have entries in .ARM.exidx sections, libunwind falls backs on unstable frame parsing-based unwind method.

As the stack frame of those stubs is not in the required form, unwind provides incorrect values, which results in the issue in #6452.

Furthermore, libunwind currently guarantees the correctness of PC and R11 (FP) when using frame parsing-based unwind method.

To workaround this issue, this commit attempts to re-enable FEATURE_STUBS_AS_IL which is turned off in #3804.

Please do NOT merge this as this PR may cause several side-effects (Its side-effect is currently under evaluation).

parjong · 2016-07-28T02:42:41Z

\CC @manu-silicon @jkotas @janvorli @benpye @leemgs @lemmaa @hseok-oh

@jotas @manu-silicon The discussion in #3804 says that FEATURE_STUBS_AS_IL has some side-effect. Could you let me know what it is?

jkotas · 2016-07-28T03:00:59Z

src/vm/comdelegate.cpp

@@ -2593,15 +2593,6 @@ BOOL COMDelegate::NeedsWrapperDelegate(MethodDesc* pTargetMD)
 {
    LIMITED_METHOD_CONTRACT;

-#ifdef _TARGET_ARM_


I do not think it is ok to just return false for ARM from here. You should see crashes in delegate tests with this change. This change would need matching changes in arm\virtualcallstubcpu.hpp and JIT.

@jkotas Without this, I encountered UNREACHABLE call as COMDelegate::GetSecureInvoke is currently not implemented.

Could you let me know which changes are required for arm\virtualcallstubcpu.hpp and JIT, or how to implement COMDelegate::GetSecureInvoke?

I believe that implementing COMDelegate::GetSecureInvoke is easier. It should be implemented similar to how COMDelegate::GetMulticastInvoke is implemented, except that there will be no loop. It should just re-push the arguments and call the actual target. The call to actual target generated by the JIT has to pass the indirection cell in R4. There will need be a new flag passed to the JIT to make it happen.

Could you remove my email address from DW? I think this discussion does not
concern me.
Cheers
Julian

28 lip 2016 4:06 PM "Jan Kotas" [email protected] napisał(a):

In src/vm/comdelegate.cpp
#6500 (comment):

@@ -2593,15 +2593,6 @@ BOOL COMDelegate::NeedsWrapperDelegate(MethodDesc* pTargetMD)
{
LIMITED_METHOD_CONTRACT;

-#ifdef TARGET_ARM

I believe that implementing COMDelegate::GetSecureInvoke is easier. It
should be implemented similar to how COMDelegate::GetMulticastInvoke is
implemented, except that there will be no loop. It should just re-push the
arguments and call the actual target. The call to actual target generated
by the JIT has to pass the indirection cell in R4. There will need be a new
flag passed to the JIT to make it happen.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/dotnet/coreclr/pull/6500/files/c7d62d06126218318efee2d7bf5ed0158ffb58d5#r72628027,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAA1nOgyJ-Ik3RQUtmQvlY0BKE3L3gPYks5qaLeBgaJpZM4JW0HY
.

jkotas · 2016-07-28T03:01:55Z

Could you let me know what it is?

It is the problem around NeedsWrapperDelegate that you have discovered as well.

parjong · 2016-07-28T04:32:26Z

@jkotas I tried to build the following example in #3804:

using System;

interface IFoo
{
  void Bar(int j);
}

class Foo : IFoo
{
  public void Bar(int j)
  {
  }

  static void Main(string[] args)
  {
    var bar = typeof(IFoo).GetMethod("Bar");
    var x = (Action<IFoo, int>)bar.CreateDelegate(typeof(Action<IFoo, int>), null);
    var ifoo = new Foo();
    for (;;)
      x(ifoo, 2);
  }
}

Unfortunately, I got the following error message:

/home/parjong/projects/toolchains/coreclr-analyze/vsd/Program.cs(16,28): error CS1061: 'Type' does not contain a definition for 'GetMethod' and no extension method 'GetMethod' accepting a first argument of type 'Type' could be found (are you missing a using directive or an assembly reference?)

Is there any further change in .NET Core from then?

jkotas · 2016-07-28T13:58:06Z

the following error message

There needs to be an extra using System.Reflection; at the top to compile this on current .NET Core.

parjong · 2016-07-29T06:34:02Z

@jkotas Thanks! It works. I'll check the issue with that example.

parjong · 2016-08-08T08:46:07Z

@jkotas Recent PR allows me to execute the failing CoreFX unittests, but it still shows segfault on the above example.

As I understood, the secure delegate invoke needs to set R4 as an indirection cell.

I managed to generate the following jitted code via manipulating src/vm/jitinterface.cpp, but it still crashes:

; Assembly listing for method DomainNeutralILStubClass:IL_STUB_SecureDelegate_Invoke(ref,int):this
; Emitting BLENDED_CODE for generic ARM CPU
; debuggable code
; r11 based frame
; fully interruptible
; Final local variable assignments
;
;  V00 this         [V00,T00] (  3,   3  )     ref  ->  [sp+0x08]   this
;  V01 arg1         [V01,T01] (  3,   3  )     ref  ->  [sp+0x04]
;  V02 arg2         [V02,T02] (  3,   3  )     int  ->  [sp+0x0C]
;# V03 OutArgs      [V03    ] (  1,   1  )  lclBlk ( 0) [sp+0x00]
;
; Lcl frame size = 16

G_M36525_IG01:
000000  E92D 4C10      push    {r4,r10,r11,lr}
000004  B084           sub     sp, 16
000006  F10D 0B18      add     r11, sp, 24
00000A  9002           str     r0, [sp+0x08]    // [V00 this]
00000C  9101           str     r1, [sp+0x04]    // [V01 arg1]
00000E  9203           str     r2, [sp+0x0c]    // [V02 arg2]

G_M36525_IG02:
000010  9802           ldr     r0, [sp+0x08]    // [V00 this]
000012  9901           ldr     r1, [sp+0x04]    // [V01 arg1]
000014  9A03           ldr     r2, [sp+0x0c]    // [V02 arg2]
000016  F24F 0480      movw    r4, 0xf080
00001A  F2CB 54A3      movt    r4, 0xb5a3
00001E  F8D4 C000      ldr     r12, [r4]
000022  47E0           blx     r12

G_M36525_IG03:
000024  B004           add     sp, 16
000026  E8BD 4C10      pop     {r4,r10,r11,lr}
00002A  4770           bx      lr

Could you let me know what is the next step to enable secure delegate in ARM?

jkotas · 2016-08-08T14:34:20Z

The code needs to be similar to code generated for the manually generated secure delegate:

coreclr/src/vm/arm/stubs.cpp

Line 2942 in deb00ad

    
           // add r4, r0+#offsetof(DelegateObject, _methodPtrAux) ; // r4 now contains indirection cell

The first argument in r0 should point to the real target:

pCode->EmitLDFLD(pCode->GetToken(MscorlibBinder::GetField(FIELD__MULTICAST_DELEGATE__INVOCATION_LIST)));
pCode->EmitLDFLD(pCode->GetToken(MscorlibBinder::GetField(FIELD__MULTICAST_DELEGATE__TARGET)));

r4 has to be initialized to point inside the delegate object. It may require an extra flag to be passed to the JIT via JIT-EE interface (src/inc/corinfo.h, src/inc/corjit.h).

jkotas · 2016-08-08T14:38:30Z

src/vm/comdelegate.cpp

+
+    ILCodeStream *pCode = sl.NewCodeStream(ILStubLinker::kDispatch);
+
+    DWORD dwReturnValNum = -1;


This special handling of return values should not be needed here. If you delete everything that has to do with dwReturnValNum and fReturnVal, everything should still work fine.

parjong · 2016-08-08T23:10:30Z

@jkotas Thanks!! I'll try. It seems that the code in EmitSecureDelegateInvoke allocates GSCookie in the stack frame. Is it unnecessary in the jitted code?

jkotas · 2016-08-08T23:14:29Z

Right, you do not have to worry about GS cookie for the IL stub.

parjong · 2016-08-09T22:44:56Z

@dotnet-bot test Ubuntu x64 Checked Build and Test please

parjong · 2016-08-09T22:53:01Z

@dotnet-bot test Windows_NT x64 Debug Build and Test please

parjong · 2016-08-09T23:04:24Z

@dotnet-bot test Linux ARM Emulator Cross Debug Build please

parjong · 2016-08-10T01:13:41Z

@dotnet-bot test Windows_NT x64 Release Priority 1 Build and Test please

parjong · 2016-08-10T04:01:42Z

@dotnet-bot test Linux ARM Emulator Cross Debug Build please

parjong · 2016-08-10T07:20:54Z

@jkotas It seems that the failure in Linux ARM Emulator Cross Debug Build is not related with this PR. Could you take a look? When I tested on ARM (softfp), this commit does not introduce any regression in CoreCLR unittests, but allows me to execute the CoreFX unittest of interest.

jkotas · 2016-08-10T14:25:43Z

The ARM emulator failure is likely caused by #6676

jkotas · 2016-08-10T14:27:18Z

src/jit/importer.cpp

@@ -6473,6 +6473,11 @@ var_types  Compiler::impImportCall(OPCODE                  opcode,
        /* Set the delegate flag */
        call->gtCall.gtCallMoreFlags |= GTF_CALL_M_DELEGATE_INV;

+        if (callInfo->secureInvoke)
+        {
+          call->gtCall.gtCallMoreFlags |= GTF_CALL_M_SECURE_DELEGATE_INV;


Nit 4 space indentation

Oops, I'll revise it.

jkotas · 2016-08-10T14:51:25Z

@dotnet/jit-contrib Could you please take a look at the JIT changes?

jkotas · 2016-08-10T14:59:29Z

@parjong Looks good to me modulo comments.

parjong · 2016-08-11T12:16:18Z

@dotnet-bot test OSX x64 Checked Build and Test please

parjong · 2016-08-11T12:16:39Z

@dotnet-bot test Ubuntu x64 Checked Build and Test please

parjong · 2016-08-11T12:18:49Z

@dotnet-bot test Windows_NT x64 Debug Build and Test please

parjong · 2016-08-11T12:19:32Z

@dotnet-bot test Ubuntu x64 Checked Build and Test please

parjong · 2016-08-11T12:19:45Z

@dotnet-bot test OSX x64 Checked Build and Test please

jkotas · 2016-08-11T16:38:07Z

Is this a per-call property or a per-method property?

I think the change is good to go as is. We can look into making this better as part of larger JIT-EE interface refactoring once/if there is one (e.g. packing all fields on CALL_INFO into single DWORD). I do not think there is a good way to make this better without it.

pgavlin · 2016-08-11T16:39:01Z

@jkotas fair enough. LGTM, then.

jkotas · 2016-08-12T04:31:08Z

This branch has conflicts that must be resolved

Could you please resolve the conflicts (and squash to a single commit while you are on it)?

This commit enables FEATURE_STUBS_AS_IL for ARM/Linux. This commit tries to fix #6452.

parjong · 2016-08-12T09:59:28Z

@jkotas I revised PR per feedback. Please take a look.

jkotas · 2016-08-12T15:02:19Z

👍

xiangzhai · 2019-11-06T02:20:59Z

:mips-interest

This commit enables FEATURE_STUBS_AS_IL for ARM/Linux. This commit tries to fix dotnet/coreclr#6452. Commit migrated from dotnet/coreclr@6abfacb

dnfclas added the cla-already-signed label Jul 28, 2016

jkotas reviewed Jul 28, 2016
View reviewed changes

jkotas reviewed Aug 8, 2016
View reviewed changes

parjong force-pushed the fix/issue_6452 branch from 5ef5dd8 to 04c5db6 Compare August 9, 2016 09:11

parjong changed the title ~~[WIP] Re-enable FEATURE_STUBS_AS_IL for ARM/Linux~~ Enable FEATURE_STUBS_AS_IL for ARM/Linux Aug 10, 2016

jkotas reviewed Aug 10, 2016
View reviewed changes

parjong force-pushed the fix/issue_6452 branch 2 times, most recently from 21a3d66 to 38c7928 Compare August 12, 2016 06:53

Enable FEATURE_STUBS_AS_IL for ARM/Linux

38c7928

This commit enables FEATURE_STUBS_AS_IL for ARM/Linux. This commit tries to fix #6452.

jkotas merged commit 6abfacb into dotnet:master Aug 12, 2016

parjong deleted the fix/issue_6452 branch August 16, 2016 23:01

YongseopKim mentioned this pull request Aug 2, 2017

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

Merged


		ILCodeStream *pCode = sl.NewCodeStream(ILStubLinker::kDispatch);

		DWORD dwReturnValNum = -1;

Enable FEATURE_STUBS_AS_IL for ARM/Linux #6500

Enable FEATURE_STUBS_AS_IL for ARM/Linux #6500

Conversation

parjong commented Jul 28, 2016

parjong commented Jul 28, 2016

jkotas Jul 28, 2016

Choose a reason for hiding this comment

parjong Jul 28, 2016 • edited Loading

Choose a reason for hiding this comment

jkotas Jul 28, 2016

Choose a reason for hiding this comment

jotas Jul 28, 2016

Choose a reason for hiding this comment

jkotas commented Jul 28, 2016

parjong commented Jul 28, 2016

jkotas commented Jul 28, 2016

parjong commented Jul 29, 2016

parjong commented Aug 8, 2016

jkotas commented Aug 8, 2016

jkotas Aug 8, 2016

Choose a reason for hiding this comment

parjong commented Aug 8, 2016

jkotas commented Aug 8, 2016

parjong commented Aug 9, 2016 • edited Loading

parjong commented Aug 9, 2016

parjong commented Aug 9, 2016

parjong commented Aug 10, 2016

parjong commented Aug 10, 2016

parjong commented Aug 10, 2016 • edited Loading

jkotas commented Aug 10, 2016

jkotas Aug 10, 2016

Choose a reason for hiding this comment

parjong Aug 10, 2016

Choose a reason for hiding this comment

jkotas commented Aug 10, 2016 • edited Loading

jkotas commented Aug 10, 2016

parjong commented Aug 11, 2016 • edited Loading

parjong commented Aug 11, 2016 • edited Loading

parjong commented Aug 11, 2016

parjong commented Aug 11, 2016

parjong commented Aug 11, 2016

jkotas commented Aug 11, 2016

pgavlin commented Aug 11, 2016

jkotas commented Aug 12, 2016

parjong commented Aug 12, 2016

jkotas commented Aug 12, 2016

xiangzhai commented Nov 6, 2019

parjong Jul 28, 2016 •

edited

Loading

parjong commented Aug 9, 2016 •

edited

Loading

parjong commented Aug 10, 2016 •

edited

Loading

jkotas commented Aug 10, 2016 •

edited

Loading

parjong commented Aug 11, 2016 •

edited

Loading

parjong commented Aug 11, 2016 •

edited

Loading