[PATCH] Static analysis checker for catch handler inversion
Aaron Ballman
aaron at aaronballman.com
Thu Feb 26 13:16:27 PST 2015
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 --------------
A non-text attachment was scrubbed...
Name: CatchHandlerChecker_v7.patch
Type: application/octet-stream
Size: 16743 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150226/bb9cf3c9/attachment.obj>
More information about the cfe-commits
mailing list