Skip to content

Commit

Permalink
Unify trim warnings for accessing compiler generated code (#86816)
Browse files Browse the repository at this point in the history 
This modifies the behavior of both the illink and ilc to match and to implement some changes we've discussed recently - #86008.

The semantic changes (mostly as compared to previous illink behavior):
* Reflection access to a RUC virtual method will produce warning on all methods which have RUC, regardless of their relationships. Specifically if both the override and base methods are accessed, and they both have RUC, they will both generate a warning. (This effectively removes an optimization which illink had to avoid "duplicate" warnings where it tried to warn only on the base methods in this case. But this has holes - see #86008).
* Reflection access to compiler generated code will not produce warnings even if the target has RUC on it. It proved to be really difficult to design a good experience for reporting warnings in these cases. The problem is that developer has little control over the generated code and fully reporting all warnings leads to lot of noise. The above optimization in the illink tried to solve some of this, but it's not very successful. So we're removing all of these warnings. The reasoning is that reflection access to compiler generated members is an undefined behavior (even on non-trimmed, normal CLR) - and is likely to break for example between compiler versions - and there's no diagnostics about it ignoring trimming. Trimming/AOT just makes it a little bit worse.
* AOT compiler will not implement warnings caused by reflection access to compiler generated code which has annotations either (so `IL2118`, `IL2119` and `IL2120`). The plan is to eventually remove these from illink as well.

Note that there are exceptions to the above rules, which can be described as: Accessing a token of a member in IL is considered a reflection access (because the token can be turned into a reflection object), but it is also considered a direct reference. So in that case marking is implemented as "reflection access", but diagnostics is implemented as "direct reference". What this means is that accessing a compiler generated member through its token will still produce all warnings (RUC or DAM) as for non-compiler-generated member. This is important for things like creation of `Action` from a lambda, where the lambda is a compiler generated method, but we need to produce warnings if it uses annotations for example.

Refactorings:
* Refactored code in MarkStep to move most of the diagnostics logic into `ReportWarningsForReflectionAccess` - this is to mirror the design in ilc and also to make it more in sync with the already existing `ReportWarningsForTypeHierarchyReflectionAccess`.

Test changes:
* Adapting existing tests to the new behavior
* Adding some new tests, specifically around reflection access to compiler generated code and token access to compiler generated code.
* Enables `CompilerGeneratedCodeAccessedViaReflection` for ilc
  • Loading branch information
@vitek-karas
vitek-karas authored Jun 3, 2023
1 parent 75a1806 commit 7319f86
Show file tree
Hide file tree
Showing 12 changed files with 713 additions and 434 deletions.
52 changes: 18 additions & 34 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/ReflectionMarker.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,30 +239,24 @@ private void ReportWarningsForReflectionAccess(in MessageOrigin origin, TypeSyst
// This is because reflection access is actually problematic on all members which are in a "requires" scope
// so for example even instance methods. See for example https://github.com/dotnet/linker/issues/3140 - it's possible
// to call a method on a "null" instance via reflection.
if (_logger.ShouldSuppressAnalysisWarningsForRequires(entity, DiagnosticUtilities.RequiresUnreferencedCodeAttribute, out CustomAttributeValue<TypeDesc>? requiresAttribute))
{
if (!ShouldSkipWarningsForOverride(entity, accessKind))
if (_logger.ShouldSuppressAnalysisWarningsForRequires(entity, DiagnosticUtilities.RequiresUnreferencedCodeAttribute, out CustomAttributeValue<TypeDesc>? requiresAttribute) &&
ShouldProduceRequiresWarningForReflectionAccess(entity, accessKind))
ReportRequires(origin, entity, DiagnosticUtilities.RequiresUnreferencedCodeAttribute, requiresAttribute.Value);
}

if (_logger.ShouldSuppressAnalysisWarningsForRequires(entity, DiagnosticUtilities.RequiresAssemblyFilesAttribute, out requiresAttribute))
{
if (!ShouldSkipWarningsForOverride(entity, accessKind))
if (_logger.ShouldSuppressAnalysisWarningsForRequires(entity, DiagnosticUtilities.RequiresAssemblyFilesAttribute, out requiresAttribute) &&
ShouldProduceRequiresWarningForReflectionAccess(entity, accessKind))
ReportRequires(origin, entity, DiagnosticUtilities.RequiresAssemblyFilesAttribute, requiresAttribute.Value);
}

if (_logger.ShouldSuppressAnalysisWarningsForRequires(entity, DiagnosticUtilities.RequiresDynamicCodeAttribute, out requiresAttribute))
{
if (!ShouldSkipWarningsForOverride(entity, accessKind))
if (_logger.ShouldSuppressAnalysisWarningsForRequires(entity, DiagnosticUtilities.RequiresDynamicCodeAttribute, out requiresAttribute) &&
ShouldProduceRequiresWarningForReflectionAccess(entity, accessKind))
ReportRequires(origin, entity, DiagnosticUtilities.RequiresDynamicCodeAttribute, requiresAttribute.Value);
}

// Below is about accessing DAM annotated members, so only RUC is applicable as a suppression scope
if (_logger.ShouldSuppressAnalysisWarningsForRequires(origin.MemberDefinition, DiagnosticUtilities.RequiresUnreferencedCodeAttribute))
return;

bool isReflectionAccessCoveredByDAM = Annotations.ShouldWarnWhenAccessedForReflection(entity);
if (isReflectionAccessCoveredByDAM && !ShouldSkipWarningsForOverride(entity, accessKind))
if (isReflectionAccessCoveredByDAM && ShouldProduceRequiresWarningForReflectionAccess(entity, accessKind))
{
if (entity is MethodDesc)
_logger.LogWarning(origin, DiagnosticId.DynamicallyAccessedMembersMethodAccessedViaReflection, entity.GetDisplayName());
Expand All @@ -273,16 +267,16 @@ private void ReportWarningsForReflectionAccess(in MessageOrigin origin, TypeSyst
// We decided to not warn on reflection access to compiler-generated methods:
// https://github.com/dotnet/runtime/issues/85042

// All override methods should have the same annotations as their base methods
// (else we will produce warning IL2046 or IL2092 or some other warning).
// When marking override methods via DynamicallyAccessedMembers, we should only issue a warning for the base method.
// PERF: Avoid precomputing this as this method is relatively expensive. Only call it once we're about to produce a warning.
static bool ShouldSkipWarningsForOverride(TypeSystemEntity entity, AccessKind accessKind)
static bool ShouldProduceRequiresWarningForReflectionAccess(TypeSystemEntity entity, AccessKind accessKind)
{
if (accessKind != AccessKind.DynamicallyAccessedMembersMark || entity is not MethodDesc method || !method.IsVirtual)
return false;
bool isCompilerGenerated = CompilerGeneratedState.IsNestedFunctionOrStateMachineMember(entity);

return MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(method) != method;
// Compiler generated code accessed via a token is considered a "hard" reference
// even though we also have to treat it as reflection access.
// So we need to enforce RUC check/warn in this case.
bool forceRequiresWarning = accessKind == AccessKind.TokenAccess;

return !isCompilerGenerated || forceRequiresWarning;
}
}

Expand Down Expand Up @@ -319,7 +313,8 @@ static bool IsDeclaredWithinType(TypeSystemEntity member, TypeDesc type)
// causing problems is pretty low.

bool isReflectionAccessCoveredByRUC = _logger.ShouldSuppressAnalysisWarningsForRequires(entity, DiagnosticUtilities.RequiresUnreferencedCodeAttribute, out CustomAttributeValue<TypeDesc>? requiresUnreferencedCodeAttribute);
if (isReflectionAccessCoveredByRUC && !ShouldSkipWarningsForOverride(entity))
bool isCompilerGenerated = CompilerGeneratedState.IsNestedFunctionOrStateMachineMember(entity);
if (isReflectionAccessCoveredByRUC && !isCompilerGenerated)
{
var id = reportOnMember ? DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesMemberWithRequiresUnreferencedCode : DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesMemberOnBaseWithRequiresUnreferencedCode;
_logger.LogWarning(origin, id, _typeHierarchyDataFlowOrigin.GetDisplayName(),
Expand All @@ -329,25 +324,14 @@ static bool IsDeclaredWithinType(TypeSystemEntity member, TypeDesc type)
}

bool isReflectionAccessCoveredByDAM = Annotations.ShouldWarnWhenAccessedForReflection(entity);
if (isReflectionAccessCoveredByDAM && !ShouldSkipWarningsForOverride(entity))
if (isReflectionAccessCoveredByDAM && !isCompilerGenerated)
{
var id = reportOnMember ? DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesMemberWithDynamicallyAccessedMembers : DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesMemberOnBaseWithDynamicallyAccessedMembers;
_logger.LogWarning(origin, id, _typeHierarchyDataFlowOrigin.GetDisplayName(), entity.GetDisplayName());
}

// We decided to not warn on reflection access to compiler-generated methods:
// https://github.com/dotnet/runtime/issues/85042

// All override methods should have the same annotations as their base methods
// (else we will produce warning IL2046 or IL2092 or some other warning).
// When marking override methods via DynamicallyAccessedMembers, we should only issue a warning for the base method.
static bool ShouldSkipWarningsForOverride(TypeSystemEntity entity)
{
if (entity is not MethodDesc method || !method.IsVirtual)
return false;

return MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(method) != method;
}
}

private void ReportRequires(in MessageOrigin origin, TypeSystemEntity entity, string requiresAttributeName, in CustomAttributeValue<TypeDesc> requiresAttribute)
Expand Down
139 changes: 74 additions & 65 deletions src/tools/illink/src/linker/Linker.Steps/MarkStep.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1628,6 +1628,74 @@ protected void MarkField (FieldReference reference, DependencyInfo reason, in Me
MarkField (field, reason, origin);
}

void ReportWarningsForReflectionAccess (in MessageOrigin origin, MethodDefinition method, DependencyKind dependencyKind)
{
if (Annotations.ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode (origin.Provider, out _))
return;

bool isReflectionAccessCoveredByRUC;
bool isCompilerGenerated = CompilerGeneratedState.IsNestedFunctionOrStateMachineMember (method);
bool forceRUCCheck = false;
RequiresUnreferencedCodeAttribute? requiresUnreferencedCode;
switch (dependencyKind) {
case DependencyKind.AttributeProperty:
// Property assignment in an attribute instance.
// This case is more like a direct method call than reflection, and should
// be logically similar to what is done in ReflectionMethodBodyScanner for method calls.
isReflectionAccessCoveredByRUC = Annotations.DoesMethodRequireUnreferencedCode (method, out requiresUnreferencedCode);
break;

case DependencyKind.Ldftn:
case DependencyKind.Ldvirtftn:
case DependencyKind.Ldtoken:
// Compiler generated code accessed via a token is considered a "hard" reference
// even though we also have to treat it as reflection access.
// So we need to enforce RUC check/warn in this case.
forceRUCCheck = true;
isReflectionAccessCoveredByRUC = Annotations.ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode (method, out requiresUnreferencedCode);
break;

default:
// If the method being accessed has warnings suppressed due to Requires attributes,
// we need to issue a warning for the reflection access. This is true even for instance
// methods, which can be reflection-invoked without ever calling a constructor of the
// accessed type.
isReflectionAccessCoveredByRUC = Annotations.ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode (method, out requiresUnreferencedCode);
break;
}

if (isReflectionAccessCoveredByRUC && (!isCompilerGenerated || forceRUCCheck))
ReportRequiresUnreferencedCode (method.GetDisplayName (), requiresUnreferencedCode!, new DiagnosticContext (origin, diagnosticsEnabled: true, Context));

bool isReflectionAccessCoveredByDAM = Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (method);
if (isReflectionAccessCoveredByDAM && (!isCompilerGenerated || forceRUCCheck)) {
// ReflectionMethodBodyScanner handles more cases for data flow annotations
// so don't warn for those.
switch (dependencyKind) {
case DependencyKind.AttributeConstructor:
case DependencyKind.AttributeProperty:
break;
default:
Context.LogWarning (origin, DiagnosticId.DynamicallyAccessedMembersMethodAccessedViaReflection, method.GetDisplayName ());
break;
}
}

// Warn on reflection access to compiler-generated methods, if the method isn't already unsafe to access via reflection
// due to annotations. For the annotation-based warnings, we skip virtual overrides since those will produce warnings on
// the base, but for unannotated compiler-generated methods this is not the case, so we must produce these warnings even
// for virtual overrides. This ensures that we include the unannotated MoveNext state machine method. Lambdas and local
// functions should never be virtual overrides in the first place.
bool isCoveredByAnnotations = isReflectionAccessCoveredByRUC || isReflectionAccessCoveredByDAM;
switch (dependencyKind) {
case DependencyKind.AccessedViaReflection:
case DependencyKind.DynamicallyAccessedMember:
if (ShouldWarnForReflectionAccessToCompilerGeneratedCode (method, isCoveredByAnnotations))
Context.LogWarning (origin, DiagnosticId.CompilerGeneratedMemberAccessedViaReflection, method.GetDisplayName ());
break;
}
}

void ReportWarningsForTypeHierarchyReflectionAccess (IMemberDefinition member, MessageOrigin origin)
{
Debug.Assert (member is MethodDefinition or FieldDefinition);
Expand All @@ -1653,14 +1721,9 @@ static bool IsDeclaredWithinType (IMemberDefinition member, TypeDefinition type)
if (reportOnMember)
origin = new MessageOrigin (member);


// All override methods should have the same annotations as their base methods
// (else we will produce warning IL2046 or IL2092 or some other warning).
// When marking override methods via DynamicallyAccessedMembers, we should only issue a warning for the base method.
bool skipWarningsForOverride = member is MethodDefinition m && m.IsVirtual && Annotations.GetBaseMethods (m) != null;

bool isReflectionAccessCoveredByRUC = Annotations.ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode (member, out RequiresUnreferencedCodeAttribute? requiresUnreferencedCodeAttribute);
if (isReflectionAccessCoveredByRUC && !skipWarningsForOverride) {
bool isCompilerGenerated = CompilerGeneratedState.IsNestedFunctionOrStateMachineMember (member);
if (isReflectionAccessCoveredByRUC && !isCompilerGenerated) {
var id = reportOnMember ? DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesMemberWithRequiresUnreferencedCode : DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesMemberOnBaseWithRequiresUnreferencedCode;
Context.LogWarning (origin, id, type.GetDisplayName (),
((MemberReference) member).GetDisplayName (), // The cast is valid since it has to be a method or field
Expand All @@ -1669,7 +1732,7 @@ static bool IsDeclaredWithinType (IMemberDefinition member, TypeDefinition type)
}

bool isReflectionAccessCoveredByDAM = Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (member);
if (isReflectionAccessCoveredByDAM && !skipWarningsForOverride) {
if (isReflectionAccessCoveredByDAM && !isCompilerGenerated) {
var id = reportOnMember ? DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesMemberWithDynamicallyAccessedMembers : DiagnosticId.DynamicallyAccessedMembersOnTypeReferencesMemberOnBaseWithDynamicallyAccessedMembers;
Context.LogWarning (origin, id, type.GetDisplayName (), ((MemberReference) member).GetDisplayName ());
}
Expand Down Expand Up @@ -3071,74 +3134,20 @@ void ProcessAnalysisAnnotationsForMethod (MethodDefinition method, DependencyKin
// Don't warn for methods kept due to non-understood DebuggerDisplayAttribute
// until https://github.com/dotnet/linker/issues/1873 is fixed.
case DependencyKind.KeptForSpecialAttribute:
return;
break;

case DependencyKind.DynamicallyAccessedMemberOnType:
// DynamicallyAccessedMembers on type gets special treatment so that the warning origin
// is the type or the annotated member.
ReportWarningsForTypeHierarchyReflectionAccess (method, origin);
return;
break;

default:
// All other cases have the potential of us missing a warning if we don't report it
// It is possible that in some cases we may report the same warning twice, but that's better than not reporting it.
ReportWarningsForReflectionAccess (origin, method, dependencyKind);
break;
};

if (Annotations.ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode (origin.Provider, out _))
return;

bool skipWarningsForOverride;
bool isReflectionAccessCoveredByRUC;
RequiresUnreferencedCodeAttribute? requiresUnreferencedCode;
if (dependencyKind == DependencyKind.AttributeProperty) {
// Property assignment in an attribute instance.
// This case is more like a direct method call than reflection, and should
// be logically similar to what is done in ReflectionMethodBodyScanner for method calls.
skipWarningsForOverride = false;
isReflectionAccessCoveredByRUC = Annotations.DoesMethodRequireUnreferencedCode (method, out requiresUnreferencedCode);
} else {
// All override methods should have the same annotations as their base methods
// (else we will produce warning IL2046 or IL2092 or some other warning).
// When marking override methods via DynamicallyAccessedMembers, we should only issue a warning for the base method.
skipWarningsForOverride = dependencyKind == DependencyKind.DynamicallyAccessedMember && method.IsVirtual && Annotations.GetBaseMethods (method) != null;
// If the method being accessed has warnings suppressed due to Requires attributes,
// we need to issue a warning for the reflection access. This is true even for instance
// methods, which can be reflection-invoked without ever calling a constructor of the
// accessed type.
isReflectionAccessCoveredByRUC = Annotations.ShouldSuppressAnalysisWarningsForRequiresUnreferencedCode (method, out requiresUnreferencedCode);
}

if (isReflectionAccessCoveredByRUC && !skipWarningsForOverride)
ReportRequiresUnreferencedCode (method.GetDisplayName (), requiresUnreferencedCode!, new DiagnosticContext (origin, diagnosticsEnabled: true, Context));

bool isReflectionAccessCoveredByDAM = Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (method);
if (isReflectionAccessCoveredByDAM && !skipWarningsForOverride) {
// ReflectionMethodBodyScanner handles more cases for data flow annotations
// so don't warn for those.
switch (dependencyKind) {
case DependencyKind.AttributeConstructor:
case DependencyKind.AttributeProperty:
break;
default:
Context.LogWarning (origin, DiagnosticId.DynamicallyAccessedMembersMethodAccessedViaReflection, method.GetDisplayName ());
break;
}
}

// Warn on reflection access to compiler-generated methods, if the method isn't already unsafe to access via reflection
// due to annotations. For the annotation-based warnings, we skip virtual overrides since those will produce warnings on
// the base, but for unannotated compiler-generated methods this is not the case, so we must produce these warnings even
// for virtual overrides. This ensures that we include the unannotated MoveNext state machine method. Lambdas and local
// functions should never be virtual overrides in the first place.
bool isCoveredByAnnotations = isReflectionAccessCoveredByRUC || isReflectionAccessCoveredByDAM;
switch (dependencyKind) {
case DependencyKind.AccessedViaReflection:
case DependencyKind.DynamicallyAccessedMember:
if (ShouldWarnForReflectionAccessToCompilerGeneratedCode (method, isCoveredByAnnotations))
Context.LogWarning (origin, DiagnosticId.CompilerGeneratedMemberAccessedViaReflection, method.GetDisplayName ());
break;
}
}

internal static void ReportRequiresUnreferencedCode (string displayName, RequiresUnreferencedCodeAttribute requiresUnreferencedCode, in DiagnosticContext diagnosticContext)
Expand Down
Loading

0 comments on commit 7319f86

Please sign in to comment.