The Jit reorders null checks on field addresses with calls that have return buffers

Reproduction:

Problem();

[MethodImpl(MethodImplOptions.NoInlining)]
private static void Problem()
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    static Guid GetStruct(ClassWithFields cls)
    {
        throw new Exception("GetStruct");
    }

    ClassWithFields cls = null;
    cls.GuidFld = GetStruct(cls);
}

class ClassWithFields
{
    public Guid GuidFld;
}

Compile and run.

Expected result: Exception("GetStruct") is thrown.

Actual result: NullReferenceException is thrown.

Cause: the FIELD = CALL => CALL(ADDR(FIELD)) transformation effectively moves the null check from the point of the store to before the call happens.

Isn’t this an expected behavior?

I’d expect the code to run like this:

Evaluate the left assignment operand (target address)
Evaluate the right assignment operand (source value)
Execute the assignment

In our case, item 1 throws.

What happens on .NET Framework..

Isn’t this an expected behavior?

No. It will be easier to see why this is not correct if we look at the IL for Problem:

IL to import:
IL_0000  14                ldnull      
IL_0001  25                dup         
IL_0002  28 c7 00 00 06    call         0x60000C7 # GetStruct
IL_0007  7d 9d 00 00 04    stfld        0x400009D # GuidFld
IL_000c  2a                ret

As can be seen, we must execute the call before stfld.

.NET Framework

I would expect full framework to also have this bug, the Jit code responsible for it is very old.

@SingleAccretion

@SingleAccretion
I’d assume the C# standard expects this behavior too (§11.19.2):

The run-time processing of a simple assignment of the form x = y consists of the following steps:

If x is classified as a variable:

x is evaluated to produce the variable.

y is evaluated and, if required, converted to the type of x through an implicit conversion (§10.2).

(This is in contrast to properties where the full evaluation of LHS is impossible because there’s no lvalue there.)

The C# standard is not really relevant here (nor is my understanding of it complete enough to judge whether it is being violated here w.r.t. IL as emitted). The IL level semantics are, however, clear, and not consistent with the code produced, which is what this bug is about.

I will also note that this behavior of "evaluating the LHS" is not what we see for other (similar) forms of assignments at the C# level:

ProblemOne();   // Throws "GetValue"
ProblemTwo();   // Throws "GetValue"
ProblemThree(); // Throws "GetValue", not "IndexOutOfRangeException"
// etc

[MethodImpl(MethodImplOptions.NoInlining)]
private static void ProblemOne()
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    static int GetValue(ClassWithFields cls)
    {
        throw new Exception("GetValue");
    }

    ClassWithFields cls = null;
    cls.Field = GetValue(cls);
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static void ProblemTwo()
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    static int GetValue(int* cls)
    {
        throw new Exception("GetValue");
    }

    int* p = null;
    *p = GetValue(p);
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static void ProblemThree()
{
    [MethodImpl(MethodImplOptions.NoInlining)]
    static int GetValue()
    {
        throw new Exception("GetValue");
    }

    var a = new int[0];
    a[1] = GetValue();
}

@MadsTorgersen

@MadsTorgersen

The ProblemThree seems to be ok because array access is in the same bucket as indexer (see next bullet in the standard).

I stumbled upon a dotnet/roslyn issue that clarifies the Roslyn vs spec situation here:
dotnet/roslyn#63441 (comment)

The funny part is that the reproduction depends on the size of the struct. Using structs that are smaller than or exactly 64 bits would exhibit Exception throwing behavior, anything larger — NullReferenceException (see the tests below). Idk, maybe that is what "returning buffers" in the issues's title is about.

As far as I understand, the issue is tightly bound to how the compiler historically violates the lang spec and how JIT historically works, so fixing would be an enormous breaking change, but I find that the current behavior is all over the place:

Consistent behavior when working with properties.
Consistent behavior when working with fields in Debug mode.
Consistent behavior when working with fields that are smaller than 64 bits in Debug mode.
Inconsistent behavior when working with fields that are larger than 64 bits in Release mode.

Maybe at least the last one could be tinkered with? At least everyone would violate the lang spec the same way =) I obviously do not have the statistics, but I doubt a lot of code depends on the specific behavior of null referenced large structs assignment.

Tests:

using System;
using System.Runtime.CompilerServices;

using Xunit;
using FluentAssertions;

namespace JitReordering;

public class ReorderingTests
{
    [Fact]
    public void FieldAccessThrowsExceptionWhenLessThan64Bits()
    {
        var act = () => AccessField<S32>();

        act.Should().ThrowExactly<Exception>();
    }

    [Fact]
    public void FieldAccessThrowsExceptionWhenExactly64Bits()
    {
        var act = () => AccessField<S64>();

        act.Should().ThrowExactly<Exception>();
    }

    [Fact]
    public void FieldAccessThrowsNRExceptionWhenMoreThan64Bits1()
    {
        var act = () => AccessField<S96>();

        act.Should().ThrowExactly<NullReferenceException>();
    }

    [Fact]
    public void FieldAccessThrowsNRExceptionWhenMoreThan64Bits2()
    {
        var act = () => AccessField<S128>();

        act.Should().ThrowExactly<NullReferenceException>();
    }

     [Fact]
    public void FieldAccessThrowsNRExceptionWhenMoreThan64Bits3()
    {
        var act = () => AccessField<S256>();

        act.Should().ThrowExactly<NullReferenceException>();
    }

    [Fact]
    public void FieldAccessThrowsNRExceptionWhenMoreThan64Bits4()
    {
        var act = () => AccessField<S512>();

        act.Should().ThrowExactly<NullReferenceException>();
    }

    [Fact]
    public void PropertyAccessThrowsExceptionWhenLessThan64Bits()
    {
        var act = () => AccessProperty<S32>();

        act.Should().ThrowExactly<Exception>();
    }

    [Fact]
    public void PropertyAccessThrowsExceptionWhenExactly64Bits()
    {
        var act = () => AccessProperty<S64>();

        act.Should().ThrowExactly<Exception>();
    }

    [Fact]
    public void PropertyAccessThrowsExceptionWhenMoreThan64Bits()
    {
        var act = () => AccessProperty<S96>();

        act.Should().ThrowExactly<Exception>();
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static void AccessField<T>()
    where T : struct
    {
        ClassWithAField<T> cls = null!;
        cls.StructField = GetStruct<T>(cls);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static void AccessProperty<T>()
    where T : struct
    {
        ClassWithAField<T> cls = null!;
        cls.StructProperty = GetStruct<T>(cls);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static T GetStruct<T>(ClassWithAField<T> cls)
    where T : struct
    {
        throw new Exception("GetStruct");
    }

    class ClassWithAField<T>
    where T : struct
    {
        public T StructField;

        public T StructProperty { get; set; }
    }

    record struct S512(S256 v1, S256 v2);
    record struct S256(S128 v1, S128 v2);
    record struct S128(S64 v1, S64 v2);
    record struct S96(S64 v1, S32 v2);
    record struct S64(S32 v1, S32 v2);
    record struct S32(int v);
}

UPD. Passing class as an argument is not essential.

Navigation Menu

Repository navigation

The Jit reorders null checks on field addresses with calls that have return buffers #77650

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

The Jit reorders null checks on field addresses with calls that have return buffers #77650

Description

Activity

vladd commented on Nov 10, 2022

danmoseley commented on Nov 10, 2022

SingleAccretion commented on Nov 10, 2022

vladd commented on Nov 10, 2022

SingleAccretion commented on Nov 10, 2022

danmoseley commented on Nov 10, 2022

vladd commented on Nov 10, 2022

jakobbotsch commented on Nov 20, 2022

quixoticaxis commented on Jan 31, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions