Improve handling of dynamic types in BulkType events #116629
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Dynamically allocated types (e.g. result of `MakeGenericType`) don't have debug information associated with them so we can't really reconstruct their names from PDB. However, we know the canonical form of the type and that one does have debug info. Emit RVA of the canonical form instead a non-sensical number. Consumers can still distinguish this is a different thing using the `TypeID` field (that is still the MethodTable pointer). The canonical form is only used for `TypeNameID`, which is RVA in native AOT case and definition token in the JIT case. The consumer can quickly see that `ModuleID + `TypeNameID != TypeID`. We could generate enough information to fully reconstruct the type name because the event can actually carry composition information, but that support was deleted in dotnet#94673 on native AOT side and it's currently not consumed by any tools I know of. It can be done later; Project N went to retirement with not even having the canonical forms in traces.
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances dynamic type support in BulkType events by emitting the RVA of a type’s canonical template rather than an arbitrary pointer value. Key changes include:
- Added
GetDynamicTemplateTypeand extendedGetFieldOffsetto support function-pointer parameters and dynamic template types. - Updated
EETypeFieldenum, flags, andKindsinMethodTable.hfor function pointers and dynamic-type metadata. - Modified
BulkTypeEventLogger::LogSingleTypeto calculateTypeNameIDfrom the canonical template for dynamic types.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/coreclr/nativeaot/Runtime/inc/MethodTable.inl | Added GetDynamicTemplateType and extended GetFieldOffset logic. |
| src/coreclr/nativeaot/Runtime/inc/MethodTable.h | Extended EETypeField, added flags/Kinds and helper methods. |
| src/coreclr/nativeaot/Runtime/eventtrace_bulktype.cpp | Adjusted TypeNameID calculation to use dynamic template types. |
Comments suppressed due to low confidence (4)
src/coreclr/nativeaot/Runtime/inc/MethodTable.inl:127
- GetFieldOffset does not handle the newly introduced ETF_DynamicTypeFlags (and subsequent dynamic fields), causing these enum values to hit the NYI assertion. Implement cases for ETF_DynamicTypeFlags, ETF_DynamicGcStatics, ETF_DynamicNonGcStatics, and ETF_DynamicThreadStaticOffset.
ASSERT(!"NYI");
src/coreclr/nativeaot/Runtime/eventtrace_bulktype.cpp:322
- The new dynamic template type path in LogSingleType isn’t covered by existing tests. Please add tests verifying the computed TypeNameID for dynamic types.
ULONGLONG rvaType = ULONGLONG(pTypeForRva) - osModuleHandle;
src/coreclr/nativeaot/Runtime/inc/MethodTable.h:287
- [nitpick] Consider marking GetDynamicTemplateType as __forceinline to match other inline methods and avoid call overhead in performance-critical paths.
MethodTable* GetDynamicTemplateType();
src/coreclr/nativeaot/Runtime/eventtrace_bulktype.cpp:321
- Add an assertion (e.g., ASSERT(pTypeForRva)) to ensure GetDynamicTemplateType returns a valid pointer before computing the RVA.
MethodTable* pTypeForRva = pEEType->IsDynamicType() ? pEEType->GetDynamicTemplateType() : pEEType;
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Dynamically allocated types (e.g. result of
MakeGenericType) don't have debug information associated with them so we can't really reconstruct their names from PDB. However, we know the canonical form of the type and that one does have debug info. Emit RVA of the canonical form instead a non-sensical number.Consumers can still distinguish this is a different thing using the
TypeIDfield (that is still the MethodTable pointer). The canonical form is only used forTypeNameID, which is RVA in native AOT case and definition token in the JIT case. The consumer can quickly see thatModuleID +TypeNameID != TypeID`.We could generate enough information to fully reconstruct the type name because the event can actually carry composition information, but that support was deleted in #94673 on native AOT side and it's currently not consumed by any tools I know of. It can be done later; Project N went to retirement with not even having the canonical forms in traces.
Cc @dotnet/ilc-contrib