Implement cDAC TraverseVirtCallStubHeap and address review polish

[Copilot]

Adding TraverseVirtCallStubHeap SOS API, using the same TraverseLoaderHeapCore loader heap traversal structure added for the TraverseLoaderHeap APIs.

Follow-up review feedback addressed in this PR:

• Added upfront callback validation so null callbacks consistently return E_INVALIDARG.
• Removed redundant debug-state reset in TraverseVirtCallStubHeap (core traversal remains the reset point).
• Added a parity note documenting intentional pAppDomain handling (validated, then global loader allocator is used).
• Used explicit contract heap keys ("IndcellHeap" / "CacheEntryHeap") in implementation and tests, per maintainer feedback.
• Explicitly set VCSHeapType backing type to int to match native.
• Expanded tests with a positive CacheEntryHeap traversal path.
• Split the combined invalid-arguments test into separate null-domain and null-callback tests for clearer diagnostics.

[Copilot]

Pull request overview

Implements cDAC support for ISOSDacInterface.TraverseVirtCallStubHeap in SOSDacImpl, aligning behavior with the native DAC by traversing VirtualCallStubManager loader heaps (Indcell / CacheEntry) via the loader contract, and adds unit tests for the new behavior.

Changes:

• Added VCSHeapType enum and updated ISOSDacInterface.TraverseVirtCallStubHeap signature to use the enum and a typed unmanaged callback.
• Implemented TraverseVirtCallStubHeap in SOSDacImpl using ILoader.GetGlobalLoaderAllocator() + GetLoaderAllocatorHeaps(...) and reusing TraverseLoaderHeapCore(...).
• Added Loader contract-based unit tests covering traversal, missing manager, invalid heap type, and invalid argument scenarios.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/native/managed/cdac/tests/LoaderTests.cs	Adds unit tests for TraverseVirtCallStubHeap behavior using a mocked ILoader contract.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs	Implements TraverseVirtCallStubHeap and refactors TraverseLoaderHeapCore to take a TargetPointer.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ISOSDacInterface.cs	Introduces VCSHeapType and updates the COM interface method signature accordingly.

[dotnet-policy-service]

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

[max-charlamb]

A few nit-level suggestions — logic is correct, these are polish items. LGTM once addressed or deferred.

[max-charlamb]

Nit: the producer (Loader_1.GetLoaderAllocatorHeaps) uses nameof(Data.VirtualCallStubManager.IndcellHeap) / nameof(Data.VirtualCallStubManager.CacheEntryHeap) as the dictionary keys. Consider using nameof here too (on the ContainsKey check and in the switch arms) so the two sides stay in sync under rename-refactorings and the contract coupling is self-documenting.

[rcj1]

nameof what? We would have to then define these strings at the abstraction level. Should we maybe define these enum instead at the abstraction level?

[Copilot]

Updated heap-key coupling to use nameof(VCSHeapType.IndcellHeap/CacheEntryHeap) in TraverseVirtCallStubHeap so key names stay refactor-safe and self-documenting. Addressed in 0593031.

[rcj1]

Yeah I don’t think this is what we want.

[max-charlamb]

Why can't we use the name of the enum type? We could even simplify the switch to use enum ToString

[noahfalk]

the producer (Loader_1.GetLoaderAllocatorHeaps) uses nameof(Data.VirtualCallStubManager.IndcellHeap)

If we are expecting contract consumers (like this code) to be able to lookup specific heaps by a well-known name then that sounds like those heap names are part of the public contract API and should not change when refactoring the implementation of the loader contract. We might want to use fixed constant strings or switch to an enum rather than deriving the names using nameof() on private implementation fields.

[rcj1]

Yes we discussed this offline and decided on the enum. I’m working on a separate pr for that.

[max-charlamb]

Isn't the problem here that adding/removing heaps on the runtime is something that can happen? I'd prefer the contract surface be "here are the named runtime heaps", and the debugger can present that data how it wishes.

(and not every heap type is in every build)

The current contract is that the loader reports exactly what the runtime calls the heaps. If we want to change it so the cDAC drives the names, that would be reasonable.

I take it back, after looking at the code, I think it is totally reasonable to change this so that we have an enum of heaps we can request. The cDAC does drive the naming currently.

[max-charlamb]

@copilot please address the feedback

[Copilot]

@copilot please address the feedback

Addressed the requested feedback in follow-up commits 0593031 and 68796e2 (implementation polish + test coverage updates), and re-ran the relevant LoaderTests suite successfully.