This is an archive of the discontinued LLVM Phabricator instance.

Allow applying attributes to subset of allowed subjects.
ClosedPublic

Authored by tra on Apr 8 2021, 1:36 PM.

Details

Summary

The original intent of enforcing exact match between apply_to and the attribute's was to
ensure that the user will know what declarations receive the attribute. If the compiler changes the set of allowed attributes in the future.
https://reviews.llvm.org/D30009?id=88764#inline-260744

However, the exact match, as implemented now, is too conservative. E.g. it does not allow using apply_to=variables(is_global) with an attribute which has SubjectList<[Var]>.
Applying an attribute to a subset of the allowed subjects should also be allowed.

Diff Detail

Event Timeline

tra created this revision.Apr 8 2021, 1:36 PM
tra requested review of this revision.Apr 8 2021, 1:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2021, 1:36 PM
tra updated this revision to Diff 336212.Apr 8 2021, 1:41 PM

Updated Sema/pragma-attribute-strict-subjects.c

rsmith added inline comments.Apr 8 2021, 2:13 PM
clang/lib/Sema/SemaAttr.cpp
896

Does this need to be a loop? (Can we have a grandparent matcher rule?)

clang/test/Sema/pragma-attribute-strict-subjects.c
59

The FIXME here is "fixed" now. Please can you add another example that shows the incorrect comma and move the FIXME there?

tra updated this revision to Diff 336236.Apr 8 2021, 2:45 PM

Addressed comments

tra added inline comments.Apr 8 2021, 2:46 PM
clang/lib/Sema/SemaAttr.cpp
896

No. We currently have two levels of tablegen classes: AttrSubjectMatcherRule<list<...AttrSubjectMatcherSubRule> subrules> and AttrSubjectMatcherSubRule does not allow further nesting.

clang/test/Sema/pragma-attribute-strict-subjects.c
59

Added another matcher to preserve the comma in the diagnostic.

rsmith accepted this revision.Apr 8 2021, 3:19 PM

LGTM

This revision is now accepted and ready to land.Apr 8 2021, 3:19 PM
aaron.ballman accepted this revision.Apr 9 2021, 3:43 AM
aaron.ballman added a subscriber: aaron.ballman.

LGTM!

This revision was automatically updated to reflect the committed changes.
brunodf added a subscriber: brunodf.Jul 5 2021, 3:20 AM

This change is important make better use of #pragma clang attribute (which is cool!), but for some uses the check still seems to restrictive.

Concretely, for an attribute where Subjects includes FunctionLike, I can only match it using the rule hasType(functionType) but not with a function rule. E.g. if I want to apply it to C++ methods, the rule function(is_member) is rejected (as attribute ... can't be applied to 'function(is_member)').

If I change the subject list of my attribute to include Function in addition to FunctionLike, it does work, but I don't know if there are other effects.

I'm not exactly sure what is the purpose of the check, since there is already a warning when your pragma does not match anything? If an attribute is updated to allow more subjects, how does that affect matching of existing rules?