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

Annotate methods that get MethodBase from a stackwalk as RUC #59851

Merged
merged 3 commits into from
Oct 4, 2021
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;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
Expand Down Expand Up @@ -69,6 +70,7 @@ private IDictionary CreateDataContainer()

public MethodBase? TargetSite
{
[RequiresUnreferencedCode("Metadata for the method might be incomplete or removed")]
get
{
if (_exceptionMethod != null)
Expand All @@ -85,13 +87,17 @@ private IDictionary CreateDataContainer()
}
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "The API will return null if the metadata for current method cannot be established.")]
private string? CreateSourceName()
{
StackTrace st = new StackTrace(this, fNeedFileInfo: false);
if (st.FrameCount > 0)
{
StackFrame sf = st.GetFrame(0)!;
MethodBase method = sf.GetMethod()!;
MethodBase? method = sf.GetMethod();
if (method == null)
return null;

Module module = method.Module;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Runtime.InteropServices;
using System.Threading;
Expand Down Expand Up @@ -36,6 +37,7 @@ public abstract partial class MethodBase : MemberInfo
return RuntimeType.GetMethodBase(declaringType.GetRuntimeType(), handle.GetMethodInfo());
}

[RequiresUnreferencedCode("Metadata for the method might be incomplete or removed")]
[System.Security.DynamicSecurityMethod] // Methods containing StackCrawlMark local var has to be marked DynamicSecurityMethod
public static MethodBase? GetCurrentMethod()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ public partial class StackFrame
public virtual int GetFileLineNumber() { throw null; }
public virtual string? GetFileName() { throw null; }
public virtual int GetILOffset() { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Metadata for the method might be incomplete or removed")]
public virtual System.Reflection.MethodBase? GetMethod() { throw null; }
public virtual int GetNativeOffset() { throw null; }
public override string ToString() { throw null; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,9 @@ public static bool Exists<T>(IEnumerable<T> collection, Predicate<T> predicate)
/// This method is used internally to trigger a failure indicating to the "programmer" that they are using the interface incorrectly.
/// It is NEVER used to indicate failure of actual contracts at runtime.
/// </summary>
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "StackFrame.GetMethod is only used to help diagnosing incorrect use of contracts. " +
"It handles missing or incomplete metadata.")]
private static void AssertMustUseRewriter(ContractFailureKind kind, string contractKind)
{
// For better diagnostics, report which assembly is at fault. Walk up stack and
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.Text;
using System.Reflection;
using System.Runtime.CompilerServices;
Expand Down Expand Up @@ -136,6 +137,7 @@ public StackFrame(string? fileName, int lineNumber, int colNumber)
/// <summary>
/// Returns the method the frame is executing
/// </summary>
[RequiresUnreferencedCode("Metadata for the method might be incomplete or removed")]
public virtual MethodBase? GetMethod()
{
return _method;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics.CodeAnalysis;

namespace System.Diagnostics
{
Expand All @@ -12,6 +13,8 @@ public static bool HasNativeImage(this StackFrame stackFrame)
return stackFrame.GetNativeImageBase() != IntPtr.Zero;
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "StackFrame.GetMethod is used to establish if method is available.")]
public static bool HasMethod(this StackFrame stackFrame)
{
return stackFrame.GetMethod() != null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,8 @@ internal string ToString(TraceFormat traceFormat)
}

#if !CORERT
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "ToString is best effort when it comes to available information.")]
internal void ToString(TraceFormat traceFormat, StringBuilder sb)
{
// Passing a default string for "at" in case SR.UsingResourceKeys() is true
Expand Down Expand Up @@ -301,8 +303,12 @@ internal void ToString(TraceFormat traceFormat, StringBuilder sb)
if (pi[j].ParameterType != null)
typeName = pi[j].ParameterType.Name;
sb.Append(typeName);
sb.Append(' ');
sb.Append(pi[j].Name);
string? parameterName = pi[j].Name;
Copy link
Member

Choose a reason for hiding this comment

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

Possible future improvement: I vaguely remember discussing this: Would it make sense to add RUC onto the ParameterInfo.Name? I know it can be null even without trimming, but it is an observable difference with trimming.

Copy link
Member Author

Choose a reason for hiding this comment

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

ILLink will not strip parameter names from things that are visible targets of reflection. By adding RUC on all APIs that get MethodBase from a stack walk, we cover all scenarios where stripping parameter names could be an observable difference (it's the API that lets you get hold of a broken MethodBase in the first place).

Since there will be a warning on getting the MethodBase, we don't need another warning on Name.

This sets us also up to do attribute stripping correctly, similar to how we strip the parameter names: https://github.com/dotnet/linker/issues/2078 - we can remove known "useless" custom attributes from methods/fields that are not visible targets of reflection for the same reason it's safe to strip parameter names and it will not cause any warningless runtime behavior differences.

if (parameterName != null)
{
sb.Append(' ');
sb.Append(parameterName);
}
}
sb.Append(')');
}
Expand Down
3 changes: 2 additions & 1 deletion src/libraries/System.Runtime/ref/System.Runtime.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2839,7 +2839,7 @@ public partial class Exception : System.Runtime.Serialization.ISerializable
public virtual string Message { get { throw null; } }
public virtual string? Source { get { throw null; } set { } }
public virtual string? StackTrace { get { throw null; } }
public System.Reflection.MethodBase? TargetSite { get { throw null; } }
public System.Reflection.MethodBase? TargetSite { [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Metadata for the method might be incomplete or removed")] get { throw null; } }
[System.ObsoleteAttribute("BinaryFormatter serialization is obsolete and should not be used. See https://aka.ms/binaryformatter for more information.", DiagnosticId = "SYSLIB0011", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")]
protected event System.EventHandler<System.Runtime.Serialization.SafeSerializationEventArgs>? SerializeObjectState { add { } remove { } }
public virtual System.Exception GetBaseException() { throw null; }
Expand Down Expand Up @@ -11873,6 +11873,7 @@ public abstract partial class MethodBase : System.Reflection.MemberInfo
public abstract System.RuntimeMethodHandle MethodHandle { get; }
public virtual System.Reflection.MethodImplAttributes MethodImplementationFlags { get { throw null; } }
public override bool Equals(object? obj) { throw null; }
[System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Metadata for the method might be incomplete or removed")]
public static System.Reflection.MethodBase? GetCurrentMethod() { throw null; }
public virtual System.Type[] GetGenericArguments() { throw null; }
public override int GetHashCode() { throw null; }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
Expand Down Expand Up @@ -35,6 +36,8 @@ public partial class StackTrace
[MethodImplAttribute(MethodImplOptions.InternalCall)]
internal static extern MonoStackFrame[] get_trace(Exception e, int skipFrames, bool needFileInfo);

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "StackFrame.GetMethod is getting compared to null but nothing else on it is touched.")]
[MethodImplAttribute(MethodImplOptions.NoInlining)]
private void InitializeForCurrentThread(int skipFrames, bool needFileInfo)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Runtime.InteropServices;
using System.Diagnostics.Tracing;
Expand Down Expand Up @@ -50,6 +51,7 @@ public DispatchState(MonoStackFrame[]? stackFrames)

public MethodBase? TargetSite
{
[RequiresUnreferencedCode("Metadata for the method might be incomplete or removed")]
get
{
StackTrace st = new StackTrace(this, true);
Expand Down Expand Up @@ -107,15 +109,19 @@ private bool CanSetRemoteStackTrace()
return true; // mono runtime doesn't have immutable agile exceptions, always return true
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "The API will return null if the metadata for current method cannot be established.")]
private string? CreateSourceName()
{
var st = new StackTrace(this, fNeedFileInfo: false);
if (st.FrameCount > 0)
{
StackFrame sf = st.GetFrame(0)!;
MethodBase? method = sf.GetMethod();
if (method == null)
return null;

Module? module = method?.Module;
Module module = method.Module;
RuntimeModule? rtModule = module as RuntimeModule;

if (rtModule == null)
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.Runtime.CompilerServices;

namespace System.Reflection
Expand Down Expand Up @@ -34,6 +35,7 @@ public partial class MethodBase
return m;
}

[RequiresUnreferencedCode("Metadata for the method might be incomplete or removed")]
[MethodImplAttribute(MethodImplOptions.InternalCall)]
public static extern MethodBase? GetCurrentMethod();

Expand Down