[PATCH] Static analysis checker for catch handler inversion

[Aaron Ballman]

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