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

Unify trim warnings for accessing compiler generated code #86816

Merged
merged 10 commits into from
Jun 3, 2023
+713 −434
Merged

Unify trim warnings for accessing compiler generated code #86816

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
6e4f0fc
First attempt at the new behavior in illink and methods only
vitek-karas May 11, 2023
f8d4bb1
Port to NativeAOT and fix tests
vitek-karas May 12, 2023
825e982
Refactorings
vitek-karas May 12, 2023
ba13eed
Fixes two bugs and test changes
vitek-karas May 14, 2023
3eb375b
More tests
vitek-karas May 19, 2023
f5bad2e
Fix an analyzer test
vitek-karas May 19, 2023
6ab425a
Fix type hierarchy marking in AOT to match linker
vitek-karas May 22, 2023
4283ff7
Add some tests
vitek-karas May 23, 2023
3d610d9
Enable a test
vitek-karas May 26, 2023
8bdbe17
Don't produce warnings when accessing compiler generated code via DAM…
vitek-karas Jun 2, 2023
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
48 changes: 18 additions & 30 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 @@ -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)
{
if (entity is MethodDesc)
_logger.LogWarning(origin, DiagnosticId.DynamicallyAccessedMembersMethodAccessedViaReflection, entity.GetDisplayName());
Expand All @@ -277,12 +271,16 @@ private void ReportWarningsForReflectionAccess(in MessageOrigin origin, TypeSyst
// (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.
vitek-karas marked this conversation as resolved.
Show resolved Hide resolved
// 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);

// 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 MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(method) != method;
return !isCompilerGenerated || forceRequiresWarning;
}
}

Expand Down Expand Up @@ -319,7 +317,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 +328,14 @@ static bool IsDeclaredWithinType(TypeSystemEntity member, TypeDesc type)
}

bool isReflectionAccessCoveredByDAM = Annotations.ShouldWarnWhenAccessedForReflection(entity);
if (isReflectionAccessCoveredByDAM && !ShouldSkipWarningsForOverride(entity))
if (isReflectionAccessCoveredByDAM)
sbomer marked this conversation as resolved.
Show resolved Hide resolved
{
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
Open in desktop
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) {
// 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) {
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