Skip to content

Conversation

rcj1
Copy link
Contributor

@rcj1 rcj1 commented Jul 11, 2025

No description provided.

@Copilot Copilot AI review requested due to automatic review settings July 11, 2025 04:39
Copy link
Contributor

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

Copy link
Contributor

@Copilot 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 adds a full managed implementation of the GetPendingReJITID cDAC API, including argument validation, contract-based lookup, HRESULT translation, and debug assertions against the legacy DAC behavior.

  • Added argument checks and HRESULT handling around ReJIT lookups
  • Replaced expression-bodied member with expanded try/catch logic
  • Introduced DEBUG-only assertions comparing to the legacy implementation
Comments suppressed due to low confidence (3)

src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs:1793

  • The legacy fallback is only exercised under DEBUG. If you intend to preserve previous DAC behavior in release builds, consider invoking the legacy implementation when contracts are unavailable or unimplemented.
#if DEBUG

src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs:1767

  • This new API implementation needs unit tests covering: valid pending ReJIT, non-pending paths, invalid arguments, and exception handling to ensure correct HRESULT and out-parameter behavior.
    int ISOSDacInterface7.GetPendingReJITID(ClrDataAddress methodDesc, int* pRejitId)

src/native/managed/cdac/mscordaccore_universal/Legacy/SOSDacImpl.cs:1769

  • Comparing methodDesc to 0 may not correctly detect a null address and could fail to compile. Use the appropriate null check on the ClrDataAddress type (e.g., a hypothetical methodDesc.IsNull) or compare against default(ClrDataAddress).
        if (methodDesc == 0 || pRejitId == null)

@rcj1 rcj1 requested a review from max-charlamb July 11, 2025 18:22
Copy link
Member

@max-charlamb max-charlamb left a comment

Choose a reason for hiding this comment

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

lgtm 👍 only test failure is the known COM double free issue I'm investigating

@rcj1 rcj1 merged commit 7905f0e into dotnet:main Jul 15, 2025
47 of 49 checks passed
@rcj1 rcj1 deleted the GetPendingReJITID branch July 15, 2025 20:44
@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 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.

2 participants