[PATCH][StaticAnalyzer] new check comparing equal expression

Jordan Rose jordan_rose at apple.com
Fri Nov 8 09:45:44 PST 2013 

Okay, the checker is committed in r194236. Thanks, Per!

Jordan


On Nov 5, 2013, at 9:39 , Anna Zaks <ganna at apple.com> wrote:

> 
> 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/20131108/93d2a07b/attachment.html>

More information about the cfe-commits mailing list