Enable basic Kept validation in NativeAOT running linker tests #78828
Conversation
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsThis implements basic Type and Method Kept attribute validation. The validation is done by going over all
I also copied all of the "other assemblies" kept validation code from ResultChecker (were it lives in linker) to AssemblyChecker where it will need to be to actually work (and where it should probably be anyway). That code is not used yet. Left lot of TODO/commented out code in the AssemblyChecker - lots of other validation to enable eventually. Fixed/adapted all of the tests which were already ported to NativeAOT to pass with this new validation. One tiny product change:
|
| @@ -36,7 +36,8 @@ public static string GetDisplayName(this MethodDesc method) | |||
|
|
|||
| if (method.IsConstructor) | |||
| { | |||
| sb.Append(method.OwningType.GetDisplayNameWithoutNamespace()); | |||
| DefType defType = method.OwningType.GetTypeDefinition() as DefType; | |||
| sb.Append(defType?.Name ?? method.Name); | |||
There was a problem hiding this comment.
I'll just assume the changes here match what IL Linker is doing. But for the ?? part I want to point out that we would only take that codepath for array constructors (so e.g. new int[10, 20, 30];) and whether that part is intentional.
If so, I'd suggest we change the if above to if (method.IsConstructor && method.OwningType is DefType defType). Array constructors will fall down to the last if in the cascade.
| /// This property can override that by setting only the platforms | ||
| /// which are expected to keep the target. | ||
| /// </summary> | ||
| public ProducedBy KeptBy { get; set; } = ProducedBy.TrimmerAnalyzerAndNativeAot; |
There was a problem hiding this comment.
nit: kind of weird name to repeat the name of the attribute on the property.
There was a problem hiding this comment.
So just [Kept (By = Tool.Trimmer)]? (After we rename the enum...)
I can change that, sounds good.
src/coreclr/tools/aot/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs
Outdated
Show resolved
Hide resolved
| @@ -310,6 +311,7 @@ public static StringBuilder GetDisplayNameWithoutNamespace (this TypeReference t | |||
| break; | |||
| } | |||
|
|
|||
| type = type.GetElementType (); | |||
There was a problem hiding this comment.
Could you elaborate more on why this is needed? I just read what the GetElementType method does, but still not fully sure why is always necessary, nor why it doesn't return null for some of the types.
There was a problem hiding this comment.
In Cecil this method is something like "Get me the type without the decorators". So for arrays it returns the element type, but it also works for things like pointer types (where it returns the non-pointer version), by-ref types and so on. And on all other "normal" types it just returns this.
The case I ran into was a by-ref of a nested type. Before this change we would write it out as simple NestedType&, after the change it prints out ParentType.NestedType&.
src/coreclr/tools/aot/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs
Outdated
Show resolved
Hide resolved
src/coreclr/tools/aot/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs
Outdated
Show resolved
Hide resolved
…Checker.cs Co-authored-by: Tlakaelel Axayakatl Ceja <tlakaelel.ceja@microsoft.com>
ghost
locked as resolved and limited conversation to collaborators
This implements basic Type and Method Kept attribute validation.
It adds a
KeptByproperty on allKeptAttributewhich uses the sameProducedByenum to specify exceptions for each target product. (Eventually after the source merge we should renameProducedByenum to justToolorPlatform, but it's not important and I didn't want to do it here as it would be a giant diff).The validation is done by going over all
ConstructedEETypeNodeandIMethodBodyNodenodes in the final graph and remembers that list. Then it compares that list to the expected kept members as per the attributes in tests. There are some tweaks:ConstructedEETypeNode- this is technically wrong (since NativeAOT can remove the type if for example only static methods are used from it), but it would require major changes to the linker tests (since in IL this is a necessity). If we ever need precise validation of this, we would probably add a new attribute to check just for this.I also copied all of the "other assemblies" kept validation code from ResultChecker (were it lives in linker) to AssemblyChecker where it will need to be to actually work (and where it should probably be anyway). That code is not used yet.
Left lot of TODO/commented out code in the AssemblyChecker - lots of other validation to enable eventually.
Fixed/adapted all of the tests which were already ported to NativeAOT to pass with this new validation.
Removed a test for unresolved members, since it actually fails NativeAOT compilation, so it doesn't test anything really.
One tiny product change:
Display names now consistently include all parent types when writing out nested type names. ILLink already does this and NativeAOT was a bit inconsistent. Since the display names are used only in diagnostics, this brings the diagnostics experience closer between the two tools. The added benefit is that we can use display names to compare members between Cecil and Internal.TypeSystem more precisely.