[PATCH] Static analysis checker for catch handler inversion

[Aaron Ballman]

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_v6.patch
Type: application/octet-stream
Size: 14882 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141117/21b09b4a/attachment.obj>