[PATCH] Static analysis checker for catch handler inversion

Richard Smith richard at metafoo.co.uk
Fri Apr 3 17:44:03 PDT 2015 

Please give QualTypeExt a more specific name to indicate that it represents
a caught type. CatchHandlerType perhaps?

Please give PublicBasesOfType a name that somehow relates to catch
handlers, since it's specific to them. Also, perhaps make
FindPublicBasesOfType a static member of it, so that you don't need to
provide so many public accessors. Or maybe make PublicBasesOfType a struct,
nest it inside ActOnCXXTryBlock, and use a lambda as the callback function.

+    for (const auto &P : Path) {

Do you really need to walk the path? You should get called back once for
each base class, so this seems redundant.

On Fri, Apr 3, 2015 at 5:30 PM, Aaron Ballman <aaron at aaronballman.com>
wrote:

> Ping
>
> On Sun, Mar 22, 2015 at 4:09 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
> > Ping?
> >
> > On Thu, Feb 26, 2015 at 4:16 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
> >> Updated patch attached. Comment below.
> >>
> >> On Wed, Feb 18, 2015 at 4:54 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> >>> 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.
> >>
> >> You can still have a handler of reference type that the exception
> >> object binds to (which is the type the user is most likely to notice
> >> since it's visibly spelled out). Also, since this diagnostic sometimes
> >> needs to print the pointer, I'm not certain it's worth the added
> >> complexity to strip in the case of references.
> >>
> >> Thanks!
> >>
> >> ~Aaron
> >>
> >>>
> >>>>
> >>>> >        // 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/20150403/2fb55666/attachment.html>

More information about the cfe-commits mailing list