[cdac] Read/store globals from contract descriptor (dotnet#101450)

- Map indirect global values to corresponding values in `pointer_data` array from contract descriptor - Add `[Try]Read<T>`, `[Try]ReadPointer`, `[Try]ReadGlobal<T>` and `[Try]ReadGlobalPointer` to `Target` - Make cDAC implementation of `ISOSDacInterface9.GetBreakingChangeVersion` read value from globals - Create test helpers for mocking out reading from the target and providing a contract descriptor for different bitness/endianness - Add unit tests for reading global values (direct and indirect)
michaelgsharp · May 8, 2024 · 3fddcf7 · 3fddcf7
1 parent 4a38697
commit 3fddcf7
Show file tree

Hide file tree

Showing 8 changed files with 507 additions and 54 deletions.
diff --git a/docs/design/datacontracts/contract-descriptor.md b/docs/design/datacontracts/contract-descriptor.md
@@ -45,7 +45,7 @@ reserved bits should be written as zero.  Diagnostic tooling may ignore non-zero
 
 The `descriptor` is a pointer to a UTF-8 JSON string described in [data descriptor physical layout](./data_descriptor.md#Physical_JSON_descriptor).  The total number of bytes is given by `descriptor_size`.
 
-The auxiliary data for the JSON descriptor is stored at the location `aux_data` in `aux_data_count` pointer-sized slots.
+The auxiliary data for the JSON descriptor is stored at the location `pointer_data` in `pointer_data_count` pointer-sized slots.
 
 ### Architecture properties
 
@@ -83,7 +83,7 @@ a JSON integer constant.
   "globals":
   {
     "FEATURE_COMINTEROP": 0,
-    "s_pThreadStore": [ 0 ] // indirect from aux data offset 0
+    "s_pThreadStore": [ 0 ] // indirect from pointer data offset 0
   },
   "contracts": {"Thread": 1, "GCHandle": 1, "ThreadStore": 1}
 }

diff --git a/src/coreclr/debug/daccess/cdac.cpp b/src/coreclr/debug/daccess/cdac.cpp
@@ -41,27 +41,31 @@ CDAC CDAC::Create(uint64_t descriptorAddr, ICorDebugDataTarget* target)
 {
     HMODULE cdacLib;
     if (!TryLoadCDACLibrary(&cdacLib))
-        return CDAC::Invalid();
+        return {};
 
-    return CDAC{cdacLib, descriptorAddr, target};
+    decltype(&cdac_reader_init) init = reinterpret_cast<decltype(&cdac_reader_init)>(::GetProcAddress(cdacLib, "cdac_reader_init"));
+    _ASSERTE(init != nullptr);
+
+    intptr_t handle;
+    if (init(descriptorAddr, &ReadFromTargetCallback, target, &handle) != 0)
+    {
+        ::FreeLibrary(cdacLib);
+        return {};
+    }
+
+    return CDAC{cdacLib, handle, target};
 }
 
-CDAC::CDAC(HMODULE module, uint64_t descriptorAddr, ICorDebugDataTarget* target)
-    : m_module(module)
+CDAC::CDAC(HMODULE module, intptr_t handle, ICorDebugDataTarget* target)
+    : m_module{module}
+    , m_cdac_handle{handle}
     , m_target{target}
 {
-    if (m_module == NULL)
-    {
-        m_cdac_handle = 0;
-        return;
-    }
+    _ASSERTE(m_module != NULL && m_cdac_handle != 0 && m_target != NULL);
 
     m_target->AddRef();
-    decltype(&cdac_reader_init) init = reinterpret_cast<decltype(&cdac_reader_init)>(::GetProcAddress(m_module, "cdac_reader_init"));
     decltype(&cdac_reader_get_sos_interface) getSosInterface = reinterpret_cast<decltype(&cdac_reader_get_sos_interface)>(::GetProcAddress(m_module, "cdac_reader_get_sos_interface"));
-    _ASSERTE(init != nullptr && getSosInterface != nullptr);
-
-    init(descriptorAddr, &ReadFromTargetCallback, m_target, &m_cdac_handle);
+    _ASSERTE(getSosInterface != nullptr);
     getSosInterface(m_cdac_handle, &m_sos);
 }
 

diff --git a/src/coreclr/debug/daccess/cdac.h b/src/coreclr/debug/daccess/cdac.h
@@ -9,12 +9,9 @@ class CDAC final
 public: // static
     static CDAC Create(uint64_t descriptorAddr, ICorDebugDataTarget *pDataTarget);
 
-    static CDAC Invalid()
-    {
-        return CDAC{nullptr, 0, nullptr};
-    }
-
 public:
+    CDAC() = default;
+
     CDAC(const CDAC&) = delete;
     CDAC& operator=(const CDAC&) = delete;
 
@@ -56,7 +53,7 @@ class CDAC final
     IUnknown* SosInterface();
 
 private:
-    CDAC(HMODULE module, uint64_t descriptorAddr, ICorDebugDataTarget* target);
+    CDAC(HMODULE module, intptr_t handle, ICorDebugDataTarget* target);
 
 private:
     HMODULE m_module;

diff --git a/src/coreclr/debug/daccess/daccess.cpp b/src/coreclr/debug/daccess/daccess.cpp
@@ -3038,7 +3038,7 @@ class DacStreamManager
 //----------------------------------------------------------------------------
 
 ClrDataAccess::ClrDataAccess(ICorDebugDataTarget * pTarget, ICLRDataTarget * pLegacyTarget/*=0*/)
-    : m_cdac{CDAC::Invalid()}
+    : m_cdac{}
 {
     SUPPORTS_DAC_HOST_ONLY;     // ctor does no marshalling - don't check with DacCop
 

diff --git a/src/native/managed/cdacreader/src/Constants.cs b/src/native/managed/cdacreader/src/Constants.cs
@@ -0,0 +1,13 @@
+// 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;
+
+internal static class Constants
+{
+    internal static class Globals
+    {
+        // See src/coreclr/debug/runtimeinfo/datadescriptor.h
+        internal const string SOSBreakingChangeVersion = nameof(SOSBreakingChangeVersion);
+    }
+}
diff --git a/src/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs b/src/native/managed/cdacreader/src/Legacy/SOSDacImpl.cs
@@ -35,8 +35,7 @@ public SOSDacImpl(Target target)
 
     public int GetBreakingChangeVersion()
     {
-        // TODO: Return non-hard-coded version
-        return 4;
+        return _target.ReadGlobal<byte>(Constants.Globals.SOSBreakingChangeVersion);
     }
 
     public unsafe int GetCCWData(ulong ccw, void* data) => HResults.E_NOTIMPL;

diff --git a/src/native/managed/cdacreader/src/Target.cs b/src/native/managed/cdacreader/src/Target.cs
@@ -4,6 +4,8 @@
 using System;
 using System.Buffers.Binary;
 using System.Collections.Generic;
+using System.Numerics;
+using System.Runtime.CompilerServices;
 
 namespace Microsoft.Diagnostics.DataContractReader;
 
@@ -15,7 +17,7 @@ public struct TargetPointer
     public TargetPointer(ulong value) => Value = value;
 }
 
-internal sealed unsafe class Target
+public sealed unsafe class Target
 {
     private const int StackAllocByteThreshold = 1024;
 
@@ -29,7 +31,7 @@ private readonly struct Configuration
     private readonly Reader _reader;
 
     private readonly IReadOnlyDictionary<string, int> _contracts = new Dictionary<string, int>();
-    private readonly TargetPointer[] _pointerData = [];
+    private readonly IReadOnlyDictionary<string, (ulong Value, string? Type)> _globals = new Dictionary<string, (ulong, string?)>();
 
     public static bool TryCreate(ulong contractDescriptor, delegate* unmanaged<ulong, byte*, uint, void*, int> readFromTarget, void* readContext, out Target? target)
     {
@@ -49,11 +51,30 @@ private Target(Configuration config, ContractDescriptorParser.ContractDescriptor
         _config = config;
         _reader = reader;
 
-        // TODO: [cdac] Read globals and types
+        // TODO: [cdac] Read types
         // note: we will probably want to store the globals and types into a more usable form
         _contracts = descriptor.Contracts ?? [];
 
-        _pointerData = pointerData;
+        // Read globals and map indirect values to pointer data
+        if (descriptor.Globals is not null)
+        {
+            Dictionary<string, (ulong Value, string? Type)> globals = [];
+            foreach ((string name, ContractDescriptorParser.GlobalDescriptor global) in descriptor.Globals)
+            {
+                ulong value = global.Value;
+                if (global.Indirect)
+                {
+                    if (value >= (ulong)pointerData.Length)
+                        throw new InvalidOperationException($"Invalid pointer data index {value}.");
+
+                    value = pointerData[value].Value;
+                }
+
+                globals[name] = (value, global.Type);
+            }
+
+            _globals = globals;
+        }
     }
 
     // See docs/design/datacontracts/contract-descriptor.md
@@ -81,7 +102,7 @@ private static bool TryReadContractDescriptor(
             return false;
 
         // Flags - uint32_t
-        if (!TryReadUInt32(address, isLittleEndian, reader, out uint flags))
+        if (!TryRead(address, isLittleEndian, reader, out uint flags))
             return false;
 
         address += sizeof(uint);
@@ -92,7 +113,7 @@ private static bool TryReadContractDescriptor(
         config = new Configuration { IsLittleEndian = isLittleEndian, PointerSize = pointerSize };
 
         // Descriptor size - uint32_t
-        if (!TryReadUInt32(address, config.IsLittleEndian, reader, out uint descriptorSize))
+        if (!TryRead(address, config.IsLittleEndian, reader, out uint descriptorSize))
             return false;
 
         address += sizeof(uint);
@@ -104,7 +125,7 @@ private static bool TryReadContractDescriptor(
         address += (uint)pointerSize;
 
         // Pointer data count - uint32_t
-        if (!TryReadUInt32(address, config.IsLittleEndian, reader, out uint pointerDataCount))
+        if (!TryRead(address, config.IsLittleEndian, reader, out uint pointerDataCount))
             return false;
 
         address += sizeof(uint);
@@ -138,30 +159,33 @@ private static bool TryReadContractDescriptor(
         return true;
     }
 
-    public uint ReadUInt32(ulong address)
+    public T Read<T>(ulong address, out T value) where T : unmanaged, IBinaryInteger<T>, IMinMaxValue<T>
     {
-        if (!TryReadUInt32(address, out uint value))
-            throw new InvalidOperationException($"Failed to read uint32 at 0x{address:x8}.");
+        if (!TryRead(address, out value))
+            throw new InvalidOperationException($"Failed to read {typeof(T)} at 0x{address:x8}.");
 
         return value;
     }
 
-    public bool TryReadUInt32(ulong address, out uint value)
-        => TryReadUInt32(address, _config.IsLittleEndian, _reader, out value);
+    public bool TryRead<T>(ulong address, out T value) where T : unmanaged, IBinaryInteger<T>, IMinMaxValue<T>
+        => TryRead(address, _config.IsLittleEndian, _reader, out value);
 
-    private static bool TryReadUInt32(ulong address, bool isLittleEndian, Reader reader, out uint value)
+    private static bool TryRead<T>(ulong address, bool isLittleEndian, Reader reader, out T value) where T : unmanaged, IBinaryInteger<T>, IMinMaxValue<T>
     {
-        value = 0;
-
-        Span<byte> buffer = stackalloc byte[sizeof(uint)];
+        value = default;
+        Span<byte> buffer = stackalloc byte[sizeof(T)];
         if (reader.ReadFromTarget(address, buffer) < 0)
             return false;
 
-        value = isLittleEndian
-            ? BinaryPrimitives.ReadUInt32LittleEndian(buffer)
-            : BinaryPrimitives.ReadUInt32BigEndian(buffer);
+        return isLittleEndian
+            ? T.TryReadLittleEndian(buffer, !IsSigned<T>(), out value)
+            : T.TryReadBigEndian(buffer, !IsSigned<T>(), out value);
+    }
 
-        return true;
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    private static bool IsSigned<T>() where T : struct, INumberBase<T>, IMinMaxValue<T>
+    {
+        return T.IsNegative(T.MinValue);
     }
 
     public TargetPointer ReadPointer(ulong address)
@@ -183,21 +207,79 @@ private static bool TryReadPointer(ulong address, Configuration config, Reader r
         if (reader.ReadFromTarget(address, buffer) < 0)
             return false;
 
-        if (config.PointerSize == sizeof(uint))
+        if (config.PointerSize == sizeof(uint)
+            && TryRead(address, config.IsLittleEndian, reader, out uint value32))
         {
-            pointer = new TargetPointer(
-                config.IsLittleEndian
-                    ? BinaryPrimitives.ReadUInt32LittleEndian(buffer)
-                    : BinaryPrimitives.ReadUInt32BigEndian(buffer));
+            pointer = new TargetPointer(value32);
+            return true;
         }
-        else if (config.PointerSize == sizeof(ulong))
+        else if (config.PointerSize == sizeof(ulong)
+            && TryRead(address, config.IsLittleEndian, reader, out ulong value64))
         {
-            pointer = new TargetPointer(
-                config.IsLittleEndian
-                    ? BinaryPrimitives.ReadUInt64LittleEndian(buffer)
-                    : BinaryPrimitives.ReadUInt64BigEndian(buffer));
+            pointer = new TargetPointer(value64);
+            return true;
         }
 
+        return false;
+    }
+
+    public T ReadGlobal<T>(string name) where T : struct, INumber<T>
+    {
+        if (!TryReadGlobal(name, out T value))
+            throw new InvalidOperationException($"Failed to read global {typeof(T)} '{name}'.");
+
+        return value;
+    }
+
+    public bool TryReadGlobal<T>(string name, out T value) where T : struct, INumber<T>, INumberBase<T>
+    {
+        value = default;
+        if (!_globals.TryGetValue(name, out (ulong Value, string? Type) global))
+            return false;
+
+        // TODO: [cdac] Move type validation out of the read such that it does not have to happen for every read
+        if (global.Type is not null)
+        {
+            string? expectedType = Type.GetTypeCode(typeof(T)) switch
+            {
+                TypeCode.SByte => "int8",
+                TypeCode.Byte => "uint8",
+                TypeCode.Int16 => "int16",
+                TypeCode.UInt16 => "uint16",
+                TypeCode.Int32 => "int32",
+                TypeCode.UInt32 => "uint32",
+                TypeCode.Int64 => "int64",
+                TypeCode.UInt64 => "uint64",
+                _ => null,
+            };
+            if (expectedType is null || global.Type != expectedType)
+            {
+                return false;
+            }
+        }
+
+        value = T.CreateChecked(global.Value);
+        return true;
+    }
+
+    public TargetPointer ReadGlobalPointer(string name)
+    {
+        if (!TryReadGlobalPointer(name, out TargetPointer pointer))
+            throw new InvalidOperationException($"Failed to read global pointer '{name}'.");
+
+        return pointer;
+    }
+
+    public bool TryReadGlobalPointer(string name, out TargetPointer pointer)
+    {
+        pointer = TargetPointer.Null;
+        if (!_globals.TryGetValue(name, out (ulong Value, string? Type) global))
+            return false;
+
+        if (global.Type is not null && Array.IndexOf(["pointer", "nint", "nuint"], global.Type) == -1)
+            return false;
+
+        pointer = new TargetPointer(global.Value);
         return true;
     }