[clang] Omit most AttributedStatements from the CFG

[thakis]

Summary

[[clang::fallthrough]] has meaning for the CFG, but all other
StmtAttrs we currently have don't. So omit them, as AttributedStatements
with children cause several issues and there's no benefit in including
them.

Fixes PR52103 and PR49454. See PR52103 for details.

[thakis]

clang/lib/Analysis/CFG.cpp

2410

(Compare this function to CFGBuilder::VisitStmt in this file, which is the default function that's called for Stmts and which is what was called for AttributedStmt before this patch added a dedicated handler for AttributedStmt.)

[aaron.ballman]

Thank you for the fix to this!

clang/lib/Analysis/CFG.cpp

2418–2420

What about OpenCLUnrollHintAttr, NoMergeAttr, and MustTailAttr? These all have some semantic effect as statement attributes in terms of changing codegen, but perhaps they don't need modelling in the CFG?

(I'm trying to decide whether we may want to tablegen this functionality and so we might want something more general than isFallthroughStatement().)

[thakis]

clang/lib/Analysis/CFG.cpp

2418–2420

Right, I think they all have no interesting effect on the CFG.

It's hard to predict the future, so I'd say let's wait and see until there are more StmtAttrs :)

[hans]

clang/lib/Analysis/CFG.cpp

545

Change param name C -> A as in the function definition?

2407

Can fallthrough statements ever have children? If not, should it be an assert instead of a check here?

[thakis]

clang/lib/Analysis/CFG.cpp

2407

Good question. Attr.td says:

// The attribute only applies to a NullStmt, but we have special fix-it
// behavior if applied to a case label.
let Subjects = SubjectList<[NullStmt, SwitchCase], ErrorDiag,
                           "empty statements">;

Which I suppose triggers for this:

switch (argc) {
  [[fallthrough]] case 4:
    break;
}

foo.cc:6:7: error: 'fallthrough' attribute is only allowed on empty statements
    [[fallthrough]] case 4:
      ^             ~~~~
foo.cc:6:20: note: did you forget ';'?
    [[fallthrough]] case 4:

But that doesn't seem to make it into the AST, according to -dump-ast. So I suppose it could be an assert as well. Want me to change this?

[hans]

clang/lib/Analysis/CFG.cpp

2407

Yes, I think an assert would make sense, otherwise the reader has to think about what would the code be doing for an AttributedStmt with non-null substmt.

[thakis]

rename param; assert isa<NullStmt>

[aaron.ballman]

LGTM, thank you @thakis!

clang/lib/Analysis/CFG.cpp

2407

I think it's fine to assert because we're required to reject fallthrough statements applying to something other than a null statement: https://eel.is/c++draft/dcl.attr.fallthrough#1.sentence-1

2418–2420

SGTM!

clang/lib/Analysis/CFG.cpp

public:
   std::unique_ptr<CFG> buildCFG(const Decl *D, Stmt *Statement);
 
   bool alwaysAdd(const Stmt *stmt);
 
 private:
   // Visitors to walk an AST and construct the CFG.
   CFGBlock *VisitInitListExpr(InitListExpr *ILE, AddStmtChoice asc);
   CFGBlock *VisitAddrLabelExpr(AddrLabelExpr *A, AddStmtChoice asc);
+  CFGBlock *VisitAttributedStmt(AttributedStmt *A, AddStmtChoice asc);
   CFGBlock *VisitBinaryOperator(BinaryOperator *B, AddStmtChoice asc);
   CFGBlock *VisitBreakStmt(BreakStmt *B);
   CFGBlock *VisitCallExpr(CallExpr *C, AddStmtChoice asc);
   CFGBlock *VisitCaseStmt(CaseStmt *C);
   CFGBlock *VisitChooseExpr(ChooseExpr *C, AddStmtChoice asc);
   CFGBlock *VisitCompoundStmt(CompoundStmt *C, bool ExternallyDestructed);
   CFGBlock *VisitConditionalOperator(AbstractConditionalOperator *C,
                                      AddStmtChoice asc);
switch (S->getStmtClass()) {
     case Stmt::ImplicitValueInitExprClass:
       if (BuildOpts.OmitImplicitValueInitializers)
         return Block;
       return VisitStmt(S, asc);
 
     case Stmt::InitListExprClass:
       return VisitInitListExpr(cast<InitListExpr>(S), asc);
 
+    case Stmt::AttributedStmtClass:
+      return VisitAttributedStmt(cast<AttributedStmt>(S), asc);
+
     case Stmt::AddrLabelExprClass:
       return VisitAddrLabelExpr(cast<AddrLabelExpr>(S), asc);
 
     case Stmt::BinaryConditionalOperatorClass:
       return VisitConditionalOperator(cast<BinaryConditionalOperator>(S), asc);
 
     case Stmt::BinaryOperatorClass:
       return VisitBinaryOperator(cast<BinaryOperator>(S), asc);
CFGBlock *CFGBuilder::VisitAddrLabelExpr(AddrLabelExpr *A,
   if (asc.alwaysAdd(*this, A)) {
     autoCreateBlock();
     appendStmt(Block, A);
   }
 
   return Block;
 }
 
-CFGBlock *CFGBuilder::VisitUnaryOperator(UnaryOperator *U,
+static bool isFallthroughStatement(const AttributedStmt *A) {
+  bool isFallthrough = hasSpecificAttr<FallThroughAttr>(A->getAttrs());
+  assert((!isFallthrough || isa<NullStmt>(A->getSubStmt())) &&
+         "expected fallthrough not to have children");
+  return isFallthrough;
+}
+
+CFGBlock *CFGBuilder::VisitAttributedStmt(AttributedStmt *A,
                                           AddStmtChoice asc) {
+  // AttributedStmts for [[likely]] can have arbitrary statements as children,
+  // and the current visitation order here would add the AttributedStmts
+  // for [[likely]] after the child nodes, which is undesirable: For example,
+  // if the child contains an unconditional return, the [[likely]] would be
+  // considered unreachable.
+  // So only add the AttributedStmt for FallThrough, which has CFG effects and
+  // also no children, and omit the others. None of the other current StmtAttrs
+  // have semantic meaning for the CFG.
+  if (isFallthroughStatement(A) && asc.alwaysAdd(*this, A)) {
+    autoCreateBlock();
+    appendStmt(Block, A);
+  }
+
+  return VisitChildren(A);
+}
+
+CFGBlock *CFGBuilder::VisitUnaryOperator(UnaryOperator *U, AddStmtChoice asc) {
   if (asc.alwaysAdd(*this, U)) {
     autoCreateBlock();
     appendStmt(Block, U);
   }
 
   if (U->getOpcode() == UO_LNot)
     tryEvaluateBool(U->getSubExpr()->IgnoreParens());
 

clang/test/SemaCXX/switch-implicit-fallthrough.cpp

default:
       if (n % 10)
         goto label_default;
       break;
     case 70:
       n += 335;
       break;
   }
   switch (n / 20) {
+    [[likely]] case 6:
+      [[clang::fallthrough]];
     case 7:
       n += 400;
       [[clang::fallthrough]];
     case 9:  // no warning here, intended fall-through marked with an attribute
       n += 800;
       [[clang::fallthrough]];
     default: { // no warning here, intended fall-through marked with an attribute
       if (n % 2 == 0) {
         return 1;
       } else {
         [[clang::fallthrough]];
       }
     }
     case 10:  // no warning here, intended fall-through marked with an attribute
       if (n % 3 == 0) {
         n %= 3;
       } else {
         [[clang::fallthrough]];
       }
     case 110:  // expected-warning{{unannotated fall-through between switch labels}} but no fix-it hint as we have one fall-through annotation!
       n += 800;
   }
   switch (n / 30) {
+    case 6:
+      [[unlikely, clang::fallthrough]];
     case 11:
     case 12:  // no warning here, intended fall-through, no statement between labels
       n += 1600;
   }
   switch (n / 40) {
     case 13:
       if (n % 2 == 0) {
         return 1;

clang/test/SemaCXX/unreachable-code.cpp

 extern "C" void foo(void);
 extern "C" __attribute__((weak)) decltype(foo) foo;
 
 void weak_redecl() {
   if (foo)
     return;
   bar(); // no-warning
 }
+
+namespace pr52103 {
+
+void g(int a);
+
+void f(int a) {
+  if (a > 4) [[ likely ]] { // no-warning
+    return;
+  }
+
+  if (a > 4) [[ unlikely ]] { // no-warning
+    return;
+
+    return; // expected-warning {{will never be executed}}
+  }
+
+  [[clang::musttail]] return g(a); // no-warning
+
+  [[clang::musttail]] return g(a); // expected-warning {{will never be executed}}
+}
+
+}