Skip to content

[cDAC] fix ICF function logic#120301

Merged
max-charlamb merged 1 commit intodotnet:mainfrom
max-charlamb:cdac-bug-fix
Oct 2, 2025
Merged

[cDAC] fix ICF function logic#120301
max-charlamb merged 1 commit intodotnet:mainfrom
max-charlamb:cdac-bug-fix

Conversation

@max-charlamb
Copy link
Member

@max-charlamb max-charlamb commented Oct 1, 2025

This logic was previously reversed from the runtime. See:

BOOL HasFunction()
{
WRAPPER_NO_CONTRACT;
#ifdef HOST_64BIT
// See code:GenericPInvokeCalliHelper
return ((m_Datum != NULL) && !(dac_cast<TADDR>(m_Datum) & 0x1));
#else // HOST_64BIT
return ((dac_cast<TADDR>(m_Datum) & ~0xffff) != 0);
#endif // HOST_64BIT
}

Todo: Add more SOS testing, CI should have caught this.

Copilot AI review requested due to automatic review settings October 1, 2025 18:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the logic for determining if an InlinedCallFrame has a function in the cDAC (contract-based data access component). The changes correct both the pointer size check condition and the bitmask comparison to match the runtime implementation.

Key Changes

  • Corrects the pointer size condition from checking for 32-bit to checking for 64-bit systems
  • Inverts the bitmask logic to check for equality with 0 instead of inequality
Comments suppressed due to low confidence (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StackWalk/FrameHandling/FrameIterator.cs:257

  • Using sizeof(ulong) to check for 64-bit systems is unclear. Consider using a more explicit constant like 8 or IntPtr.Size on a 64-bit system, or define a constant that clearly indicates this is checking for 64-bit pointer size.
        if (target.PointerSize == sizeof(ulong))

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Oct 1, 2025
@max-charlamb max-charlamb added area-Diagnostics-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Oct 1, 2025
@max-charlamb max-charlamb requested a review from rcj1 October 1, 2025 18:03
@dotnet-policy-service
Copy link
Contributor

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

@max-charlamb max-charlamb merged commit 6a62a75 into dotnet:main Oct 2, 2025
51 of 54 checks passed
@max-charlamb max-charlamb deleted the cdac-bug-fix branch October 2, 2025 14:45
@github-actions github-actions bot locked and limited conversation to collaborators Nov 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants