[clang] 5d7fa48 - [analyzer] Do not emit redundant SymbolCasts

[Gabor Marton via cfe-commits]

Author: Gabor Marton
Date: 2022-07-05T18:42:34+02:00
New Revision: 5d7fa481cf4d36e14345b1c3ebc951edda0b40b1

URL: https://github.com/llvm/llvm-project/commit/5d7fa481cf4d36e14345b1c3ebc951edda0b40b1
DIFF: https://github.com/llvm/llvm-project/commit/5d7fa481cf4d36e14345b1c3ebc951edda0b40b1.diff

LOG: [analyzer] Do not emit redundant SymbolCasts

In `RegionStore::getBinding` we call `evalCast` unconditionally to align
the stored value's type to the one that is being queried. However, the
stored type might be the same, so we may end up having redundant
`SymbolCasts` emitted.

The solution is to check whether the `to` and `from` type are the same
in `makeNonLoc`.

Note, we can't just do type equivalence check at the beginning of `evalCast`
because when `evalCast` is called from `getBinding` then the original type
(`OriginalTy`) is not set, so one operand is missing for the comparison. In
`evalCastSubKind(nonloc::SymbolVal)` when the original type is not set,
we get the `from` type via `SymbolVal::getType()`.

Differential Revision: https://reviews.llvm.org/D128068

Added: 
    clang/test/Analysis/symbolcast-floatingpoint.cpp

Modified: 
    clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
index 13fac37899cd2..7b08acdb569b0 100644
--- a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -113,6 +113,8 @@ nonloc::SymbolVal SValBuilder::makeNonLoc(const SymExpr *operand,
                                           QualType fromTy, QualType toTy) {
   assert(operand);
   assert(!Loc::isLocType(toTy));
+  if (fromTy == toTy)
+    return operand;
   return nonloc::SymbolVal(SymMgr.getCastSymbol(operand, fromTy, toTy));
 }
 

diff  --git a/clang/test/Analysis/symbolcast-floatingpoint.cpp b/clang/test/Analysis/symbolcast-floatingpoint.cpp
new file mode 100644
index 0000000000000..8ea4c9b6c24c1
--- /dev/null
+++ b/clang/test/Analysis/symbolcast-floatingpoint.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_analyze_cc1 -x c++ -analyzer-checker=debug.ExprInspection \
+// RUN:    -analyzer-config support-symbolic-integer-casts=false \
+// RUN:    -verify %s
+
+// RUN: %clang_analyze_cc1 -x c++ -analyzer-checker=debug.ExprInspection \
+// RUN:    -analyzer-config support-symbolic-integer-casts=true \
+// RUN:    -verify %s
+
+template <typename T>
+void clang_analyzer_dump(T);
+
+void test_no_redundant_floating_point_cast(int n) {
+
+  double D = n / 30;
+  clang_analyzer_dump(D); // expected-warning{{(double) ((reg_$0<int n>) / 30)}}
+
+  // There are two cast operations evaluated above:
+  // 1. (n / 30) is cast to a double during the store of `D`.
+  // 2. Then in the next line, in RegionStore::getBinding during the load of `D`.
+  //
+  // We should not see in the dump of the SVal any redundant casts like
+  // (double) ((double) $n / 30)
+
+}