[Analyzer] ConversionChecker: track back the cast expression

[martong]

Summary

Adding trackExpressionValue to the checker so it tracks the value of the
implicit cast's DeclRefExpression up to initialization/assignment. This
way the report becomes cleaner.

[steakhal]

LGTM.
Thanks for fixing this.

Should this checker remain in alpha in its current form?
WDYT? @all

[martong]

In D109836#3002811, @steakhal wrote:

LGTM.
Thanks for fixing this.

Thanks for the review!

Should this checker remain in alpha in its current form?
WDYT? @all

There is no clear explanation for this checker being alpha. TLDR; I think this could be moved out of alpha now. @NoQ ?

The checker was accepted in this patch https://reviews.llvm.org/D13126 . Here, Artem vaguely explains he would "probably move out of alpha whenever it's tweaked to somehow make sure it finds enough real bugs among intentional integer truncations, which would most likely require some nontrivial heuristics". Daniel (the author) claims that he found roughly 10-15% false positives: "I have looked all warnings that I got. 1678 projects where scanned. In total I got 124 warnings. I classified 91 warnings as TP. 14 as FP. and then there were 19 that I failed to triage."

Since then, the author created a patch that targets some of the FPs: https://reviews.llvm.org/D25596
With this patch, the bugreports are cleaner. Also, in our bugreport database, I could not found any obvious false positives for the checker.

clang/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp

 private:
   mutable std::unique_ptr<BuiltinBug> BT;
 
   bool isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType,
                          CheckerContext &C) const;
 
   bool isLossOfSign(const ImplicitCastExpr *Cast, CheckerContext &C) const;
 
-  void reportBug(ExplodedNode *N, CheckerContext &C, const char Msg[]) const;
+  void reportBug(ExplodedNode *N, const Expr *E, CheckerContext &C,
+                 const char Msg[]) const;
 };
 }
 
 void ConversionChecker::checkPreStmt(const ImplicitCastExpr *Cast,
                                      CheckerContext &C) const {
   // TODO: For now we only warn about DeclRefExpr, to avoid noise. Warn for
   // calculations also.
   if (!isa<DeclRefExpr>(Cast->IgnoreParenImpCasts()))
void ConversionChecker::checkPreStmt(const ImplicitCastExpr *Cast,
   }
 
   if (LossOfSign || LossOfPrecision) {
     // Generate an error node.
     ExplodedNode *N = C.generateNonFatalErrorNode(C.getState());
     if (!N)
       return;
     if (LossOfSign)
-      reportBug(N, C, "Loss of sign in implicit conversion");
+      reportBug(N, Cast, C, "Loss of sign in implicit conversion");
     if (LossOfPrecision)
-      reportBug(N, C, "Loss of precision in implicit conversion");
+      reportBug(N, Cast, C, "Loss of precision in implicit conversion");
   }
 }
 
-void ConversionChecker::reportBug(ExplodedNode *N, CheckerContext &C,
-                                  const char Msg[]) const {
+void ConversionChecker::reportBug(ExplodedNode *N, const Expr *E,
+                                  CheckerContext &C, const char Msg[]) const {
   if (!BT)
     BT.reset(
         new BuiltinBug(this, "Conversion", "Possible loss of sign/precision."));
 
   // Generate a report for this bug.
   auto R = std::make_unique<PathSensitiveBugReport>(*BT, Msg, N);
+  bugreporter::trackExpressionValue(N, E, *R);
   C.emitReport(std::move(R));
 }
 
 bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast,
                                           QualType DestType,
                                           CheckerContext &C) const {
   // Don't warn about explicit loss of precision.
   if (Cast->isEvaluatable(C.getASTContext()))

clang/test/Analysis/conversion-tracking-notes.c

+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -Wno-conversion -Wno-tautological-constant-compare \
+// RUN:   -analyzer-checker=core,apiModeling,alpha.core.Conversion \
+// RUN:   -analyzer-output=text \
+// RUN:   -verify
+
+unsigned char U8;
+signed char S8;
+
+void track_assign() {
+  unsigned long L = 1000; // expected-note {{'L' initialized to 1000}}
+  int I = -1;             // expected-note {{'I' initialized to -1}}
+  U8 *= L; // expected-warning {{Loss of precision in implicit conversion}}
+           // expected-note@-1 {{Loss of precision in implicit conversion}}
+  L *= I;  // expected-warning {{Loss of sign in implicit conversion}}
+           // expected-note@-1 {{Loss of sign in implicit conversion}}
+}
+
+void track_relational(unsigned U, signed S) {
+  if (S < -10) { // expected-note    {{Taking true branch}}
+                 // expected-note@-1 {{Assuming the condition is true}}
+    if (U < S) { // expected-warning {{Loss of sign in implicit conversion}}
+                 // expected-note@-1 {{Loss of sign in implicit conversion}}
+    }
+  }
+}

clang/test/Analysis/conversion.c

-// RUN: %clang_analyze_cc1 -Wno-conversion -Wno-tautological-constant-compare -analyzer-checker=core,apiModeling,alpha.core.Conversion -verify %s
+// RUN: %clang_analyze_cc1 %s \
+// RUN:   -Wno-conversion -Wno-tautological-constant-compare \
+// RUN:   -analyzer-checker=core,apiModeling,alpha.core.Conversion \
+// RUN:   -verify
 
 unsigned char U8;
 signed char S8;
 
 void assign(unsigned U, signed S) {
   if (S < -10)
     U8 = S; // expected-warning {{Loss of sign in implicit conversion}}
   if (U > 300)