[cDAC] Use precise EnC IL

[rcj1]

Use latest IL from EnC edit when applicable, as in ClrDataMethodDefinition::GetIlMethod.

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[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]

lgtm after unit test fix

[Copilot]

Pull request overview

Updates the cDAC legacy IXCLRDataMethodDefinition implementation to prefer the active Edit-and-Continue (EnC) IL header when an EnC edit is currently active, matching established behavior elsewhere in the debugging stack.

Changes:

• In ClrDataMethodDefinition, attempt to resolve the MethodDesc and query ICodeVersions for an active EnC IL header, falling back to ILoader.GetILHeader when unavailable.
• Extend IXCLRDataProcessTests to set up the new lookup-table calls and add a unit test that validates the EnC IL header is used when active.

Show a summary per file

File	Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs	Prefer active EnC IL header (via ICodeVersions) when computing IL extent start.
src/native/managed/cdac/tests/UnitTests/IXCLRDataProcessTests.cs	Update mocks for lookup-table calls and add coverage ensuring active EnC IL is used.

Copilot's findings

• Files reviewed: 2/2 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

Suppressed comments (4)

src/native/managed/cdac/tests/UnitTests/IXCLRDataProcessTests.cs:250

• The test sets up ILoader.GetLookupTables(...) and ILoader.GetModuleLookupMapElement(TargetPointer, ...), but those members don't exist on ILoader (the only GetModuleLookupMapElement overload takes (ModuleHandle, ModuleLookupMapKind, ...)). With MockBehavior.Strict, this will either fail to compile or fail at runtime. Set up the existing GetModuleLookupMapElement(module, ModuleLookupMapKind.MethodDefToDesc, ...) overload instead.

            ModuleLookupTables lookupTables = new() { MethodDefToDesc = new TargetPointer(MethodDefToDescAddress) };
            loader.Setup(l => l.GetLookupTables(module)).Returns(lookupTables);
            loader.Setup(l => l.GetModuleLookupMapElement(
                lookupTables.MethodDefToDesc,
                It.IsAny<uint>(),
                out It.Ref<TargetNUInt>.IsAny)).Returns(TargetPointer.Null);

src/native/managed/cdac/tests/UnitTests/IXCLRDataProcessTests.cs:374

• ILoader.GetLookupTables(...) and ILoader.GetModuleLookupMapElement(TargetPointer, ...) are not part of ILoader in this repo. Since ClrDataMethodDefinition now consults GetModuleLookupMapElement(module, ModuleLookupMapKind.MethodDefToDesc, ...) when computing IL extents, this test should set up that overload (returning null here).

        loader.Setup(l => l.GetLookupTables(module)).Returns(default(ModuleLookupTables));
        loader.Setup(l => l.GetModuleLookupMapElement(
            TargetPointer.Null,
            Token,
            out It.Ref<TargetNUInt>.IsAny)).Returns(TargetPointer.Null);

src/native/managed/cdac/tests/UnitTests/IXCLRDataProcessTests.cs:414

• This test uses GetLookupTables and the non-existent GetModuleLookupMapElement(TargetPointer, ...) overload. ClrDataMethodDefinition.TryResolveMethodDesc() calls GetModuleLookupMapElement(module, ModuleLookupMapKind.MethodDefToDesc, ...), so the mock should set up that overload to return the MethodDescAddress.

        const ulong MethodDescAddress = 0x5000;
        const ulong MethodDefToDescAddress = 0x6000;
        const uint Token = 0x06000001;
        const byte TinyFormat = 0x2;
        const int DefaultCodeSize = 1;
        const int EnCCodeSize = 3;

        ModuleHandle module = new(new TargetPointer(ModuleAddress));
        ModuleLookupTables lookupTables = new() { MethodDefToDesc = new TargetPointer(MethodDefToDescAddress) };
        Mock<ILoader> loader = new(MockBehavior.Strict);
        loader.Setup(l => l.GetModuleHandleFromModulePtr(new TargetPointer(ModuleAddress))).Returns(module);
        loader.Setup(l => l.GetLookupTables(module)).Returns(lookupTables);
        loader.Setup(l => l.GetModuleLookupMapElement(
            lookupTables.MethodDefToDesc,
            Token,
            out It.Ref<TargetNUInt>.IsAny)).Returns(new TargetPointer(MethodDescAddress));
        loader.Setup(l => l.GetILHeader(module, Token)).Returns(new TargetPointer(DefaultHeaderAddress));

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs:73

• GetILExtentStart now calls TryResolveMethodDesc() unconditionally, even when the ICodeVersions contract isn't present. That adds an unnecessary lookup on the common path and (more importantly) introduces an extra dependency that can fail even though the fallback GetILHeader path would have worked. Consider only resolving the MethodDesc when ICodeVersions is available.

        TargetPointer ilHeader = TargetPointer.Null;
        TargetPointer methodDesc = TryResolveMethodDesc();
        if (methodDesc != TargetPointer.Null && _target.Contracts.TryGetContract(out ICodeVersions codeVersions))
        {
            ILCodeVersionHandle activeVersion = codeVersions.GetActiveILCodeVersion(methodDesc);
            if (activeVersion.IsValid && codeVersions.GetSource(activeVersion) == CodeVersionSource.EnC)
            {
                ilHeader = codeVersions.GetIL(activeVersion);
            }
        }

• Files reviewed: 2/2 changed files
• Comments generated: 1

[Copilot]

Copilot's findings

Suppressed comments (2)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs:66

• GetILExtentStart now always resolves the MethodDesc via TryResolveMethodDesc(), even when the CodeVersions contract isn’t present, and it also calls GetModuleHandleFromModulePtr twice (once here and once inside TryResolveMethodDesc). This adds avoidable work to a potentially frequently-called path (StartEnumExtents / GetRepresentativeEntryAddress). Consider only doing the MethodDesc lookup when ICodeVersions is available, and reuse the already-computed moduleHandle for the lookup to avoid the extra module-handle roundtrip.

        ILoader loader = _target.Contracts.Loader;
        Contracts.ModuleHandle moduleHandle = loader.GetModuleHandleFromModulePtr(_module);
        TargetPointer ilHeader = TargetPointer.Null;
        TargetPointer methodDesc = TryResolveMethodDesc();
        if (methodDesc != TargetPointer.Null && _target.Contracts.TryGetContract(out ICodeVersions codeVersions))

src/native/managed/cdac/tests/UnitTests/IXCLRDataProcessTests.cs:416

• The new coverage validates the EnC-active case, but there’s no test for the (newly relevant) scenario where ICodeVersions is present yet the active IL version’s source is not EnC (or returns Unknown/ReJIT). In that case the code should fall back to Loader.GetILHeader; adding a test for that would protect against accidentally returning no extents or using the wrong IL when code versioning is enabled but there’s no EnC edit.

        ILCodeVersionHandle activeVersion = ILCodeVersionHandle.CreateExplicit(new TargetPointer(0x7000));
        Mock<ICodeVersions> codeVersions = new(MockBehavior.Strict);
        codeVersions.Setup(c => c.GetActiveILCodeVersion(new TargetPointer(MethodDescAddress))).Returns(activeVersion);
        codeVersions.Setup(c => c.GetSource(activeVersion)).Returns(CodeVersionSource.EnC);
        codeVersions.Setup(c => c.GetIL(activeVersion)).Returns(new TargetPointer(EnCHeaderAddress));

• Files reviewed: 2/2 changed files
• Comments generated: 0 new

[rcj1]

/ba-g "build monitor helix jobs"