Skip to content
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

[cDAC] Implement ReJIT portion of SOSDacImpl::GetMethodDescData #109936

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

Conversation

max-charlamb
Copy link
Contributor

@max-charlamb max-charlamb commented Nov 18, 2024

Adds ReJIT support to SOSDacImpl::GetMethodDescData.

Modifies ICodeVersions contract

interface ICodeVersions
{
    /* adds */ ILCodeVersionHandle GetActiveILCodeVersion(TargetPointer methodDesc);
    /* adds */ ILCodeVersionHandle GetILCodeVersion(NativeCodeVersionHandle codeVersionHandle);
    /* adds */ IEnumerable<ILCodeVersionHandle> GetILCodeVersions(TargetPointer methodDesc);
    /* adds */ NativeCodeVersionHandle GetActiveNativeCodeVersionForILCodeVersion(TargetPointer methodDesc, ILCodeVersionHandle ilCodeVersionHandle);

    /* removes */ NativeCodeVersionHandle GetActiveNativeCodeVersion(TargetPointer methodDesc);
}

static class ICodeVersionsExtensions
{
    /* adds */ static NativeCodeVersionHandle GetActiveNativeCodeVersion(this ICodeVersions cv, TargetPointer methodDesc);
}

GetActiveNativeCodeVersion can be implemented in terms of existing ICodeVersions methods GetActiveILCodeVersion and GetActiveNativeCodeVersionForILCodeVersion. With the goal of keeping the contract surface area as small as possible, I think it would be best to remove this method and implement as an extension method on ICodeVersions. Since it only relies on the existing contract APIs, the extension method does not need to be versioned.

Adds to IReJIT contract

interface IReJIT
{
    /* adds */ RejitState GetRejitState(ILCodeVersionHandle codeVersionHandle);

    /* adds */ TargetNUInt GetRejitId(ILCodeVersionHandle codeVersionHandle);
}

static class IReJITExtensions
{
    /* adds */ static IEnumerable<TargetNUInt> GetRejitIds(this IReJIT rejit, Target target, TargetPointer methodDesc);
}

Adds fields to existing cDAC Data Types

  • Adds FirstVersionNode pointer to ILCodeVersioningState. This is used to iterate the linked list of explicit ILCodeVersionNodes`.
  • Adds Next pointer to ILCodeVersionNode to iterate the linked list of explicit nodes.
  • Adds RejitState uint to ILCodeVersionNode`

Testing

Tested manually using the ReJIT tooling in the coreclr rejit test. I ran the test in WinDBG and debugged WinDBG in VS to verify the SOS calls had the expected behavior.

Contributes to #109426

Copy link
Contributor

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

@@ -7,10 +7,20 @@ namespace Microsoft.Diagnostics.DataContractReader;


[DebuggerDisplay("{Hex}")]
public readonly struct TargetNUInt
public readonly struct TargetNUInt : IEquatable<TargetNUInt>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds IEquatable to make comparing TargetNUInts less verbose.


kSuppressParams = 0x80000000
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Debated making this part of the data type, but didn't want to tie it in case the values change. Hiding them in the ReJIT implementation means we need a second copy for testing.

bool IsEnabled() => throw new NotImplementedException();

RejitState GetRejitState(ILCodeVersionHandle codeVersionHandle) => throw new NotImplementedException();
Copy link
Member

Choose a reason for hiding this comment

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

With the cdac I feel like we should either make an effort to make the data structures and algorithms match the runtime directly or make a conscious effort to have a higher level API that abstracts away some of the details. Putting GetRejitState and GetRejitId here doesn't feel like it accomplishes either of those tasks. In the runtime they are part of ILCodeVersion and in the cdac they are now part of a different conceptual contract, but we don't change the abstraction level.

Curious to know what other people's thoughts are here.

Comment on lines +72 to +77
IRuntimeTypeSystem rts = _target.Contracts.RuntimeTypeSystem;
MethodDescHandle md = rts.GetMethodDescHandle(methodDesc);
TargetPointer mtAddr = rts.GetMethodTable(md);
TypeHandle typeHandle = rts.GetTypeHandle(mtAddr);
TargetPointer module = rts.GetModule(typeHandle);
uint methodDefToken = rts.GetMethodToken(md);
Copy link
Member

Choose a reason for hiding this comment

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

I see these lines repeated 3-4 times, could we have a GetModuleAndMethodDef helper?

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.

3 participants