-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
base: main
Are you sure you want to change the base?
Conversation
Tagging subscribers to this area: @tommcdon |
cbb0de3
to
b5f76de
Compare
02dafce
to
6610aa6
Compare
6610aa6
to
d4de7c9
Compare
....Diagnostics.DataContractReader.Abstractions/Contracts/Extensions/ICodeVersionsExtensions.cs
Outdated
Show resolved
Hide resolved
@@ -7,10 +7,20 @@ namespace Microsoft.Diagnostics.DataContractReader; | |||
|
|||
|
|||
[DebuggerDisplay("{Hex}")] | |||
public readonly struct TargetNUInt | |||
public readonly struct TargetNUInt : IEquatable<TargetNUInt> |
There was a problem hiding this comment.
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 TargetNUInt
s less verbose.
|
||
kSuppressParams = 0x80000000 | ||
} | ||
|
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
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); |
There was a problem hiding this comment.
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?
Adds ReJIT support to
SOSDacImpl::GetMethodDescData
.Modifies ICodeVersions contract
GetActiveNativeCodeVersion
can be implemented in terms of existingICodeVersions
methodsGetActiveILCodeVersion
andGetActiveNativeCodeVersionForILCodeVersion
. 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 onICodeVersions
. Since it only relies on the existing contract APIs, the extension method does not need to be versioned.Adds to IReJIT contract
Adds fields to existing cDAC Data Types
FirstVersionNode
pointer toILCodeVersioningState. This is used to iterate the linked list of explicit
ILCodeVersionNodes`.Next
pointer toILCodeVersionNode
to iterate the linked list of explicit nodes.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