Leaf level AOT annotation for CoreLib library #66976

LakshanF · 2022-03-22T05:22:14Z

First round of CoreLib (coreclr\System.Private.CoreLib\System.Private.CoreLib.csproj) annotation to get feedback on warning suppression strategy for this level before propagating to all impacted public APIs:

Suppressed warnings from ComparerHelpers as this internal class seems to be hardened for generic instantiations. The document sample code works in NativeAot mode.
Suppressed warnings from TypeBuilder as Reflection.Emit is not supported in NativeAot mode and will throw a NotSupportedException. I want to explore if this option works instead of adding multiple annotations for this class.
Suppressed warnings from MemberInfoCache<T>.PopulateInterfaces to match the trimmer suppression since this is an internal API.
All other leaf level warnings are propagated including IL3051 ones.

ghost · 2022-03-22T05:22:22Z

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

First round of CoreLib (coreclr\System.Private.CoreLib\System.Private.CoreLib.csproj) annotation to get feedback on warning suppression strategy for this level before propagating to all impacted public APIs:

Suppressed warnings from ComparerHelpers as this internal class seems to be hardened for generic instantiations. The document sample code works in NativeAot mode.
Suppressed warnings from TypeBuilder as Reflection.Emit is not supported in NativeAot mode and will throw a NotSupportedException. I want to explore if this option works instead of adding multiple annotations for this class.
Suppressed warnings from MemberInfoCache<T>.PopulateInterfaces to match the trimmer suppression since this is an internal API.
All other leaf level warnings are propagated including IL3051 ones.

Author:	LakshanF
Assignees:	LakshanF
Labels:	`area-System.Reflection`
Milestone:	-

MichalStrehovsky

Most of these changes are to files that are not built with NativeAOT's CoreLib (we really only care about NativeAOT's CoreLib).

I wonder if it would be a better strategy to disable the analyzer on CoreCLR's corelib and only run it on NativeAOT's.

MichalStrehovsky · 2022-03-22T05:25:30Z

src/coreclr/System.Private.CoreLib/src/System/Attribute.CoreCLR.cs

@@ -434,6 +434,7 @@ private static AttributeUsageAttribute InternalGetAttributeUsage(Type type)
                SR.Format(SR.Format_AttributeUsage, type));
        }

+        [RequiresDynamicCode("The code for an array of the specified type might not be available.")]


This can be a suppression - we're creating an instance of a reference type array (System.Attribute is a reference type and Type is expected to be deriving from Attribute or the cast would fail).

Array.CreateInstance with a reference type element is safe.

MichalStrehovsky · 2022-03-22T05:32:56Z

src/coreclr/System.Private.CoreLib/src/System/Collections/Generic/ComparerHelpers.cs

@@ -25,6 +26,8 @@ internal static class ComparerHelpers
        /// The logic in this method is replicated in vm/compile.cpp to ensure that NGen saves the right instantiations,
        /// and in vm/jitinterface.cpp so the jit can model the behavior of this method.
        /// </remarks>
+        [UnconditionalSuppressMessage("AotAnalysis", "IL3050:UnrecognizedReflectionPattern",


This file is not used with NativeAOT - src/coreclr/System.Private.Corelib are files specific to the JIT-based CoreCLR.

It would not be safe - NativeAOT approaches comparers differently and does some work in the compiler to support them.

I don't know how best to approach this - suppressing this runs a risk that if we ever move this code under src/libraries/System.Private.CoreLib and for whatever reason it gets activated under NativeAOT, we'll have a bug.

Maybe the suppression is the best we can do, but it gives me a bad feeling.

As an option to consider, we could also disable the analyzer for CoreCLR's CoreLib.

MichalStrehovsky · 2022-03-22T05:38:46Z

src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs

@@ -55,6 +55,8 @@ public void Bake(ModuleBuilder module, int token)
        #region Public Static Methods
        [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:UnrecognizedReflectionPattern",
            Justification = "MakeGenericType is only called on a TypeBuilder which is not subject to trimming")]
+        [UnconditionalSuppressMessage("AotAnalysis", "IL3050:UnrecognizedReflectionPattern",
+            Justification = "MakeGenericType is only called on a TypeBuilder which is not subject to trimming")]


(Similar comments apply below too - the mention of trimming doesn't describe the reason well enough)

Suggested change

Justification = "MakeGenericType is only called on a TypeBuilder which is not subject to trimming")]

Justification = "First parameter has to be a TypeBuilder, but it's not possible to obtain a TypeBuilder without getting an AOT warning.")]

MichalStrehovsky · 2022-03-22T05:47:06Z

src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs

@@ -557,6 +557,7 @@ private static RuntimeType ResolveType(RuntimeModule scope, string typeName)
        }
        #endregion

+        [RequiresDynamicCode("The code for an array of the specified type might not be available.")]


Same comment as ComparerHelpers applies - this is not used for NativeAOT and the compiler otherwise ensures custom attribute blobs are constructable.

MichalStrehovsky · 2022-03-22T05:48:54Z

src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs

@@ -1433,6 +1434,7 @@ internal static AttributeUsageAttribute GetAttributeUsage(RuntimeType decoratedA
            return attributeUsageAttribute ?? AttributeUsageAttribute.Default;
        }

+        [RequiresDynamicCode("The code for an array of the specified type might not be available.")]


This can be suppressed because the type used in CreateInstance is a reference type (we handle IsValueType with an object array).

MichalStrehovsky · 2022-03-22T05:51:06Z

src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs

@@ -1015,6 +1015,10 @@ private void PopulateLiteralFields(Filter filter, RuntimeType declaringType, ref
                    Justification = "Calls to GetInterfaces technically require all interfaces on ReflectedType" +
                        "But this is not a public API to enumerate reflection items, all the public APIs which do that" +
                        "should be annotated accordingly.")]
+                [UnconditionalSuppressMessage("AotAnalysis", "IL3050:UnrecognizedReflectionPattern",


This would not be safe (and the justification is off) - this code is not active for NativeAOT. Similar comments to the other place applies.

I really wonder if it would be better to disable the analyzer for CoreCLR's corelib.

MichalStrehovsky · 2022-03-22T05:59:03Z

src/libraries/System.Private.CoreLib/src/System/Enum.cs

@@ -302,6 +302,7 @@ internal static ulong ToUInt64(object value)
            enumType.GetEnumUnderlyingType();

 #if !CORERT
+        [RequiresDynamicCode("It might not be possible to create an array of the enum type at runtime. Use the GetValues<TEnum> overload instead.")]


The implementation for NativeAOT of this is AOT friendly - we don't want to bubble this out like this because the only form factor that currently cares is safe:

runtime/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Enum.CoreRT.cs

Lines 116 to 122 in b2dc37b

public static TEnum[] GetValues<TEnum>() where TEnum : struct, Enum

{

Array values = GetEnumInfo(typeof(TEnum)).ValuesAsUnderlyingType;

TEnum[] result = new TEnum[values.Length];

Array.Copy(values, result, values.Length);

return result;

}

LakshanF · 2022-03-31T11:04:56Z

Will close this one without merging since the annotations for CoreClr-CoreLib is not as important for the Native Aot form factor

CoreLib leaf level annotation

1bc8876

LakshanF requested review from jkotas and MichalStrehovsky March 22, 2022 05:22

dotnet-issue-labeler bot added the area-System.Reflection label Mar 22, 2022

ghost assigned LakshanF Mar 22, 2022

LakshanF marked this pull request as draft March 22, 2022 05:23

MichalStrehovsky reviewed Mar 22, 2022

View reviewed changes

This was referenced Mar 22, 2022

ThreadPoolTests.CooperativeBlockingCanCreateThreadsFaster timed out #64964

Closed

Threadpool test CooperativeBlockingCanCreateThreadsFaster failing on Mac #66852

Closed

am11 added area-NativeAOT-coreclr and removed area-System.Reflection labels Mar 23, 2022

LakshanF closed this Mar 31, 2022

ghost locked as resolved and limited conversation to collaborators Apr 30, 2022

LakshanF deleted the NativeAOTCoreLibAnnotation branch May 10, 2022 13:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leaf level AOT annotation for CoreLib library #66976

Leaf level AOT annotation for CoreLib library #66976

LakshanF commented Mar 22, 2022

ghost commented Mar 22, 2022

MichalStrehovsky left a comment

MichalStrehovsky Mar 22, 2022

MichalStrehovsky Mar 22, 2022

MichalStrehovsky Mar 22, 2022

MichalStrehovsky Mar 22, 2022

MichalStrehovsky Mar 22, 2022

MichalStrehovsky Mar 22, 2022

MichalStrehovsky Mar 22, 2022

LakshanF commented Mar 31, 2022

	Justification = "MakeGenericType is only called on a TypeBuilder which is not subject to trimming")]
	Justification = "First parameter has to be a TypeBuilder, but it's not possible to obtain a TypeBuilder without getting an AOT warning.")]

	public static TEnum[] GetValues<TEnum>() where TEnum : struct, Enum
	{
	Array values = GetEnumInfo(typeof(TEnum)).ValuesAsUnderlyingType;
	TEnum[] result = new TEnum[values.Length];
	Array.Copy(values, result, values.Length);
	return result;
	}

Leaf level AOT annotation for CoreLib library #66976

Leaf level AOT annotation for CoreLib library #66976

Conversation

LakshanF commented Mar 22, 2022

ghost commented Mar 22, 2022

MichalStrehovsky left a comment

Choose a reason for hiding this comment

MichalStrehovsky Mar 22, 2022

Choose a reason for hiding this comment

MichalStrehovsky Mar 22, 2022

Choose a reason for hiding this comment

MichalStrehovsky Mar 22, 2022

Choose a reason for hiding this comment

MichalStrehovsky Mar 22, 2022

Choose a reason for hiding this comment

MichalStrehovsky Mar 22, 2022

Choose a reason for hiding this comment

MichalStrehovsky Mar 22, 2022

Choose a reason for hiding this comment

MichalStrehovsky Mar 22, 2022

Choose a reason for hiding this comment

LakshanF commented Mar 31, 2022