Validate the Kept attribute against the presence of EEType in NativeAOT #81830
Conversation
Previously we validated the Kept on types against the presence of constructed EEType. But the compiler avoids constructed types as much as possible and the tests do use cases where the type in question is never instantiated (or fully static type). Validating against the presence of EEType (basically a method table) is much closer to the ILLink behavior which validates the presence of the type in IL metadata. Fixes dotnet#80408
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsPreviously we validated the Kept on types against the presence of constructed EEType. But the compiler avoids constructed types as much as possible and the tests do use cases where the type in question is never instantiated (or fully static type). Validating against the presence of EEType (basically a method table) is much closer to the ILLink behavior which validates the presence of the type in IL metadata. Fixes #80408
|
| } | ||
|
|
||
| void AddMethod (MethodDesc method) | ||
| void AddMethod (MethodDesc method) |
There was a problem hiding this comment.
nit: seems like now this line is not well indented
| @@ -691,6 +691,8 @@ public IEnumerable<TypeDesc> ConstructedEETypes | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| public DependencyAnalyzerBase<NodeFactory> Graph => _graph; | |||
There was a problem hiding this comment.
Could you expose this on CompilationResults instead? If the whole graph is public, there's no point to have CompilationResults.
I kind of like that IEETypeNode and IMethodBodyNode are implementation details. Going over the graph exposes e.g. the duality of Constructed/Unconstructed types (some types will appear twice).
There was a problem hiding this comment.
(I mean expose a way to get to the list of all method tables constructed or not. I don't mean a way to get to every marked node.)
| { | ||
| foreach (var node in MarkedNodes) | ||
| { | ||
| if (node is IEETypeNode) |
There was a problem hiding this comment.
Note this will have duplicates if constructed and unconstructed forms exist. Probably fine.
There was a problem hiding this comment.
The infra must handle duplicates for other reasons already, so this is not a problem.
|
2 known failures, I created dotnet/arcade#12531 for the infra problem. |
ghost
locked as resolved and limited conversation to collaborators
Previously we validated the Kept on types against the presence of constructed EEType. But the compiler avoids constructed types as much as possible and the tests do use cases where the type in question is never instantiated (or fully static type). Validating against the presence of EEType (basically a method table) is much closer to the ILLink behavior which validates the presence of the type in IL metadata.
Fixes #80408