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

Wrong value passed to generic interface method in release #18259

Open
jakobbotsch opened this Issue Jun 2, 2018 · 11 comments

Comments

Projects
None yet
5 participants
@jakobbotsch
Collaborator

jakobbotsch commented Jun 2, 2018

On .NET core 2.1.0 and .NET framework 4.7.1 the following program prints 0 in debug but 1 in release:

using System;

class C0
{
    public ulong F6;
    public C0(ulong f6)
    {
        F6 = f6;
    }
}

struct S0
{
    public C0 F2;
    public S0(C0 f2)
    {
        F2 = f2;
    }
}

struct S1
{
    public S0 F0;
    public S0 F2;
    public S1(S0 f2)
    {
        F0 = default(S0);
        F2 = f2;
    }
}

interface II0
{
    void WriteLine<T>(T b);
}

class C1 : II0
{
    public void WriteLine<T>(T b)
    {
        Console.WriteLine(b);
    }
}

public class Program
{
    private static II0 s_0;
    public static void Main()
    {
        s_0 = new C1();
        S1[] var0 = {new S1(new S0(new C0(0)))};
        var0[0].F2.F2.F6 = var0[0].F2.F2.F6;
        var0[0].F2.F2 = new C0(1); // The value specified here is printed in release
        var0[0].F2.F2.F6 = 0;
        s_0.WriteLine(var0[0].F2.F2.F6);
    }
}
@jakobbotsch

This comment has been minimized.

Show comment
Hide comment
@jakobbotsch

jakobbotsch Jun 2, 2018

Collaborator

I wasn't able to reduce this to a smaller example, unfortunately. However, the error does not occur in a few situations:

  • If s_0 is made a local
  • If var0 is not an array
  • If S1.F2 is made C0 and a level of nesting removed in the bottom lines
  • If II0.WriteLine is not generic
Collaborator

jakobbotsch commented Jun 2, 2018

I wasn't able to reduce this to a smaller example, unfortunately. However, the error does not occur in a few situations:

  • If s_0 is made a local
  • If var0 is not an array
  • If S1.F2 is made C0 and a level of nesting removed in the bottom lines
  • If II0.WriteLine is not generic

@jkotas jkotas added the area-CodeGen label Jun 2, 2018

@jkotas jkotas added this to the 3.0 milestone Jun 2, 2018

@jkotas jkotas added the bug label Jun 2, 2018

@mikedn

This comment has been minimized.

Show comment
Hide comment
@mikedn

mikedn Jun 3, 2018

Contributor

This looks like another VN/CSE issue, different from the cast/indir issue. The relevant generated code looks like this:

;  var0[0].F2.F2.F6 = var0[0].F2.F2.F6;
       488B7E18             mov      rdi, gword ptr [rsi+24]
       488B4F08             mov      rcx, qword ptr [rdi+8]
       48894F08             mov      qword ptr [rdi+8], rcx
; var0[0].F2.F2 = new C0(1);
       48B9604A1DC2FB7F0000 mov      rcx, 0x7FFBC21D4A60
       E877FC815F           call     CORINFO_HELP_NEWSFAST
       488D5610             lea      rdx, bword ptr [rsi+16]
       488D4A08             lea      rcx, bword ptr [rdx+8]
       48C7400801000000     mov      qword ptr [rax+8], 1
       488BD0               mov      rdx, rax
       E8AF226C5F           call     CORINFO_HELP_CHECKED_ASSIGN_REF
; this is var0[0].F2.F2.F6 = 0
; the C0 reference it uses comes from rdi, this was loaded before
; the above assignment `var0[0].F2.F2 = new C0(1);`
       33C9                 xor      rcx, rcx
       48894F08             mov      qword ptr [rdi+8], rcx
Contributor

mikedn commented Jun 3, 2018

This looks like another VN/CSE issue, different from the cast/indir issue. The relevant generated code looks like this:

;  var0[0].F2.F2.F6 = var0[0].F2.F2.F6;
       488B7E18             mov      rdi, gword ptr [rsi+24]
       488B4F08             mov      rcx, qword ptr [rdi+8]
       48894F08             mov      qword ptr [rdi+8], rcx
; var0[0].F2.F2 = new C0(1);
       48B9604A1DC2FB7F0000 mov      rcx, 0x7FFBC21D4A60
       E877FC815F           call     CORINFO_HELP_NEWSFAST
       488D5610             lea      rdx, bword ptr [rsi+16]
       488D4A08             lea      rcx, bword ptr [rdx+8]
       48C7400801000000     mov      qword ptr [rax+8], 1
       488BD0               mov      rdx, rax
       E8AF226C5F           call     CORINFO_HELP_CHECKED_ASSIGN_REF
; this is var0[0].F2.F2.F6 = 0
; the C0 reference it uses comes from rdi, this was loaded before
; the above assignment `var0[0].F2.F2 = new C0(1);`
       33C9                 xor      rcx, rcx
       48894F08             mov      qword ptr [rdi+8], rcx
@mikedn

This comment has been minimized.

Show comment
Hide comment
@mikedn

mikedn Jun 3, 2018

Contributor

var0[0].F2.F2.F6 = 0 tree post VN is

     ( 14, 14) [000111] ------------              *  STMT      void  (IL 0x06D...0x080)
N014 (  1,  1) [000108] ------------              |  /--*  CNS_INT   long   0 $c0
N015 ( 14, 14) [000110] -A-XG-------              \--*  ASG       long   $VN.Void
N013 ( 12, 12) [000109] ---XG--N----                 \--*  IND       long   $c0
N011 (  1,  1) [000294] ------------                    |  /--*  CNS_INT   long   8 field offset Fseq[F6] $c1
N012 ( 10, 10) [000295] ----G--N----                    \--*  ADD       byref  <l:$383, c:$386>
N010 (  9,  9) [000106] *---G-------                       \--*  IND       ref    <l:$28e, c:$251>
N008 (  1,  1) [000296] ------------                          |  /--*  CNS_INT   long   8 field offset Fseq[F2, F2] $c1
N009 (  7,  7) [000297] ----G--N----                          \--*  ADD       byref  $483
N006 (  6,  6) [000308] ----G-------                             |  /--*  ADDR      byref  $480
N005 (  4,  4) [000102] a---G--N----                             |  |  \--*  IND       struct <l:$5c0, c:$504>
N003 (  1,  1) [000305] ------------                             |  |     |  /--*  CNS_INT   long   16 Fseq[#FirstElem] $c2
N004 (  2,  2) [000306] -------N----                             |  |     \--*  ADD       byref  $480
N002 (  1,  1) [000298] ------------                             |  |        \--*  LCL_VAR   ref    V03 tmp2         u:3 $340
N007 (  6,  6) [000307] ----G--N----                             \--*  COMMA     byref  $480
N001 (  0,  0) [000348] ------------                                \--*  NOP       void   $444

So the VN of the C0 reference where 0 is store is $28e. You'd expect this to be the VN produced by new C0 above but it's not (that one is $28f), it's the VN produce by the load in var0[0].F2.F2.F6 = var0[0].F2.F2.F6;. That is, VN missed the store var0[0].F2.F2 = new C0(1); and thinks that 0 is stored in the old C0 instance.

Contributor

mikedn commented Jun 3, 2018

var0[0].F2.F2.F6 = 0 tree post VN is

     ( 14, 14) [000111] ------------              *  STMT      void  (IL 0x06D...0x080)
N014 (  1,  1) [000108] ------------              |  /--*  CNS_INT   long   0 $c0
N015 ( 14, 14) [000110] -A-XG-------              \--*  ASG       long   $VN.Void
N013 ( 12, 12) [000109] ---XG--N----                 \--*  IND       long   $c0
N011 (  1,  1) [000294] ------------                    |  /--*  CNS_INT   long   8 field offset Fseq[F6] $c1
N012 ( 10, 10) [000295] ----G--N----                    \--*  ADD       byref  <l:$383, c:$386>
N010 (  9,  9) [000106] *---G-------                       \--*  IND       ref    <l:$28e, c:$251>
N008 (  1,  1) [000296] ------------                          |  /--*  CNS_INT   long   8 field offset Fseq[F2, F2] $c1
N009 (  7,  7) [000297] ----G--N----                          \--*  ADD       byref  $483
N006 (  6,  6) [000308] ----G-------                             |  /--*  ADDR      byref  $480
N005 (  4,  4) [000102] a---G--N----                             |  |  \--*  IND       struct <l:$5c0, c:$504>
N003 (  1,  1) [000305] ------------                             |  |     |  /--*  CNS_INT   long   16 Fseq[#FirstElem] $c2
N004 (  2,  2) [000306] -------N----                             |  |     \--*  ADD       byref  $480
N002 (  1,  1) [000298] ------------                             |  |        \--*  LCL_VAR   ref    V03 tmp2         u:3 $340
N007 (  6,  6) [000307] ----G--N----                             \--*  COMMA     byref  $480
N001 (  0,  0) [000348] ------------                                \--*  NOP       void   $444

So the VN of the C0 reference where 0 is store is $28e. You'd expect this to be the VN produced by new C0 above but it's not (that one is $28f), it's the VN produce by the load in var0[0].F2.F2.F6 = var0[0].F2.F2.F6;. That is, VN missed the store var0[0].F2.F2 = new C0(1); and thinks that 0 is stored in the old C0 instance.

@mikedn

This comment has been minimized.

Show comment
Hide comment
@mikedn

mikedn Jun 3, 2018

Contributor

The way the missing store is done is interesting:

	IL_005c: ldflda valuetype S0 S1::F2
	IL_0061: ldc.i4.1
	IL_0062: conv.i8
	IL_0063: newobj instance void C0::.ctor(uint64)
	IL_0068: stfld class C0 S0::F2

So the address of var0[0].F2 is taken and then the new object is stored via stdfld.

     (  8,  8) [000094] ------------              *  STMT      void  (IL   ???...  ???)
N008 (  1,  1) [000279] ------------              |     /--*  CNS_INT   long   8 field offset Fseq[F2] $c1
N009 (  8,  8) [000280] ----G-------              |  /--*  ADD       byref  $482
N006 (  6,  6) [000291] ----G-------              |  |  |  /--*  ADDR      byref  $480
N005 (  4,  4) [000077] a---G--N----              |  |  |  |  \--*  IND       struct <l:$4c2, c:$503>
N003 (  1,  1) [000288] ------------              |  |  |  |     |  /--*  CNS_INT   long   16 Fseq[#FirstElem] $c2
N004 (  2,  2) [000289] -------N----              |  |  |  |     \--*  ADD       byref  $480
N002 (  1,  1) [000281] ------------              |  |  |  |        \--*  LCL_VAR   ref    V03 tmp2         u:3 $340
N007 (  6,  6) [000290] ----G--N----              |  |  \--*  COMMA     byref  $480
N001 (  0,  0) [000347] ------------              |  |     \--*  NOP       void   $443
N011 (  8,  8) [000093] -A--G---R---              \--*  ASG       byref  $482
N010 (  1,  1) [000092] D------N----                 \--*  LCL_VAR   byref  V08 tmp7         d:3 $482
…
     (  5,  4) [000099] ------------              *  STMT      void  (IL 0x068...  ???)
N003 (  1,  1) [000096] ------------              |  /--*  LCL_VAR   ref    V07 tmp6         u:3 (last use) $28f
N004 (  5,  4) [000098] -A-XG-------              \--*  ASG       ref    $VN.Void
N002 (  3,  2) [000097] *--XG--N----                 \--*  IND       ref    $28f
N001 (  1,  1) [000095] ------------                    \--*  LCL_VAR   byref  V08 tmp7         u:3 (last use) $482

I would guess that VN gets confused. The store address ($482) has field sequence [F2] and the store is supposed to be at F2.F2. But since the second F2 has offset 0 it disappeared into thin air.

That said, I'm not very familiar with this part of the JIT so it's not clear if VN is somehow wrong here or if the real problem is in how stfld class C0 S0::F2 ends up being morphed in such a way that there's no associated field sequence.

Contributor

mikedn commented Jun 3, 2018

The way the missing store is done is interesting:

	IL_005c: ldflda valuetype S0 S1::F2
	IL_0061: ldc.i4.1
	IL_0062: conv.i8
	IL_0063: newobj instance void C0::.ctor(uint64)
	IL_0068: stfld class C0 S0::F2

So the address of var0[0].F2 is taken and then the new object is stored via stdfld.

     (  8,  8) [000094] ------------              *  STMT      void  (IL   ???...  ???)
N008 (  1,  1) [000279] ------------              |     /--*  CNS_INT   long   8 field offset Fseq[F2] $c1
N009 (  8,  8) [000280] ----G-------              |  /--*  ADD       byref  $482
N006 (  6,  6) [000291] ----G-------              |  |  |  /--*  ADDR      byref  $480
N005 (  4,  4) [000077] a---G--N----              |  |  |  |  \--*  IND       struct <l:$4c2, c:$503>
N003 (  1,  1) [000288] ------------              |  |  |  |     |  /--*  CNS_INT   long   16 Fseq[#FirstElem] $c2
N004 (  2,  2) [000289] -------N----              |  |  |  |     \--*  ADD       byref  $480
N002 (  1,  1) [000281] ------------              |  |  |  |        \--*  LCL_VAR   ref    V03 tmp2         u:3 $340
N007 (  6,  6) [000290] ----G--N----              |  |  \--*  COMMA     byref  $480
N001 (  0,  0) [000347] ------------              |  |     \--*  NOP       void   $443
N011 (  8,  8) [000093] -A--G---R---              \--*  ASG       byref  $482
N010 (  1,  1) [000092] D------N----                 \--*  LCL_VAR   byref  V08 tmp7         d:3 $482
…
     (  5,  4) [000099] ------------              *  STMT      void  (IL 0x068...  ???)
N003 (  1,  1) [000096] ------------              |  /--*  LCL_VAR   ref    V07 tmp6         u:3 (last use) $28f
N004 (  5,  4) [000098] -A-XG-------              \--*  ASG       ref    $VN.Void
N002 (  3,  2) [000097] *--XG--N----                 \--*  IND       ref    $28f
N001 (  1,  1) [000095] ------------                    \--*  LCL_VAR   byref  V08 tmp7         u:3 (last use) $482

I would guess that VN gets confused. The store address ($482) has field sequence [F2] and the store is supposed to be at F2.F2. But since the second F2 has offset 0 it disappeared into thin air.

That said, I'm not very familiar with this part of the JIT so it's not clear if VN is somehow wrong here or if the real problem is in how stfld class C0 S0::F2 ends up being morphed in such a way that there's no associated field sequence.

@jakobbotsch

This comment has been minimized.

Show comment
Hide comment
@jakobbotsch

jakobbotsch Jun 3, 2018

Collaborator

If the problem is that the store disappears, isn't it strange that the signature of the interface method affects the bug? Do you know why it is sensitive to that?

Collaborator

jakobbotsch commented Jun 3, 2018

If the problem is that the store disappears, isn't it strange that the signature of the interface method affects the bug? Do you know why it is sensitive to that?

@mikedn

This comment has been minimized.

Show comment
Hide comment
@mikedn

mikedn Jun 3, 2018

Contributor

If the problem is that the store disappears, isn't it strange that the signature of the interface method affects the bug? Do you know why it is sensitive to that?

Yep. It's probably easier to explain if I show the code generated in both cases (virtual generic and virtual non-generic method):
Original:

       E8AF226C5F           call     CORINFO_HELP_CHECKED_ASSIGN_REF
; broken code, 0 gets stored to a different object
       33C9                 xor      rcx, rcx
       48894F08             mov      qword ptr [rdi+8], rcx
; interface lookup for WriteLine
       48B950291A57DF010000 mov      rcx, 0x1DF571A2950
       488B39               mov      rdi, gword ptr [rcx]
       488BCF               mov      rcx, rdi
       48BA48541DC2FB7F0000 mov      rdx, 0x7FFBC21D5448
       49B808571DC2FB7F0000 mov      r8, 0x7FFBC21D5708
       E8609C385F           call     CORINFO_HELP_VIRTUAL_FUNC_PTR
; s_0.WriteLine(var0[0].F2.F2.F6) - note that both the object reference and the ulong are reloaded
; this reads from the newly created C0, correct
       488B5618             mov      rdx, gword ptr [rsi+24]
       488B5208             mov      rdx, qword ptr [rdx+8]
       488BCF               mov      rcx, rdi
       FFD0                 call     rax

Change method to non-generic:

       E8AF226C5F           call     CORINFO_HELP_CHECKED_ASSIGN_REF
; broken code still present, it's again storing 0 to the wrong object
       33D2                 xor      rdx, rdx
       48895708             mov      qword ptr [rdi+8], rdx
; oh snap, the presence of the interface lookup helper call prevented CSE of var0[0].F2.F2
; now we're also loading the ulong from the wrong object
; we get the 0 stored above so it appears as if the program is working correctly
; but in fact it is still broken
       488B5708             mov      rdx, qword ptr [rdi+8]
       48B95029855E87020000 mov      rcx, 0x2875E852950
       488B09               mov      rcx, gword ptr [rcx]
       49BB200056D6FB7F0000 mov      r11, 0x7FFBD6560020
       3909                 cmp      dword ptr [rcx], ecx
       FF1506E4EEFF         call     [II0:WriteLine(long):this]
Contributor

mikedn commented Jun 3, 2018

If the problem is that the store disappears, isn't it strange that the signature of the interface method affects the bug? Do you know why it is sensitive to that?

Yep. It's probably easier to explain if I show the code generated in both cases (virtual generic and virtual non-generic method):
Original:

       E8AF226C5F           call     CORINFO_HELP_CHECKED_ASSIGN_REF
; broken code, 0 gets stored to a different object
       33C9                 xor      rcx, rcx
       48894F08             mov      qword ptr [rdi+8], rcx
; interface lookup for WriteLine
       48B950291A57DF010000 mov      rcx, 0x1DF571A2950
       488B39               mov      rdi, gword ptr [rcx]
       488BCF               mov      rcx, rdi
       48BA48541DC2FB7F0000 mov      rdx, 0x7FFBC21D5448
       49B808571DC2FB7F0000 mov      r8, 0x7FFBC21D5708
       E8609C385F           call     CORINFO_HELP_VIRTUAL_FUNC_PTR
; s_0.WriteLine(var0[0].F2.F2.F6) - note that both the object reference and the ulong are reloaded
; this reads from the newly created C0, correct
       488B5618             mov      rdx, gword ptr [rsi+24]
       488B5208             mov      rdx, qword ptr [rdx+8]
       488BCF               mov      rcx, rdi
       FFD0                 call     rax

Change method to non-generic:

       E8AF226C5F           call     CORINFO_HELP_CHECKED_ASSIGN_REF
; broken code still present, it's again storing 0 to the wrong object
       33D2                 xor      rdx, rdx
       48895708             mov      qword ptr [rdi+8], rdx
; oh snap, the presence of the interface lookup helper call prevented CSE of var0[0].F2.F2
; now we're also loading the ulong from the wrong object
; we get the 0 stored above so it appears as if the program is working correctly
; but in fact it is still broken
       488B5708             mov      rdx, qword ptr [rdi+8]
       48B95029855E87020000 mov      rcx, 0x2875E852950
       488B09               mov      rcx, gword ptr [rcx]
       49BB200056D6FB7F0000 mov      r11, 0x7FFBD6560020
       3909                 cmp      dword ptr [rcx], ecx
       FF1506E4EEFF         call     [II0:WriteLine(long):this]
@jakobbotsch

This comment has been minimized.

Show comment
Hide comment
@jakobbotsch

jakobbotsch Jun 3, 2018

Collaborator

I see, that makes sense. So the CORINFO_HELP_VIRTUAL_FUNC_PTR call inhibits the optimization that makes the result correct otherwise, even though the store is wrong in both cases?
Do you have an idea on an easier way to inhibit the optimization than this particular pattern?

Collaborator

jakobbotsch commented Jun 3, 2018

I see, that makes sense. So the CORINFO_HELP_VIRTUAL_FUNC_PTR call inhibits the optimization that makes the result correct otherwise, even though the store is wrong in both cases?
Do you have an idea on an easier way to inhibit the optimization than this particular pattern?

@jakobbotsch

This comment has been minimized.

Show comment
Hide comment
@jakobbotsch

jakobbotsch Jun 3, 2018

Collaborator

This code produces the bug too:

public static void Main()
{
    S1[] var0 = {new S1(new S0(new C0(0)))};
    var0[0].F2.F2.F6 = var0[0].F2.F2.F6;
    var0[0].F2.F2 = new C0(1); // The value specified here is printed in release
    var0[0].F2.F2.F6 = 0;
    GC.KeepAlive(var0);
    Console.WriteLine(var0[0].F2.F2.F6);
}

I assume because the JIT assumes that GC.KeepAlive could change var0[0], so it has to reload it like before?

Collaborator

jakobbotsch commented Jun 3, 2018

This code produces the bug too:

public static void Main()
{
    S1[] var0 = {new S1(new S0(new C0(0)))};
    var0[0].F2.F2.F6 = var0[0].F2.F2.F6;
    var0[0].F2.F2 = new C0(1); // The value specified here is printed in release
    var0[0].F2.F2.F6 = 0;
    GC.KeepAlive(var0);
    Console.WriteLine(var0[0].F2.F2.F6);
}

I assume because the JIT assumes that GC.KeepAlive could change var0[0], so it has to reload it like before?

@mikedn

This comment has been minimized.

Show comment
Hide comment
@mikedn

mikedn Jun 3, 2018

Contributor

So the CORINFO_HELP_VIRTUAL_FUNC_PTR call inhibits the optimization that makes the result correct otherwise, even though the store is wrong in both cases?

Yes, but

  • it probably should not, I don't think this particular helper call changes memory in an observable manner
  • as you pretty much found out already, other calls can have the same effect. In general, any call that is not inlined is assumed to change memory in unspecified manner and that blocks CSEing of memory loads across call sites.
Contributor

mikedn commented Jun 3, 2018

So the CORINFO_HELP_VIRTUAL_FUNC_PTR call inhibits the optimization that makes the result correct otherwise, even though the store is wrong in both cases?

Yes, but

  • it probably should not, I don't think this particular helper call changes memory in an observable manner
  • as you pretty much found out already, other calls can have the same effect. In general, any call that is not inlined is assumed to change memory in unspecified manner and that blocks CSEing of memory loads across call sites.
@BruceForstall

This comment has been minimized.

Show comment
Hide comment
@BruceForstall
Contributor

BruceForstall commented Jun 11, 2018

@jakobbotsch

This comment has been minimized.

Show comment
Hide comment
@jakobbotsch

jakobbotsch Aug 6, 2018

Collaborator

A bit simpler example:

// Debug: Outputs 0
// Release: Outputs 2236837378
struct S1
{
    public uint F0;
    public S1(uint f0): this() { F0 = f0; }
}

struct S2
{
    public S1 F1;
    public S2(S1 f1): this() { F1 = f1; }
}

public class Program
{
    static S2[] s_11 = new S2[]{new S2(new S1(2236837378U))};
    public static void Main()
    {
        ref S1 vr7 = ref s_11[0].F1;
        vr7.F0 = vr7.F0;
        vr7.F0 = 0;
        vr7.F0 = vr7.F0;
        System.Console.WriteLine(vr7.F0);
    }
}
Collaborator

jakobbotsch commented Aug 6, 2018

A bit simpler example:

// Debug: Outputs 0
// Release: Outputs 2236837378
struct S1
{
    public uint F0;
    public S1(uint f0): this() { F0 = f0; }
}

struct S2
{
    public S1 F1;
    public S2(S1 f1): this() { F1 = f1; }
}

public class Program
{
    static S2[] s_11 = new S2[]{new S2(new S1(2236837378U))};
    public static void Main()
    {
        ref S1 vr7 = ref s_11[0].F1;
        vr7.F0 = vr7.F0;
        vr7.F0 = 0;
        vr7.F0 = vr7.F0;
        System.Console.WriteLine(vr7.F0);
    }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment