Improve -Winvalid-noreturn

[rtrieu]

Summary

No longer warn on this false positive for -Winvalid-noreturn

__attribute__((noreturn)) void fail();

struct A {
  ~A() __attribute__((noreturn)) { fail(); }
};
struct B : A {
  B() {}
};

__attribute__((noreturn)) void test_1() { A a; }  // no warning
__attribute__((noreturn)) void test_2() { B b; }  // false positive warning here

A new method CXXDesctructorDecl:isAnyDestructorNoReturn that checks if any destructor invoked from the destructor is marked no return. This includes base classes, virtual base classes, and members. Then, replace use of FunctionDecl::isNoReturn with this new method when constructing the CFG.

[rsmith]

lib/AST/DeclCXX.cpp

1940–1946

This seems redundant -- the previous loop already checked all direct bases -- and will be expensive because you'll check every virtual base once for each direct base that inherits from it directly or indirectly.

Maybe delete this loop and only check direct bases (recursively)?

1950

You should insert a getBaseElementTypeUnsafe() call here before grabbing the CXXRecordDecl, in case it's an array of class type.

[rtrieu]

Fix test case in summary. Address comments.

[rtrieu]

lib/AST/DeclCXX.cpp

1940–1946

Removed loop.

1950

Added call to get base element. Added array test case.

[rtrieu]

Ping.

[rtrieu]

Aaron Ballman brought up a point on the mailing list. CXXRecordDecl::getDestructor() can return null in some places. This patch restructures the isAnyDestructorNoReturn() to be on the CXXRecordDecl instead of on the CXXDestructorDecl. If the destructor does not exist, it is safe to assume it does not have the noreturn attribute.

include/clang/AST/DeclCXX.h

public:
   CXXRecordDecl *getTemplateInstantiationPattern() {
     return const_cast<CXXRecordDecl *>(const_cast<const CXXRecordDecl *>(this)
                                            ->getTemplateInstantiationPattern());
   }
 
   /// \brief Returns the destructor decl for this class.
   CXXDestructorDecl *getDestructor() const;
 
+  // Returns true if the class destructor, or any implicitly invoked
+  // destructors are marked noreturn.
+  bool isAnyDestructorNoReturn() const;
+
   /// \brief If the class is a local class [class.local], returns
   /// the enclosing function declaration.
   const FunctionDecl *isLocalClass() const {
     if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(getDeclContext()))
       return RD->isLocalClass();
 
     return dyn_cast<FunctionDecl>(getDeclContext());
   }

lib/AST/DeclCXX.cpp

CXXDestructorDecl *CXXRecordDecl::getDestructor() const {
   DeclContext::lookup_result R = lookup(Name);
   if (R.empty())
     return nullptr;
 
   CXXDestructorDecl *Dtor = cast<CXXDestructorDecl>(R.front());
   return Dtor;
 }
 
+bool CXXRecordDecl::isAnyDestructorNoReturn() const {
+  // Destructor is noreturn
+  if (const CXXDestructorDecl *Destructor = getDestructor())
+    if (Destructor->isNoReturn())
+      return true;
+
+  // Check base classes destructor for noreturn
+  for (const auto &Base : bases())
+    if (Base.getType()
+            ->getAsCXXRecordDecl()
+            ->isAnyDestructorNoReturn())
+      return true;
+
+  // Check fields for noreturn
+  for (const auto *Field : fields())
+    if (const CXXRecordDecl *RD =
+            Field->getType()->getBaseElementTypeUnsafe()->getAsCXXRecordDecl())
+      if (RD->isAnyDestructorNoReturn())
+        return true;
+
+  // All destructors are not noreturn
+  return false;
+}
+
 void CXXRecordDecl::completeDefinition() {
   completeDefinition(nullptr);
 }
 
 void CXXRecordDecl::completeDefinition(CXXFinalOverriderMap *FinalOverriders) {
   RecordDecl::completeDefinition();
   
   // If the class may be abstract (but hasn't been marked as such), check for
return new (C, ID) CXXConversionDecl(C, nullptr, SourceLocation(),
                                        nullptr, false, false, false,
                                        SourceLocation());
 }
 
 CXXConversionDecl *
 CXXConversionDecl::Create(ASTContext &C, CXXRecordDecl *RD,
                           SourceLocation StartLoc,
                           const DeclarationNameInfo &NameInfo,
                           QualType T, TypeSourceInfo *TInfo,
                           bool isInline, bool isExplicit,
                           bool isConstexpr, SourceLocation EndLocation) {
   assert(NameInfo.getName().getNameKind()
          == DeclarationName::CXXConversionFunctionName &&
          "Name must refer to a conversion function");
   return new (C, RD) CXXConversionDecl(C, RD, StartLoc, NameInfo, T, TInfo,
                                        isInline, isExplicit, isConstexpr,
                                        EndLocation);
 }
 
 bool CXXConversionDecl::isLambdaToBlockPointerConversion() const {
   return isImplicit() && getParent()->isLambda() &&
          getConversionType()->isBlockPointerType();
 }
 
 void LinkageSpecDecl::anchor() { }
 
 LinkageSpecDecl *LinkageSpecDecl::Create(ASTContext &C,

lib/Analysis/CFG.cpp

for (SmallVectorImpl<VarDecl*>::reverse_iterator I = Decls.rbegin(),
     // create a new block for the destructor which does not have as a successor
     // anything built thus far: control won't flow out of this block.
     QualType Ty = (*I)->getType();
     if (Ty->isReferenceType()) {
       Ty = getReferenceInitTemporaryType(*Context, (*I)->getInit());
     }
     Ty = Context->getBaseElementType(Ty);
 
-    const CXXDestructorDecl *Dtor = Ty->getAsCXXRecordDecl()->getDestructor();
-    if (Dtor->isNoReturn())
+    if (Ty->getAsCXXRecordDecl()->isAnyDestructorNoReturn())
       Block = createNoReturnBlock();
     else
       autoCreateBlock();
 
     appendAutomaticObjDtor(Block, *I, S);
   }
 }
 
CFGBlock *CFGBuilder::VisitCXXBindTemporaryExprForTemporaryDtors(
   // First add destructors for temporaries in subexpression.
   CFGBlock *B = VisitForTemporaryDtors(E->getSubExpr(), false, Context);
   if (!BindToTemporary) {
     // If lifetime of temporary is not prolonged (by assigning to constant
     // reference) add destructor for it.
 
     const CXXDestructorDecl *Dtor = E->getTemporary()->getDestructor();
 
-    if (Dtor->isNoReturn()) {
+    if (Dtor->getParent()->isAnyDestructorNoReturn()) {
       // If the destructor is marked as a no-return destructor, we need to
       // create a new block for the destructor which does not have as a
       // successor anything built thus far. Control won't flow out of this
       // block.
       if (B) Succ = B;
       Block = createNoReturnBlock();
     } else if (Context.needsTempDtorBranch()) {
       // If we need to introduce a branch, we add a new block that we will hook

test/SemaCXX/attr-noreturn.cpp

int &test1() {
     a.nofail();
   } // expected-warning {{control reaches end of non-void function}}
 
   int &test2() {
     a.fail();
   }
 }
 
+namespace destructor_tests {
+  __attribute__((noreturn)) void fail();
+
+  struct A {
+    ~A() __attribute__((noreturn)) { fail(); }
+  };
+  struct B {
+    B() {}
+    ~B() __attribute__((noreturn)) { fail(); }
+  };
+  struct C : A {};
+  struct D : B {};
+  struct E : virtual A {};
+  struct F : A, virtual B {};
+  struct G : E {};
+  struct H : virtual D {};
+  struct I : A {};
+  struct J : I {};
+  struct K : virtual A {};
+  struct L : K {};
+  struct M : virtual C {};
+  struct N : M {};
+  struct O { N n; };
+
+  __attribute__((noreturn)) void test_1() { A a; }
+  __attribute__((noreturn)) void test_2() { B b; }
+  __attribute__((noreturn)) void test_3() { C c; }
+  __attribute__((noreturn)) void test_4() { D d; }
+  __attribute__((noreturn)) void test_5() { E e; }
+  __attribute__((noreturn)) void test_6() { F f; }
+  __attribute__((noreturn)) void test_7() { G g; }
+  __attribute__((noreturn)) void test_8() { H h; }
+  __attribute__((noreturn)) void test_9() { I i; }
+  __attribute__((noreturn)) void test_10() { J j; }
+  __attribute__((noreturn)) void test_11() { K k; }
+  __attribute__((noreturn)) void test_12() { L l; }
+  __attribute__((noreturn)) void test_13() { M m; }
+  __attribute__((noreturn)) void test_14() { N n; }
+  __attribute__((noreturn)) void test_15() { O o; }
+
+  __attribute__((noreturn)) void test_16() { const A& a = A(); }
+  __attribute__((noreturn)) void test_17() { const B& b = B(); }
+  __attribute__((noreturn)) void test_18() { const C& c = C(); }
+  __attribute__((noreturn)) void test_19() { const D& d = D(); }
+  __attribute__((noreturn)) void test_20() { const E& e = E(); }
+  __attribute__((noreturn)) void test_21() { const F& f = F(); }
+  __attribute__((noreturn)) void test_22() { const G& g = G(); }
+  __attribute__((noreturn)) void test_23() { const H& h = H(); }
+  __attribute__((noreturn)) void test_24() { const I& i = I(); }
+  __attribute__((noreturn)) void test_25() { const J& j = J(); }
+  __attribute__((noreturn)) void test_26() { const K& k = K(); }
+  __attribute__((noreturn)) void test_27() { const L& l = L(); }
+  __attribute__((noreturn)) void test_28() { const M& m = M(); }
+  __attribute__((noreturn)) void test_29() { const N& n = N(); }
+  __attribute__((noreturn)) void test_30() { const O& o = O(); }
+
+  struct AA {};
+  struct BB { BB() {} ~BB() {} };
+  struct CC : AA {};
+  struct DD : BB {};
+  struct EE : virtual AA {};
+  struct FF : AA, virtual BB {};
+  struct GG : EE {};
+  struct HH : virtual DD {};
+  struct II : AA {};
+  struct JJ : II {};
+  struct KK : virtual AA {};
+  struct LL : KK {};
+  struct MM : virtual CC {};
+  struct NN : MM {};
+  struct OO { NN n; };
+
+  __attribute__((noreturn)) void test_31() {
+    AA a;
+    BB b;
+    CC c;
+    DD d;
+    EE e;
+    FF f;
+    GG g;
+    HH h;
+    II i;
+    JJ j;
+    KK k;
+    LL l;
+    MM m;
+    NN n;
+    OO o;
+
+    const AA& aa = AA();
+    const BB& bb = BB();
+    const CC& cc = CC();
+    const DD& dd = DD();
+    const EE& ee = EE();
+    const FF& ff = FF();
+    const GG& gg = GG();
+    const HH& hh = HH();
+    const II& ii = II();
+    const JJ& jj = JJ();
+    const KK& kk = KK();
+    const LL& ll = LL();
+    const MM& mm = MM();
+    const NN& nn = NN();
+    const OO& oo = OO();
+  }  // expected-warning {{function declared 'noreturn' should not return}}
+
+  struct P {
+    ~P() __attribute__((noreturn)) { fail(); }
+    void foo() {}
+  };
+  struct Q : P { };
+  __attribute__((noreturn)) void test31() {
+    P().foo();
+  }
+  __attribute__((noreturn)) void test32() {
+    Q().foo();
+  }
+
+  struct R {
+    A a[5];
+  };
+  __attribute__((noreturn)) void test33() {
+    R r;
+  }
+}
+
 // PR5620
 void f0() __attribute__((__noreturn__));
 void f1(void (*)()); 
 void f2() { f1(f0); }
 
 // Taking the address of a noreturn function
 void test_f0a() {
   void (*fp)() = f0;