Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable more ILLink test cases with native AOT #110166

Merged
merged 3 commits into from
Dec 3, 2024
Merged

Enable more ILLink test cases with native AOT #110166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5587786
Enable more ILLink test cases with native AOT
MichalStrehovsky Nov 25, 2024
3b95e0b
Broke an unrelated test
MichalStrehovsky Nov 25, 2024
a53c0c3
Update src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCases/Test…
MichalStrehovsky Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/HandleCallAction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,17 +638,31 @@ private partial bool TryGetBaseType(TypeProxy type, out TypeProxy? baseType)
return false;
}

#pragma warning disable IDE0060
private partial bool TryResolveTypeNameForCreateInstanceAndMark(in MethodProxy calledMethod, string assemblyName, string typeName, out TypeProxy resolvedType)
{
// TODO: niche APIs that we probably shouldn't even have added
// We have to issue a warning, otherwise we could break the app without a warning.
// This is not the ideal warning, but it's good enough for now.
_diagnosticContext.AddDiagnostic(DiagnosticId.UnrecognizedParameterInMethodCreateInstance, calledMethod.GetParameter((ParameterIndex)(1 + (calledMethod.HasImplicitThis() ? 1 : 0))).GetDisplayName(), calledMethod.GetDisplayName());
resolvedType = default;
return false;
if (!System.Reflection.Metadata.AssemblyNameInfo.TryParse(assemblyName, out var an)
|| _callingMethod.Context.ResolveAssembly(an) is not ModuleDesc resolvedAssembly)
{
_diagnosticContext.AddDiagnostic(DiagnosticId.UnresolvedAssemblyInCreateInstance,
assemblyName,
calledMethod.GetDisplayName());
resolvedType = default;
return false;
}

if (!_reflectionMarker.TryResolveTypeNameAndMark(resolvedAssembly, typeName, _diagnosticContext, "Reflection", out TypeDesc? foundType))
{
// It's not wrong to have a reference to non-existing type - the code may well expect to get an exception in this case
// Note that we did find the assembly, so it's not a ILLink config problem, it's either intentional, or wrong versions of assemblies
// but ILLink can't know that. In case a user tries to create an array using System.Activator we should simply ignore it, the user
Copy link
Member

Choose a reason for hiding this comment

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

For ILLink this logic has a check for array types - why do we check that for ILLink but not ILC?

Copy link
Member Author

@MichalStrehovsky MichalStrehovsky Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That part traces back to dotnet/linker#1566.

The original logic was actually:

							var typeRef = _context.TypeNameResolver.ResolveTypeName (resolvedAssembly, typeNameStringValue.Contents);
							var resolvedType = typeRef?.Resolve ();
							if (resolvedType == null || typeRef is ArrayType) {

So this compensates for the Cecil quirk where a TypeReference for an array type will "resolve" into System.Array for reasons that I never understood.

What is there today is semantically different, it was lost in some of the refactors:

			if (!_reflectionMarker.TryResolveTypeNameAndMark (resolvedAssembly, typeName, _diagnosticContext, out TypeReference? foundType)
				|| foundType.IsTypeOf (WellKnownType.System_Array)) {

This will let arrays through and instead treats System.Array as nothing.

In the end, I don't think this matters. If we were to delete the array special handling, I don't think it would have any observable effect.

// might expect an exception to be thrown.
resolvedType = default;
return false;
}

resolvedType = new TypeProxy(foundType);
return true;
}
#pragma warning restore IDE0060

private partial void MarkStaticConstructor(TypeProxy type)
=> _reflectionMarker.MarkStaticConstructor(_diagnosticContext.Origin, type.Type, _reason);
Expand Down
27 changes: 27 additions & 0 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,33 @@ internal bool TryResolveTypeNameAndMark(string typeName, in DiagnosticContext di
return true;
}

internal bool TryResolveTypeNameAndMark(ModuleDesc assembly, string typeName, in DiagnosticContext diagnosticContext, string reason, [NotNullWhen(true)] out TypeDesc? type)
{
List<ModuleDesc> referencedModules = new();
TypeDesc foundType = CustomAttributeTypeNameParser.GetTypeByCustomAttributeTypeNameForDataFlow(typeName, assembly, assembly.Context,
referencedModules, needsAssemblyName: false, out _);
if (foundType == null)
{
type = default;
return false;
}

if (_enabled)
{
foreach (ModuleDesc referencedModule in referencedModules)
{
// Also add module metadata in case this reference was through a type forward
if (Factory.MetadataManager.CanGenerateMetadata(referencedModule.GetGlobalModuleType()))
_dependencies.Add(Factory.ModuleMetadata(referencedModule), reason);
}

MarkType(diagnosticContext.Origin, foundType, reason);
}

type = foundType;
return true;
}

internal void MarkType(in MessageOrigin origin, TypeDesc type, string reason, AccessKind accessKind = AccessKind.Unspecified)
{
if (!_enabled)
Expand Down
18 changes: 9 additions & 9 deletions src/coreclr/tools/aot/ILCompiler.Trimming.Tests/TestCases/TestSuites.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,17 +69,17 @@ public void LinkXml (string t)
[MemberData (nameof (TestDatabase.Reflection), MemberType = typeof (TestDatabase))]
public void Reflection (string t)
{
switch (t) {
case "TypeHierarchyReflectionWarnings":
case "ParametersUsedViaReflection":
case "UnsafeAccessor":
case "TypeUsedViaReflection":
case "RunClassConstructor":
case "NestedTypeUsedViaReflection":
Run (t);
switch (t)
{
case "ObjectGetType":
// Skip for now
break;
case "ObjectGetTypeLibraryMode":
case "TypeHierarchyLibraryModeSuppressions":
// No Library mode
break;
default:
// Skip the rest for now
Run (t);
break;
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/ComplexTypeHandling.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ static void TestArrayTypeGetType ()
// doesn't work on arrays, but the current implementation will preserve it anyway due to how it processes
// string -> Type resolution. This will only impact code which would have failed at runtime, so very unlikely to
// actually occur in real apps (and even if it does happen, it just increases size, doesn't break behavior).
[Kept (By = Tool.Trimmer)] // NativeAOT doesn't preserve array element types just due to the usage of the array type
[Kept (By = Tool.Trimmer | Tool.NativeAot)]
class ArrayCreateInstanceByNameElement
{
public ArrayCreateInstanceByNameElement ()
Expand All @@ -151,7 +151,6 @@ public ArrayCreateInstanceByNameElement ()
}

[Kept]
[ExpectedWarning ("IL2032", Tool.NativeAot, "https://github.com/dotnet/runtime/issues/82447")]
static void TestArrayCreateInstanceByName ()
{
Activator.CreateInstance ("test", "Mono.Linker.Tests.Cases.DataFlow.ComplexTypeHandling+ArrayCreateInstanceByNameElement[]");
Expand Down
7 changes: 4 additions & 3 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ActivatorCreateInstance.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ public FromParameterOnStaticMethodTypeB (int arg) { }
}

// Small formatting difference
[ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type, Object[])", Tool.Trimmer, "")]
[ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type, Object[])", Tool.Trimmer | Tool.NativeAot, "")]
[ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type, params Object[])", Tool.Analyzer, "")]
[ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance), nameof (CultureInfo))]
[Kept]
Expand All @@ -197,7 +197,7 @@ public FromParameterOnInstanceMethodType (int arg, int arg2) { }

[ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type)")]
// Small formatting difference
[ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type, Object[])", Tool.Trimmer, "")]
[ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type, Object[])", Tool.Trimmer | Tool.NativeAot, "")]
[ExpectedWarning ("IL2067", nameof (Activator) + "." + nameof (Activator.CreateInstance) + "(Type, params Object[])", Tool.Analyzer, "")]
[Kept]
private static void FromParameterWithNonPublicConstructors (
Expand Down Expand Up @@ -552,6 +552,7 @@ private static void TestCreateInstanceOfTWithNewConstraint<
[Kept]
class TestCreateInstanceOfTWithNoConstraintType
{
[Kept(By = Tool.NativeAot /* native AOT intrinsically expands CreateInstance<T> and would keep this method, albeit not reflection-visible */)]
public TestCreateInstanceOfTWithNoConstraintType ()
{
}
Expand Down Expand Up @@ -680,7 +681,7 @@ public void TestCreateInstance ()

[Kept]
[KeptBaseType (typeof (AnnotatedBase))]
[KeptMember (".ctor()")]
[KeptMember (".ctor()", By = Tool.Trimmer /* This type is never allocated so there's no reason to keep ctor due to a GetType call */)]
class Derived : AnnotatedBase
{
[Kept]
Expand Down
4 changes: 2 additions & 2 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/ExpressionPropertyMethodInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,10 @@ public int InstancePropertyViaReflection {
}

[Kept]
[ExpectedWarning ("IL2026", nameof (StaticPropertyExpressionAccess), Tool.Trimmer, "https://github.com/dotnet/linker/issues/2669")]
[ExpectedWarning ("IL2026", nameof (StaticPropertyExpressionAccess), Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/linker/issues/2669")]
[ExpectedWarning ("IL2026", nameof (StaticPropertyViaReflection))]
[ExpectedWarning ("IL2026", nameof (StaticPropertyViaRuntimeMethod))]
[ExpectedWarning ("IL2026", nameof (InstancePropertyExpressionAccess), Tool.Trimmer, "https://github.com/dotnet/linker/issues/2669")]
[ExpectedWarning ("IL2026", nameof (InstancePropertyExpressionAccess), Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/linker/issues/2669")]
[ExpectedWarning ("IL2026", nameof (InstancePropertyViaReflection))]
public static void Test ()
{
Expand Down
2 changes: 1 addition & 1 deletion src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/MethodsUsedViaReflection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ private void PrivateMethodWithRUC () { }
}

[Kept]
[ExpectedWarning ("IL2026", Tool.Trimmer, "https://github.com/dotnet/linker/issues/2638")]
[ExpectedWarning ("IL2026", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/linker/issues/2638")]
public static void Test ()
{
BindingFlags left = BindingFlags.Instance | BindingFlags.Static;
Expand Down
Loading