Use CodeVersioning for EnC - #130159
Conversation
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR extends the existing code-versioning infrastructure so Edit-and-Continue (EnC) edits are represented as explicit IL code versions (distinct from ReJIT), and updates DAC/cDAC and debugger plumbing to treat only true ReJIT versions as ReJIT.
Changes:
- Adds a “source” discriminator for IL code versions (ReJIT vs EnC) plus an EnC version counter, and threads this through CoreCLR, data descriptors, and cDAC contracts.
- Updates ReJIT enumeration / DAC-facing ReJIT APIs to exclude EnC code versions (and excludes the synthetic default version from ReJIT ID enumeration).
- Adjusts IL retrieval paths in legacy cDAC/DAC DBI paths to read IL from the active EnC code version where appropriate; updates/extends unit tests and contract docs.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tests/UnitTests/ReJITTests.cs | Updates expected ReJIT ID enumeration behavior and mocks ICodeVersions.IsReJIT. |
| src/native/managed/cdac/tests/UnitTests/MockDescriptors/MockDescriptors.CodeVersions.cs | Extends mock IL code version node layout with Source/EnCVersion; updates builder helpers. |
| src/native/managed/cdac/tests/UnitTests/CodeVersionsTests.cs | Adds coverage asserting EnC/default versions are excluded from ReJIT detection and enumeration. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs | Filters ReJIT-related legacy SOS DAC behaviors using ICodeVersions.IsReJIT. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs | Fetches IL from active EnC code version when available; filters IL code version node APIs to ReJIT only. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataFrame.cs | Uses active EnC IL for local signature parsing when applicable. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/ILCodeVersionNode.cs | Adds Source and EnCVersion fields to the ILCodeVersionNode contract data type. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CodeVersions_1.cs | Implements ICodeVersions.IsReJIT based on the new ILCodeVersionNode.Source. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ICodeVersions.cs | Introduces public CodeVersionSource and new ICodeVersions.IsReJIT API. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/Extensions/IReJITExtensions.cs | Filters ReJIT ID enumeration to true ReJIT versions via cv.IsReJIT. |
| src/coreclr/vm/rejit.cpp | Tags configured ReJIT IL code versions with CodeVersionSource::kReJIT; filters ReJIT ID queries. |
| src/coreclr/vm/method.hpp | Makes EnC IL methods versionable via CodeVersionManager; enforces EnC/ReJIT mutual exclusion. |
| src/coreclr/vm/method.cpp | Returns active EnC IL from CodeVersionManager in MethodDesc::GetILHeader. |
| src/coreclr/vm/encee.cpp | Creates/activates EnC IL code versions via CodeVersionManager; records source and EnC version. |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Adds ILCodeVersionNode.Source and ILCodeVersionNode.EnCVersion to the CDAC descriptor. |
| src/coreclr/vm/codeversion.h | Adds CodeVersionSource enum and stores Source/EnCVersion on ILCodeVersionNode. |
| src/coreclr/vm/codeversion.cpp | Implements ILCodeVersion::IsReJIT / GetEnCVersion, plus node accessors/mutators. |
| src/coreclr/debug/ee/functioninfo.cpp | Treats only ReJIT (not EnC) explicit versions as “rejitted” for boundary mapping behavior. |
| src/coreclr/debug/ee/debugger.cpp | Updates debugger ReJIT-related checks to ignore EnC versions; tags created versions as ReJIT. |
| src/coreclr/debug/daccess/request.cpp | Filters DAC ReJIT data paths to only ReJIT IL code versions. |
| src/coreclr/debug/daccess/dacdbiimpl.cpp | Uses CodeVersionManager to resolve EnC versions and EnC IL in DBI helper paths. |
| docs/design/datacontracts/CodeVersions.md | Documents the new CodeVersionSource/IsReJIT concepts (but needs an EnCVersion row added). |
Copilot's findings
- Files reviewed: 22/22 changed files
- Comments generated: 4
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/coreclr/debug/daccess/request.cpp:4693
- GetPendingReJITID now rejects the active ILCodeVersion unless ilVersion.IsReJIT() is true. However newly-requested ReJIT versions start in kStateRequested with CodeVersionSource still kUnknown (it isn't set until later in ConfigureILCodeVersion), so pending ReJIT queries will incorrectly return E_INVALIDARG. Reorder the checks to treat kStateRequested as a ReJIT-pending version even before Source is populated, and only use IsReJIT() to reject non-requested non-ReJIT (e.g., EnC) versions.
CodeVersionManager::LockHolder codeVersioningLockHolder;
ILCodeVersion ilVersion = pCodeVersionManager->GetActiveILCodeVersion(pMD);
if (ilVersion.IsNull() || !ilVersion.IsReJIT())
{
hr = E_INVALIDARG;
}
else if (ilVersion.GetRejitState() == RejitFlags::kStateRequested)
{
- Files reviewed: 24/24 changed files
- Comments generated: 3
| Module *pModule = GetModule(); | ||
|
|
||
| #ifdef FEATURE_CODE_VERSIONING | ||
| if (InEnCEnabledModule()) |
There was a problem hiding this comment.
Should both EnC and ReJIT be on the same plan? Either keep using dynamic IL to track last IL for both. Or drop dynamic IL for both and use dynamic IL for reflection emit only.
There was a problem hiding this comment.
ReJIT already uses the code version to store IL. It looks like we had just never captured ReJIT IL here. We probably should
There was a problem hiding this comment.
I wouldn't expect this method to get called for ReJIT methods and with the other changes in this PR I wouldn't expect ENC to call it either now. Unless there is something I am missing I'd suggest reverting this part of the change.
Both ReJIT and ENC should be getting their IL from the ILCodeVersion now:
ILCodeVersion::GetIL
VersionedPrepareCodeConfig::GetILHeader
GetAndVerifyMetadataILHeader
GetAndVerifyILHeader
JitCompileCodeLockedEventWrapper
JitCompileCode
PrepareCode
PublishVersionableCodeIfNecessary
DoPrestub
That VersionedPrepareCodeConfig is a derived type of PrepareCodeConfig that we use whenever compiling a non-default version of the code. The code in PublishVersionableCodeIfNecessary should detect the non-default version and use it.
There was a problem hiding this comment.
There are some other callers where I can't rule it out being called on an EnC method: for example, GetAndVerifyMetadataILHeader
There was a problem hiding this comment.
All calls to GetAndVerifyMetadataILHeader for a non-default version of a method are supposed to be using VersionedPrepareCodeConfig as the 'pConfig' argument which directs them down the code path I mentioned above and they don't get here. Do you see a path where VersionedPrepareCodeConfig won't be used for a non-default code version?
There was a problem hiding this comment.
There is also MethodDescGetILHeader and the various callers on the DAC side (task.cpp, stack.cpp).
There was a problem hiding this comment.
Ideally now that we have represented ENC code with explicit ILCodeVersions we should also only use version-aware handling of the IL. This ensures code paths get the IL that corresponds to particular version of the code they are introspecting as opposed to guessing that the default version or the latest version happens to be the right one. If that is a lot of work I don't mind if you include a comment saying the conversion is incomplete and assuming the caller wants the most recent ENC version is probably no worse than the behavior was in the past. However if we want to complete the conversion we should eliminate any code paths that call MethodDesc::GetILHeader() on non-default code versions.
For the MethodDescGetILHeader you mentioned the call path is:
-> Compiler::compCompile()
-> Compiler::eeGetVars()
-> ICorJitInfo::getVars / CEECodeGenInfo::getVars()
-> g_pDebugInterface->getVars(...)
-> Debugger::getVars(...)
-> g_pEEInterface->MethodDescGetILHeader(md)
-> MethodDesc::GetILHeader()
CEECodeGenInfo::getVars has access to the m_ILHeader for the exact version of the code being compiled so it could pass that along to Debugger::getVars() rather than having Debugger do a separate non-version-aware lookup.
There was a problem hiding this comment.
I see, that seems good. There are a few debugger scenarios where we currently do not handle EnC. It would be an improvement here, for example to fetch the correct IL for frames on the stack.
There was a problem hiding this comment.
MethodDescGetILHeader, however, since it is called upon compilation, should always return the active version.
There was a problem hiding this comment.
You'd still be relying on a race condition if you assume that the version of the method that was active when JIT compilation started is still the active version at a later point in compilation. Nothing guarantees that the active version doesn't change mid-compilation.
| #endif // !DACCESS_COMPILE | ||
| } | ||
|
|
||
| COR_ILMETHOD* MethodDesc::GetActiveILHeader() |
There was a problem hiding this comment.
I think it would be useful to make some more updates...
I am guessing we could hit this one by taking a dump of process where EnC or ReJIT was in use and loading it in windbg. GetActiveILHeader() is probably more often correct than GetILHeader() so its preferable to use that one. Regardless which API we call a comment would be useful explaining we don't have sufficient information to know what version of the code the caller actually wants.
runtime/src/coreclr/debug/daccess/task.cpp
Line 3573 in 7db4382
I think this one is unreachable and PrepareCodeConfig::GetILHeader() does return the right header. To reduce confusion it would be best to delete the if statement.
runtime/src/coreclr/vm/prestub.cpp
Line 716 in 7db4382
After some refactoring this one looks like dead code that will be removable (see comments in getVars):
runtime/src/coreclr/vm/eedbginterfaceimpl.cpp
Line 695 in 7db4382
These don't look reachable in normal builds but it would be nice to capture in comments that the correct behavior is to pass through the version being compiled, not assuming default version. If the build define ever were enabled then presumably these code paths would become reachable for ENC and its a bug at that point.
runtime/src/coreclr/vm/gdbjit.cpp
Line 233 in 7db4382
runtime/src/coreclr/vm/gdbjit.cpp
Line 1412 in 7db4382
Although this one probably isn't reachable for EnC it still looks like a bug that I'd rather fix. I assume right now if we ReJITed a method that had PGO data we'd incorrectly hash the original IL version and conclude the PGO data is valid even though the IL has changed. The hash should be based on the version of the code being compiled by the JIT, not the default version.
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ICodeVersions.cs:14
- This introduces new public API surface (
public enum CodeVersionSourceand newICodeVersionsmembers). Per the API approval verification procedure, new public APIs need to be linked to anapi-approvedissue; none is referenced in the PR metadata. Either link the approved API issue/proposal or make this new surfaceinternaluntil it’s approved.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:5200 - The null-output-pointer check throws
ArgumentExceptionwithout indicating which argument was null. This makes failures harder to diagnose and is inconsistent with nearby methods that includenameof(...)for pointer parameters.
- Files reviewed: 46/46 changed files
- Comments generated: 0 new
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:5241
- Null-output validation should include parameter names for consistency with the rest of this file (e.g., AreOptimizationsDisabled uses ArgumentException(..., nameof(...))). This keeps the resulting E_INVALIDARG easier to diagnose when surfaced through the PreserveSig COM boundary.
- Files reviewed: 46/46 changed files
- Comments generated: 0 new
Co-authored-by: Noah Falk <noahfalk@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CodeVersions_1.cs:420
GetSourcecasts the rawILCodeVersionNode.Sourcevalue directly toCodeVersionSource. If the target reports an unexpected value, this will leak an undefined enum value to callers. Consider validating/mapping unknown values back toCodeVersionSource.Unknown.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ICodeVersions.cs:13- This change introduces new public API surface (
public CodeVersionSourceand newICodeVersionsvirtual methods). Per dotnet/runtime policy, new public APIs require a linkedapi-approvedissue/proposal. If this API isn't approved yet, consider making it internal (or otherwise non-public) until the API review completes.
- Files reviewed: 46/46 changed files
- Comments generated: 3
| *pCodeInfo = default; | ||
| *pLocalSigToken = (uint)EcmaMetadataUtils.TokenType.mdtSignature; | ||
|
|
| TargetPointer ilHeader = TargetPointer.Null; | ||
| ICodeVersions cv = _target.Contracts.CodeVersions; | ||
| ILCodeVersionHandle activeVersion = cv.GetActiveILCodeVersion(mdh.Address); | ||
| if (activeVersion.IsValid && activeVersion.IsExplicit && cv.GetSource(activeVersion) != CodeVersionSource.ReJIT) | ||
| ilHeader = cv.GetIL(activeVersion); |
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs:5244
GetEnCILCodeAndSiginitializes*pLocalSigTokentomdtSignature(0x11000000). This is notmdSignatureNiland can be misinterpreted as a real token when no local sig exists or when no matching EnC version is found. The native DAC implementation initializes this output tomdSignatureNil(0) and only sets a token when present.
Initialize *pLocalSigToken to 0 (mdSignatureNil), and keep setting it to the actual local sig token only when found.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ICodeVersions.cs:15
- This change introduces new public API surface (
CodeVersionSourceenum, plusICodeVersions.GetSource/GetEnCVersion). The PR description doesn't reference any linked issue with theapi-approvedlabel, which is required for new public APIs in dotnet/runtime.
Either link the approved API review issue in the PR description, or make these APIs internal until API review is complete.
- Files reviewed: 47/47 changed files
- Comments generated: 0 new
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (2)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ICodeVersions.cs:15
- This change adds new public surface area to the cDAC Abstractions transport package (the public CodeVersionSource enum plus ICodeVersions.GetSource/GetEnCVersion). Per dotnet/runtime policy, new public API must be linked to an issue with the
api-approvedlabel (or the API should be kept internal until it’s approved). PR #130159 doesn’t reference anapi-approvedissue, so the approval status can’t be verified.
src/native/managed/cdac/tests/UnitTests/CodeVersionsTests.cs:660 - The new GetEnCVersion contract member is used by the legacy DacDbi implementation, but the updated unit test only validates GetSource. Adding assertions for GetEnCVersion here would cover both the EnC explicit version and the synthetic default version and make this test fail if the new behavior regresses.
- Files reviewed: 47/47 changed files
- Comments generated: 2
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot's findings
Comments suppressed due to low confidence (1)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/ICodeVersions.cs:14
- This change introduces new public API surface (
CodeVersionSourceplusICodeVersions.GetSource/GetEnCVersion). Per repo policy, new public APIs require an approved API proposal (typically an issue labeledapi-approved) or should be keptinternaluntil approved. Please either link the approved issue/proposal in the PR description or adjust the surface area to be non-public if it is not intended as a supported API.
- Files reviewed: 47/47 changed files
- Comments generated: 0 new
Follow-up on #128338 (comment).
Sourcefield toILCodeVersionNodeto distinguish the sources.