[PATCH] Static analysis checker for catch handler inversion

[Richard Smith]

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

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.

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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141112/9035f4a0/attachment.html>