[cDAC] Update DebugInfo contract for new format

[max-charlamb]

See context for changes: #120303 (comment)

markdown lint failure is unrelated and fixed in: #121421

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull Request Overview

This pull request introduces version 2 of the DebugInfo contract with a unified header format, while refactoring common code into a helper class. The new version replaces the flag byte approach of version 1 with a fat/slim chunk table for better extensibility.

• Adds DebugInfo_2 implementing a new header format that encodes chunk sizes in nibble format
• Extracts the bounds decoding logic into DebugInfoHelpers.DoBounds() for code reuse between versions
• Updates documentation to describe the version 2 header encoding format
• Removes PatchpointInfo data descriptors and related CDAC infrastructure

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/DebugInfo/DebugInfo_2.cs	Implements version 2 of the DebugInfo contract with unified fat/slim header format for chunk size encoding
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/DebugInfo/DebugInfo_1.cs	Refactored to use shared DebugInfoHelpers.DoBounds() method
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/DebugInfo/DebugInfoHelpers.cs	New helper class containing shared bounds decoding logic with parameterized IL offset bias
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/DebugInfo/DebugInfoFactory.cs	Registers version 2 implementation in the factory
src/coreclr/vm/datadescriptor/datadescriptor.inc	Removes PatchpointInfo type definition from CDAC data descriptors
src/coreclr/inc/patchpointinfo.h	Removes CDAC-related template specialization and friend declarations
docs/design/datacontracts/DebugInfo.md	Documents version 2 header encoding and removes incorrect CodeVersions contract reference from version 1

Comments suppressed due to low confidence (1)

src/coreclr/vm/datadescriptor/datadescriptor.inc:667

• The removal of the PatchpointInfo type will break DebugInfo_1 which still depends on it. The DebugInfo_1.cs implementation (lines 51-55) uses DataType.PatchpointInfo to read patchpoint information when the EXTRA_DEBUG_INFO_PATCHPOINT flag is set. This type definition should not be removed unless DebugInfo_1 is also updated to not rely on it, or Version 1 is being completely removed.

CDAC_TYPE_BEGIN(CodeHeapListNode)
CDAC_TYPE_FIELD(CodeHeapListNode, /*pointer*/, Next, offsetof(HeapList, hpNext))
CDAC_TYPE_FIELD(CodeHeapListNode, /*pointer*/, StartAddress, offsetof(HeapList, startAddress))
CDAC_TYPE_FIELD(CodeHeapListNode, /*pointer*/, EndAddress, offsetof(HeapList, endAddress))
CDAC_TYPE_FIELD(CodeHeapListNode, /*pointer*/, MapBase, offsetof(HeapList, mapBase))
CDAC_TYPE_FIELD(CodeHeapListNode, /*pointer*/, HeaderMap, offsetof(HeapList, pHdrMap))
CDAC_TYPE_END(CodeHeapListNode)

[noahfalk]

@davidwr @jakobbotsch - I noticed you were chatting about the extra bit on the other PR. These four source types are all mutually exclusive so they are encodable in only two bits if you'd like to update the runtime implementation. The interface defines them as a bit field implying they could be combined, but they can't in practice. (Technically the 'Invalid' value and the 'StackEmpty' value are combinable, buts its fine to encode that combination as 'StackEmpty')

[max-charlamb]

I think this is a reasonable discussion to have, but would like to push this out of scope for the current PR. If this changes, we can update cDAC version 2 contract.

[noahfalk]

Although the encoding allows this combination to be represented, I'd never expect to see it. The bit patterns I'd expect to see are 0, 1, 2, and 4. We can either change the runtime implementation to make the combinations unrepresentable (a 4 value enumeration in 2 bits), or we could document it as-is.

[jakobbotsch]

STACK_EMPTY | CALL_INSTRUCTION is produced by the JIT today -- in debug codegen when a call also happens to be the stack empty position.
What mappings would you expect to see in that case? It is odd to me that the source types are flags in the first place if this is not an expected possibility.

[noahfalk]

Interesting, we've probably got some bugs then as different parts of our stack don't agree on the design. Other parts of the code assume CALL_INSTRUCTION is mutually exclusive with everything else (Example1, Example2).

I think this is the history which helps the current state make a little more sense, but we've still arrived at an odd spot:

• Initially the source types represented different reasons why a sequence point could be created (SOURCE_TYPE_INVALID, STACK_EMPTY, CALL_SITE, SEQUENCE_POINT). It is legal for these to be combined so the original design was flags.
• Later folks needed to identify the call instruction assembly instruction for an IL call instruction and since we already had a table with (IL offset, native offset) tuples they decided to add these in as well with a new CALL_INSTRUCTION flag value.
• Initially CALL_INSTRUCTION entries were also used for general IL<->native mapping. That created a bunch of bugs and we figured out CALL_INSTRUCTION entries could not be treated as sequence points. They had their own distinct meaning and just happened to be sharing the same table as an encoding convenience. This is the point where all the debugger code started isolating them and assuming they were mutually exclusive from the other flags. If the JIT code was still allowing them to be merged then we probably exchanged our more obvious bugs for less obvious bugs rather than fixing the issue entirely.
• Most recently we added ASYNC and because it only applies to IPs in the suspend/resume region of the jitted code I expected it would be mutually exclusive with the others.

So we started with a bunch of flags that were combinable, then added a couple more that weren't, and then our compression scheme dropped all but one of the combinable flags. Thats how we ended up with a flags enum where everything appears mutually exclusive in the compressed state.

I think we've got two options, I'm not sure which one encodes better:

1. Allow CALL_INSTRUCTION to merge with other mappings. This would require at least 3 source type bits + debugger changes in a few places. It also requires that JIT sequence points for NOP and CALLSITE_BOUNDARIES have an explicit bit representation if it is possible that they too could be merged with a CALL_INSTRUCTION. Today I assume a CALL_SITE | CALL_INSTRUCTION union would get represented as CALL_INSTRUCTION in the compressed debug info. That would make it impossible for the debugger to tell whether the mapping should be treated as a sequence point.
2. Encode CALL_INSTRUCTION tuples separately from the other tuples, even if they have identical IL and native offsets. 2 bits should be sufficient for source type, the debugger code won't change, but some JIT code would.

Jakob, is one of those substantially easier/harder for the JIT, or is there a substantial difference in compressed debug info size? From the debugger side option (2) saves us work, but its not that hard to do (1) if we need to.

[jakobbotsch]

Thats how we ended up with a flags enum where everything appears mutually exclusive in the compressed state.

The old compression scheme does allow for this specific combination:

runtime/src/coreclr/vm/debuginfostore.cpp

Lines 567 to 569 in c99d384

	case (int)(ICorDebugInfo::CALL_INSTRUCTION | ICorDebugInfo::STACK_EMPTY):
	sourceBits = 3;
	break;

Jakob, is one of those substantially easier/harder for the JIT, or is there a substantial difference in compressed debug info size? From the debugger side option (2) saves us work, but its not that hard to do (1) if we need to.

I would prefer that we do 2 and then also update the enum to make it clear that this is a singular "source reason" for why a mapping was created. These mappings are already created through a different channel in the JIT, so this may not be much work in the JIT either. In fact it is probably as simple as skipping STACK_EMPTY for these mappings; the merging behavior may already be right.

Should we update ICorDebugInfo::SourceTypes as well? I have lived under the wrong assumption about these values for 5 years now based on the flags and the comment on the source line you linked to above. In the end we would have STACK_EMPTY, CALL_SITE, CALL_INSTRUCTION and ASYNC only in ICorDebugInfo::SourceTypes.

If the specific values are preferred to be kept because consumers depend on them exactly (and we don't want to have an intermediate translation), then I guess we should just make it clear from the comment on the enum.

[noahfalk]

Should we update ICorDebugInfo::SourceTypes as well?

That sounds good to me assuming we don't discover any other surprises.

In the end we would have STACK_EMPTY, CALL_SITE, CALL_INSTRUCTION and ASYNC

I think we might want slightly adjusted names: SEQUENCE_POINT_STACK_EMPTY, SEQUENCE_POINT_OTHER, CALL_INSTRUCTION and ASYNC. SEQUENCE_POINT_OTHER would include not only CALL_SITE but also NOP or explicit sequence points.

[noahfalk]

Rather than defining a new implementation that replicates most of the code from V1, what if we modify the existing DebugInfo_1 into DebugInfo_1_To_2(Target target, int version) and give it a little bit of conditional behavior in the right places? While I don't think this example is bad on its own, I'm hoping we can avoid having cDAC grow into something that has lots of duplicated code as contracts keep versioning. We always have the option to create a new type if we need it, but I hope we can reserve that for situations where the new version is substantially different.

[max-charlamb]

While this is a small contract, the bulk of the implementation is changing (everything but fetching the DebugInfo pointer and the Bounds reading), I had a previous implementation that shared some bounds reading but found it more confusing and potentially prone to errors.

I can move back to that approach but believe the memory saved will be relatively minimal.

[noahfalk]

if sharing a type doesn't actually result in sharing much code then a new type is just fine.

[davidwrighton]

Maybe instead of uint sourceTypeBits we could pass the version number down here and do the right thing with it. That would make it easy to adjust the source type bits details in a future PR.