Skip to content

Adding GetILForModule cDAC API #118546

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Adding GetILForModule cDAC API #118546

wants to merge 2 commits into from

Conversation

rcj1
Copy link
Contributor

@rcj1 rcj1 commented Aug 8, 2025

A couple comments on this one:

  • I am assuming that IsMapped() is always true, excluding ReflectionEmit methods.
  • I am also omitting the checks here; they don't seem important for the read-only dumping of IL and they would add a decent amount of complexity to the descriptor and contracts.
    • The first check validates various alignment and flags; to me they look superfluous for read-only operation.
    • The second check ensures that the RVA is valid within the PE file. We omit a check for the IL size in the existing DAC code; it seems we have accepted the risk of this leading to failure on a ReadVirtual if the given size is incorrect/corrupted.

@Copilot Copilot AI review requested due to automatic review settings August 8, 2025 18:37
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 implements the GetILForModule cDAC API, which retrieves the address of IL (Intermediate Language) code for a given module and relative virtual address (RVA). The implementation replaces a placeholder that delegated to legacy code with a full cDAC-based solution.

Key changes:

  • Adds complete implementation of GetILForModule in SOSDacImpl with proper parameter validation and error handling
  • Introduces new GetILAddr method in the ILoader contract to calculate IL addresses from PE assembly data
  • Includes DEBUG-only verification against legacy implementation to ensure compatibility

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
SOSDacImpl.cs Replaces placeholder with full GetILForModule implementation including validation, error handling, and debug verification
Loader_1.cs Implements GetILAddr method to calculate IL addresses by traversing PE assembly, image, and layout structures
ILoader.cs Adds GetILAddr method signature to the ILoader contract interface

@rcj1 rcj1 requested a review from noahfalk August 8, 2025 18:37
Copy link
Contributor

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

@rcj1 rcj1 requested review from max-charlamb and removed request for noahfalk and max-charlamb August 8, 2025 18:51
@rcj1 rcj1 marked this pull request as draft August 8, 2025 18:52
@rcj1 rcj1 marked this pull request as ready for review August 8, 2025 18:57
@rcj1 rcj1 requested review from noahfalk and max-charlamb August 8, 2025 18:57
@jkotas
Copy link
Member

jkotas commented Aug 8, 2025

I am assuming that IsMapped() is always true, excluding ReflectionEmit methods.

That's only true on Windows for files loaded from disk.

IsMapped is typically false on non-Windows for non-R2R binaries. Also, it is likely false for binaries loaded from memory (e.g. using Assembly.Load(byte[]) API) on all platforms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants