Remove development-only contract versions - #131790
Conversation
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR consolidates (“squashes”) several cDAC CoreCLR contract version lines back down to c1 by removing intermediate development-only versions and moving the latest semantics into the remaining c1 implementations, along with corresponding test and documentation updates.
Changes:
- Remove
c2/c3registrations and implementations for several contracts (e.g., PrecodeStubs, DebugInfo, ExecutionManager, StressLog) and keep a singlec1implementation. - Update CoreCLR/NativeAOT data descriptor contract version declarations to match the squashed numbering.
- Update unit/integration tests and design docs to reflect the new single-version contract surface.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/tools/CdacUsageGraph/tests/CdacUsageGraph.Tests/UsageWalkerIntegrationTests.cs | Updates usage-walker expectations to target c1 only and assert interpreter-precode reachability under c1. |
| src/native/managed/cdac/tests/UnitTests/PrecodeStubsTests.cs | Collapses tests to c1 only and adjusts mock descriptors/stub encoding for the new single-version PrecodeStubs behavior. |
| src/native/managed/cdac/tests/UnitTests/MockDescriptors/MockDescriptors.ExecutionManager.cs | Removes c2 nibble map builder selection. |
| src/native/managed/cdac/tests/UnitTests/FunctionTableAccessTests.cs | Removes “all versions” iteration; now tests only c1. |
| src/native/managed/cdac/tests/UnitTests/ExecutionManager/NibbleMapTests.cs | Switches test to NibbleMapTestBuilder_1 to match new c1 algorithm selection. |
| src/native/managed/cdac/tests/UnitTests/ExecutionManager/NibbleMapTestBuilder.cs | Removes NibbleMapTestBuilder_2 and updates _1 to implement constant-lookup nibble map behavior. |
| src/native/managed/cdac/tests/UnitTests/ExecutionManager/ExecutionManagerTests.cs | Removes c2 coverage; now tests only c1. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Data/StubPrecodeData.cs | Removes the _2 data shape and standardizes StubPrecodeData_1 to use SecretParam + Type. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/CoreCLRContracts.cs | Removes c2/c3 contract registrations; registers only c1 for the squashed contracts. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/StressLog.cs | Removes the old “small” reader path and makes StressLog_1 use the newer message encoding. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/PrecodeStubs_3.cs | Deletes the c3 implementation. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/PrecodeStubs_2.cs | Deletes the c2 implementation. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/PrecodeStubs_1.cs | Moves the newer precode detection/interpreter support behavior into c1. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManager_2.cs | Deletes the c2 implementation. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/ExecutionManager/ExecutionManager_1.cs | Switches c1 to the constant-lookup nibble map algorithm. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/DebugInfo/DebugInfo_2.cs | Deletes the c2 implementation. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/DebugInfo/DebugInfo_1.cs | Moves the newer debug-info decoding (fat header, vars, async info, 3-bit source flags) into c1. |
| src/coreclr/vm/datadescriptor/datadescriptor.inc | Renumbers several CDAC_GLOBAL_CONTRACT(..., c*) declarations down to c1. |
| src/coreclr/nativeaot/Runtime/datadescriptor/datadescriptor.inc | Renumbers StressLog contract reference from c2 to c1 for NativeAOT descriptors. |
| docs/design/datacontracts/StressLog.md | Removes the old version split and documents the remaining c1 semantics. |
| docs/design/datacontracts/PrecodeStubs.md | Removes multi-version diffs and updates the single c1 doc. |
| docs/design/datacontracts/ExecutionManager.md | Updates nibble map documentation to the constant-time lookup algorithm under c1. |
| docs/design/datacontracts/DebugInfo.md | Collapses versioned documentation to a single c1 spec matching the newer encoding/decoding. |
| docs/design/datacontracts/data-descriptor-meanings.json | Removes version-specific wording and aligns meanings with the squashed contract versions. |
max-charlamb
left a comment
There was a problem hiding this comment.
Would it be possible to do this in two parts to reduce the diff? We could remove all but the highest version contract, then have a second PR that renames the contracts to 1.
PR is updated to only do the contract removal without the renumbering. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/PrecodeStubs_3.cs:28
ThisPtrRetBufPrecode_GetMethodDescrepeats the stub-data address calculation; usingGetStubPrecodeDatahere keeps all stub-precode-data reads consistent and avoids duplication.
TargetPointer stubPrecodeDataAddress = instrPointer + precodeMachineDescriptor.StubCodePageSize;
Data.StubPrecodeData_2 stubPrecodeData = target.ProcessedData.GetOrAdd<Data.StubPrecodeData_2>(stubPrecodeDataAddress);
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/PrecodeStubs_3.cs:15
StubPrecode_GetMethodDescduplicates the stub-data address computation that's already encapsulated inGetStubPrecodeData(used byTryGetKnownPrecodeType). Using the helper here reduces duplication and keeps the address calculation consistent if it ever changes.
This issue also appears on line 27 of the same file.
TargetPointer stubPrecodeDataAddress = instrPointer + precodeMachineDescriptor.StubCodePageSize;
Data.StubPrecodeData_2 stubPrecodeData = target.ProcessedData.GetOrAdd<Data.StubPrecodeData_2>(stubPrecodeDataAddress);
return stubPrecodeData.SecretParam;
While developing data contracts we've mostly been making in-place breaking changes on v1 contracts but for a few we added multiple versions before our development time versioning policy was decided on. As preparation to lock down contracts we are simplifying by getting rid of contract versions that only existed during unsupported development builds. Going forward we don't expect a numbering reset like this to happen again.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ab1b53f7-74ef-44f9-8bdc-95691a2f5138
9d99cb6 to
b5788d1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/native/managed/cdac/tests/UnitTests/FunctionTableAccessTests.cs:23
- StdArchAllVersions() no longer returns “all versions” (it always yields only "c2"), which makes the name misleading and harder to understand when reading the test matrix. Consider renaming it (and all MemberData references) to reflect the single supported version.
public static IEnumerable<object[]> StdArchAllVersions()
{
foreach (object[] arr in new MockTarget.StdArch())
{
MockTarget.Architecture arch = (MockTarget.Architecture)arr[0];
yield return new object[] { "c2", arch };
}
src/native/managed/cdac/tests/UnitTests/ExecutionManager/ExecutionManagerTests.cs:920
- StdArchAllVersions() now yields only "c2". The helper name implies a version cross-product, but the data set is now single-version, which can confuse future edits (e.g., adding c3 would require revisiting this). Rename the helper and update the MemberData references accordingly.
public static IEnumerable<object[]> StdArchAllVersions()
{
foreach (object[] arr in new MockTarget.StdArch())
{
MockTarget.Architecture arch = (MockTarget.Architecture)arr[0];
yield return new object[] { "c2", arch };
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/native/managed/cdac/tests/UnitTests/PrecodeStubsTests.cs:192
- The mocked
PrecodeMachineDescriptorlayout here doesn’t match the actual CoreCLR layout (e.g.,src/coreclr/vm/precode.hincludesFixupPrecodeType,FixupCodeOffset, fixed-sizeFixupBytes/StubBytesarrays, and placesThisPointerRetBufPrecodeType/InterpreterPrecodeTypeafter those arrays). With c1/c2 removed, this test setup can produce field offsets that don’t reflect real targets, so the contract can appear correct under test while reading the wrong offsets on real dumps.
Consider updating the mock Target.TypeInfo layouts in this method to mirror the data-descriptor/native struct layouts (including “unused” intermediate fields and realistic fixed array sizes), so offsets for fields the contract reads are representative.
layout = targetTestHelpers.LayoutFields([
new(nameof(Data.PrecodeMachineDescriptor.StubCodePageSize), DataType.uint32),
new(nameof(Data.PrecodeMachineDescriptor.InvalidPrecodeType), DataType.uint8),
new(nameof(Data.PrecodeMachineDescriptor.StubPrecodeType), DataType.uint8),
new(nameof(Data.PrecodeMachineDescriptor.ThisPointerRetBufPrecodeType), DataType.uint8),
|
/ba-g build monitor is having issues |
Follow up to #131790 to renumber the contracts starting from 1.
While developing data contracts we've mostly been making in-place breaking changes on v1 contracts but for a few we added multiple versions before our development time versioning policy was decided on. As preparation to lock down contracts we are simplifying by getting rid of contract versions that only existed during unsupported development builds. A follow up PR will renumber all the contract versions to 1 (except StressLog where both contract versions already have use from the StressLogAnalyzer tool). We don't expect to do any renumbering similar to this in the future as the versioning rules will be active even during development.