Skip to content

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

Closed
#90825
Closed
[Mono] RuntimeTypeHandle.Equals seems to be broken in Release mode#90800
#90825
Assignees
lambdageek
Labels
area-Codegen-JIT-monoruntime-monospecific to the Mono runtime
Milestone
8.0.0

Description

@simonrozsival
simonrozsival
opened on Aug 18, 2023

Description

A recent Xamarin iOS PR (dotnet/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

Activity

  1. simonrozsival
    added
    area-CoreLib-mono
    runtime-monospecific to the Mono runtime
    on Aug 18, 2023
  2. ghost added
    untriagedNew issue has not been triaged by the area owner
    on Aug 18, 2023

  3. ivanpovazan commented on Aug 18, 2023

    @ivanpovazan
    ivanpovazan
    on Aug 18, 2023
    Member

    /cc: @lambdageek

  4. ivanpovazan
    added this to the 8.0.0 milestone on Aug 18, 2023
  5. ghost removed
    untriagedNew issue has not been triaged by the area owner
    on Aug 18, 2023

  6. lambdageek commented on Aug 18, 2023

    @lambdageek
    lambdageek
    on Aug 18, 2023
    Member

    yikes

  7. lambdageek
    self-assigned this
    on Aug 18, 2023

  8. lambdageek commented on Aug 18, 2023

    @lambdageek
    lambdageek
    on Aug 18, 2023
    Member

    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

  9. simonrozsival commented on Aug 18, 2023

    @simonrozsival
    simonrozsival
    on Aug 18, 2023
    MemberAuthor

    @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.

  10. lambdageek commented on Aug 18, 2023

    @lambdageek
    lambdageek
    on Aug 18, 2023
    Member

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

  11. filipnavara
    added
    area-Codegen-JIT-mono
    and removed
    area-CoreLib-mono
    on Aug 18, 2023

  12. lambdageek commented on Aug 18, 2023

    @lambdageek
    lambdageek
    on Aug 18, 2023
    Member

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

  13. lambdageek commented on Aug 18, 2023

    @lambdageek
    lambdageek
    on Aug 18, 2023
    Member

    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
    }

  14. 2 remaining items

  15. lambdageek commented on Aug 18, 2023

    @lambdageek
    lambdageek
    on Aug 18, 2023
    Member

    @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?

  16. lambdageek commented on Aug 18, 2023

    @lambdageek
    lambdageek
    on Aug 18, 2023
    Member

    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

  17. filipnavara commented on Aug 18, 2023

    @filipnavara
    filipnavara
    on Aug 18, 2023
    Member

    Great find 😅

  18. lambdageek
    added a commit that references this issue on Aug 18, 2023

    Fix typo in mono_decompose_vtype_opts

    3bc8010
  19. ghost added
    in-prThere is an active PR which will close this issue when it is merged
    on Aug 18, 2023
  20. github-actions
    added a commit that references this issue on Aug 18, 2023

    Fix typo in mono_decompose_vtype_opts

    9822277
  21. lewing
    closed this as completedin #90825on Aug 19, 2023
  22. lewing
    added a commit that references this issue on Aug 19, 2023

    [mini] Fix typo in mono_decompose_vtype_opts (#90825)

    940b332
  23. ghost removed
    in-prThere is an active PR which will close this issue when it is merged
    on Aug 19, 2023
  24. carlossanlop
    added a commit that references this issue on Aug 19, 2023

    [release/8.0-rc1] [mini] Fix typo in mono_decompose_vtype_opts (#90832)

    6aef01d
  25. filipnavara
    added a commit that references this issue on Aug 19, 2023

    Add workaround for dotnet/runtime#90800

    959cef6

  26. rolfbjarne commented on Aug 22, 2023

    @rolfbjarne
    rolfbjarne
    on Aug 22, 2023
    Member

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

  27. rolfbjarne commented on Aug 22, 2023

    @rolfbjarne
    rolfbjarne
    on Aug 22, 2023
    Member

    @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.

  28. filipnavara
    added a commit that references this issue on Aug 25, 2023

    Revert "Add workaround for dotnet/runtime#90800"

    cdd9be2
  29. ghost locked as resolved and limited conversation to collaborators on Sep 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

area-Codegen-JIT-monoruntime-monospecific to the Mono runtime

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions