[PATCH] Static analysis checker for catch handler inversion

[Aaron Ballman]

On Wed, Nov 12, 2014 at 5:05 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> I think it's not too hard to avoid the quadratic performance here. Algorithm
> outline:
>
> Start with empty map of type -> handlers
> For each catch handler for a type T:
>   For each base class U of T (including T itself), most-derived-first:
>     If we have a handler for U in our map, warn, break
>   Add handler for T to our map
>
> (This is still quadratic if you have struct A<n> : A<n-1>, and catch A<0>,
> A<1>, ..., A<n> in that order, but in that case I think your approach is
> cubic (because IsDerivedFrom is itself linear in the number of base
> classes.)

I've reworked the patch to implement your algorithm. Thanks!

>
> Also, I'm not sure you're correctly handling this case:
>
>   try { ... }
>   catch (Base *) {}
>   catch (Derived &) {}
>
> We shouldn't warn here, but I think you might, because you strip off a
> top-level * or & quite early. I'd also like to see some test coverage for
> catch parameters with cv-qualifiers.

Correct, that was handled incorrectly before, but has been fixed in
this patch. I've added some further tests to make sure of it.

Thanks!

~Aaron

>
> On Tue, Oct 28, 2014 at 7:23 AM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> I noticed that this warning was a bit anachronistic, so I've done some
>> updates. This patch now gives the warning a new group (GCC does not
>> have a group for this, so I've created -Wexceptions), and now prints
>> the QualType directly instead of forcing it through a string.
>>
>> ~Aaron
>>
>> On Mon, Oct 27, 2014 at 9:00 PM, Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>> > Here is a reworked version of the same patch -- one thing to note is
>> > that this already had a FIXME in the code regarding it being quadratic
>> > performance. This patch retains the note about that issue, but I am
>> > not too worried about the performance as this is per-try block. If
>> > performance is truly questionable, we can always use the checker-based
>> > approach instead.
>> >
>> > I've removed the TypeWithHandler helper class as it's essentially just
>> > a std::pair with special logic for operator<. Since we have to perform
>> > the quadratic check for derivative classes, it doesn't make sense to
>> > then loop a second time over the same (sorted) data just for a simple
>> > comparison of QualTypes. This makes my patch a performance regression
>> > for testing duplicate handlers -- I do not have numbers regarding
>> > whether this is actually a measurable regression for realistic test
>> > cases or not, but my gut reaction based on our assumption of 8
>> > handlers being a reasonable number suggests this likely isn't
>> > perceptible.
>> >
>> > ~Aaron
>> >
>> > On Mon, Oct 27, 2014 at 7:40 PM, Aaron Ballman <aaron at aaronballman.com>
>> > wrote:
>> >> On Mon, Oct 27, 2014 at 7:38 PM, Richard Smith <richard at metafoo.co.uk>
>> >> wrote:
>> >>> On Mon, Oct 27, 2014 at 4:35 PM, Aaron Ballman
>> >>> <aaron at aaronballman.com>
>> >>> wrote:
>> >>>>
>> >>>> On Mon, Oct 27, 2014 at 7:30 PM, Jordan Rose <jordan_rose at apple.com>
>> >>>> wrote:
>> >>>> > Nifty! But do you think this is cheap enough for a general compiler
>> >>>> > warning? It certainly doesn't depend on the analyzer's
>> >>>> > path-sensitive
>> >>>> > analysis, so it's mostly just how much we care about the cost of
>> >>>> > isDerivedFrom.
>> >>>>
>> >>>> This should be relatively inexpensive, so it may make sense as a
>> >>>> general compiler warning if others feel that's a better approach.
>> >>>
>> >>>
>> >>> Yes, I think this is a good candidate for an (on-by-default) compiler
>> >>> warning.
>> >>
>> >> Then I'll rework this, thanks!
>> >>
>> >> ~Aaron
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: CatchHandlerChecker_v4.patch
Type: application/octet-stream
Size: 12304 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141113/affe6e5e/attachment.obj>