[PATCH] Static analysis checker for catch handler inversion

[Richard Smith]

+  friend struct llvm::DenseMapInfo <QualTypeExt>;

Extra space before <.

+// It's OK to treat BaseSubobject as a POD type.

Typo: BaseSubobject should be QualTypeExt.

+        // Add direct base classes to the list of types to be checked.
[...]
+        RD->lookupInBases(

lookupInBases finds all bases, not just the direct ones. You should be able
to do something like this:

1) If the type is a class type, call lookupInBases, and check none of the
public unambiguous bases are in the map.
2) Add the type itself to the map and diagnose if it was already there.

(and remove your explicit queue of types to process).


--- test/CXX/drs/dr3xx.cpp (revision 222171)
+++ test/CXX/drs/dr3xx.cpp (working copy)
@@ -170,9 +170,9 @@
   void f() {
     try {
       throw D();
-    } catch (const A&) {
+    } catch (const A&) { // expected-note {{for type 'const dr308::A &'}}
       // unreachable
-    } catch (const B&) {
+    } catch (const B&) { // expected-warning {{exception of type 'const
dr308::B &' will be caught by earlier handler}}
       // get here instead
     }
   }

Yikes, the warning is sort of a false-positive here! (The text of the
warning is true, but the 'throw D()' will actually be caught by the second
handler, because A is an ambiguous base of D).

On Thu, Dec 4, 2014 at 7:23 AM, Aaron Ballman <aaron at aaronballman.com>
wrote:

> Ping
>
> On Tue, Nov 25, 2014 at 9:59 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
> > Ping
> >
> > On Mon, Nov 17, 2014 at 4:15 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
> >> On Fri, Nov 14, 2014 at 5:36 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> >>> On Fri, Nov 14, 2014 at 7:55 AM, Aaron Ballman <aaron at aaronballman.com
> >
> >>> wrote:
> >>>>
> >>>> On Thu, Nov 13, 2014 at 10:06 PM, Richard Smith <
> richard at metafoo.co.uk>
> >>>> wrote:
> >>>> > +    std::list<QualType> BaseClassTypes;
> >>>> >
> >>>> > This will allocate memory for every node; it would be better to use
> a
> >>>> > SmallVector here (and use a depth-first search rather than a
> >>>> > breadth-first
> >>>> > one so you're only modifying one end of your list).
> >>>> >
> >>>> > +    for (auto &CurType : BaseClassTypes) {
> >>>> >
> >>>> > It's a bit scary to be iterating over your container while
> modifying it
> >>>> > (even though it's actually correct in this case). Alternative idiom:
> >>>> >
> >>>> > SmallVector<QualType, 8> BaseClassTypes;
> >>>> > BaseClassTypes.push_back(...);
> >>>> > while (!BaseClassTypes.empty()) {
> >>>> >   QualType T = BaseClassTypes.pop_back_val();
> >>>> >   // ... maybe push back some values.
> >>>> > }
> >>>>
> >>>> I have a strong preference for your idiom. ;-)
> >>>>
> >>>> >
> >>>> > +      auto I = std::find_if(
> >>>> > +          HandledTypes.begin(), HandledTypes.end(),
> >>>> >
> >>>> > This is still quadratic-time; maybe use a DenseMap with the
> canonical
> >>>> > QualType plus a "was it a pointer" flag as a key?
> >>>>
> >>>> I've given this a shot in my latest patch. Thank you for the feedback!
> >>>
> >>>
> >>>  +  unsigned IsReference : 1;
> >>>
> >>> I think we don't need / want an IsReference flag. Consider:
> >>>
> >>>   try { throw Derived(); }
> >>>   catch (Base) {}
> >>>   catch (Derived &d) {}
> >>>
> >>> The second catch handler is unreachable even though we have a by
> >>> reference/by value mismatch.
> >>
> >> That's a very good point. Thanks! I've added an additional test for
> that case.
> >>
> >>>
> >>>
> >>> +      QualType Underlying = CurType.underlying();
> >>> +      if (auto *RD = Underlying->getAsCXXRecordDecl()) {
> >>> +        for (const auto &B : RD->bases())
> >>> +          BaseClassTypes.push_back(QualTypeExt(B.getType(),
> >>> CurType.isPointer(),
> >>> +
>  CurType.isReference()));
> >>>
> >>> Per [except.handle]p1, you should only consider public, unambiguous
> base
> >>> classes here. Rather than walk the base classes yourself, you could use
> >>> CXXRecordDecl::lookupInBases to enumerate the base classes and paths,
> and
> >>> then check the public unambiguous ones.
> >>
> >> I've made this change in the attached patch. Additionally, this patch
> >> no longer pays attention to exception declarations that are invalid,
> >> such as can be had from using auto in a catch clause.
> >>
> >> Thanks!
> >>
> >> ~Aaron
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150114/7a1e2fe0/attachment.html>