Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NullReferenceException thrown for multi-dimensional arrays in release #18232

Closed
jakobbotsch opened this Issue Jun 1, 2018 · 10 comments

Comments

Projects
None yet
6 participants
@jakobbotsch
Copy link
Collaborator

jakobbotsch commented Jun 1, 2018

The following program runs successfully in debug, but throws a NullReferenceException when run in release under .NET core 2.1. This issue repros on .NET framework as well with 64-bit JIT (it does not repro with 32-bit JIT).

class Program
{
    static int[,] s_2 = new int[1, 1];
    static void Main()
    {
        M(s_2[0, 0] & 0);
    }

    static void M(int arg0)
    {
        s_2[0, 0] = arg0;
    }
}

The bug is not triggered if the array is changed to a single-dimensional array, or if a different operator than & is used.

@mikedn

This comment has been minimized.

Copy link
Contributor

mikedn commented Jun 1, 2018

Hmm, looks like gtExtractSideEffList does not handle GT_ARR_ELEM and drops an assignment tree (introduced by CSE for the static field access) on the floor. This results in a CSE definition being removed so the subsequent use of s_2 will get null.

Also, assertionprop does not specify GTF_EXCEPT when calling gtExtractSideEffList. This seems incorrect, GT_ARR_ELEM should not be dropped to begin with because it has an exception side effect. This resembles #8648 except that in this case I see no reason to ignore exception side effects.

Working on a fix(es)...

@jakobbotsch

This comment has been minimized.

Copy link
Collaborator Author

jakobbotsch commented Jun 1, 2018

Here is a variant which throws IndexOutOfRangeException instead, and has some really strange behavior with regards to the variables/static initializers:

using System.Runtime.CompilerServices;

public class Program
{
    static ulong s_6 = 0; // Removing this initializer hides the problem (what?)
    public static void Main()
    {
        char[][,] a = { new char[,] { { 'a' } } };
        M0(a);
    }

    static void M0(char[][,] arg2)
    {
        if (0 >= (0 & arg2[0][0, 0])) // Exception is thrown here
        {
            M2(); // Commenting this changes it to NRE instead
            s_6 = s_6; // Commenting this hides the problem
        }

        int var = arg2[0][0, 0]; // Commenting this hides the problem
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static void M2()
    {
    }
}
@jkotas

This comment has been minimized.

Copy link
Member

jkotas commented Jun 1, 2018

@BruceForstall BruceForstall added this to the 3.0 milestone Jun 1, 2018

@mikedn

This comment has been minimized.

Copy link
Contributor

mikedn commented Jun 1, 2018

Here is a variant which throws IndexOutOfRangeException instead, and has some really strange behavior with regards to the variables/static initializers:

I have to look at that more closely but it does seem to be the same bug with different manifestations:

  • The out of range exception comes from arg2[0]. arg2.Length gets CSEd and then the CSE definition gets dropped and the range check uses a garbage length value.
  • The null ref exception comes from [0, 0]. In this situation arg2[0] gets CSEd and then dropped so the multi dim array reference is null. This also means that if arg2 is empty you won't get an exception because arg2[0] has completely disappeared.

Variations such as removing M2() call or s_6 assignment simply affect what, if anything, gets CSEd. For example, the presence of M2() means that arg2[0] cannot be CSEd because M2 might change it.

@jakobbotsch

This comment has been minimized.

Copy link
Collaborator Author

jakobbotsch commented Jul 3, 2018

Here is one that I think is unrelated, but a confirmation would be nice before I open a new issue:

// Generated by Fuzzlyn on 2018-07-02 17:44:35
// Seed: 7883145904562184282
// Reduced from 39.4 KiB to 0.4 KiB
// Debug: Runs successfully
// Release: Throws 'System.NullReferenceException'

class C0
{
    public sbyte F0;
    public ushort F7;
    public uint F8;
}

public class Program
{
    static int s_0;
    static C0 s_1 = new C0();

    public static void Main()
    {
        s_0 = 0; // ensure no runtime checks in M0
        M0();
    }

    static void M0()
    {
        bool vr0 = (s_1.F7 < s_1.F0) ^ (s_1.F8 != s_1.F8);
        if (vr0)
        {
            s_1.F7 = s_1.F7;
        }
    }
}

The ASM generated for M0 is clearly wrong:

; Assembly listing for method Program:M0()
; Emitting BLENDED_CODE for X64 CPU with AVX
; optimized code
; rsp based frame
; partially interruptible
; Final local variable assignments
;
;# V00 OutArgs      [V00    ] (  1,  1   )  lclBlk ( 0) [rsp+0x00]
;  V01 cse0         [V01,T01] ( 14, 14   )     int  ->  rcx
;  V02 cse1         [V02,T02] ( 13, 11.50)     int  ->  rdx
;  V03 cse2         [V03,T00] ( 15, 14   )     ref  ->  rax         must-init
;
; Lcl frame size = 0

G_M23186_IG01:
       33C0                 xor      rax, rax

G_M23186_IG02:
       8B4008               mov      eax, dword ptr [rax+8]     ; ouch
       48B87829ABD740010000 mov      rax, 0x140D7AB2978
       488B00               mov      rax, gword ptr [rax]
       0FB7500C             movzx    rdx, word  ptr [rax+12]
       480FBE480E           movsx    rcx, byte  ptr [rax+14]
       3BD1                 cmp      edx, ecx
       0F9CC1               setl     cl
       0FB6C9               movzx    rcx, cl
       85C9                 test     ecx, ecx
       7404                 je       SHORT G_M23186_IG03
       6689500C             mov      word  ptr [rax+12], dx

G_M23186_IG03:
       C3                   ret

; Total bytes of code 44, prolog size 0 for method Program:M0()
; ============================================================

If I understand correctly, CSE optimizes this by pulling s_1 into a local (cse2 in the dump below), but then ends up using cse2 before it is assigned:

N030 ( 29, 33)              [000016] -A-XG-------              *  JTRUE     void  
N028 (  1,  1)              [000014] ------------              |  /--*  CNS_INT   int    0 $82
N029 ( 27, 31)              [000015] JA-XG--N----              \--*  EQ        int    <l:$347, c:$346>
N026 (  1,  1)              [000050] ------------                 |  /--*  LCL_VAR   int    V01 cse0          <l:$341, c:$340>
N027 ( 25, 29)              [000052] -A-XG-------                 \--*  COMMA     int    <l:$341, c:$340>
N021 (  5,  5)              [000004] ---XG-------                    |        /--*  IND       byte   <l:$2c1, c:$300>
N019 (  1,  1)              [000031] ------------                    |        |  |  /--*  CNS_INT   long   14 field offset Fseq[F0] $1c1
N020 (  2,  2)              [000032] ----G--N----                    |        |  \--*  ADD       byref  <l:$203, c:$202>
N018 (  1,  1)              [000062] ------------                    |        |     \--*  LCL_VAR   ref    V03 cse2          <l:$140, c:$180>
N022 ( 20, 24)              [000005] -A-XG-------                    |     /--*  LT        int    <l:$341, c:$340>
N016 (  1,  1)              [000055] ------------                    |     |  |  /--*  LCL_VAR   int    V02 cse1          <l:$241, c:$280>
N017 ( 11, 18)              [000056] -A-XG-------                    |     |  \--*  COMMA     int    <l:$241, c:$280>
N013 ( 10, 17)              [000002] -A-XG-------                    |     |     |  /--*  IND       ushort <l:$241, c:$280>
N011 (  1,  1)              [000028] ------------                    |     |     |  |  |  /--*  CNS_INT   long   12 field offset Fseq[F7] $1c0
N012 (  7, 14)              [000029] -A--G--N----                    |     |     |  |  \--*  ADD       byref  <l:$201, c:$200>
N009 (  1,  1)              [000060] ------------                    |     |     |  |     |  /--*  LCL_VAR   ref    V03 cse2          <l:$140, c:$180>
N010 (  6, 13)              [000061] -A--G-------                    |     |     |  |     \--*  COMMA     ref    <l:$140, c:$180>
N006 (  5, 12)              [000001] x---G-------                    |     |     |  |        |  /--*  IND       ref    <l:$140, c:$180>
N005 (  3, 10)              [000030] ------------                    |     |     |  |        |  |  \--*  CNS_INT(h) long   0x1c4f1e92978 static Fseq[s_1] $100
N008 (  5, 12)              [000059] -A--G---R---                    |     |     |  |        \--*  ASG       ref    $VN.Void
N007 (  1,  1)              [000058] D------N----                    |     |     |  |           \--*  LCL_VAR   ref    V03 cse2          <l:$140, c:$180>
N015 ( 10, 17)              [000054] -A-XG---R---                    |     |     \--*  ASG       int    $VN.Void
N014 (  1,  1)              [000053] D------N----                    |     |        \--*  LCL_VAR   int    V02 cse1          <l:$241, c:$280>
N024 ( 20, 24)              [000047] -A-XG---R---                    |  /--*  ASG       int    $VN.Void
N023 (  1,  1)              [000046] D------N----                    |  |  \--*  LCL_VAR   int    V01 cse0          <l:$341, c:$340>
N025 ( 24, 28)              [000051] -A-XG-------                    \--*  COMMA     void   $VN.Void
N004 (  4,  4) CSE #02 (def)[000007] ---XG-------                       \--*  IND       int    <l:$343, c:$380>
N002 (  1,  1)              [000034] ------------                          |  /--*  CNS_INT   long   8 field offset Fseq[F8] $1c2
N003 (  2,  2)              [000035] ----G--N----                          \--*  ADD       byref  <l:$205, c:$204>
N001 (  1,  1)              [000063] ------------                             \--*  LCL_VAR   ref    V03 cse2          <l:$140, c:$180>        ; this has not been assigned yet

The dump can be seen here.

cc @mikedn

@AndyAyersMS

This comment has been minimized.

Copy link
Member

AndyAyersMS commented Jul 3, 2018

also cc @briansull

@mikedn

This comment has been minimized.

Copy link
Contributor

mikedn commented Jul 3, 2018

Here is one that I think is unrelated, but a confirmation would be nice before I open a new issue:

Yep, this looks like some sort of CSE bug - in this case the CSE def is not dropped, it's simply out of order. It's easier to see after rationalization, in LIR, that a use of cse2 occurs before its def:

------------ BB01 [000..032) -> BB03 (cond), preds={} succs={BB02,BB03}
     ( 29, 33) [000017] ------------                 IL_OFFSET void   IL offset: 0x0
N001 (  1,  1) [000063] ------------        t63 =    LCL_VAR   ref    V03 cse2          <l:$140, c:$180>
N002 (  1,  1) [000034] ------------        t34 =    CNS_INT   long   8 field offset Fseq[F8] $1c2
                                                 /--*  t63    ref    
                                                 +--*  t34    long   
N003 (  2,  2) [000035] ----G--N----        t35 = *  ADD       byref  <l:$205, c:$204>
                                                 /--*  t35    byref  
N004 (  4,  4) [000007] ---XG-------         t7 = *  IND       int    <l:$343, c:$380>
N005 (  3, 10) [000030] ------------        t30 =    CNS_INT(h) long   0x1c4f1e92978 static Fseq[s_1] $100
                                                 /--*  t30    long   
N006 (  5, 12) [000001] x---G-------         t1 = *  IND       ref    <l:$140, c:$180>
                                                 /--*  t1     ref    
N008 (  5, 12) [000059] DA--G-------              *  STORE_LCL_VAR ref    V03 cse2         
N009 (  1,  1) [000060] ------------        t60 =    LCL_VAR   ref    V03 cse2          <l:$140, c:$180>
@mikedn

This comment has been minimized.

Copy link
Contributor

mikedn commented Jul 3, 2018

Hmm, I find it interesting that both the XOR and LT got the same VN, that's because x XOR 0 = x. This might be what sends CSE into a tailspin as LT is a child of XOR. It attempts to replace XOR and to do that it needs to extract side effects. When it does it looks like the tree gets reordered and probably CSE doesn't handle that.

@briansull

This comment has been minimized.

Copy link
Contributor

briansull commented Jul 3, 2018

I think that the problem is related to incorrectly setting/clearing the reverse flag for GT_IND.
The RyuJIT has some like a requirement for liveness computation that required someone to ser/clear the reverse flag on Indirections.

@jakobbotsch

This comment has been minimized.

Copy link
Collaborator Author

jakobbotsch commented Jul 3, 2018

I have opened #18770 for it, thanks for the reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.