Static cctor analysis for reflection #84089
Conversation
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsThis brings the NativeAOT behavior around static cctor analysis on part with illink. Main change is to add a new node which checks for RUC/RAF/RDC on static cctor and produces a warning if there is one (as these are not allowed on static cctor). The node is added for every static cctor method which has its entry point included in the app. The rest of the changes are just minor fixes and adjusting tests to the new behavior.
|
| @@ -563,6 +563,8 @@ protected override void GetDependenciesDueToMethodCodePresenceInternal(ref Depen | |||
| { | |||
| AddDataflowDependency(ref dependencies, factory, methodIL, "Method has annotated parameters"); | |||
| } | |||
|
|
|||
| StaticConstructorAnalysisNode.GetDependencies(ref dependencies, factory, method); | |||
There was a problem hiding this comment.
Why don't we do the check here? (Why is the new node needed?)
There was a problem hiding this comment.
Can we guarantee that this is only called once per method? The new node is to avoid duplicate warnings.
There was a problem hiding this comment.
I think so - this callback is called when a method body is generated. We only generate them once.
There was a problem hiding this comment.
OK- I'll look around and make the change then.
There was a problem hiding this comment.
It can happen that it will be called more than once (I think) for GVMs, the RuntimeMethodHandleNode will call this on the cannon abstract GVM method - and that can probably happen more than once if it finds more than one instantiation.
But since the new code only cares about static cctor, that has no effect (cctor is never going to be a GVM).
I simplified this change by removing the new node.
The main open question of this change is where to trigger the data flow for static cctors from. EEType presence is not enough, if the only thing access on the type is a field there doesn't seem to be EEType generated for it. So I added a hook from NonGCStatics. But it seems that a better solution would be to introduce a notion of ".cctor access" into the metadata manager and call it from all the places which actually get a cctor entry point (3 places).
The only case where it might matter is if there's a handle access to a GVM. Then we go through this code: https://github.com/dotnet/runtime/blob/57ae91cf6fc5672e34705b1a272cf268761d505a/src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/RuntimeMethodHandleNode.cs#L60 That will potentially call the `GetDependenciesDueToMethodCodePresence` multiple times on the same method. But that should not affect static cctors (since they're not GVMs) so the `GetDependenciesDueToMethodCodePresense` should only be called once per static cctor. And even if it does happen (in the future) all it would cause is potentially generating duplicate warnings, no real functional problems.
vitek-karas
force-pushed
the
StaticCCtorAnalysis
branch
from
cfab344
to
bf53f0d
Compare
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@MichalStrehovsky could you please take another look - I think this is ready. |
| @@ -198,6 +198,9 @@ internal void MarkStaticConstructor(in MessageOrigin origin, TypeDesc type, stri | |||
| if (!_enabled) | |||
| return; | |||
|
|
|||
| if (type.HasStaticConstructor) | |||
| CheckAndWarnOnReflectionAccess(origin, type.GetStaticConstructor()); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I'm adding a test for this behavior as well.
ghost
locked as resolved and limited conversation to collaborators
This brings the NativeAOT behavior around static cctor analysis on part with illink.
Main change is to add a new node which checks for RUC/RAF/RDC on static cctor and produces a warning if there is one (as these are not allowed on static cctor). The node is added for every static cctor method which has its entry point included in the app.
The rest of the changes are just minor fixes and adjusting tests to the new behavior.
Contributes to #82447.