Skip to content

Implement DacDbi cDAC APIs and simplify generic type context APIs#128263

Merged
rcj1 merged 2 commits into
mainfrom
copilot/implement-dacdbi-cdac-apis
May 16, 2026
Merged

Implement DacDbi cDAC APIs and simplify generic type context APIs#128263
rcj1 merged 2 commits into
mainfrom
copilot/implement-dacdbi-cdac-apis

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 15, 2026

Implements four DacDbi APIs in the managed cDAC.

cDAC DacDbi implementations

  • GetSimpleType — resolves CorElementType to its metadata token + module via _target.Contracts.RuntimeTypeSystem.
  • ResolveAssembly — looks up an assembly ref token through the loader contract.
  • ResolveExactGenericArgsToken — walks the generic dictionary to recover the exact instantiation token.
  • GetGenericArgTokenIndex — mirrors the native three-state result (S_FALSE / *pIndex = 0 / *pIndex = TYPECTXT_ILNUM).

Each implementation is gated by the existing #if DEBUG legacy cross-check pattern.

…actGenericArgsToken, GetGenericArgTokenIndex

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>
Copilot AI self-assigned this May 15, 2026
Copilot AI review requested due to automatic review settings May 15, 2026 17:09
Copilot AI review requested due to automatic review settings May 15, 2026 17:09
@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.

@rcj1 rcj1 marked this pull request as ready for review May 15, 2026 19:51
Copilot AI review requested due to automatic review settings May 15, 2026 19:51
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@max-charlamb
Copy link
Copy Markdown
Member

max-charlamb commented May 15, 2026

I just noticed there is a small pre-existing divergence in RequiresInstArg that we should probably fix.

Native MethodDesc::AcquiresInstMethodTableFromThis excludes only non-abstract interface methods (i.e., DIMs):

 return
     IsSharedByGenericInstantiations()  &&
     !HasMethodInstantiation() &&
     !IsStatic() &&
     !GetMethodTable()->IsValueType() &&
     !(GetMethodTable()->IsInterface() && !IsAbstract());

The cDAC's RequiresInstArg drops the !IsAbstract qualifier and returns true for any interface method.
For a shared generic abstract interface method, this PR's GetGenericArgTokenIndex will return *pIndex = TYPECTXT_ILNUM, while native returns *pIndex = 0 (acquires from this).

@rcj1
Copy link
Copy Markdown
Contributor

rcj1 commented May 15, 2026

I just noticed there is a small pre-existing divergence in RequiresInstArg that we should probably fix.

Native MethodDesc::AcquiresInstMethodTableFromThis excludes only non-abstract interface methods (i.e., DIMs):

 return
     IsSharedByGenericInstantiations()  &&
     !HasMethodInstantiation() &&
     !IsStatic() &&
     !GetMethodTable()->IsValueType() &&
     !(GetMethodTable()->IsInterface() && !IsAbstract());

The cDAC's RequiresInstArg drops the !IsAbstract qualifier and returns true for any interface method. For a shared generic abstract interface method, this PR's GetGenericArgTokenIndex will return *pIndex = TYPECTXT_ILNUM, while native returns *pIndex = 0 (acquires from this).

I saw that too, PR will be up shortly

Edit: Your suggestion on the generics API sounds good, I think I will just take that and incorporate the changes into this PR

@rcj1 rcj1 changed the title Implement DacDbi cDAC APIs and document IsSharedByGenericInstantiations Implement DacDbi cDAC APIs and simplify generic type context APIs May 16, 2026
@rcj1
Copy link
Copy Markdown
Contributor

rcj1 commented May 16, 2026

/ba-g timeout

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.

4 participants