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.

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

Suppress trim analysis warnings inside reflection implementation #43594

Merged
merged 7 commits into from
Oct 23, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;

namespace System.Reflection
{
Expand Down Expand Up @@ -31,6 +32,11 @@ internal static bool IncludeAccessor(MethodInfo? associate, bool nonPublic)
return false;
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "Module.ResolveMethod is marked as RequiresUnreferencedCode because it relies on tokens" +
"which are not guaranteed to be stable across trimming. So if somebody harcodes a token it could break." +
"The usage here is not like that as all these tokes come from existing metadata loaded from some IL" +
"and so trimming has no effect (the tokens are read AFTER trimming occured).")]
private static RuntimeMethodInfo? AssignAssociates(
int tkMethod,
RuntimeType declaredType,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Runtime.InteropServices;
using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;

namespace System.Reflection
{
Expand Down Expand Up @@ -289,6 +290,10 @@ protected CustomAttributeData()
{
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2075:UnrecognizedReflectionPattern",
Justification = "property setters and fiels which are accessed by any attribute instantiation which is present in the code linker has analyzed." +
"As such enumerating all fields and properties may return different results fater trimming" +
"but all those which are needed to actually have data should be there.")]
private CustomAttributeData(RuntimeModule scope, MetadataToken caCtorToken, in ConstArray blob)
{
m_scope = scope;
Expand Down Expand Up @@ -1160,6 +1165,10 @@ private static object[] GetCustomAttributes(
return result;
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2065:UnrecognizedReflectionPattern",
Justification = "Linker guarantees presence of all the constructor parameters, property setters and fiels which are accessed by any " +
"attribute instantiation which is present in the code linker has analyzed." +
"As such the reflection usage in this method should never fail as those methods/fields should always be present.")]
private static void AddCustomAttributes(
ref RuntimeType.ListBuilder<object> attributes,
RuntimeModule decoratedModule, int decoratedMetadataToken,
Expand Down Expand Up @@ -1294,6 +1303,11 @@ private static void AddCustomAttributes(
}
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "Module.ResolveMethod and Module.ResolveType are marked as RequiresUnreferencedCode because they rely on tokens" +
"which are not guaranteed to be stable across trimming. So if somebody harcodes a token it could break." +
"The usage here is not like that as all these tokes come from existing metadata loaded from some IL" +
"and so trimming has no effect (the tokens are read AFTER trimming occured).")]
private static bool FilterCustomAttributeRecord(
MetadataToken caCtorToken,
in MetadataImport scope,
Expand Down Expand Up @@ -1426,6 +1440,11 @@ private static bool AttributeUsageCheck(
return true;
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "Module.ResolveType is marked as RequiresUnreferencedCode because it relies on tokens" +
"which are not guaranteed to be stable across trimming. So if somebody harcodes a token it could break." +
"The usage here is not like that as all these tokes come from existing metadata loaded from some IL" +
"and so trimming has no effect (the tokens are read AFTER trimming occured).")]
internal static AttributeUsageAttribute GetAttributeUsage(RuntimeType decoratedAttribute)
{
RuntimeModule decoratedModule = decoratedAttribute.GetRuntimeModule();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,11 @@ public override Type MakeArrayType(int rank)
public override string? FullName => m_strFullQualName ??= TypeNameBuilder.ToString(this, TypeNameBuilder.Format.FullName);
public override string? Namespace => m_type.Namespace;
public override string? AssemblyQualifiedName => TypeNameBuilder.ToString(this, TypeNameBuilder.Format.AssemblyQualifiedName);
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:UnrecognizedReflectionPatter",
Justification = "The entire TypeBuilderInstantiation is serving the MakeGenericType implementation. " +
"Currently this is not supported by linker. Once it is supported the outercall (Type.MakeGenericType)" +
"will validate that the types fullfill the necessary requirements of annotations on type parameters." +
"As such the actual internals of the implementation are not interesting.")]
private Type Substitute(Type[] substitutes)
{
Type[] inst = GetGenericArguments();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ internal void ThrowNoInvokeException()
return RuntimeMethodHandle.InvokeMethod(obj, null, sig, false, wrapExceptions);
}

[RequiresUnreferencedCode("Trimming may change method bodies. For example it can change some instructions, remove branches or local variables.")]
public override MethodBody? GetMethodBody()
{
RuntimeMethodBody? mb = RuntimeMethodHandle.GetMethodBody(this, ReflectedTypeInternal);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics.CodeAnalysis;
using System.Globalization;

namespace System.Reflection
Expand Down Expand Up @@ -38,6 +39,11 @@ public override int FilterOffset

public override Type? CatchType
{
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Copy link
Member

Choose a reason for hiding this comment

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

Should we mark MethodBody -> ExceptionHandlingCaluse -> ... and friends as RequiresUnreferencedCode instead?

My line of thinking is that we want to warn whenever trimming can produce different results than pre-trimming. Since linker is going to potentially stub out method bodies, replace basic blocks with nops, etc. maybe these should all warn?

Copy link
Member Author

Choose a reason for hiding this comment

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

It seem marking MethodBase.GetMethodBody() with RequiresUnreferencedCode would be enough - it should cover changes to IL, local variables, exception clauses, ... As far as I can tell all of those have to start with somehow getting the MethodBody object, which seems to be only possible via MethodBase.GetMethodBody.

Justification = "Module.ResolveType is marked as RequiresUnreferencedCode because it relies on tokens" +
"which are not guaranteed to be stable across trimming. So if somebody harcodes a token it could break." +
"The usage here is not like that as all these tokes come from existing metadata loaded from some IL" +
"and so trimming has no effect (the tokens are read AFTER trimming occured).")]
get
{
if (_flags != ExceptionHandlingClauseOptions.Clause)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,7 @@ public override MethodImplAttributes GetMethodImplementationFlags()

public override CallingConventions CallingConvention => Signature.CallingConvention;

[RequiresUnreferencedCode("Trimming may change method bodies. For example it can change some instructions, remove branches or local variables.")]
public override MethodBody? GetMethodBody()
{
RuntimeMethodBody? mb = RuntimeMethodHandle.GetMethodBody(this, ReflectedTypeInternal);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1744,6 +1744,9 @@ internal FieldInfo GetField(RuntimeFieldHandleInternal field)
return retval;
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2070:UnrecognizedReflectionPattern",
Justification = "The code in this method looks up the method by name, but it always starts with a method handle." +
"To get here something somwhere had to get the method handle and thus the method must exist.")]
internal static MethodBase? GetMethodBase(RuntimeType? reflectedType, RuntimeMethodHandleInternal methodHandle)
{
Debug.Assert(!methodHandle.IsNullHandle());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3564,6 +3564,13 @@ private static void DebugCheckEvent(ref Dictionary<string, string>? eventsByName
/// </summary>
/// <param name="method">The method to probe.</param>
/// <returns>The literal value or -1 if the value could not be determined. </returns>
#if !ES_BUILD_STANDALONE
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "The method calls MethodBase.GetMethodBody. Trimming application can change IL of various methods" +
"which can lead to change of behavior. This method only uses this to validate usage of event source APIs." +
"In the worst case it will not be able to determine the value it's looking for and will not perform" +
"any validation.")]
#endif
private static int GetHelperCallFirstArg(MethodInfo method)
{
// Currently searches for the following pattern
Expand Down Expand Up @@ -4417,7 +4424,7 @@ private void CallBackForExistingEventSources(bool addToListenersList, EventHandl
/// Used to register AD/Process shutdown callbacks.
/// </summary>
private static bool s_EventSourceShutdownRegistered;
#endregion
#endregion
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Globalization;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using System.Text;

Expand All @@ -16,7 +17,10 @@ protected MethodBase() { }
public abstract MethodAttributes Attributes { get; }
public virtual MethodImplAttributes MethodImplementationFlags => GetMethodImplementationFlags();
public abstract MethodImplAttributes GetMethodImplementationFlags();

[RequiresUnreferencedCode("Trimming may change method bodies. For example it can change some instructions, remove branches or local variables.")]
public virtual MethodBody? GetMethodBody() { throw new InvalidOperationException(); }

public virtual CallingConventions CallingConvention => CallingConventions.Standard;

public bool IsAbstract => (Attributes & MethodAttributes.Abstract) != 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ internal static bool MatchesExactly(this SignatureType pattern, Type actual)
return signatureType.TryResolve(genericMethod.GetGenericArguments());
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:UnrecognizedReflectionPattern",
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "Used to find matching method overloads. Only used for assignability checks.")]
private static Type? TryResolve(this SignatureType signatureType, Type[] genericMethodParameters)
{
Expand Down Expand Up @@ -212,6 +212,7 @@ internal static bool MatchesExactly(this SignatureType pattern, Type actual)
}
}

[RequiresUnreferencedCode("Wrapper around MakeGenericType which itself has RequiresUnreferencedCode")]
private static Type? TryMakeGenericType(this Type type, Type[] instantiation)
{
try
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Runtime/ref/System.Runtime.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8335,6 +8335,7 @@ protected MethodBase() { }
public static System.Reflection.MethodBase? GetCurrentMethod() { throw null; }
public virtual System.Type[] GetGenericArguments() { throw null; }
public override int GetHashCode() { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Trimming may change method bodies. For example it can change some instructions, remove branches or local variables.")]
public virtual System.Reflection.MethodBody? GetMethodBody() { throw null; }
public static System.Reflection.MethodBase? GetMethodFromHandle(System.RuntimeMethodHandle handle) { throw null; }
public static System.Reflection.MethodBase? GetMethodFromHandle(System.RuntimeMethodHandle handle, System.RuntimeTypeHandle declaringType) { throw null; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ internal static MethodBase GetMethodFromHandleNoGenericCheck(RuntimeMethodHandle
[DynamicDependency("#ctor(System.Reflection.ExceptionHandlingClause[],System.Reflection.LocalVariableInfo[],System.Byte[],System.Boolean,System.Int32,System.Int32)", typeof(RuntimeMethodBody))]
internal static extern MethodBody GetMethodBodyInternal(IntPtr handle);

[RequiresUnreferencedCode("Trimming may change method bodies. For example it can change some instructions, remove branches or local variables.")]
internal static MethodBody GetMethodBody(IntPtr handle)
{
return GetMethodBodyInternal(handle);
Expand Down Expand Up @@ -749,6 +750,7 @@ public override bool ContainsGenericParameters
}
}

[RequiresUnreferencedCode("Trimming may change method bodies. For example it can change some instructions, remove branches or local variables.")]
public override MethodBody GetMethodBody()
{
return GetMethodBody(mhandle);
Expand Down Expand Up @@ -972,6 +974,7 @@ public override object[] GetCustomAttributes(Type attributeType, bool inherit)
return CustomAttribute.GetCustomAttributes(this, attributeType, inherit);
}

[RequiresUnreferencedCode("Trimming may change method bodies. For example it can change some instructions, remove branches or local variables.")]
public override MethodBody GetMethodBody()
{
return RuntimeMethodInfo.GetMethodBody(mhandle);
Expand Down