Faulty codegen SkipLocalsInit Nullable<T> default value arm64

[NinoFloris]

Description

A user of Npgsql ran into garbled nullable values on his M2.

The full context can be found here npgsql/npgsql#6058 (comment)

Reproduction Steps

using System;
using System.Runtime.CompilerServices;

// Necessary
[module: SkipLocalsInit]

static class Program
{
    public static void Main()
    {
        var i = 0;
        // Let the JIT tier up.
        while (true)
        {
            var data = FaultyDefaultNullable<long?>();
            var d = data.GetValueOrDefault();
            // We need to dirty the stack and make sure this won't be optimized out by doing GC.Keepalive.
            var iObject = (object)i;
            var dataObject = (object?)data;
            var dObject = (object?)d;
            var hasValueObject = (object?)data.HasValue;
            // Alternatively
            // Console.WriteLine("{0}: `{1}` / {2}, `{3}`", i, data, d, data.HasValue);

            if (d != 0)
            {
                Console.WriteLine($"Unexpected value: {d} / {d:X} / {d:b8}");
                throw new Exception("We are here");
            }

            GC.KeepAlive(iObject);
            GC.KeepAlive(dataObject);
            GC.KeepAlive(dObject);
            GC.KeepAlive(hasValueObject);

            i++;
        }
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static T FaultyDefaultNullable<T>()
    {
        // When T is a Nullable<T> (and only in that case), we support returning null
        if (default(T) is null && typeof(T).IsValueType)
            return default!;

        throw new InvalidOperationException("Not nullable");
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static T OkDefaultNullable<T>() => default!;
}

Expected behavior

OkDefaultNullable

; Assembly listing for method Program:OkDefaultNullable[System.Nullable`1[long]]():System.Nullable`1[long] (Tier1)
; Emitting BLENDED_CODE for generic ARM64 - Apple
; Tier1 code
; optimized code
; optimized using Synthesized PGO
; fp based frame
; partially interruptible
; with Synthesized PGO: fgCalledCount is 100
; No PGO data

G_M000_IG01:                ;; offset=0x0000
            stp     fp, lr, [sp, #-0x10]!
            mov     fp, sp
 
G_M000_IG02:                ;; offset=0x0008
            mov     w0, wzr
            mov     x1, xzr
 
G_M000_IG03:                ;; offset=0x0010
            ldp     fp, lr, [sp], #0x10
            ret     lr
 
; Total bytes of code 24

Actual behavior

FaultyDefaultNullable

; Assembly listing for method Program:FaultyDefaultNullable[System.Nullable`1[long]]():System.Nullable`1[long] (Tier1)
; Emitting BLENDED_CODE for generic ARM64 - Apple
; Tier1 code
; optimized code
; optimized using Synthesized PGO
; fp based frame
; partially interruptible
; with Synthesized PGO: fgCalledCount is 100
; No PGO data

G_M000_IG01:                ;; offset=0x0000
            stp     fp, lr, [sp, #-0x20]!
            mov     fp, sp
 
G_M000_IG02:                ;; offset=0x0008
            mov     w0, wzr
            ldr     x1, [fp, #0x18]
 
G_M000_IG03:                ;; offset=0x0010
            ldp     fp, lr, [sp], #0x20
            ret     lr
 
; Total bytes of code 24

Regression?

This code works in .NET 8.0, starts failing in 9.0 and onwards (also tested 10.0 preview 1)

Known Workarounds

Remove SkipLocalsInit from Npgsql

Configuration

No response

Other information

No response

[dotnet-policy-service]

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

[NinoFloris]

The codegen for FaultyDefaultNullable without SkipLocalsInit looks similar to OkDefaultNullable

; Assembly listing for method Program:FaultyDefaultNullable[System.Nullable`1[long]]():System.Nullable`1[long] (Tier1)
; Emitting BLENDED_CODE for generic ARM64 - Apple
; Tier1 code
; optimized code
; optimized using Synthesized PGO
; fp based frame
; partially interruptible
; with Synthesized PGO: fgCalledCount is 100
; No PGO data

G_M000_IG01:                ;; offset=0x0000
            stp     fp, lr, [sp, #-0x10]!
            mov     fp, sp
            mov     x1, xzr
 
G_M000_IG02:                ;; offset=0x000C
            mov     w0, wzr
 
G_M000_IG03:                ;; offset=0x0010
            ldp     fp, lr, [sp], #0x10
            ret     lr
 
; Total bytes of code 24

[EgorBo]

Definitely looks like a bug, but I'd recommend to avoid SkipLocalsInit unless you see clear wins from it (I think it used to help with slow performance of stackalloc zero init in the past, unlikely an issue today)

[NinoFloris]

It's been enabled in the codebase for a long time, so we'd have to patch it out to work around this bug. Doesn't the BCL have it on as well? We do also still have some stackallocs, but it might indeed be less worthwhile at this point, need to test.

[jakobbotsch]

This looks like a bug in local assertion prop, or rather an interaction between block morphing and local assertion prop.

fgMorphTree BB01, STMT00000 (before)
               [000002] DA---------                         ▌  STORE_LCL_VAR struct<System.Nullable`1, 16>(P) V00 loc0         
                                                            ▌    ubyte  field V00.hasValue (fldOffset=0x0) -> V03 tmp2         
                                                            ▌    long   field V00.value (fldOffset=0x8) -> V04 tmp3          (last use)
               [000001] -----------                         └──▌  CNS_INT   int    0
MorphInitBlock:
PrepareDst for [000002] have found a local var V00.
GenTreeNode creates assertion:
               [000002] DA---------                         ▌  STORE_LCL_VAR struct<System.Nullable`1, 16>(P) V00 loc0         
                                                            ▌    ubyte  field V00.hasValue (fldOffset=0x0) -> V03 tmp2         
                                                            ▌    long   field V00.value (fldOffset=0x8) -> V04 tmp3          (last use)
In BB01 New Local Constant Assertion: V00 == ZeroObj, index = #01                                                    <- ZeroObj assertion created for all of V00
 using field by field initialization.
GenTreeNode creates assertion:
               [000031] DA---------                         ▌  STORE_LCL_VAR ubyte  V03 tmp2         
In BB01 New Local Constant Assertion: V03 == 0, index = #02
GenTreeNode creates assertion:
               [000031] DA---------                         ▌  STORE_LCL_VAR ubyte  V03 tmp2         
In BB01 New Local Subrange Assertion: V03 in [0..1], index = #03
Field-by-field init skipping write to dead field V04                                                                  <- V04 is dead, so write to V04 is skipped. Hence ZeroObj created above is now wrong
MorphInitBlock (after):
               [000031] DA---+-----                         ▌  STORE_LCL_VAR ubyte  V03 tmp2         
               [000030] -----+-----                         └──▌  CNS_INT   int    0

fgMorphTree BB01, STMT00000 (after)
               [000031] DA---+-----                         ▌  STORE_LCL_VAR ubyte  V03 tmp2         
               [000030] -----+-----                         └──▌  CNS_INT   int    0
...
Morphing BB02
Assertions in: #01 #02 #03
fgMorphTree BB02, STMT00005 (before)
               [000027] DA---------                         ▌  STORE_LCL_VAR struct<System.Nullable`1, 16>(P) V00 loc0         
                                                            ▌    ubyte  field V00.hasValue (fldOffset=0x0) -> V03 tmp2         
                                                            ▌    long   field V00.value (fldOffset=0x8) -> V04 tmp3         
               [000026] -----------                         └──▌  CNS_INT   int    0
[000027] is assigning a constant zero to a struct field or gc local that is already zero
Constant Assertion: V00 == ZeroObj                                                                                     <- using ZeroObj assertion to remove V04 store, which is wrong now

fgMorphTree BB02, STMT00005 (after)
               [000027] -----+-----                         ▌  NOP       void  

[NinoFloris]

Just for our information, is this something that will be patched for 9.0?

It's pretty easy to hit in npgsql (reader.GetFieldValue<int?>(0)) so ideally we could explain some runtime version fixes it, but otherwise we may have to remove SkipLocalsInit on .NET 9.0.

[jakobbotsch]

Sorry for the late response @NinoFloris. Yes, we can backport a fix for this to 9.0.