Skip to content

[cDAC] Fix IsSharedByGenericInstantiations for non-shared canonical MTs#129721

Merged
max-charlamb merged 1 commit into
mainfrom
fix/generic-loc-issues
Jun 23, 2026
Merged

[cDAC] Fix IsSharedByGenericInstantiations for non-shared canonical MTs#129721
max-charlamb merged 1 commit into
mainfrom
fix/generic-loc-issues

Conversation

@max-charlamb

Copy link
Copy Markdown
Member

The cDAC's IsSharedByGenericInstantiations fell back to IsCanonMT && HasInstantiation, which is too permissive: a value-type generic instantiation like ArraySortHelper<int> is its own canonical MT (value-type instantiations don't share with __Canon), so the check returned true and GetGenericContextLoc reported InstArgMethodTable for methods that don't actually need an inst arg.

This mirrors the runtime's MethodTable::IsSharedByGenericInstantiations (methodtable.inl:1044): GenericsMask bits == enum_flag_GenericsMask_SharedInst. The cDAC's MethodTableFlags_1 was missing the SharedInst (0x20) value of the GenericsMask bits; this adds it (the GenericInst 0x10 value is not used by the contract so left out per usual cDAC convention) and uses it directly.

Methods now correctly classified (previously reported a spurious TYPE_PARAM):

  • ArraySortHelper<int>.cctor() / .CreateArraySortHelper()
  • ArraySortHelper<ulong>.cctor() / .CreateArraySortHelper()
  • ReadOnlySpan<char>.ToString()
  • Dictionary.CollectionsMarshalHelper.GetValueRefOrAddDefault(...)

Also updates docs/design/datacontracts/RuntimeTypeSystem.md to reflect the new flag value and the simplified implementation, and the [cDAC] [RuntimeTypeSystem] dependency comment in src/coreclr/vm/methodtable.h.

Surfaced by the cdacstress ArgIterator sub-check (DOTNET_CdacStress=0x201).

Note

This PR description was authored with assistance from GitHub Copilot.

…shared canonical MTs

The cDAC's IsSharedByGenericInstantiations fell back to `IsCanonMT && HasInstantiation`, which is too permissive: a value-type generic instantiation like ArraySortHelper<int> is its own canonical MT (value-type instantiations don't share with __Canon), so the check returned true and GetGenericContextLoc reported InstArgMethodTable for methods that don't actually need an inst arg.

Mirror the runtime's MethodTable::IsSharedByGenericInstantiations (methodtable.inl:1044): GenericsMask bits == enum_flag_GenericsMask_SharedInst. The cDAC's MethodTableFlags_1 was missing the SharedInst (0x20) value of the GenericsMask bits; add it (the GenericInst 0x10 value is not used by the contract so left out per usual cDAC convention) and use it directly.

Methods now correctly classified (previously reported a spurious TYPE_PARAM):
  ArraySortHelper<int>.cctor() / .CreateArraySortHelper()
  ArraySortHelper<ulong>.cctor() / .CreateArraySortHelper()
  ReadOnlySpan<char>.ToString()
  Dictionary.CollectionsMarshalHelper.GetValueRefOrAddDefault(...)

Update docs/design/datacontracts/RuntimeTypeSystem.md to reflect the new flag value and the simplified IsSharedByGenericInstantiations implementation. Update the [cDAC] [RuntimeTypeSystem] dependency comment in src/coreclr/vm/methodtable.h to also call out enum_flag_GenericsMask_SharedInst.

Surfaced by the cdacstress ArgIterator sub-check (DOTNET_CdacStress=0x201).

> [!NOTE]
> This commit was authored with assistance from GitHub Copilot.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 22, 2026 23:58
@max-charlamb max-charlamb changed the title [cdac RuntimeTypeSystem] Fix IsSharedByGenericInstantiations for non-shared canonical MTs [cDAC] Fix IsSharedByGenericInstantiations for non-shared canonical MTs Jun 22, 2026
@max-charlamb max-charlamb marked this pull request as ready for review June 22, 2026 23:58

@hoyosjs hoyosjs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a bug fix perspective, makes sense

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR corrects the cDAC RuntimeTypeSystem contract’s notion of “shared by generic instantiations” by using the runtime’s GenericsMask == SharedInst bit pattern instead of the broader IsCanonMT && HasInstantiation heuristic, and aligns documentation/comments with the updated contract flag value.

Changes:

  • Add GenericsMask_SharedInst (0x20) to the contract’s MethodTableFlags_1 and expose IsSharedByGenericInstantiations.
  • Update RuntimeTypeSystem_1.IsSharedByGenericInstantiations to use the new flag-derived property.
  • Update RuntimeTypeSystem contract documentation and the CoreCLR dependency comment to reflect the new flag value.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/RuntimeTypeSystemHelpers/MethodTableFlags_1.cs Adds GenericsMask_SharedInst and a corresponding IsSharedByGenericInstantiations helper to match runtime semantics.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs Switches shared-generic detection to the new flag-based check for correct GetGenericContextLoc classification.
src/coreclr/vm/methodtable.h Updates the [cDAC] [RuntimeTypeSystem] dependency comment to include SharedInst.
docs/design/datacontracts/RuntimeTypeSystem.md Documents the new flag value and the simplified shared-generic detection logic.

@max-charlamb

Copy link
Copy Markdown
Member Author

/ba-g known issue: #129705

@max-charlamb max-charlamb merged commit c143e87 into main Jun 23, 2026
139 of 150 checks passed
@max-charlamb max-charlamb deleted the fix/generic-loc-issues branch June 23, 2026 15:41
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants