Almost complete sync of logic for generating DAM/RUC reflection access warnings between ILLink and NativeAOT #84770

vitek-karas · 2023-04-13T13:13:10Z

This enables the last of the disabled RequiresCapability tests for NativeAOT.

The implementation basically copies the logic from ILLink on how to produce warnings when method or field is marked due to reflection access.

We decided that producing warnings due to reflection access to compiler generated methods is more trouble than benefit and thus we won't do that anymore. See reasoning in #85042.

Otherwise the behavior in NativeAOT fully copies the one in ILLink (small exceptions due to better method overload resolution in NativeAOT).

High-level behavior changes in NativeAOT:

Finally allow the NativeAOT compiler to produce IL2110, IL2111, IL2113 and IL2115
Avoid generating warnings for override methods if the warning is already generated on the base method (for DAM marking)
Implement correct marking (and warnings) for "DAM on type" when the derived class has broader annotation than the base class/interface (this is mostly to avoid noise warnings)
Rename a set of methods and classes in data flow scanner to "Token", because they're only used when a given entity is accessed through its token (so ldtoken and similar). This allows for more precise handling of the cases above.

Testing changes:

Allow specifying tool exceptions for SkipKepItemsValidationAttribute - this allows to enable one of the reflection tests for its diagnostics coverage, avoiding the necessity to fix everything around Kept behavior yet.
Couple of fixes in the test validator to correctly compare expected/actual diagnostics

This fixes final bits of #68786
Fixes #68688
This helps with #82447

…equire dataflow. That one is going to be hard.

Note that the current implementation of override detection only works on overrides of base methods. It does NOT work for overrides/implementations of interface methods.

ghost · 2023-04-13T13:13:25Z

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

This enables the last of the disabled RequiresCapability tests for NativeAOT.

The implementation basically copies the logic from ILLink on how to produce warnings when method or field is marked due to reflection access.

This also introduces a new cache into CompilerGeneratedCodeState which can answer the question "Does this compiler generated method require data flow analysis". Note that it is technically a heuristic since it doesn't run the same code as the compiler proper, but it should be very close as it uses the same primitives.

This had to be a new cache since it requires FlowAnnotations (also a cache). Original idea to put it into the existing CompilerGeneratedCodeState cache didn't work, because FlowAnnotations has a dependency on it - which would create a circular dependency between the two caches.

The new cache should end up being very small/cheap, since we only populate it for compiler generated code which is accessed via reflection (should be rare).

There's one missing bit: ILLink has a detection to figure out if a method is an implementation of an interface method and will suppress warnings due to annotations on such method, since the annotation on the interface methods must match. This effectively avoid duplicate warning generation. The NativeAOT implementation doesn't do this for interfaces (only for overrides of base class mehods).

This fixes final bits of #68786
This helps with #82447

Author:	vitek-karas
Assignees:	vitek-karas
Labels:	`area-NativeAOT-coreclr`
Milestone:	8.0.0

MichalStrehovsky

Left a question. I didn't review the algorithm in detail because it's rather complicated and I already lack quite some context. I still wish we just ignored the fact that compiler-generated things can be reflected on and left it as a wart.

MichalStrehovsky · 2023-04-17T04:25:12Z

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedState.cs

+
+                Method = method;
+
+                bool requiresDataflowProcessing = ReflectionMethodBodyScanner.RequiresReflectionMethodBodyScannerForMethodBody(flowAnnotations, method);


Is there any reason to introduce a local here instead of setting RequiresDataflow directly? We could add an early out after processing an instruction as well.

MichalStrehovsky · 2023-04-17T04:26:25Z

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedState.cs

+            public MethodDesc Method;
+            public bool RequiresDataflow;


Suggested change

public MethodDesc Method;

public bool RequiresDataflow;

public readonly MethodDesc Method;

public readonly bool RequiresDataflow;

MichalStrehovsky · 2023-04-17T04:28:00Z

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedState.cs

+            if (_dataflowRequirementHashtable == null)
+                throw new InvalidOperationException();


This looks unnecessary. It's not a public API.

MichalStrehovsky · 2023-04-17T04:55:23Z

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/CompilerGeneratedState.cs

+                => new DataflowRequirementValue(key, _flowAnnotations);
+        }
+
+        public void InitializeFlowAnnotations(FlowAnnotations flowAnnotations)


I wonder - could we avoid having this double initialization?

Can we for example move the logic that is getting added to CompilerGeneratedState somewhere else? Can this just be part of the regular FlowAnnotations state? And if not, could this logic be moved to a CompilerGeneratedState subclass (that is a nested type of FlowAnnotations) and change FlowAnnotations ctor to just new it up (instead of taking it as a parameter)?

vitek-karas · 2023-04-17T07:55:17Z

I must admit I'm starting to agree that we should probably give up on at least some of this. For example I found out that ILLink doesn't warn in all cases of accessing compiler generated code, even when it technically should.
When ILLink detects if a method is potentially dangerous it doesn't detect usage of annotated generics - so if there's a method which only calls for example new Lazy<T>() it won't produce a warning, even though it's a potential hole since we can't guarantee that T will have default .ctor in this case.

I'm going to look into a couple more cases which popped up, just to make sure we're not missing something bigger first though.

… code. dotnet#85042

vitek-karas · 2023-04-20T07:49:20Z

/azp run runtime-extra-platforms

azure-pipelines · 2023-04-20T07:49:40Z

Azure Pipelines successfully started running 1 pipeline(s).

vitek-karas · 2023-04-21T11:35:10Z

This is ready for review.

I updated the comment to match the implemented behavior.

All the CI failures are unrelated.

MichalStrehovsky

Thank you!

vitek-karas added 7 commits April 12, 2023 02:35

First

18e6aa9

Basically done except detection of compiler generated methods which r…

37f3e44

…equire dataflow. That one is going to be hard.

Remove resolved bug references

ffbf799

Implement automatic suppression for CGC->CGC references.

54c785f

Improve perf of method override lookup for warnings

9e5142c

Correctly propagate information about DAM to suppress override warnings

0b3b314

Note that the current implementation of override detection only works on overrides of base methods. It does NOT work for overrides/implementations of interface methods.

Fix comment

d27a06f

vitek-karas added the area-NativeAOT-coreclr label Apr 13, 2023

vitek-karas added this to the 8.0.0 milestone Apr 13, 2023

vitek-karas requested review from sbomer and MichalStrehovsky April 13, 2023 13:13

vitek-karas requested a review from marek-safar as a code owner April 13, 2023 13:13

vitek-karas self-assigned this Apr 13, 2023

build-analysis bot mentioned this pull request Apr 13, 2023

WasmTestOnBrowser-System.Text.Json.Tests.WorkItemExecution timing out #84434

Closed

MichalStrehovsky reviewed Apr 17, 2023

View reviewed changes

vitek-karas added 5 commits April 17, 2023 03:53

Fix a null ref and enable another test

25626d9

prog

5b45903

Tests

89adbae

Fix hierarchy marking to avoid duplicate warnings

18bea76

Remove warning generation for reflection access to compiler-generated…

f24495f

… code. dotnet#85042

build-analysis bot mentioned this pull request Apr 20, 2023

[wasm] interpreter timeouts when WebSocket closes unexpectedly #84101

Closed

MichalStrehovsky approved these changes Apr 24, 2023

View reviewed changes

vitek-karas merged commit f8eb926 into dotnet:main Apr 24, 2023

vitek-karas deleted the AdvancedDAMWarnings branch April 24, 2023 10:34

vitek-karas mentioned this pull request Apr 24, 2023

Requires on NativeAOT has problems with CompilerGeneratedCode #68786

Closed

ghost locked as resolved and limited conversation to collaborators May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Almost complete sync of logic for generating DAM/RUC reflection access warnings between ILLink and NativeAOT #84770

Almost complete sync of logic for generating DAM/RUC reflection access warnings between ILLink and NativeAOT #84770

vitek-karas commented Apr 13, 2023 •

edited

Loading

ghost commented Apr 13, 2023

MichalStrehovsky left a comment

MichalStrehovsky Apr 17, 2023

MichalStrehovsky Apr 17, 2023

MichalStrehovsky Apr 17, 2023

MichalStrehovsky Apr 17, 2023

vitek-karas commented Apr 17, 2023

vitek-karas commented Apr 20, 2023

azure-pipelines bot commented Apr 20, 2023

vitek-karas commented Apr 21, 2023

MichalStrehovsky left a comment


		Method = method;

		bool requiresDataflowProcessing = ReflectionMethodBodyScanner.RequiresReflectionMethodBodyScannerForMethodBody(flowAnnotations, method);

		if (_dataflowRequirementHashtable == null)
		throw new InvalidOperationException();

Almost complete sync of logic for generating DAM/RUC reflection access warnings between ILLink and NativeAOT #84770

Almost complete sync of logic for generating DAM/RUC reflection access warnings between ILLink and NativeAOT #84770

Conversation

vitek-karas commented Apr 13, 2023 • edited Loading

ghost commented Apr 13, 2023

MichalStrehovsky left a comment

Choose a reason for hiding this comment

MichalStrehovsky Apr 17, 2023

Choose a reason for hiding this comment

MichalStrehovsky Apr 17, 2023

Choose a reason for hiding this comment

MichalStrehovsky Apr 17, 2023

Choose a reason for hiding this comment

MichalStrehovsky Apr 17, 2023

Choose a reason for hiding this comment

vitek-karas commented Apr 17, 2023

vitek-karas commented Apr 20, 2023

azure-pipelines bot commented Apr 20, 2023

vitek-karas commented Apr 21, 2023

MichalStrehovsky left a comment

Choose a reason for hiding this comment

vitek-karas commented Apr 13, 2023 •

edited

Loading