[cdac] Fix calculation of MethodDesc optional slot addresses (#110491)

The cDAC was assuming that the additional data after a MethodDesc were each just pointers. That is only true for the non-vtable slot and native code slot, not method impl. This change includes the size of those additional slots in the data descriptor and uses them when computing the addresses those optional slots. As part of this, I pulled out shared helpers around dealing with optional slots for use by `MethodValidation` and `RuntimeTypeSystem_1`.
dotnet · hez2010 · Dec 6, 2024 · Dec 9, 2024 · Dec 9, 2024 · Dec 9, 2024
commit dede4f9256526e4bf7733d208f9ad25e7c656bce
diff --git a/src/coreclr/debug/runtimeinfo/datadescriptor.h b/src/coreclr/debug/runtimeinfo/datadescriptor.h
@@ -342,6 +342,18 @@ CDAC_TYPE_FIELD(MethodDescChunk, /*uint8*/, Count, cdac_data<MethodDescChunk>::C
 CDAC_TYPE_FIELD(MethodDescChunk, /*uint16*/, FlagsAndTokenRange, cdac_data<MethodDescChunk>::FlagsAndTokenRange)
 CDAC_TYPE_END(MethodDescChunk)
 
+CDAC_TYPE_BEGIN(NonVtableSlot)
+CDAC_TYPE_SIZE(sizeof(MethodDesc::NonVtableSlot))
+CDAC_TYPE_END(NonVtableSlot)
+
+CDAC_TYPE_BEGIN(MethodImpl)
+CDAC_TYPE_SIZE(sizeof(MethodImpl))
+CDAC_TYPE_END(MethodImpl)
+
+CDAC_TYPE_BEGIN(NativeCodeSlot)
+CDAC_TYPE_SIZE(sizeof(MethodDesc::NativeCodeSlot))
+CDAC_TYPE_END(NativeCodeSlot)
+
 CDAC_TYPE_BEGIN(InstantiatedMethodDesc)
 CDAC_TYPE_SIZE(sizeof(InstantiatedMethodDesc))
 CDAC_TYPE_FIELD(InstantiatedMethodDesc, /*pointer*/, PerInstInfo, cdac_data<InstantiatedMethodDesc>::PerInstInfo)

diff --git a/src/coreclr/vm/method.hpp b/src/coreclr/vm/method.hpp
@@ -1853,10 +1853,8 @@ class MethodDesc
     //
     // Optional MethodDesc slots appear after the end of base MethodDesc in this order:
     //
-
-    // class MethodImpl;                            // Present if HasMethodImplSlot() is true
-
     typedef PCODE NonVtableSlot;   // Present if HasNonVtableSlot() is true
+    // class MethodImpl;           // Present if HasMethodImplSlot() is true
     typedef PCODE NativeCodeSlot;  // Present if HasNativeCodeSlot() is true
 
 // Stub Dispatch code

diff --git a/...tive/managed/cdacreader/Microsoft.Diagnostics.DataContractReader.Abstractions/DataType.cs b/...tive/managed/cdacreader/Microsoft.Diagnostics.DataContractReader.Abstractions/DataType.cs
@@ -73,4 +73,7 @@ public enum DataType
     HashMap,
     Bucket,
     UnwindInfo,
+    NonVtableSlot,
+    MethodImpl,
+    NativeCodeSlot,
 }
diff --git a/...eader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs b/...eader/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs
@@ -143,45 +143,14 @@ private static uint ComputeToken(Target target, Data.MethodDesc desc, Data.Metho
         public TargetPointer CodeData => _desc.CodeData;
         public bool IsIL => Classification == MethodClassification.IL || Classification == MethodClassification.Instantiated;
 
-        public bool HasNativeCodeSlot => HasFlags(MethodDescFlags_1.MethodDescFlags.HasNativeCodeSlot);
-        internal bool HasNonVtableSlot => HasFlags(MethodDescFlags_1.MethodDescFlags.HasNonVtableSlot);
-        internal bool HasMethodImpl => HasFlags(MethodDescFlags_1.MethodDescFlags.HasMethodImpl);
+        internal bool HasNonVtableSlot => MethodDescOptionalSlots.HasNonVtableSlot(_desc.Flags);
+        internal bool HasNativeCodeSlot => MethodDescOptionalSlots.HasNativeCodeSlot(_desc.Flags);
 
         internal bool HasStableEntryPoint => HasFlags(MethodDescFlags_1.MethodDescFlags3.HasStableEntryPoint);
         internal bool HasPrecode => HasFlags(MethodDescFlags_1.MethodDescFlags3.HasPrecode);
 
-        #region Additional Pointers
-        private int AdditionalPointersHelper(MethodDescFlags_1.MethodDescFlags extraFlags)
-            => int.PopCount(_desc.Flags & (ushort)extraFlags);
-
-        // non-vtable slot, native code slot and MethodImpl slots are stored after the MethodDesc itself, packed tightly
-        // in the order: [non-vtable; methhod impl; native code].
-        internal int NonVtableSlotIndex => HasNonVtableSlot ? 0 : throw new InvalidOperationException("no non-vtable slot");
-        internal int MethodImplIndex
-        {
-            get
-            {
-                if (!HasMethodImpl)
-                {
-                    throw new InvalidOperationException("no method impl slot");
-                }
-                return AdditionalPointersHelper(MethodDescFlags_1.MethodDescFlags.HasNonVtableSlot);
-            }
-        }
-        internal int NativeCodeSlotIndex
-        {
-            get
-            {
-                if (!HasNativeCodeSlot)
-                {
-                    throw new InvalidOperationException("no native code slot");
-                }
-                return AdditionalPointersHelper(MethodDescFlags_1.MethodDescFlags.HasNonVtableSlot | MethodDescFlags_1.MethodDescFlags.HasMethodImpl);
-            }
-        }
-
-        internal int AdditionalPointersCount => AdditionalPointersHelper(MethodDescFlags_1.MethodDescFlags.MethodDescAdditionalPointersMask);
-        #endregion Additional Pointers
+        internal TargetPointer GetAddressOfNonVtableSlot() => MethodDescOptionalSlots.GetAddressOfNonVtableSlot(Address, Classification, _desc.Flags, _target);
+        internal TargetPointer GetAddressOfNativeCodeSlot() => MethodDescOptionalSlots.GetAddressOfNativeCodeSlot(Address, Classification, _desc.Flags, _target);
 
         internal bool IsLoaderModuleAttachedToChunk => HasFlags(MethodDescChunkFlags.LoaderModuleAttachedToChunk);
 
@@ -994,48 +963,10 @@ bool IRuntimeTypeSystem.HasNativeCodeSlot(MethodDescHandle methodDesc)
         return md.HasNativeCodeSlot;
     }
 
-    internal static DataType GetMethodClassificationDataType(MethodClassification classification)
-    => classification switch
-    {
-        MethodClassification.IL => DataType.MethodDesc,
-        MethodClassification.FCall => throw new NotImplementedException(), //TODO[cdac]:
-        MethodClassification.PInvoke => throw new NotImplementedException(), //TODO[cdac]:
-        MethodClassification.EEImpl => throw new NotImplementedException(), //TODO[cdac]:
-        MethodClassification.Array => throw new NotImplementedException(), //TODO[cdac]:
-        MethodClassification.Instantiated => DataType.InstantiatedMethodDesc,
-        MethodClassification.ComInterop => throw new NotImplementedException(), //TODO[cdac]:
-        MethodClassification.Dynamic => DataType.DynamicMethodDesc,
-        _ => throw new InvalidOperationException($"Unexpected method classification 0x{classification:x2} for MethodDesc")
-    };
-
-    private uint MethodDescAdditionalPointersOffset(MethodDesc md)
-    {
-        // See MethodDesc::GetBaseSize and s_ClassificationSizeTable
-        // sizeof(MethodDesc),                 mcIL
-        // sizeof(FCallMethodDesc),            mcFCall
-        // sizeof(NDirectMethodDesc),          mcPInvoke
-        // sizeof(EEImplMethodDesc),           mcEEImpl
-        // sizeof(ArrayMethodDesc),            mcArray
-        // sizeof(InstantiatedMethodDesc),     mcInstantiated
-        // sizeof(CLRToCOMCallMethodDesc),     mcComInterOp
-        // sizeof(DynamicMethodDesc)           mcDynamic
-        MethodClassification cls = md.Classification;
-        DataType type = GetMethodClassificationDataType(cls);
-        return _target.GetTypeInfo(type).Size ?? throw new InvalidOperationException($"size of MethodDesc not known");
-    }
-
     TargetPointer IRuntimeTypeSystem.GetAddressOfNativeCodeSlot(MethodDescHandle methodDesc)
     {
         MethodDesc md = _methodDescs[methodDesc.Address];
-        uint offset = MethodDescAdditionalPointersOffset(md);
-        offset += (uint)(_target.PointerSize * md.NativeCodeSlotIndex);
-        return methodDesc.Address + offset;
-    }
-    private TargetPointer GetAddressOfNonVtableSlot(TargetPointer methodDescPointer, MethodDesc md)
-    {
-        uint offset = MethodDescAdditionalPointersOffset(md);
-        offset += (uint)(_target.PointerSize * md.NonVtableSlotIndex);
-        return methodDescPointer.Value + offset;
+        return md.GetAddressOfNativeCodeSlot();
     }
 
     TargetCodePointer IRuntimeTypeSystem.GetNativeCode(MethodDescHandle methodDescHandle)
@@ -1047,30 +978,30 @@ TargetCodePointer IRuntimeTypeSystem.GetNativeCode(MethodDescHandle methodDescHa
             // When profiler is enabled, profiler may ask to rejit a code even though we
             // we have ngen code for this MethodDesc.  (See MethodDesc::DoPrestub).
             // This means that *ppCode is not stable. It can turn from non-zero to zero.
-            TargetPointer ppCode = ((IRuntimeTypeSystem)this).GetAddressOfNativeCodeSlot(methodDescHandle);
+            TargetPointer ppCode = md.GetAddressOfNativeCodeSlot();
             TargetCodePointer pCode = _target.ReadCodePointer(ppCode);
             return CodePointerUtils.CodePointerFromAddress(pCode.AsTargetPointer, _target);
         }
 
         if (!md.HasStableEntryPoint || md.HasPrecode)
             return TargetCodePointer.Null;
 
-        return GetStableEntryPoint(methodDescHandle.Address, md);
+        return GetStableEntryPoint(md);
     }
 
-    private TargetCodePointer GetStableEntryPoint(TargetPointer methodDescAddress, MethodDesc md)
+    private TargetCodePointer GetStableEntryPoint(MethodDesc md)
     {
-        // TODO(cdac): _ASSERTE(HasStableEntryPoint());
         // TODO(cdac): _ASSERTE(!IsVersionableWithVtableSlotBackpatch());
+        Debug.Assert(md.HasStableEntryPoint);
 
-        return GetMethodEntryPointIfExists(methodDescAddress, md);
+        return GetMethodEntryPointIfExists(md);
     }
 
-    private TargetCodePointer GetMethodEntryPointIfExists(TargetPointer methodDescAddress, MethodDesc md)
+    private TargetCodePointer GetMethodEntryPointIfExists(MethodDesc md)
     {
         if (md.HasNonVtableSlot)
         {
-            TargetPointer pSlot = GetAddressOfNonVtableSlot(methodDescAddress, md);
+            TargetPointer pSlot = md.GetAddressOfNonVtableSlot();
             return _target.ReadCodePointer(pSlot);
         }
 

diff --git a/...ft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodDescFlags_1.cs b/...ft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodDescFlags_1.cs
@@ -10,14 +10,12 @@ internal static class MethodDescFlags_1
     internal enum MethodDescFlags : ushort
     {
         ClassificationMask = 0x7,
-        #region Additional pointers
-        // The below flags each imply that there's an extra pointer-sized piece of data after the MethodDesc in the MethodDescChunk
+        #region Optional slots
+        // The below flags each imply that there's an extra pointer-size-aligned piece of data after the MethodDesc in the MethodDescChunk
         HasNonVtableSlot = 0x0008,
         HasMethodImpl = 0x0010,
         HasNativeCodeSlot = 0x0020,
-        // Mask for the above flags
-        MethodDescAdditionalPointersMask = 0x0038,
-        #endregion Additional pointers
+        #endregion Optional slots
     }
 
     [Flags]
@@ -35,5 +33,4 @@ internal enum MethodDescEntryPointFlags : byte
     {
         TemporaryEntryPointAssigned = 0x04,
     }
-
 }
diff --git a/...gnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodDescOptionalSlots.cs b/...gnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodDescOptionalSlots.cs
@@ -0,0 +1,92 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+using System;
+
+namespace Microsoft.Diagnostics.DataContractReader.RuntimeTypeSystemHelpers;
+
+// Non-vtable slot, native code slot, and MethodImpl slots are stored after the MethodDesc itself, packed tightly
+// in the order: [non-vtable; method impl; native code].
+internal static class MethodDescOptionalSlots
+{
+    internal static bool HasNonVtableSlot(ushort flags)
+        => (flags & (ushort)MethodDescFlags_1.MethodDescFlags.HasNonVtableSlot) != 0;
+
+    internal static bool HasMethodImpl(ushort flags)
+        => (flags & (ushort)MethodDescFlags_1.MethodDescFlags.HasMethodImpl) != 0;
+
+    internal static bool HasNativeCodeSlot(ushort flags)
+        => (flags & (ushort)MethodDescFlags_1.MethodDescFlags.HasNativeCodeSlot) != 0;
+
+    internal static TargetPointer GetAddressOfNonVtableSlot(TargetPointer methodDesc, MethodClassification classification, ushort flags, Target target)
+    {
+        uint offset = StartOffset(classification, target);
+        offset += NonVtableSlotOffset(flags);
+        return methodDesc + offset;
+    }
+
+    internal static TargetPointer GetAddressOfNativeCodeSlot(TargetPointer methodDesc, MethodClassification classification, ushort flags, Target target)
+    {
+        uint offset = StartOffset(classification, target);
+        offset += NativeCodeSlotOffset(flags, target);
+        return methodDesc + offset;
+    }
+
+    // Offset from the MethodDesc address to the start of its optional slots
+    private static uint StartOffset(MethodClassification classification, Target target)
+    {
+        // See MethodDesc::GetBaseSize and s_ClassificationSizeTable
+        // sizeof(MethodDesc),                 mcIL
+        // sizeof(FCallMethodDesc),            mcFCall
+        // sizeof(NDirectMethodDesc),          mcPInvoke
+        // sizeof(EEImplMethodDesc),           mcEEImpl
+        // sizeof(ArrayMethodDesc),            mcArray
+        // sizeof(InstantiatedMethodDesc),     mcInstantiated
+        // sizeof(CLRToCOMCallMethodDesc),     mcComInterOp
+        // sizeof(DynamicMethodDesc)           mcDynamic
+        DataType type = classification switch
+        {
+            MethodClassification.IL => DataType.MethodDesc,
+            MethodClassification.FCall => throw new NotImplementedException(), //TODO[cdac]:
+            MethodClassification.PInvoke => throw new NotImplementedException(), //TODO[cdac]:
+            MethodClassification.EEImpl => throw new NotImplementedException(), //TODO[cdac]:
+            MethodClassification.Array => throw new NotImplementedException(), //TODO[cdac]:
+            MethodClassification.Instantiated => DataType.InstantiatedMethodDesc,
+            MethodClassification.ComInterop => throw new NotImplementedException(), //TODO[cdac]:
+            MethodClassification.Dynamic => DataType.DynamicMethodDesc,
+            _ => throw new InvalidOperationException($"Unexpected method classification 0x{classification:x2} for MethodDesc")
+        };
+        return target.GetTypeInfo(type).Size ?? throw new InvalidOperationException($"size of MethodDesc not known");
+    }
+
+    // Offsets are from the start of optional slots data (so right after the MethodDesc), obtained via StartOffset
+    private static uint NonVtableSlotOffset(ushort flags)
+    {
+        if (!HasNonVtableSlot(flags))
+            throw new InvalidOperationException("no non-vtable slot");
+
+        return 0u;
+    }
+
+    private static uint MethodImplOffset(ushort flags, Target target)
+    {
+        if (!HasMethodImpl(flags))
+            throw new InvalidOperationException("no method impl slot");
+
+        return HasNonVtableSlot(flags) ? (uint)target.PointerSize : 0;
+    }
+
+    private static uint NativeCodeSlotOffset(ushort flags, Target target)
+    {
+        if (!HasNativeCodeSlot(flags))
+            throw new InvalidOperationException("no native code slot");
+
+        uint offset = 0;
+        if (HasNonVtableSlot(flags))
+            offset += target.GetTypeInfo(DataType.NonVtableSlot).Size!.Value;
+
+        if (HasMethodImpl(flags))
+            offset += target.GetTypeInfo(DataType.MethodImpl).Size!.Value;
+
+        return offset;
+    }
+}