From 331a8dbae2e130a6faabf0f1dfcc686a27a825bc Mon Sep 17 00:00:00 2001 From: Max Charlamb Date: Wed, 5 Aug 2026 14:40:34 -0400 Subject: [PATCH 1/3] Make cDAC AMD64 unwinding breakpoint-aware Expose debugger patch metadata to cDAC and restore original breakpoint bytes on demand while decoding AMD64 epilogues. Fixes #112273 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ac4f2b2a-736b-4295-b586-50887eabfc60 --- docs/design/datacontracts/Debugger.md | 28 +++ .../data-descriptor-meanings.json | 7 + src/coreclr/debug/ee/controller.h | 18 +- .../vm/datadescriptor/datadescriptor.h | 2 + .../vm/datadescriptor/datadescriptor.inc | 18 ++ .../Contracts/IDebugger.cs | 1 + .../Constants.cs | 2 + .../Contracts/Debugger/Debugger_1.cs | 71 +++++- .../StackWalk/Context/AMD64/AMD64Unwinder.cs | 4 +- .../Data/DebuggerControllerPatch.cs | 11 + .../Data/DebuggerPatchTable.cs | 11 + .../DataType.cs | 2 + .../cdac/tests/UnitTests/DebuggerTests.cs | 211 +++++++++++++++++- 13 files changed, 380 insertions(+), 6 deletions(-) create mode 100644 src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/DebuggerControllerPatch.cs create mode 100644 src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/DebuggerPatchTable.cs diff --git a/docs/design/datacontracts/Debugger.md b/docs/design/datacontracts/Debugger.md index 0d5d8e61dc42d1..a3c551298ba92b 100644 --- a/docs/design/datacontracts/Debugger.md +++ b/docs/design/datacontracts/Debugger.md @@ -28,6 +28,7 @@ void SetSendExceptionsOutsideOfJMC(bool sendExceptionsOutsideOfJMC); TargetPointer GetDebuggerControlBlockAddress(); void EnableGCNotificationEvents(bool fEnable); HijackKind GetHijackKind(TargetCodePointer controlPC); +byte ReadInstructionByte(TargetPointer address); TargetPointer PrepareExceptionHijack(byte[] context, TargetPointer vmThread, byte[]? exceptionRecord, int reason, TargetPointer userData) ``` @@ -44,6 +45,11 @@ TargetPointer PrepareExceptionHijack(byte[] context, TargetPointer vmThread, byt | `Debugger` | `RgHijackFunction` | `pointer` | Pointer to the runtime's array of hijack-stub address ranges. | | `Debugger` | `RSRequestedSync` | `int32` | Sync-at-event request flag | | `Debugger` | `SendExceptionsOutsideOfJMC` | `int32` | Exception delivery policy flag | +| `DebuggerControllerPatch` | *(type size)* | `uint32` | Size in bytes of each DebuggerControllerPatch entry. Only available on AMD64. | +| `DebuggerControllerPatch` | `Address` | `pointer` | Address patched with a debugger breakpoint, or null for an inactive entry. Only available on AMD64. | +| `DebuggerControllerPatch` | `Opcode` | `nuint` | Original instruction value replaced by the debugger breakpoint. Only available on AMD64. | +| `DebuggerPatchTable` | `Count` | `uint32` | Capacity of the DebuggerControllerPatch backing array. Only available on AMD64. | +| `DebuggerPatchTable` | `Entries` | `pointer` | Pointer to the backing array of DebuggerControllerPatch entries. Only available on AMD64. | | `DebuggerRCThread` | `DCB` | `pointer` | Pointer to DebuggerIPCControlBlock | | `MemoryRange` | *(type size)* | `uint32` | Size of the data descriptor layout | | `MemoryRange` | `Size` | `nuint` | Size of the range in bytes; the range covers [StartAddress, StartAddress + Size) | @@ -56,6 +62,8 @@ TargetPointer PrepareExceptionHijack(byte[] context, TargetPointer vmThread, byt | `CLRJitAttachState` | `pointer` | Pointer to the CLR JIT attach state flags | | `CORDebuggerControlFlags` | `pointer` | Pointer to g_CORDebuggerControlFlags | | `Debugger` | `pointer` | Address of the pointer to the Debugger instance (&g_pDebugger) | +| `DebuggerPatchTable` | `pointer` | Address of the pointer to the debugger breakpoint patch table. Only available on AMD64. | +| `DebuggerPatchTableValid` | `pointer` | Pointer to the flag indicating whether the debugger patch table can be inspected. Only available on AMD64. | | `MaxHijackFunctions` | `uint32` | Number of entries in the hijack function array. | | `MetadataUpdatesApplied` | `pointer` | Pointer to the g_metadataUpdatesApplied flag | @@ -195,6 +203,26 @@ HijackKind GetHijackKind(TargetCodePointer controlPC) return HijackKind.None; } +byte ReadInstructionByte(TargetPointer address) +{ + if (!target.TryReadGlobalPointer("DebuggerPatchTableValid", out TargetPointer patchTableValidAddress)) + return target.Read(address); + + if (target.Read(patchTableValidAddress) == 0) + return target.Read(address); + + Dictionary patches = + cachedPatches ??= ReadActivePatches(); + if (patches.TryGetValue(address, out byte opcode)) + return opcode; + + return target.Read(address); +} + +// ReadActivePatches reads the patch table global and creates an address-indexed +// map from backing-array entries with nonzero addresses and opcodes. Clear +// cachedPatches for every contract flush scope. + private TargetPointer GetHijackAddress() { // Returns the start address of the unhandled-exception hijack function diff --git a/docs/design/datacontracts/data-descriptor-meanings.json b/docs/design/datacontracts/data-descriptor-meanings.json index 8b43a93d8ae884..f246daba6a8c03 100644 --- a/docs/design/datacontracts/data-descriptor-meanings.json +++ b/docs/design/datacontracts/data-descriptor-meanings.json @@ -85,6 +85,11 @@ "DebuggerEval.EvalUsesHijack": "Flag used in processing FuncEvalFrame", "DebuggerEval.MethodToken": "Metadata token of the method being evaluated", "DebuggerEval.TargetContext": "Context saved inside DebuggerEval", + "DebuggerControllerPatch.Address": "Address patched with a debugger breakpoint, or null for an inactive entry. Only available on AMD64.", + "DebuggerControllerPatch.Opcode": "Original instruction value replaced by the debugger breakpoint. Only available on AMD64.", + "DebuggerControllerPatch.Size": "Size in bytes of each DebuggerControllerPatch entry. Only available on AMD64.", + "DebuggerPatchTable.Count": "Capacity of the DebuggerControllerPatch backing array. Only available on AMD64.", + "DebuggerPatchTable.Entries": "Pointer to the backing array of DebuggerControllerPatch entries. Only available on AMD64.", "DebuggerRCThread.DCB": "Pointer to DebuggerIPCControlBlock", "Delegate.ExtraData": "Invocation count for multicast, UnmanagedMarker for unmanaged, MethodDesc otherwise", "Delegate.HelperObject": "Invocation list for multicast, MethodInfo otherwise", @@ -731,6 +736,8 @@ "CurrentGCState": "c_gc_state enum value. Only available when GCIdentifiers contains background.", "DebugDestroyedHandleValue": "Sentinel handle value used for destroyed handles", "Debugger": "Address of the pointer to the Debugger instance (&g_pDebugger)", + "DebuggerPatchTable": "Address of the pointer to the debugger breakpoint patch table. Only available on AMD64.", + "DebuggerPatchTableValid": "Pointer to the flag indicating whether the debugger patch table can be inspected. Only available on AMD64.", "DispatchThisPtrMask": "Used to mask low bits of CCW pointer to the nearest valid address from which to read a managed object wrapper", "DynamicAdaptationMode": "GC heap dynamic adaptation mode. Only available when GCIdentifiers contains dynamic_heap.", "EEConfig": "Pointer to the runtime configuration", diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index 21052da20b7010..4577d97cb303d7 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/src/coreclr/debug/ee/controller.h @@ -668,6 +668,8 @@ class DebuggerPatchTable : private CHashTableAndData { VPTR_BASE_CONCRETE_VTABLE_CLASS(DebuggerPatchTable); + friend struct ::cdac_data; + public: virtual ~DebuggerPatchTable() = default; @@ -922,6 +924,13 @@ class DebuggerPatchTable : private CHashTableAndData int GetNumberOfPatches(); }; +template<> +struct cdac_data +{ + static constexpr size_t Entries = offsetof(DebuggerPatchTable, m_pcEntries); + static constexpr size_t Count = offsetof(DebuggerPatchTable, m_iEntries); +}; + typedef VPTR(class DebuggerPatchTable) PTR_DebuggerPatchTable; @@ -1055,6 +1064,7 @@ inline void VerifyExecutableAddress(const BYTE* address) class DebuggerController { VPTR_BASE_CONCRETE_VTABLE_CLASS(DebuggerController); + friend struct ::cdac_data; #if !defined(DACCESS_COMPILE) @@ -1496,9 +1506,15 @@ class DebuggerController #endif // !DACCESS_COMPILE }; - #if !defined(DACCESS_COMPILE) +template<> +struct cdac_data +{ + static constexpr DebuggerPatchTable **PatchTable = &DebuggerController::g_patches; + static constexpr BOOL *PatchTableValid = &DebuggerController::g_patchTableValid; +}; + // this structure stores useful information about single-stepping over a call instruction // it is used to communicate the patch skip opcode and current state between the controller on left side and HandleSetThreadContextNeeded on the right side class DebuggerSteppingInfo diff --git a/src/coreclr/vm/datadescriptor/datadescriptor.h b/src/coreclr/vm/datadescriptor/datadescriptor.h index 297a073fe58bd2..c9628b93bd94c2 100644 --- a/src/coreclr/vm/datadescriptor/datadescriptor.h +++ b/src/coreclr/vm/datadescriptor/datadescriptor.h @@ -30,6 +30,8 @@ #include "virtualcallstub.h" #include "../debug/ee/debugger.h" +#include "../debug/ee/walker.h" +#include "../debug/ee/controller.h" #include "patchpointinfo.h" #ifdef HAVE_GCCOVER diff --git a/src/coreclr/vm/datadescriptor/datadescriptor.inc b/src/coreclr/vm/datadescriptor/datadescriptor.inc index 6242f9f49da888..467c11aaadd10d 100644 --- a/src/coreclr/vm/datadescriptor/datadescriptor.inc +++ b/src/coreclr/vm/datadescriptor/datadescriptor.inc @@ -506,6 +506,20 @@ CDAC_TYPE_INDETERMINATE(DebuggerRCThread) CDAC_TYPE_FIELD(DebuggerRCThread, T_POINTER, DCB, cdac_data::DCB) CDAC_TYPE_END(DebuggerRCThread) +#if defined(TARGET_AMD64) +CDAC_TYPE_BEGIN(DebuggerPatchTable) +CDAC_TYPE_INDETERMINATE(DebuggerPatchTable) +CDAC_TYPE_FIELD(DebuggerPatchTable, T_POINTER, Entries, cdac_data::Entries) +CDAC_TYPE_FIELD(DebuggerPatchTable, T_UINT32, Count, cdac_data::Count) +CDAC_TYPE_END(DebuggerPatchTable) + +CDAC_TYPE_BEGIN(DebuggerControllerPatch) +CDAC_TYPE_SIZE(sizeof(DebuggerControllerPatch)) +CDAC_TYPE_FIELD(DebuggerControllerPatch, T_POINTER, Address, offsetof(DebuggerControllerPatch, address)) +CDAC_TYPE_FIELD(DebuggerControllerPatch, T_NUINT, Opcode, offsetof(DebuggerControllerPatch, opcode)) +CDAC_TYPE_END(DebuggerControllerPatch) +#endif // TARGET_AMD64 + CDAC_TYPE_BEGIN(MemoryRange) CDAC_TYPE_SIZE(sizeof(MemoryRange)) CDAC_TYPE_FIELD(MemoryRange, T_POINTER, StartAddress, cdac_data::StartAddress) @@ -1690,6 +1704,10 @@ CDAC_GLOBAL_POINTER(ThePreStub, &g_cdacThePreStub) #endif // !FEATURE_PORTABLE_ENTRYPOINTS #if defined(DEBUGGING_SUPPORTED) && !defined(TARGET_WASM) CDAC_GLOBAL_POINTER(Debugger, &::g_pDebugger) +#if defined(TARGET_AMD64) +CDAC_GLOBAL_POINTER(DebuggerPatchTable, cdac_data::PatchTable) +CDAC_GLOBAL_POINTER(DebuggerPatchTableValid, cdac_data::PatchTableValid) +#endif // TARGET_AMD64 CDAC_GLOBAL_POINTER(CLRJitAttachState, &::CLRJitAttachState) CDAC_GLOBAL_POINTER(CORDebuggerControlFlags, &::g_CORDebuggerControlFlags) CDAC_GLOBAL(MaxHijackFunctions, T_UINT32, cdac_data::MaxHijackFunctions) diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs index 2803be014c423c..733a4b0c042c3e 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IDebugger.cs @@ -28,6 +28,7 @@ public interface IDebugger : IContract TargetPointer GetDebuggerControlBlockAddress() => throw new NotImplementedException(); void EnableGCNotificationEvents(bool fEnable) => throw new NotImplementedException(); HijackKind GetHijackKind(TargetCodePointer controlPC) => throw new NotImplementedException(); + byte ReadInstructionByte(TargetPointer address) => throw new NotImplementedException(); TargetPointer PrepareExceptionHijack(byte[] context, TargetPointer vmThread, byte[]? exceptionRecord, int reason, TargetPointer userData) => throw new NotImplementedException(); } diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs index afce81bbf1c224..f1d06315a77a49 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs @@ -15,6 +15,8 @@ public static class Globals public const string FunctionTableIndexRangeList = nameof(FunctionTableIndexRangeList); public const string GCThread = nameof(GCThread); public const string Debugger = nameof(Debugger); + public const string DebuggerPatchTable = nameof(DebuggerPatchTable); + public const string DebuggerPatchTableValid = nameof(DebuggerPatchTableValid); public const string MaxHijackFunctions = nameof(MaxHijackFunctions); public const string CLRJitAttachState = nameof(CLRJitAttachState); public const string CORDebuggerControlFlags = nameof(CORDebuggerControlFlags); diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs index 9fa0c75a7b36ab..f99c041b9075de 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs @@ -3,6 +3,7 @@ using System; using System.Buffers.Binary; +using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Runtime.InteropServices; @@ -10,7 +11,7 @@ namespace Microsoft.Diagnostics.DataContractReader.Contracts; -internal readonly struct Debugger_1 : IDebugger +internal sealed class Debugger_1 : IDebugger { private enum DebuggerControlFlag_1 : uint { @@ -20,12 +21,18 @@ private enum DebuggerControlFlag_1 : uint private const uint UnhandledExceptionHijackIndex = 0; private readonly Target _target; + private Dictionary? _patches; internal Debugger_1(Target target) { _target = target; } + public void Flush(FlushScope scope) + { + _patches = null; + } + private bool TryGetDebuggerAddress(out TargetPointer debuggerAddress) { debuggerAddress = TargetPointer.Null; @@ -156,6 +163,68 @@ HijackKind IDebugger.GetHijackKind(TargetCodePointer controlPC) return HijackKind.None; } + byte IDebugger.ReadInstructionByte(TargetPointer address) + { + try + { + if (!_target.TryReadGlobalPointer(Constants.Globals.DebuggerPatchTableValid, out TargetPointer? patchTableValidAddress)) + return _target.Read(address); + + if (_target.Read(patchTableValidAddress.Value) == 0) + return _target.Read(address); + + Dictionary patches = GetPatches(); + if (patches.TryGetValue(address, out byte opcode)) + { + return opcode; + } + } + catch (VirtualReadException) + { + // Patch metadata is optional. Fall back to reading the instruction directly. + } + + return _target.Read(address); + } + + private Dictionary GetPatches() + { + Dictionary? patches = _patches; + if (patches is not null) + return patches; + + return _patches ??= ReadPatches(); + } + + private Dictionary ReadPatches() + { + if (!_target.TryReadGlobalPointer(Constants.Globals.DebuggerPatchTable, out TargetPointer? patchTablePointerAddress)) + return []; + + TargetPointer patchTableAddress = _target.ReadPointer(patchTablePointerAddress.Value); + if (patchTableAddress == TargetPointer.Null) + return []; + + Data.DebuggerPatchTable patchTable = _target.ProcessedData.GetOrAdd(patchTableAddress); + if (patchTable.Entries == TargetPointer.Null) + return []; + + Dictionary patches = []; + uint patchSize = Data.DebuggerControllerPatch.GetSize(_target); + + for (uint i = 0; i < patchTable.Count; i++) + { + TargetPointer patchAddress = patchTable.Entries + ((ulong)i * patchSize); + Data.DebuggerControllerPatch patch = _target.ProcessedData.GetOrAdd(patchAddress); + if (patch.CodeAddress != TargetPointer.Null && patch.Opcode.Value != 0) + { + patches[patch.CodeAddress] = (byte)patch.Opcode.Value; + } + } + + return patches; + } + private TargetPointer GetHijackAddress() { return TryGetHijackFunctionRange(UnhandledExceptionHijackIndex, out Data.MemoryRange? range) diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/AMD64/AMD64Unwinder.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/AMD64/AMD64Unwinder.cs index bf76953355416e..a823719c509bde 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/AMD64/AMD64Unwinder.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/Context/AMD64/AMD64Unwinder.cs @@ -174,6 +174,8 @@ public bool Unwind(ref AMD64Context context) if (unwindInfo.Version < 2) { + byte ReadByteAt(TargetPointer address) => _target.Contracts.Debugger.ReadInstructionByte(address); + TargetPointer nextByte = controlPC; // @@ -1263,8 +1265,6 @@ private Data.RuntimeFunction LookupPrimaryFunctionEntry(Data.RuntimeFunction fun #endregion #region Helpers - private byte ReadByteAt(TargetPointer address) => _target.Read(address); - private static bool IsRexPrefix(byte b) => (b & 0xf0) == 0x40; private static TargetPointer GetRegister(AMD64Context context, byte register) diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/DebuggerControllerPatch.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/DebuggerControllerPatch.cs new file mode 100644 index 00000000000000..ae4198e11ebb81 --- /dev/null +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/DebuggerControllerPatch.cs @@ -0,0 +1,11 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.Diagnostics.DataContractReader.Data; + +[CdacType(nameof(DataType.DebuggerControllerPatch))] +internal sealed partial class DebuggerControllerPatch : IData +{ + [Field("Address")] public partial TargetPointer CodeAddress { get; } + [Field] public partial TargetNUInt Opcode { get; } +} diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/DebuggerPatchTable.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/DebuggerPatchTable.cs new file mode 100644 index 00000000000000..b3e548ca1c31a2 --- /dev/null +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/DebuggerPatchTable.cs @@ -0,0 +1,11 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace Microsoft.Diagnostics.DataContractReader.Data; + +[CdacType(nameof(DataType.DebuggerPatchTable))] +internal sealed partial class DebuggerPatchTable : IData +{ + [Field] public partial TargetPointer Entries { get; } + [Field] public partial uint Count { get; } +} diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/DataType.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/DataType.cs index be5194e0a537a6..3f3ba14eda8e19 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/DataType.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/DataType.cs @@ -45,6 +45,8 @@ public enum DataType AppDomain, Debugger, DebuggerRCThread, + DebuggerPatchTable, + DebuggerControllerPatch, MemoryRange, SystemDomain, Assembly, diff --git a/src/native/managed/cdac/tests/UnitTests/DebuggerTests.cs b/src/native/managed/cdac/tests/UnitTests/DebuggerTests.cs index 69bbe906ae21e1..7a52d471c28955 100644 --- a/src/native/managed/cdac/tests/UnitTests/DebuggerTests.cs +++ b/src/native/managed/cdac/tests/UnitTests/DebuggerTests.cs @@ -118,10 +118,12 @@ private static TestPlaceholderTarget BuildTarget( return builder.Build(); } - private static TestPlaceholderTarget BuildNullDebuggerTarget(MockTarget.Architecture arch) + private static TestPlaceholderTarget BuildNullDebuggerTarget( + MockTarget.Architecture arch, + (ulong Address, byte[] Data)? memory = null) { TargetTestHelpers helpers = new(arch); - var builder = new TestPlaceholderTarget.Builder(arch); + TestPlaceholderTarget.Builder builder = new(arch); MockMemorySpace.Builder memBuilder = builder.MemoryBuilder; MockMemorySpace.BumpAllocator allocator = memBuilder.CreateAllocator(0x1_0000, 0x2_0000); @@ -131,6 +133,16 @@ private static TestPlaceholderTarget BuildNullDebuggerTarget(MockTarget.Architec builder.AddGlobals((Constants.Globals.Debugger, debuggerPtrFrag.Address)); builder.AddContract(version: "c1"); + if (memory is not null) + { + memBuilder.AddHeapFragment(new MockMemorySpace.HeapFragment + { + Address = memory.Value.Address, + Data = memory.Value.Data, + Name = "Target memory", + }); + } + return builder.Build(); } @@ -510,6 +522,201 @@ public void GetHijackKind_ReturnsNoneWhenTableEmpty(MockTarget.Architecture arch Assert.Equal(HijackKind.None, debugger.GetHijackKind(new TargetCodePointer(0x10_0080))); } + // ----------------------------------------------------------------------- + // ReadInstructionByte + // ----------------------------------------------------------------------- + + private static TestPlaceholderTarget BuildTargetWithPatchTable( + MockTarget.Architecture arch, + bool patchTableValid, + (ulong Address, ulong Opcode)[] patches, + (ulong Address, byte[] Data)? memory = null, + bool patchTableValidReadable = true) + { + TargetTestHelpers helpers = new(arch); + TestPlaceholderTarget.Builder builder = new(arch); + MockMemorySpace.BumpAllocator allocator = builder.MemoryBuilder.CreateAllocator(0x1_0000, 0x10_0000); + + TargetTestHelpers.LayoutResult patchTableLayout = helpers.LayoutFields( + [ + new(nameof(Data.DebuggerPatchTable.Entries), DataType.pointer), + new(nameof(Data.DebuggerPatchTable.Count), DataType.uint32), + ]); + TargetTestHelpers.LayoutResult patchLayout = helpers.LayoutFields( + [ + new("Address", DataType.pointer), + new(nameof(Data.DebuggerControllerPatch.Opcode), DataType.nuint), + ]); + builder.AddTypes(new Dictionary + { + [DataType.DebuggerPatchTable] = new() { Fields = patchTableLayout.Fields, Size = patchTableLayout.Stride }, + [DataType.DebuggerControllerPatch] = new() { Fields = patchLayout.Fields, Size = patchLayout.Stride }, + }); + + MockMemorySpace.HeapFragment entriesFragment = allocator.Allocate( + (ulong)patches.Length * patchLayout.Stride, + "DebuggerControllerPatch entries"); + int addressOffset = patchLayout.Fields["Address"].Offset; + int opcodeOffset = patchLayout.Fields[nameof(Data.DebuggerControllerPatch.Opcode)].Offset; + for (int i = 0; i < patches.Length; i++) + { + int entryOffset = i * (int)patchLayout.Stride; + helpers.WritePointer( + entriesFragment.Data.AsSpan(entryOffset + addressOffset, helpers.PointerSize), + patches[i].Address); + helpers.WriteNUInt( + entriesFragment.Data.AsSpan(entryOffset + opcodeOffset, helpers.PointerSize), + new TargetNUInt(patches[i].Opcode)); + } + + MockMemorySpace.HeapFragment patchTableFragment = allocator.Allocate(patchTableLayout.Stride, "DebuggerPatchTable"); + helpers.WritePointer( + patchTableFragment.Data.AsSpan( + patchTableLayout.Fields[nameof(Data.DebuggerPatchTable.Entries)].Offset, + helpers.PointerSize), + entriesFragment.Address); + helpers.Write( + patchTableFragment.Data.AsSpan( + patchTableLayout.Fields[nameof(Data.DebuggerPatchTable.Count)].Offset, + sizeof(uint)), + (uint)patches.Length); + + MockMemorySpace.HeapFragment patchTablePointerFragment = allocator.Allocate((ulong)helpers.PointerSize, "g_patches"); + helpers.WritePointer(patchTablePointerFragment.Data, patchTableFragment.Address); + + ulong patchTableValidAddress = 0x60_0000; + if (patchTableValidReadable) + { + MockMemorySpace.HeapFragment patchTableValidFragment = allocator.Allocate(sizeof(int), "g_patchTableValid"); + helpers.Write( + patchTableValidFragment.Data.AsSpan(0, sizeof(int)), + patchTableValid ? 1 : 0); + patchTableValidAddress = patchTableValidFragment.Address; + } + + builder.AddGlobals( + (Constants.Globals.DebuggerPatchTable, patchTablePointerFragment.Address), + (Constants.Globals.DebuggerPatchTableValid, patchTableValidAddress)); + builder.AddContract(version: "c1"); + + if (memory is not null) + { + builder.MemoryBuilder.AddHeapFragment(new MockMemorySpace.HeapFragment + { + Address = memory.Value.Address, + Data = memory.Value.Data, + Name = "Target memory", + }); + } + + return builder.Build(); + } + + [Theory] + [ClassData(typeof(MockTarget.StdArch))] + public void ReadInstructionByte_ReturnsOriginalOpcodeWithoutReadingTargetMemory(MockTarget.Architecture arch) + { + const ulong InstructionAddress = 0x50_0000; + TestPlaceholderTarget target = BuildTargetWithPatchTable( + arch, + patchTableValid: true, + [(InstructionAddress, 0x1_C3)]); + + byte instruction = target.Contracts.Debugger.ReadInstructionByte(InstructionAddress); + + Assert.Equal(0xC3, instruction); + } + + [Theory] + [ClassData(typeof(MockTarget.StdArch))] + public void ReadInstructionByte_ReadsTargetMemoryWhenAddressIsNotPatched(MockTarget.Architecture arch) + { + const ulong InstructionAddress = 0x50_0000; + TestPlaceholderTarget target = BuildTargetWithPatchTable( + arch, + patchTableValid: true, + [(InstructionAddress + 1, 0x90)], + (InstructionAddress, [0x41])); + + byte instruction = target.Contracts.Debugger.ReadInstructionByte(InstructionAddress); + + Assert.Equal(0x41, instruction); + } + + [Theory] + [ClassData(typeof(MockTarget.StdArch))] + public void ReadInstructionByte_ReadsTargetMemoryWhenPatchTableIsInvalid(MockTarget.Architecture arch) + { + const ulong InstructionAddress = 0x50_0000; + TestPlaceholderTarget target = BuildTargetWithPatchTable( + arch, + patchTableValid: false, + [(InstructionAddress, 0x90)], + (InstructionAddress, [0xCC])); + + byte instruction = target.Contracts.Debugger.ReadInstructionByte(InstructionAddress); + + Assert.Equal(0xCC, instruction); + } + + [Theory] + [ClassData(typeof(MockTarget.StdArch))] + public void ReadInstructionByte_ReadsTargetMemoryWhenPatchTableGlobalsAreUnavailable(MockTarget.Architecture arch) + { + const ulong InstructionAddress = 0x50_0000; + TestPlaceholderTarget target = BuildNullDebuggerTarget( + arch, + (InstructionAddress, [0x41])); + + byte instruction = target.Contracts.Debugger.ReadInstructionByte(InstructionAddress); + + Assert.Equal(0x41, instruction); + } + + [Theory] + [ClassData(typeof(MockTarget.StdArch))] + public void ReadInstructionByte_ReadsTargetMemoryWhenPatchMetadataIsUnavailable(MockTarget.Architecture arch) + { + const ulong InstructionAddress = 0x50_0000; + TestPlaceholderTarget target = BuildTargetWithPatchTable( + arch, + patchTableValid: true, + [(0, 0)], + (InstructionAddress, [0x41]), + patchTableValidReadable: false); + + byte instruction = target.Contracts.Debugger.ReadInstructionByte(InstructionAddress); + + Assert.Equal(0x41, instruction); + } + + [Theory] + [ClassData(typeof(MockTarget.StdArch))] + public void ReadInstructionByte_RefreshesCachedPatchesAfterFlush(MockTarget.Architecture arch) + { + const ulong InstructionAddress = 0x50_0000; + TestPlaceholderTarget target = BuildTargetWithPatchTable( + arch, + patchTableValid: true, + [(InstructionAddress, 0x90)]); + IDebugger debugger = target.Contracts.Debugger; + + Assert.Equal(0x90, debugger.ReadInstructionByte(InstructionAddress)); + + TargetPointer patchTablePointerAddress = target.ReadGlobalPointer(Constants.Globals.DebuggerPatchTable); + TargetPointer patchTableAddress = target.ReadPointer(patchTablePointerAddress); + Data.DebuggerPatchTable patchTable = target.ProcessedData.GetOrAdd(patchTableAddress); + int opcodeOffset = target.GetTypeInfo(DataType.DebuggerControllerPatch).Fields["Opcode"].Offset; + target.WriteNUInt(patchTable.Entries.Value + (ulong)opcodeOffset, new TargetNUInt(0xC3)); + + Assert.Equal(0x90, debugger.ReadInstructionByte(InstructionAddress)); + + target.Flush(FlushScope.ForwardExecution); + debugger.Flush(FlushScope.ForwardExecution); + + Assert.Equal(0xC3, debugger.ReadInstructionByte(InstructionAddress)); + } + // ----------------------------------------------------------------------- // PrepareExceptionHijack // ----------------------------------------------------------------------- From b24958f45c2f2b031ca97454483cbd30aba3a6a5 Mon Sep 17 00:00:00 2001 From: Max Charlamb Date: Wed, 5 Aug 2026 15:32:06 -0400 Subject: [PATCH 2/3] Simplify cDAC patch table discovery Use the published patch table pointer as the availability signal instead of exposing the legacy patch-table-valid flag. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ac4f2b2a-736b-4295-b586-50887eabfc60 --- docs/design/datacontracts/Debugger.md | 7 ---- .../data-descriptor-meanings.json | 1 - src/coreclr/debug/ee/controller.h | 1 - .../vm/datadescriptor/datadescriptor.inc | 1 - .../Constants.cs | 1 - .../Contracts/Debugger/Debugger_1.cs | 6 --- .../cdac/tests/UnitTests/DebuggerTests.cs | 37 +++++++------------ 7 files changed, 14 insertions(+), 40 deletions(-) diff --git a/docs/design/datacontracts/Debugger.md b/docs/design/datacontracts/Debugger.md index a3c551298ba92b..7d54c4ec32f1a7 100644 --- a/docs/design/datacontracts/Debugger.md +++ b/docs/design/datacontracts/Debugger.md @@ -63,7 +63,6 @@ TargetPointer PrepareExceptionHijack(byte[] context, TargetPointer vmThread, byt | `CORDebuggerControlFlags` | `pointer` | Pointer to g_CORDebuggerControlFlags | | `Debugger` | `pointer` | Address of the pointer to the Debugger instance (&g_pDebugger) | | `DebuggerPatchTable` | `pointer` | Address of the pointer to the debugger breakpoint patch table. Only available on AMD64. | -| `DebuggerPatchTableValid` | `pointer` | Pointer to the flag indicating whether the debugger patch table can be inspected. Only available on AMD64. | | `MaxHijackFunctions` | `uint32` | Number of entries in the hijack function array. | | `MetadataUpdatesApplied` | `pointer` | Pointer to the g_metadataUpdatesApplied flag | @@ -205,12 +204,6 @@ HijackKind GetHijackKind(TargetCodePointer controlPC) byte ReadInstructionByte(TargetPointer address) { - if (!target.TryReadGlobalPointer("DebuggerPatchTableValid", out TargetPointer patchTableValidAddress)) - return target.Read(address); - - if (target.Read(patchTableValidAddress) == 0) - return target.Read(address); - Dictionary patches = cachedPatches ??= ReadActivePatches(); if (patches.TryGetValue(address, out byte opcode)) diff --git a/docs/design/datacontracts/data-descriptor-meanings.json b/docs/design/datacontracts/data-descriptor-meanings.json index f246daba6a8c03..124bfbb592ebf8 100644 --- a/docs/design/datacontracts/data-descriptor-meanings.json +++ b/docs/design/datacontracts/data-descriptor-meanings.json @@ -737,7 +737,6 @@ "DebugDestroyedHandleValue": "Sentinel handle value used for destroyed handles", "Debugger": "Address of the pointer to the Debugger instance (&g_pDebugger)", "DebuggerPatchTable": "Address of the pointer to the debugger breakpoint patch table. Only available on AMD64.", - "DebuggerPatchTableValid": "Pointer to the flag indicating whether the debugger patch table can be inspected. Only available on AMD64.", "DispatchThisPtrMask": "Used to mask low bits of CCW pointer to the nearest valid address from which to read a managed object wrapper", "DynamicAdaptationMode": "GC heap dynamic adaptation mode. Only available when GCIdentifiers contains dynamic_heap.", "EEConfig": "Pointer to the runtime configuration", diff --git a/src/coreclr/debug/ee/controller.h b/src/coreclr/debug/ee/controller.h index 4577d97cb303d7..4b9ec42a7ac02d 100644 --- a/src/coreclr/debug/ee/controller.h +++ b/src/coreclr/debug/ee/controller.h @@ -1512,7 +1512,6 @@ template<> struct cdac_data { static constexpr DebuggerPatchTable **PatchTable = &DebuggerController::g_patches; - static constexpr BOOL *PatchTableValid = &DebuggerController::g_patchTableValid; }; // this structure stores useful information about single-stepping over a call instruction diff --git a/src/coreclr/vm/datadescriptor/datadescriptor.inc b/src/coreclr/vm/datadescriptor/datadescriptor.inc index 467c11aaadd10d..43e2d3f17249ee 100644 --- a/src/coreclr/vm/datadescriptor/datadescriptor.inc +++ b/src/coreclr/vm/datadescriptor/datadescriptor.inc @@ -1706,7 +1706,6 @@ CDAC_GLOBAL_POINTER(ThePreStub, &g_cdacThePreStub) CDAC_GLOBAL_POINTER(Debugger, &::g_pDebugger) #if defined(TARGET_AMD64) CDAC_GLOBAL_POINTER(DebuggerPatchTable, cdac_data::PatchTable) -CDAC_GLOBAL_POINTER(DebuggerPatchTableValid, cdac_data::PatchTableValid) #endif // TARGET_AMD64 CDAC_GLOBAL_POINTER(CLRJitAttachState, &::CLRJitAttachState) CDAC_GLOBAL_POINTER(CORDebuggerControlFlags, &::g_CORDebuggerControlFlags) diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs index f1d06315a77a49..16466acb8e9660 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Constants.cs @@ -16,7 +16,6 @@ public static class Globals public const string GCThread = nameof(GCThread); public const string Debugger = nameof(Debugger); public const string DebuggerPatchTable = nameof(DebuggerPatchTable); - public const string DebuggerPatchTableValid = nameof(DebuggerPatchTableValid); public const string MaxHijackFunctions = nameof(MaxHijackFunctions); public const string CLRJitAttachState = nameof(CLRJitAttachState); public const string CORDebuggerControlFlags = nameof(CORDebuggerControlFlags); diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs index f99c041b9075de..0bd15dcdfc1ba7 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs @@ -167,12 +167,6 @@ byte IDebugger.ReadInstructionByte(TargetPointer address) { try { - if (!_target.TryReadGlobalPointer(Constants.Globals.DebuggerPatchTableValid, out TargetPointer? patchTableValidAddress)) - return _target.Read(address); - - if (_target.Read(patchTableValidAddress.Value) == 0) - return _target.Read(address); - Dictionary patches = GetPatches(); if (patches.TryGetValue(address, out byte opcode)) { diff --git a/src/native/managed/cdac/tests/UnitTests/DebuggerTests.cs b/src/native/managed/cdac/tests/UnitTests/DebuggerTests.cs index 7a52d471c28955..75bf4ba23c2dbb 100644 --- a/src/native/managed/cdac/tests/UnitTests/DebuggerTests.cs +++ b/src/native/managed/cdac/tests/UnitTests/DebuggerTests.cs @@ -528,10 +528,10 @@ public void GetHijackKind_ReturnsNoneWhenTableEmpty(MockTarget.Architecture arch private static TestPlaceholderTarget BuildTargetWithPatchTable( MockTarget.Architecture arch, - bool patchTableValid, (ulong Address, ulong Opcode)[] patches, (ulong Address, byte[] Data)? memory = null, - bool patchTableValidReadable = true) + bool patchTableAvailable = true, + bool patchTablePointerReadable = true) { TargetTestHelpers helpers = new(arch); TestPlaceholderTarget.Builder builder = new(arch); @@ -581,22 +581,17 @@ private static TestPlaceholderTarget BuildTargetWithPatchTable( sizeof(uint)), (uint)patches.Length); - MockMemorySpace.HeapFragment patchTablePointerFragment = allocator.Allocate((ulong)helpers.PointerSize, "g_patches"); - helpers.WritePointer(patchTablePointerFragment.Data, patchTableFragment.Address); - - ulong patchTableValidAddress = 0x60_0000; - if (patchTableValidReadable) + ulong patchTablePointerAddress = 0x60_0000; + if (patchTablePointerReadable) { - MockMemorySpace.HeapFragment patchTableValidFragment = allocator.Allocate(sizeof(int), "g_patchTableValid"); - helpers.Write( - patchTableValidFragment.Data.AsSpan(0, sizeof(int)), - patchTableValid ? 1 : 0); - patchTableValidAddress = patchTableValidFragment.Address; + MockMemorySpace.HeapFragment patchTablePointerFragment = allocator.Allocate((ulong)helpers.PointerSize, "g_patches"); + helpers.WritePointer( + patchTablePointerFragment.Data, + patchTableAvailable ? patchTableFragment.Address : 0); + patchTablePointerAddress = patchTablePointerFragment.Address; } - builder.AddGlobals( - (Constants.Globals.DebuggerPatchTable, patchTablePointerFragment.Address), - (Constants.Globals.DebuggerPatchTableValid, patchTableValidAddress)); + builder.AddGlobals((Constants.Globals.DebuggerPatchTable, patchTablePointerAddress)); builder.AddContract(version: "c1"); if (memory is not null) @@ -619,7 +614,6 @@ public void ReadInstructionByte_ReturnsOriginalOpcodeWithoutReadingTargetMemory( const ulong InstructionAddress = 0x50_0000; TestPlaceholderTarget target = BuildTargetWithPatchTable( arch, - patchTableValid: true, [(InstructionAddress, 0x1_C3)]); byte instruction = target.Contracts.Debugger.ReadInstructionByte(InstructionAddress); @@ -634,7 +628,6 @@ public void ReadInstructionByte_ReadsTargetMemoryWhenAddressIsNotPatched(MockTar const ulong InstructionAddress = 0x50_0000; TestPlaceholderTarget target = BuildTargetWithPatchTable( arch, - patchTableValid: true, [(InstructionAddress + 1, 0x90)], (InstructionAddress, [0x41])); @@ -645,14 +638,14 @@ public void ReadInstructionByte_ReadsTargetMemoryWhenAddressIsNotPatched(MockTar [Theory] [ClassData(typeof(MockTarget.StdArch))] - public void ReadInstructionByte_ReadsTargetMemoryWhenPatchTableIsInvalid(MockTarget.Architecture arch) + public void ReadInstructionByte_ReadsTargetMemoryWhenPatchTableIsUnavailable(MockTarget.Architecture arch) { const ulong InstructionAddress = 0x50_0000; TestPlaceholderTarget target = BuildTargetWithPatchTable( arch, - patchTableValid: false, [(InstructionAddress, 0x90)], - (InstructionAddress, [0xCC])); + (InstructionAddress, [0xCC]), + patchTableAvailable: false); byte instruction = target.Contracts.Debugger.ReadInstructionByte(InstructionAddress); @@ -680,10 +673,9 @@ public void ReadInstructionByte_ReadsTargetMemoryWhenPatchMetadataIsUnavailable( const ulong InstructionAddress = 0x50_0000; TestPlaceholderTarget target = BuildTargetWithPatchTable( arch, - patchTableValid: true, [(0, 0)], (InstructionAddress, [0x41]), - patchTableValidReadable: false); + patchTablePointerReadable: false); byte instruction = target.Contracts.Debugger.ReadInstructionByte(InstructionAddress); @@ -697,7 +689,6 @@ public void ReadInstructionByte_RefreshesCachedPatchesAfterFlush(MockTarget.Arch const ulong InstructionAddress = 0x50_0000; TestPlaceholderTarget target = BuildTargetWithPatchTable( arch, - patchTableValid: true, [(InstructionAddress, 0x90)]); IDebugger debugger = target.Contracts.Debugger; From d8a4f9c796cf05e71826405c581e3c74e8c101f1 Mon Sep 17 00:00:00 2001 From: Max Charlamb <44248479+max-charlamb@users.noreply.github.com> Date: Thu, 6 Aug 2026 10:57:42 -0400 Subject: [PATCH 3/3] Update src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs Co-authored-by: Theodore Tsirpanis --- .../Contracts/Debugger/Debugger_1.cs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs index 0bd15dcdfc1ba7..9e9d3118f50dd0 100644 --- a/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs +++ b/src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/Debugger/Debugger_1.cs @@ -181,14 +181,7 @@ byte IDebugger.ReadInstructionByte(TargetPointer address) return _target.Read(address); } - private Dictionary GetPatches() - { - Dictionary? patches = _patches; - if (patches is not null) - return patches; - - return _patches ??= ReadPatches(); - } + private Dictionary GetPatches() => _patches ??= ReadPatches(); private Dictionary ReadPatches() {