[PATCH] Static analysis checker for catch handler inversion

Aaron Ballman aaron at aaronballman.com
Tue Apr 7 07:21:45 PDT 2015 

On Fri, Apr 3, 2015 at 8:44 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> Please give QualTypeExt a more specific name to indicate that it represents
> a caught type. CatchHandlerType perhaps?

Renamed to CatchHandlerType (also changed variables named QTE).

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

I changed the class name to CatchTypePublicBases, and changed
FindPublicBasesOfType to be a static member. Also removed accessors
that were no longer required.

> +    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.

You're correct, this was redundant. I changed it over to using the
CXXBaseSpecifier directly, instead of the path elements.

Thanks!

~Aaron

>
> 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 --------------
A non-text attachment was scrubbed...
Name: CatchHandlerChecker_v8.patch
Type: application/octet-stream
Size: 16738 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150407/0bf78983/attachment.obj>

More information about the cfe-commits mailing list