[PATCH] Improve -Winvalid-noreturn

Richard Trieu rtrieu at google.com
Wed May 27 17:22:38 PDT 2015


On Wed, May 27, 2015 at 6:30 AM, Aaron Ballman <aaron at aaronballman.com>
wrote:

> On Tue, May 26, 2015 at 11:16 PM, Richard Trieu <rtrieu at google.com> wrote:
> > 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.
>
> Some minor nits and a question. Should we worry about a pathological case
> like:
>
> struct B {
>   virtual ~B();
> };
>
> struct D : B {
>   [[noreturn]] ~D();
> };
>
> [[noreturn]] void f() {
>   B *b = new D;
>   delete b;
> } // Does this warn?
>
> void g() {
>   B *b = new D;
>   delete b;
>   std::cout << "Does this warn about unreachable code?";
> }
>
> I'm not overly concerned about these cases and any false
> positives/negatives, but we may want to add some FIXMEs to the tests
> for them.
>
> Aside from these tiny nits, LGTM!
>
> Committed in r238382.

'delete' can be used with a null pointer, which is basically a no-op.
Without tracking non-null pointers, the warning can't be sure that a delete
of any object would definitely call a destructor.  I added the delete test
case with FIXMEs.

>
> > Index: include/clang/AST/DeclCXX.h
> > ===================================================================
> > --- include/clang/AST/DeclCXX.h
> > +++ include/clang/AST/DeclCXX.h
> > @@ -1392,6 +1392,10 @@
> >    /// \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.
>
> Use Doxygen-style comments, please.
>
Fixed.

>
> > +  bool isAnyDestructorNoReturn() const;
> > +
> >    /// \brief If the class is a local class [class.local], returns
> >    /// the enclosing function declaration.
> >    const FunctionDecl *isLocalClass() const {
> > Index: lib/AST/DeclCXX.cpp
> > ===================================================================
> > --- lib/AST/DeclCXX.cpp
> > +++ lib/AST/DeclCXX.cpp
> > @@ -1315,6 +1315,30 @@
> >    return Dtor;
> >  }
> >
> > +bool CXXRecordDecl::isAnyDestructorNoReturn() const {
> > +  // Destructor is noreturn
>
> Comments should include punctuation (here and elsewhere).
>
Fixed.

>
> > +  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;
>
> Use std::any_of? (Not required for the LGTM since that's more of a grey
> area.)
>
Skipped.

>
> > +
> > +  // 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);
> >  }
> > Index: lib/Analysis/CFG.cpp
> > ===================================================================
> > --- lib/Analysis/CFG.cpp
> > +++ lib/Analysis/CFG.cpp
> > @@ -1179,8 +1179,7 @@
> >      }
> >      Ty = Context->getBaseElementType(Ty);
> >
> > -    const CXXDestructorDecl *Dtor =
> Ty->getAsCXXRecordDecl()->getDestructor();
> > -    if (Dtor->isNoReturn())
> > +    if (Ty->getAsCXXRecordDecl()->isAnyDestructorNoReturn())
> >        Block = createNoReturnBlock();
> >      else
> >        autoCreateBlock();
> > @@ -3682,7 +3681,7 @@
> >
> >      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
> > Index: test/SemaCXX/attr-noreturn.cpp
> > ===================================================================
> > --- test/SemaCXX/attr-noreturn.cpp
> > +++ test/SemaCXX/attr-noreturn.cpp
> > @@ -17,6 +17,132 @@
> >    }
> >  }
> >
> > +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 (*)());
>
> ~Aaron
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150527/a5569227/attachment.html>


More information about the cfe-commits mailing list