Allow applying attributes to subset of allowed subjects.

[tra]

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.

[tra]

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

[rsmith]

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]

Addressed comments

[tra]

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]

LGTM

[aaron.ballman]

LGTM!

[brunodf]

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?

clang/lib/Sema/SemaAttr.cpp

if (StrictSubjectMatchRuleSet.empty()) {
     } else {
       for (const auto &Rule : Rules) {
         if (!isNegatedAttrMatcherSubRule(attr::SubjectMatchRule(Rule.first)))
           SubjectMatchRules.push_back(attr::SubjectMatchRule(Rule.first));
       }
     }
     Rules.clear();
   } else {
-    for (const auto &Rule : StrictSubjectMatchRuleSet) {
-      if (Rules.erase(Rule.first)) {
+    // Each rule in Rules must be a strict subset of the attribute's
+    // SubjectMatch rules.  I.e. we're allowed to use
+    // `apply_to=variables(is_global)` on an attrubute with SubjectList<[Var]>,
+    // but should not allow `apply_to=variables` on an attribute which has
+    // `SubjectList<[GlobalVar]>`.
+    for (const auto &StrictRule : StrictSubjectMatchRuleSet) {
+      // First, check for exact match.
+      if (Rules.erase(StrictRule.first)) {
         // Add the rule to the set of attribute receivers only if it's supported
         // in the current language mode.
-        if (Rule.second)
-          SubjectMatchRules.push_back(Rule.first);
+        if (StrictRule.second)
+          SubjectMatchRules.push_back(StrictRule.first);
+      }
+    }
+    // Check remaining rules for subset matches.
+    auto RulesToCheck = Rules;
+    for (const auto &Rule : RulesToCheck) {
+      attr::SubjectMatchRule MatchRule = attr::SubjectMatchRule(Rule.first);
+      if (auto ParentRule = getParentAttrMatcherRule(MatchRule)) {
+        if (llvm::any_of(StrictSubjectMatchRuleSet,
+                         [ParentRule](const auto &StrictRule) {
+                           return StrictRule.first == *ParentRule &&
+                                  StrictRule.second; // IsEnabled
+                         })) {
+          SubjectMatchRules.push_back(MatchRule);
+          Rules.erase(MatchRule);
+        }
       }
     }
   }
 
   if (!Rules.empty()) {
     auto Diagnostic =
         Diag(PragmaLoc, diag::err_pragma_attribute_invalid_matchers)
         << Attribute;

clang/test/Parser/pragma-attribute.cpp

 #pragma clang attribute push([[clang::uninitialized]], apply_to = variable) // expected-error {{attribute 'uninitialized' can't be applied to 'variable'}}
 #pragma clang attribute pop
 #pragma clang attribute push([[clang::uninitialized]], apply_to = variable(is_thread_local)) // expected-error {{attribute 'uninitialized' can't be applied to 'variable(is_thread_local)'}}
 #pragma clang attribute pop
 #pragma clang attribute push([[clang::uninitialized]], apply_to = variable(is_global)) // expected-error {{attribute 'uninitialized' can't be applied to 'variable(is_global)'}}
 #pragma clang attribute pop
 #pragma clang attribute push([[clang::uninitialized]], apply_to = any(variable(is_parameter), variable(unless(is_parameter)))) // expected-error {{attribute 'uninitialized' can't be applied to 'variable(is_parameter)', and 'variable(unless(is_parameter))'}}
 #pragma clang attribute pop
+// We're allowed to apply attributes to subsets of allowed subjects.
+#pragma clang attribute push([[clang::no_destroy]], apply_to = variable)
+#pragma clang attribute pop
+#pragma clang attribute push([[clang::no_destroy]], apply_to = variable(is_thread_local))
+#pragma clang attribute pop
+#pragma clang attribute push([[clang::no_destroy]], apply_to = variable(is_global))
+#pragma clang attribute pop
+#pragma clang attribute push([[clang::no_destroy]], apply_to = any(variable(is_parameter), variable(unless(is_parameter))))
+#pragma clang attribute pop

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

 #pragma clang attribute push (__attribute__((abi_tag("a"))), apply_to = any(variable, record(unless(is_union)), function))
 // No error
 #pragma clang attribute pop
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))), apply_to = any(function, record(unless(is_union)), variable, enum))
 // expected-error@-1 {{attribute 'abi_tag' can't be applied to 'enum'}}
 #pragma clang attribute pop
 
-#pragma clang attribute push (__attribute__((abi_tag("a"))), apply_to = any(enum_constant, function, record(unless(is_union)), variable, variable(is_parameter)))
+#pragma clang attribute push (__attribute__((abi_tag("a"))), apply_to = any(enum_constant, function, record(unless(is_union)), variable, variable(is_parameter), enum))
 // FIXME: comma in this diagnostic is wrong.
-// expected-error@-2 {{attribute 'abi_tag' can't be applied to 'enum_constant', and 'variable(is_parameter)'}}
+// expected-error@-2 {{attribute 'abi_tag' can't be applied to 'enum_constant', and 'enum'}}
 #pragma clang attribute pop
 
 #pragma clang attribute push (__attribute__((abi_tag("a"))), apply_to = any(function, record(unless(is_union)), enum))
 // expected-error@-1 {{attribute 'abi_tag' can't be applied to 'enum'}}
 #pragma clang attribute pop
 
 // Verify the non-strict subject set verification.