runtime: Null check incorrectly (?) optimized out for nested struct access

Repro:

using System;
using System.Runtime.CompilerServices;

unsafe struct F
{
    long X;
    fixed byte A[10];

    [MethodImpl(MethodImplOptions.NoInlining)]
    byte NextElement(int i) => A[1+i];

    static int Main() {
        F* x = null;
        return x->NextElement(100000);
    }
}

Expected result: NullReferenceException Actual result: AccessViolationException, or passed

About this issue

Original URL
State: closed
Created 5 years ago
Comments: 18 (18 by maintainers)

Commits related to this issue

JIT: update byref null check logic to handle field chains The jit was not properly accumulating offsets when figuring out if a byref should be null checked. Use GTF_DONT_CSE as a clue that GT_FIELD ... — committed to AndyAyersMS/coreclr by AndyAyersMS 5 years ago
JIT: update byref null check logic to handle field chains The jit was not properly accumulating offsets when figuring out if a byref should be null checked. Use GTF_DONT_CSE as a clue that GT_FIELD ... — committed to AndyAyersMS/coreclr by AndyAyersMS 5 years ago
JIT: update byref null check logic to handle field chains (#23850) The jit was not properly accumulating offsets when figuring out if a byref should be null checked. Use a non-null MorphAddressCo... — committed to dotnet/coreclr by AndyAyersMS 5 years ago
JIT: update byref null check logic to handle field chains (#23850) The jit was not properly accumulating offsets when figuring out if a byref should be null checked. Use a non-null MorphAddressCo... — committed to davmason/coreclr by AndyAyersMS 5 years ago

Most upvoted comments

Contextual child nodes certainly feel like an anti-pattern.

I think there may be a surgical fix for this particular issue. Let me poke at it a bit more. But I agree we should rethink this whole area.

AndyAyersMS on Apr 9, 2019

The non-intuitive behavior that made me file this issue is that whether or not ldflda throws NullReferenceException for null “this” depends on the instructions that follow it. The IL for the NextElement method from the C# repro at the top is:

ldarg.0
ldflda     valuetype F/'<A>e__FixedBuffer' F::A
ldflda     uint8 F/'<A>e__FixedBuffer'::FixedElementField
ldc.i4.1
ldarg.1
add
add
ldind.u1
ret

Changing IL of NextElement method to the following will make it throw NullReferenceException:

ldarg.0
ldflda     valuetype F/'<A>e__FixedBuffer' F::A
pop
ldc.i4 0
ret

But adding one more ldflda like this will make the NullReferenceException disappear:

ldarg.0
ldflda     valuetype F/'<A>e__FixedBuffer' F::A
ldflda     uint8 F/'<A>e__FixedBuffer'::FixedElementField
pop
ldc.i4 0
ret

jkotas on Apr 9, 2019