[PATCH] Static analysis checker for catch handler inversion

[Aaron Ballman]

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?

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

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

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_v7_unfinished.patch
Type: application/octet-stream
Size: 15358 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150210/2d7ec1dd/attachment.obj>