Differentiate MethodTables that are/aren't visible to reflection
#112782
Conversation
There was a problem hiding this comment.
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericDefinitionEETypeNode.cs:67
- Confirm that using the 'Marked' property on the constructed type symbol is the correct indicator to decide whether to skip emitting the object node for reflection-invisible generic definitions. An incorrect check here may result in the wrong emission state.
return factory.ConstructedTypeSymbol(_type).Marked;
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs:1138
- Ensure that comparing 'this' with factory.MaximallyConstructableType(_type) reliably distinguishes between using a constructed versus necessary type symbol. An unexpected outcome here could lead to incorrect dependency emission.
IEETypeNode typeDefNode = factory.MaximallyConstructableType(_type) == this ?
| @@ -591,7 +591,7 @@ private IEETypeNode CreateNecessaryTypeNode(TypeDesc type) | |||
| { | |||
| if (type.IsGenericDefinition) | |||
| { | |||
| return new GenericDefinitionEETypeNode(this, type); | |||
| return new ReflectionInvisibleGenericDefinitionEETypeNode(this, type); | |||
There was a problem hiding this comment.
Review whether treating all generic definitions as reflection invisible on the necessary type path is intentional. This change could inadvertently hide types intended for reflection, so ensure it aligns with downstream requirements.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/ConstructedEETypeNode.cs
Outdated
Show resolved
Hide resolved
...clr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/GenericDefinitionEETypeNode.cs
Show resolved
Hide resolved
MichalStrehovsky
force-pushed
the
fix112029
branch
from
2c40921 to
a73abd2
Compare
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Fixes #112029.
We use constructed/necessary
MethodTables to distinguish between MTs that are visible to user code vs those that aren't. We use unconstructed MTs for things like casts ortypeofcomparisons, and some of our data structures. We use constructed MTs for everything else. Necessary MT can skip generating some of the data such as GCDesc, vtable, or metadata.The constructed/necessary
MethodTables get reconciled at object writing - if anything needed a constructed one, all references to necessary MTs gets rewritten to point to the constructed one (we never have two MTs for a single type).Before this PR, we didn't have a "constructed" MT for uninstantiated generic type definitions. That's because those don't have GCDesc or vtable. But they still have metadata, so there is a possible saving.
This introduces a "constructed"/necessary distinction for generic type definition MTs as well. The difference is in the availability of metadata.
It fixes cases like in the referenced bug where using a generic type definition in a typeof comparison (that we normally optimize) doesn't lead to the metadata being optimized away.
Cc @dotnet/ilc-contrib