active exception regression fix #6117

chunseoklee · 2016-07-05T02:47:19Z

This patch fixes regression caused by 597e160 commit.
Since CONTEXT_EXCEPTION_ACTIVE case is not covered by CONTEXT_UNWOUND_TO_CALL
case, active exception flag is added.

jkotas · 2016-07-05T09:49:39Z

@chunseoklee What is the situation where both CONTEXT_EXCEPTION_ACTIVE and CONTEXT_EXCEPTION_ACTIVE is set that is causing the problem?

This fix makes the meaning of these flags different from Windows. It may be the best way to fix the problem that you are seeing, but there may be more issues hidden behind it. I am wondering whether we can fix the problem without diverging from Windows.

cc @janvorli

masonwheeler · 2016-07-08T00:43:01Z

@jkotas Come again? What is the situation where both X and X are set?

chunseoklee · 2016-07-08T01:01:59Z

fail case before this patch is coreclr/tests/src/baseservices/exceptions/generics/try-finally-struct01.cs in test.
I am in investigation to understand what is going on behind scene.

chunseoklee · 2016-07-08T03:59:22Z

One strange thing I found is that when we throw exception in csharp source as in coreclr/tests/src/baseservices/exceptions/generics/try-finally-struct01.cs, frameContext->ContextFlags is set to neither CONTEXT_EXCEPTION_ACTIVE nor CONTEXT_UNWOUND_TO_CALL at the beginning of UnwindManagedExceptionPass1. In this situation, PC is out of range of proper EH table range, which implies wrong EH found.
For example below,
The range of exception handler for our exception throw is [18 to 8C). EH#0: try [0018..008C) handled by [00B0..00DA) (finally)) But, when we start to look for EH for exception throw(8A), Pc indicates to address of IL 8C. Since both CONTEXT_EXCEPTION_ACTIVE and CONTEXT_UNWOUND_TO_CALL is off, IP decrement step(in ExceptionTracker::InitializeCrawlFrame) is skipped and cannot find proper EH.

In summary, I have 2 questions :
(1) Is it OK that CONTEXT_UNWOUND_TO_CALL is off? (IMHO, it should be ON here)
(2) Is it OK that CONTEXT_EXCEPTION_ACTIVE is off? (IMHO, it should be OFF here. Thus, the patch should be modified.)

I will update comment if have any progress.

    public void InternalExceptionTest(bool throwException)
    {
        hit = false; 
        try
        {
            if (throwException)
            {
                throw new GenException<T>();
            }
            Test.Eval(!throwException);
        }
        finally
        {
            hit = true;
        }
        Test.Eval(!throwException);
    }



; Assembly listing for method Gen`1[Int32][System.Int32]:InternalExceptionTest(bool):this
; Emitting BLENDED_CODE for generic ARM CPU
; optimized code
; r11 based frame
; fully interruptible
; Final local variable assignments
;
;  V00 this         [V00,T00] (  4,   4  )   byref  ->  [sp+0x04]   do-not-enreg[H] this
;  V01 arg1         [V01    ] (  5,   5  )    bool  ->  [sp+0x14]   do-not-enreg[X] addr-exposed
;  V02 tmp0         [V02,T01] (  3,   0  )     ref  ->  [sp+0x00]  
;# V03 OutArgs      [V03    ] (  1,   1  )  lclBlk ( 0) [sp+0x00]  
;  V04 PSPSym       [V04    ] (  1,   1  )     int  ->  [sp+0x08]   do-not-enreg[X] addr-exposed
;
; Lcl frame size = 12

G_M27436_IG01:
000000  B402           push    {r1}
000002  E92D 4800      push    {r11,lr}
000006  B083           sub     sp, 12
000008  F10D 0B0C      add     r11, sp, 12
00000C  A906           add     r1, sp, 24
00000E  9102           str     r1, [sp+0x08]    // [V04 PSPSym]
000010  9001           str     r0, [sp+0x04]    // [V00 this]

G_M27436_IG02:
000012  9B01           ldr     r3, [sp+0x04]    // [V00 this]
000014  2200           movs    r2, 0
000016  701A           strb    r2, [r3]

G_M27436_IG03:
000018  F89D 3014      ldrb    r3, [sp+0x14]    // [V01 arg1]
00001C  2B00           cmp     r3, 0
00001E  D110           bne     SHORT G_M27436_IG07

G_M27436_IG04:
000020  F89D 3014      ldrb    r3, [sp+0x14]    // [V01 arg1]
000024  2B00           cmp     r3, 0
000026  D001           beq     SHORT G_M27436_IG05
000028  2000           movs    r0, 0
00002A  E000           b       SHORT G_M27436_IG06

G_M27436_IG05:
00002C  2001           movs    r0, 1

G_M27436_IG06:
00002E  F24E 0339      movw    r3, 0xe039
000032  F2CB 13DE      movt    r3, 0xb1de
000036  4798           blx     r3       // Test:Eval(bool)
000038  F64E 2E03      movw    lr, LOW ADDRESS G_M27436_IG09
00003C  F2CB 1EDE      movt    lr, HIGH ADDRESS G_M27436_IG09
000040  E036           b       SHORT G_M27436_IG13

G_M27436_IG07:
000042  2001           movs    r0, 1
000044  F64D 21C0      movw    r1, 0xdac0
000048  F2CB 5198      movt    r1, 0xb598
00004C  F642 23E5      movw    r3, 0x2ae5
000050  F2CB 634C      movt    r3, 0xb64c
000054  4798           blx     r3       // CORINFO_HELP_STRCNS
000056  F24E 2341      movw    r3, 0xe241
00005A  F2CB 13DE      movt    r3, 0xb1de
00005E  4798           blx     r3       // System.Console:WriteLine(ref)
000060  F241 1024      movw    r0, 0x1124
000064  F2CB 5099      movt    r0, 0xb599
000068  F242 0305      movw    r3, 0x2005
00006C  F2CB 634C      movt    r3, 0xb64c
000070  4798           blx     r3       // CORINFO_HELP_NEWSFAST
000072  9000           str     r0, [sp] // [V02 tmp0]
000074  9800           ldr     r0, [sp]
000076  F64F 7351      movw    r3, 0xff51
00007A  F2CB 4390      movt    r3, 0xb490
00007E  4798           blx     r3       // System.Exception:.ctor():this
000080  9800           ldr     r0, [sp] // [V02 tmp0]
000082  F24C 53ED      movw    r3, 0xc5ed
000086  F2CB 634C      movt    r3, 0xb64c
00008A  4798           blx     r3       // CORINFO_HELP_THROW

G_M27436_IG08:
00008C  BF00           nop     

G_M27436_IG09:
00008E  F89D 3014      ldrb    r3, [sp+0x14]    // [V01 arg1]
000092  2B00           cmp     r3, 0
000094  D001           beq     SHORT G_M27436_IG10
000096  2000           movs    r0, 0
000098  E000           b       SHORT G_M27436_IG11

G_M27436_IG10:
00009A  2001           movs    r0, 1

G_M27436_IG11:
00009C  F24E 0339      movw    r3, 0xe039
0000A0  F2CB 13DE      movt    r3, 0xb1de
0000A4  4798           blx     r3       // Test:Eval(bool)

G_M27436_IG12:
0000A6  B003           add     sp, 12
0000A8  E8BD 4800      pop     {r11,lr}
0000AC  B001           add     sp, 4
0000AE  4770           bx      lr

G_M27436_IG13:
0000B0  E92D 480C      push    {r2,r3,r11,lr}
0000B4  F10B 030C      add     r3, r11, 12
0000B8  9300           str     r3, [sp]

G_M27436_IG14:
0000BA  F242 3338      movw    r3, 0x2338
0000BE  F2CB 3380      movt    r3, 0xb380
0000C2  6818           ldr     r0, [r3]
0000C4  F24E 2341      movw    r3, 0xe241
0000C8  F2CB 13DE      movt    r3, 0xb1de
0000CC  4798           blx     r3       // System.Console:WriteLine(ref)
0000CE  F85B 3C08      ldr     r3, [r11-0x08]   // [V00 this]
0000D2  2201           movs    r2, 1
0000D4  701A           strb    r2, [r3]

G_M27436_IG15:
0000D6  E8BD 880C      pop     {r2,r3,r11,pc}

; Total bytes of code 218, prolog size 18 for method Gen`1[Int32][System.Int32]:InternalExceptionTest(bool):this
; ============================================================
*************** EH table for Gen`1[Int32][System.Int32]:InternalExceptionTest(bool):this
1 EH table entries, 0 duplicate clauses, 1 total EH entries reported to VM
EH#0: try [0018..008C) handled by [00B0..00DA) (finally)

chunseoklee · 2016-07-08T04:04:36Z

gdb bt log
Here, frame Merge changes from TFS #2 corresponds to IL 8A.

Breakpoint 1, DispatchManagedException (ex=..., isHardwareException=false)
    at /home/twoflower/dev/coreclr/src/vm/exceptionhandling.cpp:4652
4652        do
(gdb) bt
#0  DispatchManagedException (ex=..., isHardwareException=false)
    at /home/twoflower/dev/coreclr/src/vm/exceptionhandling.cpp:4652
#1  0xb657f964 in IL_Throw (obj=0xb2719e60)
    at /home/twoflower/dev/coreclr/src/vm/jithelpers.cpp:5451
#2  0xb1b8ca00 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
(gdb)

jkotas · 2016-07-08T05:47:23Z

(1) Is it OK that CONTEXT_UNWOUND_TO_CALL is off? (IMHO, it should be ON here)
(2) Is it OK that CONTEXT_EXCEPTION_ACTIVE is off? (IMHO, it should be OFF here

Yes, having CONTEXT_UNWOUND_TO_CALL ON and CONTEXT_EXCEPTION_ACTIVE OFF is how I would expect this to work. @janvorli is the expert for this, but he is offline this week. It may be best to wait a few days until he can chime in.

chunseoklee · 2016-07-08T07:01:43Z

Thank yo for reply!

chunseoklee · 2016-07-08T08:47:19Z

I will make an issue for discussion.

This patch fixes regression caused by 597e160 commit. Previous PAL_VirtualUnwind does not set CONTEXT_UNWOUND_TO_CALL properly. In this patch, the flag is added for non-signaled exception.

chunseoklee · 2016-07-12T08:43:49Z

updated as @janvorli commented on #6185.

janvorli · 2016-07-12T08:46:38Z

LGTM. @chunseoklee - the ARM CI tests are failing due to some infrastructural issues - I assume you have tested the change locally, right?

chunseoklee · 2016-07-12T21:14:20Z

@janvorli Yes.

chunseoklee · 2016-07-12T21:28:13Z

We'd better Wait for issue #6178, then update or merge this patch.

janvorli · 2016-07-12T21:30:47Z

I've just merged it in right before your message

chunseoklee · 2016-07-12T21:52:35Z

Oh. Then, we will see the progress of issue 6178 and update later if nessasary.

parjong · 2016-07-13T00:34:24Z

@janvorli @chunseoklee When I checked this patch on my side, it solves most of regressions in ARM/Linux. Thanks 👍

This patch fixes regression caused by 597e160 commit. Previous PAL_VirtualUnwind does not set CONTEXT_UNWOUND_TO_CALL properly. In this patch, the flag is added for non-signaled exception. Commit migrated from dotnet/coreclr@adb2188

dnfclas added the cla-not-required label Jul 5, 2016

active exception regression fix

a2b91aa

This patch fixes regression caused by 597e160 commit. Previous PAL_VirtualUnwind does not set CONTEXT_UNWOUND_TO_CALL properly. In this patch, the flag is added for non-signaled exception.

chunseoklee force-pushed the fix_activeexception branch from 4758994 to a2b91aa Compare July 11, 2016 01:01

janvorli merged commit adb2188 into dotnet:master Jul 12, 2016

chunseoklee mentioned this pull request Jan 31, 2020

[arm32] throw exception does not set CONTEXT_UNWOUND_TO_CALL flag dotnet/runtime#6287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

active exception regression fix #6117

active exception regression fix #6117

Uh oh!

chunseoklee commented Jul 5, 2016

Uh oh!

jkotas commented Jul 5, 2016

Uh oh!

masonwheeler commented Jul 8, 2016

Uh oh!

chunseoklee commented Jul 8, 2016

Uh oh!

chunseoklee commented Jul 8, 2016

Uh oh!

chunseoklee commented Jul 8, 2016

Uh oh!

jkotas commented Jul 8, 2016

Uh oh!

chunseoklee commented Jul 8, 2016

Uh oh!

chunseoklee commented Jul 8, 2016

Uh oh!

chunseoklee commented Jul 12, 2016

Uh oh!

janvorli commented Jul 12, 2016

Uh oh!

chunseoklee commented Jul 12, 2016

Uh oh!

chunseoklee commented Jul 12, 2016

Uh oh!

janvorli commented Jul 12, 2016

Uh oh!

chunseoklee commented Jul 12, 2016

Uh oh!

parjong commented Jul 13, 2016

Uh oh!

Uh oh!

active exception regression fix #6117

active exception regression fix #6117

Uh oh!

Conversation

chunseoklee commented Jul 5, 2016

Uh oh!

jkotas commented Jul 5, 2016

Uh oh!

masonwheeler commented Jul 8, 2016

Uh oh!

chunseoklee commented Jul 8, 2016

Uh oh!

chunseoklee commented Jul 8, 2016

Uh oh!

chunseoklee commented Jul 8, 2016

Uh oh!

jkotas commented Jul 8, 2016

Uh oh!

chunseoklee commented Jul 8, 2016

Uh oh!

chunseoklee commented Jul 8, 2016

Uh oh!

chunseoklee commented Jul 12, 2016

Uh oh!

janvorli commented Jul 12, 2016

Uh oh!

chunseoklee commented Jul 12, 2016

Uh oh!

chunseoklee commented Jul 12, 2016

Uh oh!

janvorli commented Jul 12, 2016

Uh oh!

chunseoklee commented Jul 12, 2016

Uh oh!

parjong commented Jul 13, 2016

Uh oh!

Uh oh!