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

Jump to bottom

Static cctor analysis for reflection #84089

Merged
merged 5 commits into from
Apr 4, 2023
Merged

Static cctor analysis for reflection #84089

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a9b2803
Implements correct data flow analysis around static cctors
vitek-karas Mar 29, 2023
ee9ad7d
Introduce a new node for static ctor analysis and move away from eetypes
vitek-karas Mar 29, 2023
b7b28c7
Revert unnecessary change
vitek-karas Mar 29, 2023
bf53f0d
Remove the new node, it's likely not needed.
vitek-karas Mar 30, 2023
e369c8e
Add test for accessing RUC cctor directly
vitek-karas Apr 3, 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
2 changes: 1 addition & 1 deletion src/coreclr/tools/Common/Compiler/Dataflow/DynamicallyAccessedMembersBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ public static IEnumerable<MethodDesc> GetConstructorsOnType(this TypeDesc type,

foreach (var method in type.GetMethods())
{
if (!method.IsConstructor)
if (!method.IsConstructor && !method.IsStaticConstructor)
continue;

if (filter != null && !filter(method))
Expand Down
3 changes: 3 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 @@ -198,6 +198,9 @@ internal void MarkStaticConstructor(in MessageOrigin origin, TypeDesc type, stri
if (!_enabled)
return;

if (type.HasStaticConstructor)
CheckAndWarnOnReflectionAccess(origin, type.GetStaticConstructor());
Copy link
Member

Choose a reason for hiding this comment

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

Will this cause two warnings to be generated? If I understand this correctly, this will generate a warning for reflection access to cctor, and then we get another one when we generate the method body, because this attribute on a cctor is always an error.

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 actually won't - we intentionally ignore the Requires* attributes on static cctor for generating warnings (basically relying on the fact that they will produce a warning just due to their presence, without actual callsite).

runtime/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/DiagnosticUtilities.cs

Line 100 in 24fa97a

if (TryGetRequiresAttribute(method, requiresAttribute, out attribute) && !method.IsStaticConstructor)

runtime/src/tools/illink/src/linker/Linker/Annotations.cs

Line 609 in 24fa97a

if (TryGetLinkerAttribute (method, out attribute) && !method.IsStaticConstructor ())

runtime/src/tools/illink/src/ILLink.RoslynAnalyzer/RequiresISymbolExtensions.cs

Line 60 in 24fa97a

if (member.TryGetAttribute (attributeName, out requiresAttribute) && !member.IsStaticConstructor ())

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm adding a test for this behavior as well.


if (!type.IsGenericDefinition && !type.ContainsSignatureVariables(treatGenericParameterLikeSignatureVariable: true) && Factory.PreinitializationManager.HasLazyStaticConstructor(type))
{
// Mark the GC static base - it contains a pointer to the class constructor, but also info
Expand Down
13 changes: 13 additions & 0 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 @@ -11,6 +11,7 @@
using ILCompiler.Dataflow;
using ILCompiler.DependencyAnalysis;
using ILCompiler.DependencyAnalysisFramework;
using ILCompiler.Logging;
using ILCompiler.Metadata;
using ILLink.Shared;

Expand Down Expand Up @@ -563,6 +564,18 @@ protected override void GetDependenciesDueToMethodCodePresenceInternal(ref Depen
{
AddDataflowDependency(ref dependencies, factory, methodIL, "Method has annotated parameters");
}

if (method.IsStaticConstructor)
{
if (DiagnosticUtilities.TryGetRequiresAttribute(method, DiagnosticUtilities.RequiresUnreferencedCodeAttribute, out _))
Logger.LogWarning(new MessageOrigin(method), DiagnosticId.RequiresUnreferencedCodeOnStaticConstructor, method.GetDisplayName());

if (DiagnosticUtilities.TryGetRequiresAttribute(method, DiagnosticUtilities.RequiresDynamicCodeAttribute, out _))
Logger.LogWarning(new MessageOrigin(method), DiagnosticId.RequiresDynamicCodeOnStaticConstructor, method.GetDisplayName());

if (DiagnosticUtilities.TryGetRequiresAttribute(method, DiagnosticUtilities.RequiresAssemblyFilesAttribute, out _))
Logger.LogWarning(new MessageOrigin(method), DiagnosticId.RequiresAssemblyFilesOnStaticConstructor, method.GetDisplayName());
}
}

if (method.GetTypicalMethodDefinition() is Internal.TypeSystem.Ecma.EcmaMethod ecmaMethod)
Expand Down
39 changes: 29 additions & 10 deletions ...ols/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresOnStaticConstructor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

namespace Mono.Linker.Tests.Cases.RequiresCapability
{
[IgnoreTestCase ("Ignore in NativeAOT, see https://github.com/dotnet/runtime/issues/82447", IgnoredBy = Tool.NativeAot)]
[SkipKeptItemsValidation]
[ExpectedNoWarnings]
class RequiresOnStaticConstructor
Expand All @@ -28,14 +27,15 @@ public static void Main ()
TestTypeIsBeforeFieldInit ();
TestStaticCtorOnTypeWithRequires ();
TestRunClassConstructorOnTypeWithRequires ();
TestRunClassConstructorOnConstructorWithRequires ();
typeof (StaticCtor).RequiresNonPublicConstructors ();
}

class StaticCtor
{
[ExpectedWarning ("IL2026", "--MethodWithRequires--")]
[ExpectedWarning ("IL3002", "--MethodWithRequires--", ProducedBy = Tool.Analyzer)]
[ExpectedWarning ("IL3050", "--MethodWithRequires--", ProducedBy = Tool.Analyzer)]
[ExpectedWarning ("IL3002", "--MethodWithRequires--", ProducedBy = Tool.Analyzer | Tool.NativeAot)]
[ExpectedWarning ("IL3050", "--MethodWithRequires--", ProducedBy = Tool.Analyzer | Tool.NativeAot)]
[ExpectedWarning ("IL2116", "StaticCtor..cctor()")]
[RequiresUnreferencedCode ("Message for --TestStaticCtor--")]
static StaticCtor ()
Expand All @@ -52,8 +52,8 @@ static void TestStaticCctorRequires ()
[RequiresUnreferencedCode ("Message for --StaticCtorOnTypeWithRequires--")]
class StaticCtorOnTypeWithRequires
{
[ExpectedWarning ("IL3002", "--MethodWithRequires--", ProducedBy = Tool.Analyzer)]
[ExpectedWarning ("IL3050", "--MethodWithRequires--", ProducedBy = Tool.Analyzer)]
[ExpectedWarning ("IL3002", "--MethodWithRequires--", ProducedBy = Tool.Analyzer | Tool.NativeAot)]
[ExpectedWarning ("IL3050", "--MethodWithRequires--", ProducedBy = Tool.Analyzer | Tool.NativeAot)]
static StaticCtorOnTypeWithRequires () => MethodWithRequires ();
}

Expand All @@ -71,6 +71,23 @@ static void TestRunClassConstructorOnTypeWithRequires ()
RuntimeHelpers.RunClassConstructor (typeHandle);
}

class StaticCtorForRunClassConstructorWithRequires
{
[ExpectedWarning ("IL2116")]
[ExpectedWarning ("IL3004", ProducedBy = Tool.Analyzer | Tool.NativeAot)]
[ExpectedWarning ("IL3056", ProducedBy = Tool.Analyzer | Tool.NativeAot)]
[RequiresUnreferencedCode ("Message for --StaticCtorOnTypeWithRequires--")]
[RequiresAssemblyFiles ("Message for --StaticCtorOnTypeWithRequires--")]
[RequiresDynamicCode ("Message for --StaticCtorOnTypeWithRequires--")]
static StaticCtorForRunClassConstructorWithRequires () { }
}

static void TestRunClassConstructorOnConstructorWithRequires ()
{
// No warnings generated here - we rely on IL2116/IL3004/IL3056 in this case and avoid duplicate warnings
RuntimeHelpers.RunClassConstructor (typeof (StaticCtorForRunClassConstructorWithRequires).TypeHandle);
}

class StaticCtorTriggeredByFieldAccess
{
[ExpectedWarning ("IL2116", "StaticCtorTriggeredByFieldAccess..cctor()")]
Expand Down Expand Up @@ -105,8 +122,8 @@ static void TestStaticCtorMarkingIsTriggeredByFieldAccessOnExplicitLayout ()
class StaticCtorTriggeredByMethodCall
{
[ExpectedWarning ("IL2116", "StaticCtorTriggeredByMethodCall..cctor()")]
[ExpectedWarning ("IL3004", "StaticCtorTriggeredByMethodCall..cctor()", ProducedBy = Tool.Analyzer)]
[ExpectedWarning ("IL3056", "StaticCtorTriggeredByMethodCall..cctor()", ProducedBy = Tool.Analyzer)]
[ExpectedWarning ("IL3004", "StaticCtorTriggeredByMethodCall..cctor()", ProducedBy = Tool.Analyzer | Tool.NativeAot)]
[ExpectedWarning ("IL3056", "StaticCtorTriggeredByMethodCall..cctor()", ProducedBy = Tool.Analyzer | Tool.NativeAot)]
[RequiresUnreferencedCode ("Message for --StaticCtorTriggeredByMethodCall.Cctor--")]
[RequiresAssemblyFiles ("Message for --StaticCtorTriggeredByMethodCall.Cctor--")]
[RequiresDynamicCode ("Message for --StaticCtorTriggeredByMethodCall.Cctor--")]
Expand All @@ -124,8 +141,8 @@ public void TriggerStaticCtorMarking ()


[ExpectedWarning ("IL2026", "--StaticCtorTriggeredByMethodCall.TriggerStaticCtorMarking--")]
[ExpectedWarning ("IL3002", "--StaticCtorTriggeredByMethodCall.TriggerStaticCtorMarking--", ProducedBy = Tool.Analyzer)]
[ExpectedWarning ("IL3050", "--StaticCtorTriggeredByMethodCall.TriggerStaticCtorMarking--", ProducedBy = Tool.Analyzer)]
[ExpectedWarning ("IL3002", "--StaticCtorTriggeredByMethodCall.TriggerStaticCtorMarking--", ProducedBy = Tool.Analyzer | Tool.NativeAot)]
[ExpectedWarning ("IL3050", "--StaticCtorTriggeredByMethodCall.TriggerStaticCtorMarking--", ProducedBy = Tool.Analyzer | Tool.NativeAot)]
static void TestStaticCtorTriggeredByMethodCall ()
{
new StaticCtorTriggeredByMethodCall ().TriggerStaticCtorMarking ();
Expand All @@ -149,7 +166,9 @@ class TypeIsBeforeFieldInit
[LogContains ("IL2026: Mono.Linker.Tests.Cases.RequiresCapability.RequiresOnStaticConstructor.TypeIsBeforeFieldInit..cctor():" +
" Using member 'Mono.Linker.Tests.Cases.RequiresCapability.RequiresOnStaticConstructor.TypeIsBeforeFieldInit.AnnotatedMethod()'" +
" which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code." +
" Message from --TypeIsBeforeFieldInit.AnnotatedMethod--.", ProducedBy = Tool.Trimmer)]
" Message from --TypeIsBeforeFieldInit.AnnotatedMethod--.", ProducedBy = Tool.Trimmer | Tool.NativeAot)]
[LogContains ("IL3002: Mono.Linker.Tests.Cases.RequiresCapability.RequiresOnStaticConstructor.TypeIsBeforeFieldInit..cctor():", ProducedBy = Tool.NativeAot)]
[LogContains ("IL3050: Mono.Linker.Tests.Cases.RequiresCapability.RequiresOnStaticConstructor.TypeIsBeforeFieldInit..cctor():", ProducedBy = Tool.NativeAot)]
static void TestTypeIsBeforeFieldInit ()
{
var x = TypeIsBeforeFieldInit.field + 42;
Expand Down