Native AOT bad code gen for constrained call

[agocke]

This looks to be a complicated set of GVMs and direct dispatches to the same interface.

Works on CoreCLR, produces either a null-ref or AV with Native AOT. Core problem is that the call to TryGetNextEntry has two locations in the program: one through a GVM, and one through a constrained invocation on a struct. It appears that the constrained invocation is somehow taking the GVM path, causing an erroneous unbox node to appear that causes memory corruption.
 
project:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net9.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <PublishAot>true</PublishAot>
    <EmitCompilerGeneratedFiles>true</EmitCompilerGeneratedFiles>
    <IlcGenerateMstatFile>true</IlcGenerateMstatFile>
    <IlcGenerateDgmlFile>true</IlcGenerateDgmlFile>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Serde" Version="0.6.0-preview6" />
  </ItemGroup>

</Project>

src:

Console.WriteLine("Hello, World!");

var sampleJson = """
{"latestVersion":{"version":"1.6.8","artifacts":{"linux-x64":"http://localhost:64922/dnvm/dnvm.tar.gz",
"osx-x64":"http://localhost:64922/dnvm/dnvm.tar.gz","win-x64":"http://localhost:64922/dnvm/dnvm.zip"}}}
""";

var release = JsonSerializer.Deserialize<DnvmReleases>(sampleJson);
Console.WriteLine(release);

[GenerateSerde]
public partial record struct DnvmReleases(DnvmReleases.Release LatestVersion)
{
    [GenerateSerde]
    public partial record struct Release(
        string Version,
        Dictionary<string, string> Artifacts);
}

[ghost]

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

Issue Details

---

This looks to be a complicated set of GVMs and direct dispatches to the same interface.

Works on CoreCLR, produces either a null-ref or AV with Native AOT. Core problem is that the call to TryGetNextEntry has two locations in the program: one through a GVM, and one through a constrained invocation on a struct. It appears that the constrained invocation is somehow taking the GVM path, causing an erroneous unbox node to appear that causes memory corruption.
 
project:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net9.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <PublishAot>true</PublishAot>
    <EmitCompilerGeneratedFiles>true</EmitCompilerGeneratedFiles>
    <IlcGenerateMstatFile>true</IlcGenerateMstatFile>
    <IlcGenerateDgmlFile>true</IlcGenerateDgmlFile>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Serde" Version="0.6.0-preview6" />
  </ItemGroup>

</Project>

src:

Console.WriteLine("Hello, World!");

var sampleJson = """
{"latestVersion":{"version":"1.6.8","artifacts":{"linux-x64":"http://localhost:64922/dnvm/dnvm.tar.gz",
"osx-x64":"http://localhost:64922/dnvm/dnvm.tar.gz","win-x64":"http://localhost:64922/dnvm/dnvm.zip"}}}
""";

var release = JsonSerializer.Deserialize<DnvmReleases>(sampleJson);
Console.WriteLine(release);

[GenerateSerde]
public partial record struct DnvmReleases(DnvmReleases.Release LatestVersion)
{
    [GenerateSerde]
    public partial record struct Release(
        string Version,
        Dictionary<string, string> Artifacts);
}

Author:	agocke
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	9.0.0

[MichalStrehovsky]

Reduced repro:

new C<Foo<string>>().ToString();

static Type GetObject() => typeof(Foo<object>);
var s = typeof(C<>).MakeGenericType(GetObject()).GetMethod("Set").CreateDelegate<Set<Foo<object>>>();

Foo<object> ob = default;
s(ref ob);
Console.WriteLine(ob.Cookie1);
Console.WriteLine(ob.Cookie2);


delegate void Set<T>(ref T t);

interface IFoo
{
    void Do<T>();
}

struct Foo<T> : IFoo
{
    public nint Cookie1;
    public nint Cookie2;

    public void Do<T1>()
    {
        Cookie1 = 42;
    }
}

class C<T> where T : IFoo
{
    public static void Set(ref T t)
    {
        t.Do<T>();
    }
}

This is supposed to print 42 0, instead it prints 0 42.

This is because we messed up the constrained call in Set. The bug is in building the generic dictionary of Set. Instead of creating a fat pointer that points to the Do method, it creates a fat pointer that points to the unboxing stub of the Do method. We end up unboxing something that wasn't boxed.

This happens when the runtime type loader creates the dictionary - so the repro either needs some MakeGeneric or a GVM call to land in an instantiation of C that we haven't seen at compile time (the dictionary building in the compiler doesn't have this bug).

I haven't decided where the bug is. Some weird things are happening:

• When we're creating the generic dictionary cell, we know the correct address of the non-unboxing method. If I set a breakpoint here:
  

  runtime/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/GenericDictionaryCell.cs

  Lines 351 to 354 in 3482997

  	internal override unsafe void Prepare(TypeBuilder builder)
  	{
  	builder.PrepareMethod(Method);
  	}

  I can see that the MethodDesc we have was parsed properly and we have a good function pointer stored in it. Then we enter builder.PrepareMethod.

• As part of preparing the method, we try to locate a template for the method and as part of that, refresh the function pointer with what we found:
  

  runtime/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeBuilder.cs

  Lines 314 to 317 in 3482997

  	if (templateMethod.FunctionPointer != IntPtr.Zero)
  	{
  	nonTemplateMethod.SetFunctionPointer(templateMethod.FunctionPointer, isFunctionPointerUSG: false);
  	}

  This is where things get messed up. The function pointer we looked up was for the unboxing thunk but it's very deliberately masqueraded as not that:

  runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/NativeLayoutVertexNode.cs

  Lines 756 to 758 in 3482997

  	// Note: We don't set the IsUnboxingStub flag on template methods (all template lookups performed at runtime are performed with this flag not set,
  	// since it can't always be conveniently computed for a concrete method before looking up its template)
  	unboxingStub = false;

At minimum, it doesn't look right we allow SetFunctionPointer to stomp whatever pointer we already had. It feels like there should be an assert that we either don't have a pointer, or the pointer is equal to what we're trying to set.