Implement checking for Requires* on attributes for NativeAOT #83550
Conversation
Any of the RUC/RDC/RAF attributes can be added to an attribute (either the type or the .ctor or properties). The compiler needs to check for these and produce warnings if such attribute is instantiated somewhere. This was added to "Dataflow" existing class, while it's not exactly data flow, adding a new abstraction didn't seem worth it. Also does a simple dedupe of RUC/RDC/RAF checks in some places. Adapt the tests to the new behavior. The most notable change is that NativeAOT will only look at attributes for reflection enabled members, so had to modify the tests to reflection access basically everything.
vitek-karas
requested review from
marek-safar and
MichalStrehovsky
as code owners
|
Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas Issue DetailsAny of the RUC/RDC/RAF attributes can be added to an attribute (either the type or the .ctor or properties). The compiler needs to check for these and produce warnings if such attribute is instantiated somewhere. This was added to "Dataflow" existing class, while it's not exactly data flow, adding a new abstraction didn't seem worth it. Also does a simple dedupe of RUC/RDC/RAF checks in some places. Adapt the tests to the new behavior. The most notable change is that NativeAOT will only look at attributes for reflection enabled members, so had to modify the tests to reflection access basically everything. Contributes to #82447
|
src/tools/illink/test/Mono.Linker.Tests.Cases/RequiresCapability/RequiresOnAttribute.cs
Show resolved
Hide resolved
| // Accessing the members to test via direct calls/access is not enough | ||
| // in NativeAOT custom attributes are only looked at if the member is | ||
| // reflection enabled - so there has to be a reflection access to it somewhere. | ||
| // On the other hand the analyzer reports RDC/RAF only on direct access. So we need to have "both" |
There was a problem hiding this comment.
I assume that we ideally want linker/analyzer not to warn on direct access either - is that right?
There was a problem hiding this comment.
This goes to the larger discussion on how should linker treat attributes. Currently we're very conservative in that we keep all instantiations pretty much everywhere. NativeAOT is also conservative, but less so, it will keep all attributes but only on reflectable entities.
I do agree that it would make sense to change linker to behave more like NativeAOT - which is aligned with the long term goal of linker optimizing non-reflectable entities more. Currently we only remove parameter names for non-reflectable methods, we already have issues and ides for example to remove field names for non-reflectable fields and so on. Removing attributes on these would also fall into that bucket.
Ideally we would be able to remove unused attributes, which means the same behavior as NativeAOT, but also only keep attributes which are actually asked for. But that is pretty hard to track correctly - basically GetCustomAttributes API would have to be RUC and intrinsic. That would probably break lot of things.
There was a problem hiding this comment.
It occurs to me also that warning only on attributes on members accessed via reflection introduces a weird kind of non-local behavior. If unrelated code starts reflecting on a method which has such an attribute, that method starts giving a warning without any indication of what caused the change. Normally cases like this would warn on the reflection access site. So maybe there are advantages to the linker/analyzer behavior.
src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/AttributeDataFlow.cs
Show resolved
Hide resolved
Use GetDisplayName for anything is supported by that extension method - this fixes wrong origin printed out for property/event (we would print out 'PropertyPseudoDescriptor'). Mapping IL offset to source file/line number - if the PDB specifies that line as "hidden", then don't use it, instead look for the begining of the method - so first non-hidden sequence point in the method's body. This is a copy of the logic implemented in illink already.
|
I added fixes around warning location display - mainly because with this change some of it can now actually happen: Use GetDisplayName for anything is supported by that extension method - this fixes wrong origin printed out for property/event (we would print out 'PropertyPseudoDescriptor'). Mapping IL offset to source file/line number - if the PDB specifies that line as "hidden", then don't use it, instead look for the begining of the method - so first non-hidden sequence point in the method's body. |
|
@sbomer - Could you please take a look at the changes? |
| } | ||
|
|
||
| // If the warning comes from hidden line (compiler generated code typically) | ||
| // search for any sequence point with non-hidden line number and report that as a best effort. |
There was a problem hiding this comment.
I don't know all the cases where hidden line numbers might be emitted - but should this maybe find the last sequence point with non-hidden line number that still is <= ilOffset, if it exists, and the first subsequent sequence point with non-hidden line number, if not?
There was a problem hiding this comment.
In a way doing that can lead to more confusion. If the warning is in the compiler generated portion of the method, then "assigning" it to something "close by" is as wrong as assigning it to pretty much any other part of the method - mainly because we have no idea how the method is split between compiler generated and user code. So pointing to the method as a whole feels like a good compromise.
If we do find a case where this would improve experience I'm not against doing it, in all tools at once.
ghost
locked as resolved and limited conversation to collaborators
Any of the RUC/RDC/RAF attributes can be added to an attribute (either the type or the .ctor or properties). The compiler needs to check for these and produce warnings if such attribute is instantiated somewhere.
This was added to "Dataflow" existing class, while it's not exactly data flow, adding a new abstraction didn't seem worth it.
Also does a simple dedupe of RUC/RDC/RAF checks in some places.
Adapt the tests to the new behavior. The most notable change is that NativeAOT will only look at attributes for reflection enabled members, so had to modify the tests to reflection access basically everything.
Contributes to #82447