SV: [PATCH] [StaticAnalyzer] New checker StringPlusChar

[Daniel Marjamäki]

Thanks Jordan!

You have two comments that I'd like to comment on.

>> +    // Only check add operators.
>> +    if (B->getOpcode() != BO_Add)
>
> How about +=?

Yes. It sounds good with +=.

Is it ok if we keep this patch as it is.. for simplicitys sake.

After this patch is accepted we can add a new patch with a few improvements.

I also have in mind to extend it so it warns about CharPlusString. I am hoping it is fine to wait with that too.

>> +    const CharacterLiteral *CharExpr =
>> +        dyn_cast<CharacterLiteral>(B->getRHS()->IgnoreImpCasts());
>
> Should this be a character literal, or just any expression with "char" type? I realize that someone could be using a char-sized variable as an index, but that seems fairly rare, and harder to catch by hand.

Using char-sized variable as index is not that rare imho.

I especially think that is true when you are in a environment when you dont use 'char', 'int', etc but use s8,s16,s32 instead. as you know this is not too uncommon. if you normally use s8 instead of s32 when you know the value is small then some index variables will be s8..

Best regards,
Daniel Marjamäki

..................................................................................................................
Daniel Marjamäki Senior Engineer
Evidente ES East AB  Warfvinges väg 34  SE-112 51 Stockholm  Sweden

Mobile:                 +46 (0)709 12 42 62
E-mail:                 Daniel.Marjamaki at evidente.se

www.evidente.se



Från: cfe-commits-bounces at cs.uiuc.edu [cfe-commits-bounces at cs.uiuc.edu] för Jordan Rose [jordan_rose at apple.com]

Skickat: den 1 oktober 2013 18:32

Till: Anders Rönnholm

Cc: cfe-commits at cs.uiuc.edu

Ämne: Re: [PATCH] [StaticAnalyzer] New checker StringPlusChar



The categories are supposed to be very broad groupings: "Logic error", "Memory leak", "Convention X". "Logic error" is probably fine here, even though it's generic, since there's not really a group that fits this general kind of mistake. I just want to
 avoid magic strings—we shouldn't have variants of the same thing because they'll show up as different categories.



Comments on the patch:




+    // Only check add operators.
+    if (B->getOpcode() != BO_Add)

How about +=?







+    const CharacterLiteral *CharExpr =
+        dyn_cast<CharacterLiteral>(B->getRHS()->IgnoreImpCasts());




Should this be a character literal, or just any expression with "char" type? I realize that someone could be using a char-sized variable as an index, but that seems fairly rare, and harder to catch by hand.







+    if (!(StringRefExpr->getType() ==
+              Context.getPointerType(Context.getConstType(Context.CharTy)) ||
+          StringRefExpr->getType() == Context.getPointerType(Context.CharTy)))




Things to consider:
1. Do explicitly signed or unsigned chars count? In that case, you should use Type::isCharType().
2. Do wide chars count? Then you should use Type::isAnyCharacterType().
3. Even if none of the above, you should consider typedefs, so it's still worth going through getCanonicalType() and getPointeeType() rather than building up a new pointer type.



One possibility: StringRefExpr->getType()->getPointeeType()->isAnyCharacterType(). At this point, though, StringRefExpr->getType() probably deserves a helper variable.







+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.core.StringPlusChar -verify %s




Please include the "core" package in these checks; running without core enabled isn't a use case we're interested in.



Jordan






On Sep 30, 2013, at 3:25 , Anders Rönnholm <Anders.Ronnholm at evidente.se> wrote:

Hi
 Jordan,

Thank
 you for your comments. I have changed the check to use RecursiveASTVisitor. However i'm not sure what i should write as category in bugreport. You mentioned adding a new category in CommonBugCategories for SIzeofOnExpression, should i do that or is what i
 have enough for now? 

I
 have provided a new diff.

//Anders