Runtime: Arm64 breakpoint causes floating corruption

[sdmaclea]

When adding a regression test for https://github.com/dotnet/coreclr/pull/28033, @hoyosjs reported the test failed on 5.0 linux-arm64, but not on 3.1 linux-arm64.

[msftbot[bot]]

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

[hoyosjs]

@sdmaclea I saw the issue in windows arm64

[sdmaclea]

Does it pass on 5.0 linux-arm64? Or do you know?

[sdmaclea]

At the moment I don't have access to a Windows-arm64 machine. If you have the JIT assembly for the failing function (where the breakpoint is set to corrupt the floating point), I can take a look and see if it is an uncovered pc relative data load case that we missed in the patch function.

The other possibility since this is Windows, would be checking we are properly saving/restoring the float regs on exceptions (including breakpoint exceptions).

[hoyosjs]

Does it pass on 5.0 linux-arm64? Or do you know?

I don't know. This wasn't running.

At the moment I don't have access to a Windows-arm64 machine. If you have the JIT assembly for the failing function (where the breakpoint is set to corrupt the floating point), I can take a look and see if it is an uncovered pc relative data load case that we missed in the patch function.

The other possibility since this is Windows, would be checking we are properly saving/restoring the float regs on exceptions (including breakpoint exceptions).

Yeah, that's the next step. Just haven't gotten around to it. Will try to get it running on the ARM64 devices in the office.

[hoyosjs]

This was the original sample program:

```c#
using System;

class BreakpointPatching
{
static int Main(string[] args)
{
const double value = 0.84551240822557006;
ThrowIfNotEquals(value, value); // breakpoint here
if (WrapDegrees(-722.5f) != 357.5) throw new Exception();
return 0;
}

private static void ThrowIfNotEquals(double a, double b) {
    if (a != b) throw new Exception();
}

private static float WrapDegrees(float degrees)
{
    float remainder = Math.Abs(degrees);
    remainder %= 360; // breakpoint here
    return degrees < 0 ? 360 - remainder : remainder;
}

}

<details>
<summary>IL</summary>

IL_0000: nop
IL_0001: ldc.r8 0.845512
IL_000a: ldc.r8 0.845512
IL_0013: call void BreakpointPatching::ThrowIfNotEquals(float64,float64)
IL_0018: nop
IL_0019: ldc.r4 -722.500000
IL_001e: call float32 BreakpointPatching::WrapDegrees(float32)
IL_0023: conv.r8
IL_0024: ldc.r8 357.500000
IL_002d: ceq
IL_002f: ldc.i4.0
IL_0030: ceq
IL_0032: stloc.0
IL_0033: ldloc.0
IL_0034: brfalse.s IL_003c
IL_0036: newobj void System.Exception::.ctor()
IL_003b: throw
IL_003c: ldc.i4.0
IL_003d: stloc.1
IL_003e: br.s IL_0040
IL_0040: ldloc.1
IL_0041: ret

</details>

breakpointPatching.cs @ 8:
ThrowIfNotEquals(value, value); // breakpoint here
00007ffb1e489db4 5c000420 ldr d0,breakpointPatching!BreakpointPatching.Main(System.String[])+0xb8 (00007ffb1e489e38)
00007ffb1e489db8 5c000481 ldr d1,breakpointPatching!BreakpointPatching.Main(System.String[])+0xc8 (00007ffb1e489e48)
00007ffb1e489dbc 97ffd7ab bl CLRStub[MethodDescPrestub]@7ffb1e47fc68 (00007ffb1e47fc68)
00007ffb`1e489dc0 d503201f nop

and I see that the data it will load in d0 and d1 is as expected:

0:000> dD 0x00007ffb1e489e38 L1
00007ffb1e489e38 0.845512408226 0:000> dD 0x00007ffb1e489e48 L1 00007ffb1e489e48 0.845512408226

After the patch we end up with:

00007ffb1e489db4 d43e0000 brk #0xF000 00007ffb1e489db8 5c000481 ldr d1,breakpointPatching!BreakpointPatching.Main(System.String[])+0xc8 (00007ffb1e489e48) 00007ffb1e489dbc 97ffd7ab bl CLRStub[MethodDescPrestub]@7ffb1e47fc68 (00007ffb`1e47fc68)

If I look at the patch I see this

0:000> dx (DebuggerControllerPatch)(DebuggerController::g_patches->m_pcEntries + DebuggerController::g_patches->m_iEntrySize * 3)
(DebuggerControllerPatch)(DebuggerController::g_patches->m_pcEntries + DebuggerController::g_patches->m_iEntrySize * 3) : 0x1bad8102348 [Type: DebuggerControllerPatch *]
[+0x000] entry [Type: FREEHASHENTRY]
[+0x010] controller : 0x1bad81176d0 [Type: DebuggerController *]
[+0x018] key [Type: DebuggerFunctionKey1]
[+0x028] offset : 0x34 [Type: unsigned __int64]
[+0x030] address : 0x7ffb1e489db4 : 0x0 [Type: unsigned char *]
[+0x038] fp [Type: FramePointer]
[+0x040] opcode : 1543504928 [Type: long]
[+0x044] fSaveOpcode : 0 [Type: int]
[+0x048] opcodeSaved : 0 [Type: long]
[+0x04c] offsetIsIL : 0 [Type: int]
[+0x050] trace [Type: TraceDestination]
[+0x070] pMethodDescFilter : 0x0 [Type: MethodDesc *]
[+0x078] refCount : 1 [Type: int]
[+0x080] encVersion : 0x1bad8119490 [Type: unsigned __int64]
[+0x080] dji : 0x1bad8119490 [Type: DebuggerJitInfo *]
[+0x088] m_pSharedPatchBypassBuffer : 0x0 [Type: SharedPatchBypassBuffer *]
[+0x090] pid : 0x6 [Type: unsigned __int64]
[+0x098] pAppDomain : 0x1bad80222e0 [Type: AppDomain *]
[+0x0a0] kind : PATCH_KIND_IL_SLAVE (1) [Type: DebuggerPatchKind]
```

1) The encVersion makes no sense, but that's a separate issue.
2) This is the correct address + offset + opcode (0x5c000420)

I see that once I enter the function, d0 is a trash value. I couldn't check the prologue because the repro died. I'll continue tomorrow.

[sdmaclea]

Thanks @hoyosjs that is a lot of great info. I think I understand everything except the patch stuff. What is the offset? I assume you meant 0x34 was the correct value for the offset.

[sdmaclea]

Looks like the root cause is the Windows arm64 debugger is missing support for PC relative instructions. Essentially this needs to be ported to arm64. https://github.com/dotnet/runtime/blob/3642deea8235ce8316db8c37cc3e8f4ad5b15380/src/coreclr/src/debug/ee/controller.cpp#L4375-L4418

[hoyosjs]

Offset is the native offset from the start of the jitted method.

This is the correct address + offset + opcode (0x5c000420)

This might have been a bit confusing, it was meant more of a list (offset 0x34 from method start, which corresponds to address 0x7ffb1e489db4, contained that opcode 5c000420). Given that it's an LDR, I wonder if we are running the patch with the wrong addressing mode (non-pc rel instead of PC rel). The other option is, as you say, checking that restoring the ARM64 context restores d0 and d1. I doubt the later though because it works correctly.

[hoyosjs]

Ah, beat me 😄

[hoyosjs]

Although this feels like it would do it, right? https://github.com/dotnet/runtime/blob/6072e4d3a7a2a1493f514cdf4be75a3d56580e84/src/coreclr/src/debug/ee/arm64/arm64walker.cpp#L94-L271

[sdmaclea]

I was thinking that wasn't called when FEATURE_EMULATE_SINGLESTEP was disabled, but I could be wrong. For emulate single step, I remember that being used instead of executing the instruction. It is possible that that code is not quite right for Windows...

[hoyosjs]

That part is just outside the single step guard, under arm64 only guards. However, NativeWalker::SetupOrSimulateInstructionForPatchSkip seems like it's not handling the case correctly. Doing an instrumented run.

[sdmaclea]

@hoyosjs Tracked this down to a incorrect implementation of arm64's GetMem() and GetSimdMem().

I am wondering if the solution might be to make the https://github.com/dotnet/runtime/blob/b21f0e5cfe925d18df6eac3d45f3bdd4553a5a67/src/coreclr/src/vm/arm64singlestepper.h#L102

Function a static method and just use that for both implementations. Maybe moving it to somewhere more rational.

[hoyosjs]

I thought of going a step further and sharing the implementation as much as possible. But right now what you are saying is more or less the plan of action for 5.0.