[PATCH] D13126: New static analyzer checker for loss of sign/precision

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked 4 inline comments as done.

================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:41
@@ +40,3 @@
+    const Stmt *Parent = PM.getParent(Cast);
+    if (!Parent)
+      return;
----------------
a.sidorin wrote:
> Parent should always exist for an implicit cast. May be it's better to assert here?
If I comment out lines 41 and 42 and run "make check-all" I get:

    FAIL: Clang :: Analysis/misc-ps-region-store.cpp (668 of 24574)
    ******************** TEST 'Clang :: Analysis/misc-ps-region-store.cpp' FAILED ********************
    ....
    #0 ...
    #1 ...
    ...
    #9 0x0000000002f1d062 llvm::isa_impl_cl<clang::BinaryOperator, clang::Stmt const*>::doit(clang::Stmt const*) /home/danielm/llvm/include/llvm/Support/Casting.h:96:0



================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:44
@@ +43,3 @@
+
+    const BinaryOperator *B = dyn_cast<BinaryOperator>(Parent);
+    if (!B)
----------------
a.sidorin wrote:
> Note that InitExprs of DeclStmts are not binary operators. So you will not get a warning on initialization and this test:
> 
> ```
> void test(unsigned int p) {
>   unsigned X = 1000;
>   unsigned char uc = X; // expected-warning {{Loss of precision}}
> }
> ```
> will fail.
I have limited this patch hoping that it will be easier to triage and review. Right now I only warn if there is assignment and RHS is a DeclRefExpr and only for loss of precision. I will definitely look into initializations also later.

================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:49
@@ +48,3 @@
+    BinaryOperator::Opcode Opc = B->getOpcode();
+    if (Opc == BO_Assign || Opc == BO_MulAssign)
+      diagnoseLossOfPrecision(Cast, C);
----------------
a.sidorin wrote:
> It's not evident why do you omit other Assign operators here, like BO_SubAssign, BO_AddAssign and BO_DivAssign. As I see from your test, there are some problems with them. Could you add a comment?
DivAssign is obvious; if the rhs is too large there won't be loss of precision.
But I fail to think of additional problems to handle BO_AddAssign and BO_SubAssign also immediately.. I'll add it in next patch

================
Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:158
@@ +157,3 @@
+
+  unsigned long long MaxVal = 1ULL << W;
+  if (canBeGreaterEqual(C, Cast->getSubExpr(), MaxVal))
----------------
NoQ wrote:
> `BasicValueFactory::getMaxValue(QualType)` might be useful.
Imho this seems harder.

To start with the getMaxValue returns 127 for a signed 8-bit integer.

I don't want to warn here:

     S8 = 0xff;  // <- no loss of precision, all bits are saved

So instead of 1 line I'd get 4+ lines:

    SValBuilder &Bldr = C.getSValBuilder();
    BasicValueFactory &BVF = Bldr.getBasicValueFactory();
    QualType U = ResultType...  // I don't know how to convert ResultType
    const llvm::APSInt &MaxVal1 = BVF.getMaxValue(U);

Maybe I am missing something. Let me know if I do.


http://reviews.llvm.org/D13126