[PATCH] Static analysis checker for catch handler inversion

[Richard Smith]

On Tue, Feb 10, 2015 at 12:40 PM, Aaron Ballman <aaron at aaronballman.com>
wrote:

> On Wed, Jan 14, 2015 at 10:31 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > +  friend struct llvm::DenseMapInfo <QualTypeExt>;
> >
> > Extra space before <.
> >
> > +// It's OK to treat BaseSubobject as a POD type.
> >
> > Typo: BaseSubobject should be QualTypeExt.
>
> I've fixed both those up locally.
>
> >
> > +        // Add direct base classes to the list of types to be checked.
> > [...]
> > +        RD->lookupInBases(
> >
> > lookupInBases finds all bases, not just the direct ones.
>
> I must be missing something, because that doesn't seem to be entirely
> workable. When the callback function passed to lookupInBases returns
> true, searching along that path stops. See CXXInheritance.cpp:251; if
> BaseMatches returns true, we record the path but never visit further
> bases.
>
> That winds up being problematic for situations like:
>
> struct B {};
> struct D : B {};
> struct D2 : D {};
>
> void f6() {
>   try {
>   } catch (B &b) {  // expected-note {{for type 'HandlerInversion::B &'}}
>   } catch (D2 &d) {  // expected-warning {{exception of type
> 'HandlerInversion::D2 &' will be caught by earlier handler}}
>   }
> }
>
> We process the handler for B& and add it to the map. Then we process
> the handler for D2&. When we call lookupInBases on D2, we find D
> first, and since it's access is public, the callback returns true. The
> callback is not called a subsequent time for B as a base of D, and so
> we wind up failing to warn in that case.
>
> Changing that "else if" to be an "if" is the wrong thing to do as
> well, because then a bunch of regression tests fail. I can kind of
> hack the behavior I want by having my callback not be a lambda, and
> call lookupInBases on the base class prior to returning true, but it
> feels like I'm doing something wrong. Ideas?
>

In Frobble, I think you should return true if you find a public path to a
HandledType, not just if you find a public base class.

I've attached a WIP that attempts to capture what you're suggesting
> below, but it's also causing problems with
> test\SemaCXX\unreachable-catch-clauses.cpp reporting the warning for
> the final handler twice, so it's not ready yet. Is this moving in the
> direction you were thinking (aside from obvious issues like the
> callback being named Frobble)?


Yes, thanks.

> 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}}
>

Hmm, while we're here, it'd be good to strip off the 'const' and '&' from
this diagnostic; you can't get an exception object of reference type.


> >        // 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).
>
> Oye, that is basically a false positive, and I'm not certain there's
> much I could realistically do about it either. :-(
>
> Thanks!
>
> ~Aaron
>
> >
> > 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/20150218/c303abbb/attachment.html>