[Mono] RuntimeTypeHandle.Equals seems to be broken in Release mode

[simonrozsival]

Description

A recent Xamarin iOS PR (xamarin/xamarin-macios#18742) ran into an issue in .NET 8/Mono with the RuntimeTypeHandle.Equals method.

The Xamarin tooling generates several lookup functions in IL using Mono.Cecil which look something like this:

public uint LookupTypeId(RuntimeTypeHandle handle) {
   if (handle.Equals(typeof(NSString).TypeHandle)) {
        return 1u;
   }
   if (handle.Equals(typeof(NSSet).TypeHandle)) {
        return 2u;
   }
   // ...
   return unchecked((uint)-1);
}

While this worked fine in .NET 7, in .NET 8 calling for example LookupTypeId(typeof(NSString).TypeHandle) won't return the correct value (4294967295 instead of 1).

Reproduction Steps

The issue can be reproduced with a modified HelloWorld Mono sample:

<!-- src/mono/sample/LookupTable/LookupTable.ilproj -->
<Project Sdk="Microsoft.NET.Sdk.IL">
  <PropertyGroup>
    <OutputType>Library</OutputType>
    <TargetFramework>$(NetCoreAppCurrent)</TargetFramework>
  </PropertyGroup>
</Project>

// src/mono/sample/LookupTable/LookupTable.il
.assembly extern mscorlib{}
.assembly LookupTable{}

.class public auto ansi abstract sealed beforefieldinit LookupTable
    extends [mscorlib]System.Object
{
    .method public hidebysig static
        int32 LookupTypeId (
            valuetype [mscorlib]System.RuntimeTypeHandle handle
        ) cil managed
    {
        .maxstack 8

        IL_aaaa: ldarga.s handle
        ldtoken [mscorlib]System.String
        call instance bool [mscorlib]System.RuntimeTypeHandle::Equals(valuetype [mscorlib]System.RuntimeTypeHandle)
        brfalse.s IL_aaab

        ldc.i4.2
        ret

        IL_aaab: ldc.i4.0
        ret
    }
}

<!-- src/mono/sample/HelloWorld/HelloWorld.csproj -->
<ItemGroup>
  <ProjectReference Include="..\LookupTable\LookupTable.ilproj" />
</ItemGroup>

// src/mono/sample/HelloWorld/Program.cs

if (LookupTable.LookupTypeId(typeof(string).TypeHandle) == 2) {
    System.Console.WriteLine("FOUND");
} else {
    System.Console.WriteLine("NOT FOUND");
}

Build the mono runtime with: ./build.sh mono+libs -c Release
Build and run the sample with mono -C src/mono/sample/HelloWorld MONO_CONFIG=Release MONO_ARCH=arm64 clean run

Expected behavior

The program should print FOUND.

Actual behavior

The program prints NOT FOUND.

Regression?

The generated lookup tables worked well before bumping to .NET 8.

Known Workarounds

When we disable inlining of the RuntimeTypeHandle.Equals method in src/mono/System.Private.CoreLib/src/System/RuntimeTypeHandle.cs, the lookup function works correctly:

[MethodImpl(MethodImplOptions.NoInlining)]
public bool Equals(RuntimeTypeHandle handle)
{
    return value == handle.Value;
}

Configuration

• .NET 8 (e9ce3aac5440b8d5b76bddfea6285e546083589d)
• building on macOS 13.5
• ARM64 (M1)
• Specific to Release configuration

Other information

@filipnavara managed to workaround the issue by preventing inlining of the Equals method and so we believe that the culprit is inlining.

cc @rolfbjarne @ivanpovazan @filipnavara

[ivanpovazan]

/cc: @lambdageek

[lambdageek]

yikes

[lambdageek]

Is it important that the generated IL goes through mscorlib and not System.Runtime to find the referenced types? I wonder if it's something about type forwarding and/or trimming

[simonrozsival]

@lambdageek I don't think so. This is an example of the actual generated code:

.method private final hidebysig newslot virtual 
	instance uint32 LookupTypeId (
		valuetype [System.Private.CoreLib]System.RuntimeTypeHandle handle
	) cil managed 
{
	.override method instance uint32 [Microsoft.MacCatalyst]ObjCRuntime.IManagedRegistrar::LookupTypeId(valuetype [System.Private.CoreLib]System.RuntimeTypeHandle)
	// Method begins at RVA 0x224c
	// Header size: 12
	// Code size: 26 (0x1a)
	.maxstack 2

	// if (handle.Equals(typeof(AppDelegate).TypeHandle))
	IL_0000: ldarga.s handle
	IL_0002: ldtoken MySingleView.AppDelegate
	IL_0007: call instance bool [System.Private.CoreLib]System.RuntimeTypeHandle::Equals(valuetype [System.Private.CoreLib]System.RuntimeTypeHandle)
	IL_000c: brfalse IL_0017

	// return 0u;
	IL_0011: ldc.i4 0
	IL_0016: ret

	// return uint.MaxValue;
	IL_0017: nop
	IL_0018: ldc.i4.m1
	IL_0019: ret
} // end of method __Registrar__::LookupTypeId

Sorry for the confusion.

[lambdageek]

It's a jit problem. If I run in release mode with interp, it finds the type

[lambdageek]

It's the inliner. Running with --trace or with -O=-inline returns FOUND

[lambdageek]

It's specifically something about inlining the caller of RuntimeTypeHandle::Equals(RuntimeTypeHandle)

In this snippet if I change LookupTable::Something to be noinlining, the result is FOUND. if I remove it, it's NOT FOUND

.class public auto ansi abstract sealed beforefieldinit LookupTable
    extends [System.Runtime]System.Object
{
    .method private hidebysig static
       bool Something(valuetype [System.Runtime]System.RuntimeTypeHandle h1,
                      valuetype [System.Runtime]System.RuntimeTypeHandle h2) cil managed noinlining
    {
       .maxstack 8
       ldarga.s h1
       ldarg.1
       call instance bool [System.Runtime]System.RuntimeTypeHandle::Equals(valuetype [System.Runtime]System.RuntimeTypeHandle)
       ret
    }
    .method public hidebysig static
        int32 LookupTypeId (
            valuetype [System.Runtime]System.RuntimeTypeHandle handle
        ) cil managed 
    {
        .maxstack 8

        //IL_aaaa: ldarga.s handle
        ldarg.0
        ldtoken [System.Runtime]System.String
        // call instance bool [System.Runtime]System.RuntimeTypeHandle::Equals(valuetype [System.Runtime]System.RuntimeTypeHandle)
        call bool LookupTable::Something(valuetype [System.Runtime]System.RuntimeTypeHandle, valuetype [System.Runtime]System.RuntimeTypeHandle)
        brfalse.s IL_aaab

        ldc.i4.2
        ret

        IL_aaab: ldc.i4.0
        ret
    }

[lambdageek]

@jandupej I think this is somehow related to the OP_LDTOKEN_FIELD instruction that is now emitted (since #81695) when ldtoken doesn't follow the usual ldtoken; call GetTypeFromHandle; callvirt get_TypeHandle pattern that is used in Roslyn-generated code for typeof(string).TypeHandle. In this case manually written IL just calls ldtoken and then uses it as an argument to a method that will be inlined. Somehow OP_LDTOKEN_FIELD doesn't behave well when it is used as an argument to an inlined function.

[lambdageek]

@jandupej Maybe as a hack we can just do OP_LDTOKEN_FIELD only when the ldtoken's handle_class is RuntimeFieldHandle ? Since that's all that CreateSpan cares about?

Although I'm worried about other scenarios where handwritten code might do ldtoken on a field and then call some inlined method. Although right now that's purely theoretical...

[lambdageek]

@jandupej I think maybe it's a bug in mono_decompose_vtype_opts. I think changing the instruction type in-place was wrong.

If I change it like this:

				case OP_LDTOKEN_FIELD:
#if 0
				  ins->opcode = OP_VMOVE;
				  restart = TRUE;
				  break;
#else
                                  /* fallthru */
#endif
				case OP_VMOVE: {

the code works.

I'm not really sure why. The basic block BEFORE LOWER-VTYPE-OPTS looks like this for both versions:

 il_seq_point intr il: 0x0
 i8const R37 <- [5251559648]
 ldaddr R38 <- R36
 store_membase_reg [R38] <- R37
 ldtoken_field R39 <- R36
 vmove R42 <- R33
 vmove R48 <- R39
 ldaddr R49 <- R42
 load_membase R50 <- [R49 + 0x0]
 ldaddr R54 <- R48
 load_membase R56 <- [R54 + 0x0]
 lcompare R50 R56
 long_ceq R58 <-
 int_conv_to_u1 R60 <- R58
 int_conv_to_u1 R61 <- R60
 il_seq_point il: 0xb, nonempty-stack
 icompare_imm R61 [0]
 int_bne_un [T:B4 F:B5]

The problem happens with that last vmove vmove R48 <- R39.

In the version where decompose does a fallthru, the code works and the output AFTER LOWER-VTYPE-OPTS is like this:

created temp 5 (R39) of type System.RuntimeTypeHandle
AFTER LOWER-VTYPE-OPTS  2: [IN:  BB0(0), OUT:  BB4(2) BB5(4) ]
 il_seq_point intr il: 0x0
 i8const R37 <- [5209671904]
 ldaddr R38 <- R36
 store_membase_reg [R38] <- R37
 ldaddr R64 <- R36
 ldaddr R65 <- R39
 loadi8_membase R66 <- [R64 + 0x0]
 storei8_membase_reg [R65] <- R66
 ldaddr R67 <- R33
 ldaddr R68 <- R42
 loadi8_membase R69 <- [R67 + 0x0]
 storei8_membase_reg [R68] <- R69
 ldaddr R70 <- R39
 ldaddr R71 <- R48
 loadi8_membase R72 <- [R70 + 0x0]
 storei8_membase_reg [R71] <- R72
 ldaddr R49 <- R42
 load_membase R50 <- [R49 + 0x0]
 ldaddr R54 <- R48
 load_membase R56 <- [R54 + 0x0]
 lcompare R50 R56
 long_ceq R58 <-
 int_conv_to_u1 R60 <- R58
 int_conv_to_u1 R61 <- R60
 il_seq_point il: 0xb, nonempty-stack
 icompare_imm R61 [0]
 int_bne_un [T:B4 F:B5]

Note that the last vmove R48 <- R39 turned into

 ldaddr R70 <- R39
 ldaddr R71 <- R48
 loadi8_membase R72 <- [R70 + 0x0]
 storei8_membase_reg [R71] <- R72

In the version where we overwrite the instruction opcode, instead the output is:

created temp 5 (R48) of type System.RuntimeTypeHandle
created temp 6 (R39) of type System.RuntimeTypeHandle
AFTER LOWER-VTYPE-OPTS  2: [IN:  BB0(0), OUT:  BB4(2) BB5(4) ]
 il_seq_point intr il: 0x0
 i8const R37 <- [5251559648]
 ldaddr R38 <- R36
 store_membase_reg [R38] <- R37
 ldaddr R70 <- R36
 ldaddr R71 <- R39
 loadi8_membase R72 <- [R70 + 0x0]
 storei8_membase_reg [R71] <- R72
 ldaddr R64 <- R33
 ldaddr R65 <- R42
 loadi8_membase R66 <- [R64 + 0x0]
 storei8_membase_reg [R65] <- R66
 ldaddr R67 <- R48
 ldaddr R68 <- R48
 loadi8_membase R69 <- [R67 + 0x0]
 storei8_membase_reg [R68] <- R69
 ldaddr R49 <- R42
 load_membase R50 <- [R49 + 0x0]
 ldaddr R54 <- R48
 load_membase R56 <- [R54 + 0x0]
 lcompare R50 R56
 long_ceq R58 <-
 int_conv_to_u1 R60 <- R58
 int_conv_to_u1 R61 <- R60
 il_seq_point il: 0xb, nonempty-stack
 icompare_imm R61 [0]
 int_bne_un [T:B4 F:B5]

And the last vmove R48 <- R39 turned into

 ldaddr R67 <- R48
 ldaddr R68 <- R48
 loadi8_membase R69 <- [R67 + 0x0]
 storei8_membase_reg [R68] <- R69

which seems wrong - it's copying the value in R48 into R48 again. but there's nothing that writes a value into R48!

But just looking at the code for decompose it's really not obvious to me why doing an extra pass over this basic block would mess things up so much for that three opcode sequence

 ldtoken_field R39 <- R36
 vmove R42 <- R33
 vmove R48 <- R39

any ideas?

[lambdageek]

Oh. Got it. It's a typo. It's always a typo

		if (!src_var)
			src_var = mono_compile_create_var_for_vreg (cfg, m_class_get_byval_arg (ins->klass), OP_LOCAL, ins->dreg);

		if (!dest_var)
			dest_var = mono_compile_create_var_for_vreg (cfg, m_class_get_byval_arg (ins->klass), OP_LOCAL, ins->dreg);

In the !src_var case, the last argument shouldn't be ins->dreg it should be ins->sreg1

[filipnavara]

Great find 😅

[rolfbjarne]

@lambdageek looks like this fix is not in .NET 8, only main (.NET 9) and .NET 8 rc1?

[rolfbjarne]

@lambdageek looks like this fix is not in .NET 8, only main (.NET 9) and .NET 8 rc1?

Ah nvm, I was looking for a PR into release/8.0, but it seems fixes in release/8.0-rc1 are automatically backported.