[cDAC] Fix method table slot lookup

[max-charlamb]

Found this bug while testing EnumMethodInstancesByAddress.

MethodTable slots are pointer sized. Finding the correct one is done using pointer arithmetic on the native code but has to be done explicitly on the cDAC.

[Copilot]

Pull Request Overview

This PR fixes a critical bug in the pointer arithmetic for method table slot lookup in the cDAC runtime type system.

• Correctly adjusts the pointer arithmetic by multiplying the offset with _target.PointerSize
• Ensures that the calculation accounts for pointer-sized slots

Comments suppressed due to low confidence (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs:846

• The change correctly applies multiplication by _target.PointerSize to adjust for pointer-sized slots. Ensure that _target.PointerSize is always valid and consistent across different target environments to avoid potential underflow or miscalculation.

return nonVirtualSlotsArray - ((1 + (slotNum - mt.NumVirtuals)) * (ulong)_target.PointerSize);

[dotnet-policy-service]

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

[kg]

Is this a pattern that occurs in other places? It might be worth considering having a TargetManagedPointer that behaves the same as IntPtr* does, i.e. +1 moves to the next managed pointer, or omitting operators that are likely to indicate the caller is doing the wrong thing.

[kg]

I'd like to see (1 + (slotNum - mt.NumVirtuals)) in a separate named local that describes what it is, so the index calculation is separated from the machinery of turning an index into an address

[max-charlamb]

Is this a pattern that occurs in other places? It might be worth considering having a TargetManagedPointer that behaves the same as IntPtr* does, i.e. +1 moves to the next managed pointer, or omitting operators that are likely to indicate the caller is doing the wrong thing.

This sounds like a good idea, but the current design makes it difficult to implement.

• TargetPointer implicitly converts to ulong. This isn't a big deal, but we would need to remove this conversion and fix existing code.
• More impactfully, TargetPointer is just a typed ulong. It doesn't hold a reference to the actual Target or know the target's pointer size. Currently all consumers are expected to use the info stored on the Target to do any of these conversions.

Maybe we could add a method or extension to Target that allows pointer arithmetic.

[max-charlamb]

Merging fix for now, will take another look at improving pointer math in the future