Skip to content

Support for RequiresAttributeMismatch testing #72496

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

Merged
tlakollo merged 5 commits into from
Jul 21, 2022
Merged

Support for RequiresAttributeMismatch testing #72496

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fe20630
Default answer of ShouldProduceFullVTable to false in the MultiFileSh…
tlakollo Jul 19, 2022
0f4bb88
PR feedback
tlakollo Jul 21, 2022
d34d8cb
Gate warning generation of IL2026 to cases in which TypeHierarchy doe…
tlakollo Jul 21, 2022
8d4a5eb
Refactor code for better readability
tlakollo Jul 21, 2022
1407ccb
Move TestInfraMultiFileCompilationModuleGroup to test project
tlakollo Jul 21, 2022
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
3 changes: 2 additions & 1 deletion src/coreclr/tools/Common/Compiler/DisplayNameHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ public static string GetDisplayName(this MethodDesc method)
sb.Append(property.Name);
sb.Append('.');
sb.Append(property.GetMethod == method ? "get" : "set");
return sb.ToString();
}
else
{
Expand Down Expand Up @@ -104,7 +105,7 @@ public static string GetDisplayName(this PropertyPseudoDesc property)
.Append('.')
.Append(property.Name).ToString();
}

public static string GetDisplayName(this EventPseudoDesc @event)
{
return new StringBuilder(@event.OwningType.GetDisplayName())
Expand Down
8 changes: 8 additions & 0 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 @@ -206,6 +206,14 @@ void CheckAndWarnOnReflectionAccess(in MessageOrigin origin, TypeSystemEntity en
}
}

var diagnosticContext = new DiagnosticContext(
origin,
_logger.ShouldSuppressAnalysisWarningsForRequires(origin.MemberDefinition, DiagnosticUtilities.RequiresUnreferencedCodeAttribute),
_logger.ShouldSuppressAnalysisWarningsForRequires(origin.MemberDefinition, DiagnosticUtilities.RequiresDynamicCodeAttribute),
_logger.ShouldSuppressAnalysisWarningsForRequires(origin.MemberDefinition, DiagnosticUtilities.RequiresAssemblyFilesAttribute),
_logger);
ReflectionMethodBodyScanner.CheckAndReportRequires(diagnosticContext, entity, DiagnosticUtilities.RequiresUnreferencedCodeAttribute);
Copy link
Member

@vitek-karas vitek-karas Jul 20, 2022

Choose a reason for hiding this comment

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

Why only RUC - why not also RAF and RDC?

Copy link
Contributor Author

@tlakollo tlakollo Jul 20, 2022

Choose a reason for hiding this comment

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

This is dataflow related so marking here only happens when using DAM. The only attribute that interacts with DAM is RUC. That's why we don't produce RDC and RAF warnings here

Copy link
Member

@vitek-karas vitek-karas Jul 20, 2022

Choose a reason for hiding this comment

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

But the problem applies to all right.

Calling Reflection.Emit directly will produce an RDC warning - great.
But calling it indirectly through reflection may go through this code, in which case it would not warn? If so, that's a hole.

Copy link
Member

@vitek-karas vitek-karas Jul 20, 2022

Choose a reason for hiding this comment

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

I guess what I'm trying to say is that DAM is basically another way to gain reflection access to a method - but it's still access to a method. For RUC, RAF, RDC methods - ANY access should produce a warning.

Copy link
Contributor Author

@tlakollo tlakollo Jul 20, 2022

Choose a reason for hiding this comment

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

I couldn't find the comment, but this was discussed before for the analyzer when marking KnownTypes, and the agreement we got there was that gaining access using DAM was only interesting for trimming (RequiresUnreferencedCode). The analyzer will not produce any RAF/RDC warnings when access via DAM, in part because the analyzer is restricted to run only if PublishTrimmed/PublishAOT is enabled.
https://github.com/dotnet/linker/blob/main/src/ILLink.RoslynAnalyzer/TrimAnalysis/ReflectionAccessAnalyzer.cs#L81-L98

Copy link
Member

@MichalStrehovsky MichalStrehovsky Jul 21, 2022

Choose a reason for hiding this comment

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

RUC is a trimming-related attribute - when we're analyzing for that, we have a complete view of reflection use within the app and that all reflection in it is statically analyzable (we know there won't be any holes).

RAF and RDC don't technically need a complete view of what's reflected on. We could warn if we see provable reflection on something that is annotated, but what to do if there's unknown reflection happening? Would we warn because we don't understand the reflection pattern and the reflection might point to something RAF/RDC-annotated? We probably wouldn't want that - single file doesn't imply trimming and we shouldn't bother the user with that. RDC currently implies trimming. But maybe it doesn't always have to?

In this sense I look at RAF and RDC same as PlatformSpecific attributes - those analyzers also scope out reflection access.

Copy link
Member

@vitek-karas vitek-karas Jul 21, 2022

Choose a reason for hiding this comment

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

My view was that currently NativeAOT implies all three - trimming, single-file and AOT. Or in other words, if we're analyzing for trimming, we should use that to warn about single-file/AOT in all cases we can know about.

There was a discussion that the RUC/RDC/RAF analyzer should be merged with the trimming analyzer (for data flow reasons, not for trimming necessarily), we might as well make use of it and report better warnings if trimming is also enabled.

That said, if others think it's better to have more "consistent" behavior (the analysis behaves the same regardless if trimming is on or not) I'm fine with it.

Copy link
Contributor Author

@tlakollo tlakollo Jul 21, 2022

Choose a reason for hiding this comment

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

I'm in favor on trying to do full reachability analysis for all Requires and even Platform specific attributes independent of the trimming restriction.
I can reach methods and properties using System.Reflection (don't even need to use DAM) that could be annotated with RDC/RAF/SupportedOS, have an app that will break in runtime and the current implementation of Analyzer and NativeAOT wouldn't account for that.


if (!Annotations.ShouldWarnWhenAccessedForReflection(entity))
return;

Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/MultiFileCompilationModuleGroup.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ public MultiFileSharedCompilationModuleGroup(CompilerTypeSystemContext context,

public override bool ShouldProduceFullVTable(TypeDesc type)
{
return ConstructedEETypeNode.CreationAllowed(type);
return false;
}

public override bool ShouldPromoteToFullType(TypeDesc type)
Expand Down
31 changes: 22 additions & 9 deletions src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/UsageBasedMetadataManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,15 +783,28 @@ public bool GeneratesAttributeMetadata(TypeDesc attributeType)

public override void NoteOverridingMethod(MethodDesc baseMethod, MethodDesc overridingMethod)
{
// We validate that the various dataflow/Requires* annotations are consistent across virtual method overrides
if (HasMismatchingAttributes(baseMethod, overridingMethod, "RequiresUnreferencedCodeAttribute"))
{
Logger.LogWarning(overridingMethod, DiagnosticId.RequiresUnreferencedCodeAttributeMismatch, overridingMethod.GetDisplayName(), baseMethod.GetDisplayName());
}
bool baseMethodTypeIsInterface = baseMethod.OwningType.IsInterface;
string overridingMethodName = overridingMethod.GetDisplayName();
string baseMethodName = baseMethod.GetDisplayName();
string[] requiresCheck = new[] { DiagnosticUtilities.RequiresUnreferencedCodeAttribute, DiagnosticUtilities.RequiresDynamicCodeAttribute, DiagnosticUtilities.RequiresAssemblyFilesAttribute };
foreach (var requiresAttribute in requiresCheck)
{
// We validate that the various dataflow/Requires* annotations are consistent across virtual method overrides
if (HasMismatchingAttributes(baseMethod, overridingMethod, requiresAttribute))
{
string message = MessageFormat.FormatRequiresAttributeMismatch(overridingMethod.DoesMethodRequire(requiresAttribute, out _),
baseMethodTypeIsInterface, requiresAttribute, overridingMethodName, baseMethodName);

if (HasMismatchingAttributes(baseMethod, overridingMethod, "RequiresDynamicCodeAttribute"))
{
Logger.LogWarning(overridingMethod, DiagnosticId.RequiresDynamicCodeAttributeMismatch, overridingMethod.GetDisplayName(), baseMethod.GetDisplayName());
DiagnosticId diagnosticId = requiresAttribute switch
{
DiagnosticUtilities.RequiresUnreferencedCodeAttribute => DiagnosticId.RequiresUnreferencedCodeAttributeMismatch,
DiagnosticUtilities.RequiresDynamicCodeAttribute => DiagnosticId.RequiresDynamicCodeAttributeMismatch,
DiagnosticUtilities.RequiresAssemblyFilesAttribute => DiagnosticId.RequiresAssemblyFilesAttributeMismatch,
_ => throw new NotImplementedException($"{requiresAttribute} is not a valid supported Requires attribute"),
};

Logger.LogWarning(overridingMethod, diagnosticId, message);
}
}

bool baseMethodRequiresDataflow = FlowAnnotations.RequiresDataflowAnalysis(baseMethod);
Expand All @@ -802,7 +815,7 @@ public override void NoteOverridingMethod(MethodDesc baseMethod, MethodDesc over
}
}

public static bool HasMismatchingAttributes (MethodDesc baseMethod, MethodDesc overridingMethod, string requiresAttributeName)
public static bool HasMismatchingAttributes(MethodDesc baseMethod, MethodDesc overridingMethod, string requiresAttributeName)
{
bool baseMethodCreatesRequirement = baseMethod.DoesMethodRequire(requiresAttributeName, out _);
bool overridingMethodCreatesRequirement = overridingMethod.DoesMethodRequire(requiresAttributeName, out _);
Expand Down
46 changes: 46 additions & 0 deletions .../tools/aot/Mono.Linker.Tests.Cases/RequiresCapability/Dependencies/ReferenceInterfaces.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Mono.Linker.Tests.Cases.RequiresCapability.Dependencies
{
public class ReferenceInterfaces
{
public interface IBaseWithRequiresInReference
{
[RequiresUnreferencedCode ("Message")]
[RequiresAssemblyFiles ("Message")]
[RequiresDynamicCode ("Message")]
public void Method ();

public string PropertyAnnotationInAccesor {
[RequiresUnreferencedCode ("Message")]
[RequiresAssemblyFiles ("Message")]
[RequiresDynamicCode ("Message")]
get;
set;
}

[RequiresAssemblyFiles ("Message")]
public string PropertyAnnotationInProperty { get; set; }
}

public interface IBaseWithoutRequiresInReference
{
public void Method ();

public string PropertyAnnotationInAccesor {
get;
set;
}

public string PropertyAnnotationInProperty { get; set; }
}
}
}
Loading