JIT reads elements from the wrong ImmutableArray when enumerating ImmutableArray multiple times

[RikkiGibson]

Tagging @davidwrighton @jcouv

Roslyn has encountered a crash dotnet/roslyn#46575 which we can only reproduce when building the runtime in fairly specific circumstances. You can see stack trace info, etc. in the linked bug in Roslyn. It requires building the runtime using a release mode compiler from later than Aug 4th or so, and building using msbuild, not calling the compiler directly.

Analysis

Looking at the crash dump, @davidwrighton identified that the JIT produced incorrect assembly. In the body of ExplicitInterfaceImplementation (source), we have two locals of the same type (ImmutableArray<MethodSymbol>) and we loop through one of them twice. In the second loop, the assembly uses the wrong local to index into the array. As a result, we get a crash/NRE with method being null on this line.

Repro

To reproduce this crash in dotnet/runtime:

1. Checkout the triage-jit-crash branch and clean the artifacts out of your local repo.
   • There are currently several compiler breaking changes in flight at the same time as this crash. This repro branch is based on release/5.0, but hacking around some unrelated compiler issues in order to get us to the point where we can repro the bug.
2. Run .\Build.cmd -restore, then .\Build.cmd libs -bl (having a binlog with -bl will probably be handy).
   • You should see crashes in the projects for ref\System.Diagnostics.Process.csproj and ref\System.Data.Common.csproj.

Details

We have found differences in the native code produced for the PEMethodSymbol.get_ExplicitInterfaceImplementations method when running from msbuild and when running csc using the args produced by msbuild. I have attached the output from the Disassembly window for these methods. The interesting bit seems to be around line 210 where in the broken version, we read the length from the explicitlyOverriddenMethods array, but seem to read elements from the explicitInterfaceImplementations array. (I'm not skilled in reading disassembly, so I could have mangled the description of this, sorry.)

It could be illustrative to pop these two disassembled methods into a diff tool and compare what they are doing.

PEMethodSymbol.ExplicitInterfaceImplementations-broken.txt
PEMethodSymbol.ExplicitInterfaceImplementations-working.txt

Tagging subscribers to this area: @eiriktsarpalis
See info in area-owners.md if you want to be subscribed.

[BruceForstall]

@dotnet/jit-contrib

[davidwrighton]

Some notes on making this repro reliably.

1. You must not have the 5.0.0-preview.8.20361.2 version of .NET installed on the machine globally.
2. You must not have a 3.1 version of dotnet in the .dotnet subdirectory of your runtime repo.

With those I found it possible to run csc.dll directly to look at the problem more easily instead of having to run through msbuild.

[AndyAyersMS]

Psychic debugging would say this is an issue with CSE, but that's just a guess.

I can repro the failure via msbuild, but can't yet get the extracted command line to repro the issue. @davidwrighton any tips on how you did the latter?

[AndyAyersMS]

Can repro bad codegen via PMI of .nuget\packages\microsoft.net.compilers.toolset\3.8.0-3.20416.3\tasks\netcoreapp3.1\bincore\Microsoft.CodeAnalysis.CSharp.dll with recent master, so will start looking there. Method of interest is Microsoft.CodeAnalysis.CSharp.Symbols.Metadata.PE.PEMethodSymbol:get_ExplicitInterfaceImplementations.

And yes, there is a CSE into R14... so psychic debugging is still a contender.

;  V59 cse1         [V59,T03] (  7, 18.50)     int  ->  r14         "CSE - aggressive"

Concern is this array access

G_M34632_IG20:
       cmp      r12d, r14d
       jae      G_M34632_IG28
       movsxd   rcx, r12d
       mov      r15, gword ptr [r13+8*rcx+16]

here r12d is the IV, r14d is array length from much earlier

IG05:
       ...
       mov      r14d, dword ptr [rdi+8]
       mov      r15, rdi

and r13 from a call after that point:

G_M34632_IG19:
       ...
       call     Microsoft.CodeAnalysis.PooledObjects.ArrayBuilder`1[__Canon][System.__Canon]:ToImmutableAndFree():System.Collections.Immutable.ImmutableArray`1[__Canon]:this
       mov      r13, rax

other "valid" array accesses earlier in the loop

G_M34632_IG06:
       cmp      r12d, r14d
       jae      G_M34632_IG28
       movsxd   rcx, r12d
       mov      r13, gword ptr [r15+8*rcx+16]
...

G_M34632_IG12:
       cmp      r12d, r14d
       jae      G_M34632_IG28
       movsxd   rcx, r12d
       mov      rdi, gword ptr [r15+8*rcx+16]

So seems like (as noted above) we're not using the right array length in a bounds check.

[AndyAyersMS]

Here's a simpler repro. Things go bad in F and we end up trying to access elements in aa and get an exception. Still chasing down where things go wrong.

using System;
using System.Collections.Immutable;
using System.Runtime.CompilerServices;

class X
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    public static void E(ImmutableArray<string> a) {}

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static ImmutableArray<string> G() => ImmutableArray<string>.Empty;

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static ImmutableArray<string> H()
    {
        string[] a = new string[100];
        
        for (int i = 0; i < a.Length; i++)
        {
            a[i] = "hello";
        }

        return ImmutableArray.Create<string>(a);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]    
    public static int F()
    {
        var a = H();
        int r = 0;

        foreach (var s in a)
        {
            if (s.Equals("hello")) r++;
        }

        var aa = a;

        if (r > 0)

        {
            foreach (var s in a)
            {
                if (s.Equals("hello")) r--;
            }
        
            aa = G();

            foreach (var s in a)
            {
                if (s.Equals("hello")) r++;
            }
        }

        E(aa);

        return r;
    }

    public static int Main() => F();
}

[briansull]

I note that there is the following assignment in the source code

explicitInterfaceImplementations = explicitlyOverriddenMethods;

[AndyAyersMS]

I think assertion prop might be getting confused here (or is missing a kill).