[PATCH] Static analysis checker for catch handler inversion

Richard Smith richard at metafoo.co.uk
Thu Nov 13 19:06:46 PST 2014


+    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.
}

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

On Thu, Nov 13, 2014 at 9:33 AM, Aaron Ballman <aaron at aaronballman.com>
wrote:

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


More information about the cfe-commits mailing list