[PATCH] Static analysis checker for catch handler inversion
Richard Smith
richard at metafoo.co.uk
Wed Apr 8 11:36:25 PDT 2015
On Tue, Apr 7, 2015 at 5:01 PM, Aaron Ballman <aaron at aaronballman.com>
wrote:
> On Tue, Apr 7, 2015 at 6:02 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > LGTM, thanks!
> >
> >
> > + if (RD->lookupInBases(CatchTypePublicBases::FindPublicBasesOfType,
> > &CTPB,
> > + Paths)) {
> > + const CXXCatchStmt *Problem = CTPB.getFoundHandler();
> > + if (!Paths.isAmbiguous(CTPB.getFoundHandlerType())) {
> >
> > Is it possible that lookupInBases will find an ambiguous base (so we'll
> skip
> > issuing the warning) but there is another handler for an unambiguous
> base?
> > This seems like a really weird corner case (and a false negative for the
> > warning), so don't worry about it if ambiguity is hard to detect from
> > FindPublicBasesOfType.
>
> Is this basically dr308? We do issue a false-positive in this case,
> unfortunately, but I can't see a particularly great way to handle it.
>
I think it's an even more unlikely case than the one in DR308:
struct A {};
struct B : A {};
struct C : A {};
struct X {};
struct D : B, C, X {};
void f() {
try { throw D(); }
catch (A) {} // does not catch D
catch (X) {} // catches D
catch (D) {} // should warn here
}
I think we won't warn for 'catch (D)' because the handler found by
CatchTypePublicBases will be the A handler, which is ambiguous -- we don't
carry on and find the X handler.
(There's also the case which we discussed previously:
void g() {
try { throw D(); }
catch (A) {} // does not catch D
catch (B) {} // catches D, but we warn anyway
}
I don't think there's anything we can reasonably do to improve this, and
I'm OK with a false positive in this case.)
> I'll land the patch as-is, but we can continue to improve. I'll file a
> bug about it once I'm certain all the bots are happy with the commit.
>
SGTM
> Thanks!
>
> ~Aaron
>
> >
> > On Tue, Apr 7, 2015 at 7:21 AM, Aaron Ballman <aaron at aaronballman.com>
> > wrote:
> >>
> >> On Fri, Apr 3, 2015 at 8:44 PM, Richard Smith <richard at metafoo.co.uk>
> >> wrote:
> >> > Please give QualTypeExt a more specific name to indicate that it
> >> > represents
> >> > a caught type. CatchHandlerType perhaps?
> >>
> >> Renamed to CatchHandlerType (also changed variables named QTE).
> >>
> >> > Please give PublicBasesOfType a name that somehow relates to catch
> >> > handlers,
> >> > since it's specific to them. Also, perhaps make FindPublicBasesOfType
> a
> >> > static member of it, so that you don't need to provide so many public
> >> > accessors. Or maybe make PublicBasesOfType a struct, nest it inside
> >> > ActOnCXXTryBlock, and use a lambda as the callback function.
> >>
> >> I changed the class name to CatchTypePublicBases, and changed
> >> FindPublicBasesOfType to be a static member. Also removed accessors
> >> that were no longer required.
> >>
> >> > + for (const auto &P : Path) {
> >> >
> >> > Do you really need to walk the path? You should get called back once
> for
> >> > each base class, so this seems redundant.
> >>
> >> You're correct, this was redundant. I changed it over to using the
> >> CXXBaseSpecifier directly, instead of the path elements.
> >>
> >> Thanks!
> >>
> >> ~Aaron
> >>
> >> >
> >> > On Fri, Apr 3, 2015 at 5:30 PM, Aaron Ballman <aaron at aaronballman.com
> >
> >> > wrote:
> >> >>
> >> >> Ping
> >> >>
> >> >> On Sun, Mar 22, 2015 at 4:09 PM, Aaron Ballman <
> aaron at aaronballman.com>
> >> >> wrote:
> >> >> > Ping?
> >> >> >
> >> >> > On Thu, Feb 26, 2015 at 4:16 PM, Aaron Ballman
> >> >> > <aaron at aaronballman.com>
> >> >> > wrote:
> >> >> >> Updated patch attached. Comment below.
> >> >> >>
> >> >> >> On Wed, Feb 18, 2015 at 4:54 PM, Richard Smith
> >> >> >> <richard at metafoo.co.uk>
> >> >> >> wrote:
> >> >> >>> On Tue, Feb 10, 2015 at 12:40 PM, Aaron Ballman
> >> >> >>> <aaron at aaronballman.com>
> >> >> >>> wrote:
> >> >> >>>>
> >> >> >>>> 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?
> >> >> >>>
> >> >> >>>
> >> >> >>> In Frobble, I think you should return true if you find a public
> >> >> >>> path
> >> >> >>> to a
> >> >> >>> HandledType, not just if you find a public base class.
> >> >> >>>
> >> >> >>>> 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)?
> >> >> >>>
> >> >> >>>
> >> >> >>> Yes, thanks.
> >> >> >>>
> >> >> >>>> > 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}}
> >> >> >>>
> >> >> >>>
> >> >> >>> Hmm, while we're here, it'd be good to strip off the 'const' and
> >> >> >>> '&'
> >> >> >>> from
> >> >> >>> this diagnostic; you can't get an exception object of reference
> >> >> >>> type.
> >> >> >>
> >> >> >> You can still have a handler of reference type that the exception
> >> >> >> object binds to (which is the type the user is most likely to
> notice
> >> >> >> since it's visibly spelled out). Also, since this diagnostic
> >> >> >> sometimes
> >> >> >> needs to print the pointer, I'm not certain it's worth the added
> >> >> >> complexity to strip in the case of references.
> >> >> >>
> >> >> >> Thanks!
> >> >> >>
> >> >> >> ~Aaron
> >> >> >>
> >> >> >>>
> >> >> >>>>
> >> >> >>>> > // 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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150408/37d4bf23/attachment.html>
More information about the cfe-commits
mailing list