[PATCH] D111568: [clang] Omit most AttributedStatements from the CFG

[Nico Weber via Phabricator via cfe-commits]

thakis created this revision.
thakis added reviewers: hans, aaron.ballman.
thakis requested review of this revision.

`[[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.


https://reviews.llvm.org/D111568

Files:
  clang/lib/Analysis/CFG.cpp
  clang/test/SemaCXX/switch-implicit-fallthrough.cpp
  clang/test/SemaCXX/unreachable-code.cpp


Index: clang/test/SemaCXX/unreachable-code.cpp
===================================================================
--- clang/test/SemaCXX/unreachable-code.cpp
+++ clang/test/SemaCXX/unreachable-code.cpp
@@ -77,3 +77,25 @@
     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}}
+}
+
+}
Index: clang/test/SemaCXX/switch-implicit-fallthrough.cpp
===================================================================
--- clang/test/SemaCXX/switch-implicit-fallthrough.cpp
+++ clang/test/SemaCXX/switch-implicit-fallthrough.cpp
@@ -50,6 +50,8 @@
       break;
   }
   switch (n / 20) {
+    [[likely]] case 6:
+      [[clang::fallthrough]];
     case 7:
       n += 400;
       [[clang::fallthrough]];
@@ -73,6 +75,8 @@
       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;
Index: clang/lib/Analysis/CFG.cpp
===================================================================
--- clang/lib/Analysis/CFG.cpp
+++ clang/lib/Analysis/CFG.cpp
@@ -542,6 +542,7 @@
   // Visitors to walk an AST and construct the CFG.
   CFGBlock *VisitInitListExpr(InitListExpr *ILE, AddStmtChoice asc);
   CFGBlock *VisitAddrLabelExpr(AddrLabelExpr *A, AddStmtChoice asc);
+  CFGBlock *VisitAttributedStmt(AttributedStmt *C, AddStmtChoice asc);
   CFGBlock *VisitBinaryOperator(BinaryOperator *B, AddStmtChoice asc);
   CFGBlock *VisitBreakStmt(BreakStmt *B);
   CFGBlock *VisitCallExpr(CallExpr *C, AddStmtChoice asc);
@@ -2149,6 +2150,9 @@
     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);
 
@@ -2398,8 +2402,30 @@
   return Block;
 }
 
-CFGBlock *CFGBuilder::VisitUnaryOperator(UnaryOperator *U,
-           AddStmtChoice asc) {
+static bool isFallthroughStatement(const AttributedStmt *A) {
+  return hasSpecificAttr<FallThroughAttr>(A->getAttrs()) &&
+         isa<NullStmt>(A->getSubStmt());
+}
+
+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);

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits