Rework the way statement attributes are processed; NFC

[aaron.ballman]

Summary

This changes our approach to processing statement attributes to be more similar to how we process declaration attributes. Namely, ActOnAttributedStmt() now calls ProcessStmtAttributes() instead of vice-versa, and there is now an interface split between building an attributed statement where you already have a list of semantic attributes and building an attributed statement with attributes from the parser.

This should make it easier to support statement attributes that are dependent on a template. In that case, you would add a TransformFooAttr() function in TreeTransform.h to perform the semantic checking (morally similar to how Sema::InstantiateAttrs() already works for declaration attributes) when transforming the semantic attribute at instantiation time.

[erichkeane]

It is really sad that the attributes can't 'auto transform' themselves. ParsedAttr could (since it has a union of expressions/identifiers), but we don't really seem to have a good way to do it for Attr.

Could we possibly do some table-gen to auto create the 'simple' ones? Basically, any parameter to an attribute that is an 'expr' gets auto-transformed? Similar to how we have handleSimpleAttr, we could have TransformSimpleAttr (or, a generated one).

clang/lib/Sema/TreeTransform.h

1316

Am I missing where the attributes themselves are being rebuilt/transformed??

[aaron.ballman]

In D99896#2669471, @erichkeane wrote:

It is really sad that the attributes can't 'auto transform' themselves. ParsedAttr could (since it has a union of expressions/identifiers), but we don't really seem to have a good way to do it for Attr.

Could we possibly do some table-gen to auto create the 'simple' ones? Basically, any parameter to an attribute that is an 'expr' gets auto-transformed? Similar to how we have handleSimpleAttr, we could have TransformSimpleAttr (or, a generated one).

I think that's likely plausible, though as a follow-up. We have enough declaration attributes with template instantiation that I'd probably start with those and do statement attributes in a second cleanup.

clang/lib/Sema/TreeTransform.h

1316

The transformation happens in TreeTransform<Derived>::TransformAttributedStmt() which calls RebuildAttributedStmt() with the rebuilt attributes.

[erichkeane]

SGTM.

[haberman]

clang/lib/Sema/TreeTransform.h

1316

It appears that neither TransformAttributedStmt() nor RebuildAttributedStmt() calls ProcessStmtAttributes(), either directly or transitively, so I'm not seeing where we can run instantiation-time attribute processing logic. What am I missing?

[aaron.ballman]

clang/lib/Sema/TreeTransform.h

1316

My thinking is:

• From handleMustTailAttr() in SemaStmtAttr.cpp, call CheckMustTailAttr() to do the shared semantic checking.
• Add a TransformMustTailAttr() to TreeTransform, have it call SemaRef.CheckMustTailAttr() as well.

[aaron.ballman]

I've commit in 9711118d2edf7aed133616de1eb7f633c263c4b5, thanks for the really quick reviews!

[haberman]

clang/lib/Sema/TreeTransform.h

1316

I see. My main concern then is that TransformMustTailAttr() could get access to the ReturnExpr to perform the validation. Right now the MustTailAttr doesn't appear to have any reference to the ReturnExpr, and I don't know how to give it one.

If there is a solution to this problem, I don't have any objection. My main concern is to unblock my change which is a high priority for me and my team.

[aaron.ballman]

clang/lib/Sema/TreeTransform.h

1316

I see. My main concern then is that TransformMustTailAttr() could get access to the ReturnExpr to perform the validation. Right now the MustTailAttr doesn't appear to have any reference to the ReturnExpr, and I don't know how to give it one.

I'll add a way for that to happen today or tomorrow and we can iterate from there.

clang/include/clang/Sema/Sema.h

   // The intent is to look for an attribute on the current declarator, but not
   // one that came from a typedef.
   bool hasExplicitCallingConv(QualType T);
 
   /// Get the outermost AttributedType node that sets a calling convention.
   /// Valid types should not have multiple attributes with different CCs.
   const AttributedType *getCallingConvAttributedType(QualType T) const;
 
-  /// Stmt attributes - this routine is the top level dispatcher.
-  StmtResult ProcessStmtAttributes(Stmt *Stmt,
-                                   const ParsedAttributesView &Attrs,
-                                   SourceRange Range);
+  /// Process the attributes before creating an attributed statement. Returns
+  /// the semantic attributes that have been processed.
+  void ProcessStmtAttributes(Stmt *Stmt,
+                             const ParsedAttributesWithRange &InAttrs,
+                             SmallVectorImpl<const Attr *> &OutAttrs);
 
   void WarnConflictingTypedMethods(ObjCMethodDecl *Method,
                                    ObjCMethodDecl *MethodDecl,
                                    bool IsProtocolMethodDecl);
 
   void CheckConflictingOverridingMethod(ObjCMethodDecl *Method,
                                    ObjCMethodDecl *Overridden,
                                    bool IsProtocolMethodDecl);

   void ActOnCaseStmtBody(Stmt *CaseStmt, Stmt *SubStmt);
 
   StmtResult ActOnDefaultStmt(SourceLocation DefaultLoc,
                                       SourceLocation ColonLoc,
                                       Stmt *SubStmt, Scope *CurScope);
   StmtResult ActOnLabelStmt(SourceLocation IdentLoc, LabelDecl *TheDecl,
                             SourceLocation ColonLoc, Stmt *SubStmt);
 
-  StmtResult ActOnAttributedStmt(SourceLocation AttrLoc,
-                                 ArrayRef<const Attr*> Attrs,
+  StmtResult BuildAttributedStmt(SourceLocation AttrsLoc,
+                                 ArrayRef<const Attr *> Attrs, Stmt *SubStmt);
+  StmtResult ActOnAttributedStmt(const ParsedAttributesWithRange &AttrList,
                                  Stmt *SubStmt);
 
   class ConditionResult;
   StmtResult ActOnIfStmt(SourceLocation IfLoc, bool IsConstexpr,
                          SourceLocation LParenLoc, Stmt *InitStmt,
                          ConditionResult Cond, SourceLocation RParenLoc,
                          Stmt *ThenVal, SourceLocation ElseLoc, Stmt *ElseVal);
   StmtResult BuildIfStmt(SourceLocation IfLoc, bool IsConstexpr,

clang/lib/Parse/ParseStmt.cpp

Parser::ParseStatementOrDeclaration(StmtVector &Stmts,
   MaybeDestroyTemplateIds();
 
   assert((Attrs.empty() || Res.isInvalid() || Res.isUsable()) &&
          "attributes on empty statement");
 
   if (Attrs.empty() || Res.isInvalid())
     return Res;
 
-  return Actions.ProcessStmtAttributes(Res.get(), Attrs, Attrs.Range);
+  return Actions.ActOnAttributedStmt(Attrs, Res.get());
 }
 
 namespace {
 class StatementFilterCCC final : public CorrectionCandidateCallback {
 public:
   StatementFilterCCC(Token nextTok) : NextToken(nextTok) {
     WantTypeSpecifiers = nextTok.isOneOf(tok::l_paren, tok::less, tok::l_square,
                                          tok::identifier, tok::star, tok::amp);
else if (isDeclarationStatement()) {
       StmtVector Stmts;
       // FIXME: We should do this whether or not we have a declaration
       // statement, but that doesn't work correctly (because ProhibitAttributes
       // can't handle GNU attributes), so only call it in the one case where
       // GNU attributes are allowed.
       SubStmt = ParseStatementOrDeclarationAfterAttributes(Stmts, StmtCtx,
                                                            nullptr, TempAttrs);
       if (!TempAttrs.empty() && !SubStmt.isInvalid())
-        SubStmt = Actions.ProcessStmtAttributes(SubStmt.get(), TempAttrs,
-                                                TempAttrs.Range);
+        SubStmt = Actions.ActOnAttributedStmt(TempAttrs, SubStmt.get());
     } else {
       Diag(Tok, diag::err_expected_after) << "__attribute__" << tok::semi;
     }
   }
 
   // If we've not parsed a statement yet, parse one now.
   if (!SubStmt.isInvalid() && !SubStmt.isUsable())
     SubStmt = ParseStatement(nullptr, StmtCtx);
if (Tok.isNot(tok::kw___extension__)) {
           continue;
         }
 
         // Eat the semicolon at the end of stmt and convert the expr into a
         // statement.
         ExpectAndConsumeSemi(diag::err_expected_semi_after_expr);
         R = handleExprStmt(Res, SubStmtCtx);
         if (R.isUsable())
-          R = Actions.ProcessStmtAttributes(R.get(), attrs, attrs.Range);
+          R = Actions.ActOnAttributedStmt(attrs, R.get());
       }
     }
 
     if (R.isUsable())
       Stmts.push_back(R.get());
   }
 
   SourceLocation CloseLoc = Tok.getLocation();

clang/lib/Sema/SemaStmt.cpp

if (!TheDecl->isMSAsmLabel()) {
       // Don't update the location of MS ASM labels.  These will result in
       // a diagnostic, and changing the location here will mess that up.
       TheDecl->setLocation(IdentLoc);
     }
   }
   return LS;
 }
 
-StmtResult Sema::ActOnAttributedStmt(SourceLocation AttrLoc,
-                                     ArrayRef<const Attr*> Attrs,
+StmtResult Sema::BuildAttributedStmt(SourceLocation AttrsLoc,
+                                     ArrayRef<const Attr *> Attrs,
                                      Stmt *SubStmt) {
-  // Fill in the declaration and return it.
-  AttributedStmt *LS = AttributedStmt::Create(Context, AttrLoc, Attrs, SubStmt);
-  return LS;
+  return AttributedStmt::Create(Context, AttrsLoc, Attrs, SubStmt);
+}
+
+StmtResult Sema::ActOnAttributedStmt(const ParsedAttributesWithRange &Attrs,
+                                     Stmt *SubStmt) {
+  SmallVector<const Attr *, 1> SemanticAttrs;
+  ProcessStmtAttributes(SubStmt, Attrs, SemanticAttrs);
+  if (!SemanticAttrs.empty())
+    return BuildAttributedStmt(Attrs.Range.getBegin(), SemanticAttrs, SubStmt);
+  // If none of the attributes applied, that's fine, we can recover by
+  // returning the substatement directly instead of making an AttributedStmt
+  // with no attributes on it.
+  return SubStmt;
 }
 
 namespace {
 class CommaVisitor : public EvaluatedExprVisitor<CommaVisitor> {
   typedef EvaluatedExprVisitor<CommaVisitor> Inherited;
   Sema &SemaRef;
 public:
   CommaVisitor(Sema &SemaRef) : Inherited(SemaRef.Context), SemaRef(SemaRef) {}

clang/lib/Sema/SemaStmtAttr.cpp

default:
     // declaration attribute is not written on a statement, but this code is
     // needed for attributes in Attr.td that do not list any subjects.
     S.Diag(A.getRange().getBegin(), diag::err_decl_attribute_invalid_on_stmt)
         << A << St->getBeginLoc();
     return nullptr;
   }
 }
 
-StmtResult Sema::ProcessStmtAttributes(Stmt *S,
-                                       const ParsedAttributesView &AttrList,
-                                       SourceRange Range) {
-  SmallVector<const Attr*, 8> Attrs;
-  for (const ParsedAttr &AL : AttrList) {
-    if (Attr *a = ProcessStmtAttribute(*this, S, AL, Range))
-      Attrs.push_back(a);
+void Sema::ProcessStmtAttributes(Stmt *S,
+                                 const ParsedAttributesWithRange &InAttrs,
+                                 SmallVectorImpl<const Attr *> &OutAttrs) {
+  for (const ParsedAttr &AL : InAttrs) {
+    if (const Attr *A = ProcessStmtAttribute(*this, S, AL, InAttrs.Range))
+      OutAttrs.push_back(A);
   }
 
-  CheckForIncompatibleAttributes(*this, Attrs);
-
-  if (Attrs.empty())
-    return S;
-
-  return ActOnAttributedStmt(Range.getBegin(), Attrs, S);
+  CheckForIncompatibleAttributes(*this, OutAttrs);
 }

clang/lib/Sema/TreeTransform.h

     return SemaRef.ActOnLabelStmt(IdentLoc, L, ColonLoc, SubStmt);
   }
 
   /// Build a new attributed statement.
   ///
   /// By default, performs semantic analysis to build the new statement.
   /// Subclasses may override this routine to provide different behavior.
   StmtResult RebuildAttributedStmt(SourceLocation AttrLoc,
-                                   ArrayRef<const Attr*> Attrs,
+                                   ArrayRef<const Attr *> Attrs,
                                    Stmt *SubStmt) {
-    return SemaRef.ActOnAttributedStmt(AttrLoc, Attrs, SubStmt);
+    return SemaRef.BuildAttributedStmt(AttrLoc, Attrs, SubStmt);
   }
 
   /// Build a new "if" statement.
   ///
   /// By default, performs semantic analysis to build the new statement.
   /// Subclasses may override this routine to provide different behavior.
   StmtResult RebuildIfStmt(SourceLocation IfLoc, bool IsConstexpr,
                            SourceLocation LParenLoc, Sema::ConditionResult Cond,