[PATCH][StaticAnalyzer] new check comparing equal expression
Anna Zaks
ganna at apple.com
Tue Nov 5 09:39:13 PST 2013
On Nov 5, 2013, at 9:26 AM, Jordan Rose <jordan_rose at apple.com> wrote:
> I got a chance to look at the results of analyzing LLVM/Clang with this checker, and also found zero reports—true or false positives. On the one hand, these mistakes probably don't last long in an open-source project. On the other, is it worth having a checker that catches problems that don't show up in the real world?
I don't think it's fair to say that if the reports do not show in clang/llvm codebase, the checker catches problems that don't show up in real world. (Jordan probably misspoke.)
This type of errors would probably be more common in newly written, not very well tested code. Also, clang/llvm has been checked with other tools, which might have exposed these types of bugs already.
> Anna?
>
> (The answer is probably "yes", but I thought I might as well bring it up. Perhaps it's disabled in our "shallow" analysis mode.)
I think this checker SHOULD be enabled in shallow mode. First, shallow is mostly used when people want fast turnaround (as in running analyzer during build). So there is a high chance the analyzer will be run on freshly written code that, I think, is more likely to have bugs like these. Also, this is a syntactic check; those are almost always fast enough for the analyzer.
Anna.
> Jordan
>
>
> On Oct 31, 2013, at 3:20 , Per Viberg <Per.Viberg at evidente.se> wrote:
>
>>
>> Hi Jordan,
>>
>> here is the new patch changed according to your comments below. I have renamed the check from EqualCompareChecker to IdenticalExpChecker to accommodate for upcoming tests in this category. I also ran the analysis on an open source project called cppchecker to check performance.
>>
>> Without IdenticalExpChecker: 5197 seconds
>> With IdenticalExpChecker: 5239 seconds.
>> Result: approximately 40 seconds longer, which amounts to < 1% increase in analyse time.
>>
>> Depending on the coding style of the source code analysed, this could differ dramatically. If someone uses a lot of sub-expressions within sub-expression the analysis time increases, but then this check becomes more important to do since the error will be more difficult to detect by manual review.
>>
>> The checker didn't find any "real" errors in the cppChecker project, and not any false positives.
>>
>> best regards,
>> Per
>> .......................................................................................................................
>> Per Viberg Senior Engineer
>> Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
>> Phone: +46 (0)8 402 79 00
>> Mobile: +46 (0)70 912 42 52
>> E-mail: Per.Viberg at evidente.se
>>
>> www.evidente.se
>> This e-mail, which might contain confidential information, is addressed to the above stated person/company. If you are not the correct addressee, employee or in any other way the person concerned, please notify the sender immediately. At the same time, please delete this e-mail and destroy any prints. Thank You.
>> Från: Jordan Rose [jordan_rose at apple.com]
>> Skickat: den 11 oktober 2013 02:43
>> Till: Per Viberg
>> Cc: cfe-commits at cs.uiuc.edu
>> Ämne: Re: [PATCH][StaticAnalyzer] new check comparing equal expression
>>
>> Hi, Per. Looking good; more comments for you. The performance thing is also something I'd like to get resolved soon: how long does this checker alone take over a large body of code, compared to the existing path-sensitive analysis? (It looks like it's linear on average, quadratic at worst, so I'd expect it to be much cheaper, but still.)
>>
>>
>> +static bool isSameExpr(const ASTContext &Ctx, Expr *Expr1, Expr *Expr2);
>>
>> const Expr *, please?
>>
>>
>> + // only visit nodes that are binary expressions
>>
>> LLVM convention says that all comments should be complete sentences, including capitalization and a final period.
>>
>>
>> + bool isSameExpression;
>>
>> No reason to declare this way ahead of time. Please move this down to where it's actually used—or eliminate it altogether, and just put the call in the if-condition.
>>
>>
>> + DeclRefExpr *declref1 = dyn_cast<DeclRefExpr>(LHS);
>> + DeclRefExpr *declref2 = dyn_cast<DeclRefExpr>(RHS);
>>
>> LLVM convention says that all variable names are UpperCamelCased. There are some of these in isSameExpr as well.
>>
>>
>> + // If any side of comparison still has floatingpointrepresentation,
>> + // then it's an expression-> don't warn.
>> + // (here only LHS is checked since RHS will be implicit casted to float)
>>
>> Please turn these into complete sentences (and split up "floating-point representation").
>>
>>
>> + } else { // No special case with floating point representation, report as
>> + // usual
>> + }
>>
>> I get that it's important to call out this case, but please put the comment on its own line within the body of the if.
>>
>>
>> +// t*(u + t) and t*u + t*t are not considered equal.
>>
>> I wouldn't even worry about this. This is a pretty unlikely typo or copy-paste error. Also, no reason not to make this whole block a proper Doxygen comment. Even if it's not a public function, IDEs can still make use of the comment if it's marked up as Doxygen (and with ///).
>>
>>
>> Has this checker found any real errors in code you have access to? Has it found any false positives?
>>
>> Thanks for working on this!
>> Jordan
>>
>>
>> On Oct 10, 2013, at 8:42 , Per Viberg <Per.Viberg at evidente.se> wrote:
>>
>> >
>> > Hello,
>> >
>> > changed patch according to review comments.
>> >
>> > Please review revised patch.
>> >
>> > The patch adds a check that warns for comparison of equal expressions.
>> > example:
>> > x + 1 == x +1;
>> >
>> > best regards,
>> > Per
>> >
>> > .......................................................................................................................
>> > Per Viberg Senior Engineer
>> > Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
>> > Phone: +46 (0)8 402 79 00
>> > Mobile: +46 (0)70 912 42 52
>> > E-mail: Per.Viberg at evidente.se
>> >
>> > www.evidente.se
>> > This e-mail, which might contain confidential information, is addressed to the above stated person/company. If you are not the correct addressee, employee or in any other way the person concerned, please notify the sender immediately. At the same time, please delete this e-mail and destroy any prints. Thank You.
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
> <IdenticalExpr_rev3.diff>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131105/a40baae2/attachment.html>
More information about the cfe-commits
mailing list