Conversation
Implement proper cDAC reading for the ReJITOnAttach setting dotnet#106148
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR fixes ReJIT “enabled” detection when running under DAC/cDAC by caching the ProfAPI_RejitOnAttach configuration in the target runtime and teaching cDAC to read that cached value instead of (incorrectly) using the debugger process environment.
Changes:
- Cache
EXTERNAL_ProfAPI_RejitOnAttachintoProfControlBlock::fRejitOnAttachEnabledduring profiling initialization and use it forReJitManager::IsReJITEnabled()in DAC contexts. - Extend the cDAC data descriptor + managed
ProfControlBlockreader and update the ReJIT contract implementation to useRejitOnAttachEnabled. - Add cDAC tests and update mock descriptors for the new
ProfControlBlockfield; update the ReJIT contract design doc.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/ReJITTests.cs | Adds tests for IReJIT.IsEnabled() based on the RejitOnAttach setting. |
| src/native/managed/cdac/tests/MockDescriptors/MockDescriptors.ReJIT.cs | Extends mock ProfControlBlock layout and initializes RejitOnAttachEnabled. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ProfControlBlock.cs | Reads new RejitOnAttachEnabled field from target memory. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ReJIT_1.cs | Updates IsEnabled() to use _profControlBlock.RejitOnAttachEnabled. |
| src/coreclr/vm/rejit.inl | Makes ReJIT enablement checks DAC-safe by using cached runtime state. |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Adds ProfControlBlock.RejitOnAttachEnabled to the data descriptor and clarifies edit checklist. |
| src/coreclr/inc/profilepriv.inl | Initializes fRejitOnAttachEnabled from CLRConfig during ProfControlBlock::Init(). |
| src/coreclr/inc/profilepriv.h | Adds fRejitOnAttachEnabled field to ProfControlBlock. |
| docs/design/datacontracts/ReJIT.md | Updates ReJIT contract spec to include the new field/algorithm. |
...ive/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ProfControlBlock.cs
Outdated
Show resolved
Hide resolved
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ReJIT_1.cs
Show resolved
Hide resolved
|
I opened #124448 to discuss our policy for versioning. We'll need to decide if this change should be:
Rather than make a one-off decision for this PR only it seemed better to establish some policy because I'm sure there are likely many more cDAC changes to come. |
Change all BOOL member fields in ProfControlBlock to lowercase bool for consistency and reduced size. Update all assignment sites from TRUE/FALSE to true/false. Update the cDAC data descriptor type from uint32 to bool and the managed read from Read<uint> to Read<byte>. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
I would argue that this is an issue for after .NET 11, and that versioning WIP versions of the contract (and maintaining documentation as if they were complete/official versions) would create too much noise. While we should discuss the schema and test versioning (@max-charlamb s versioning of PrecodeStub was such a test), I think during initial development we should do it in a more controlled manner than at every non-breaking change. Perhaps we can test-version every X changes. |
|
I would avoid dealing with versioning holistically before we have cDAC v1. We want to be cleaning up the data structures as we work on cDAC v1 to make the data contracts reasonably clean. Dealing with versioning as we are doing that would defeat the point. |
I think there are two orthogonal questions around versioning:
I agree that (1) should wait until the contracts are more stable, likely late in the .NET 11 release. However I don't think that should cause us to ignore (2). If we decide we want minor versions (which I think would be good idea) doing it during .NET 11 is easy but delaying it until .NET 12 immediately creates a breaking change to add it. I raised the question now because this little change seemed like a good example of the kind of thing we'd like to add in a non-breaking way in the future, regardless whether we apply the versioning rules to it this time. |
Implement proper cDAC reading for the ReJITOnAttach setting #106148