[analyzer] Do not emit redundant SymbolCasts

[martong]

Summary

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().

[steakhal]

We can't just do that check in evalCast because there are many additonal logic before we'd end up in makeNonLoc.

I'm not exactly up to date. Could you please extend the summary with an example to underpin this statement prior to committing?

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

116–117

Should we account for qualified types?
If not, could we assert that?

clang/test/Analysis/symbolcast-floatingpoint.cpp

12–24

[martong]

In D128068#3594171, @steakhal wrote:

We can't just do that check in evalCast because there are many additonal logic before we'd end up in makeNonLoc.

I'm not exactly up to date. Could you please extend the summary with an example to underpin this statement prior to committing?

Yeah, okay, I've updated the summary and will do so with the commit message as well.

clang/lib/StaticAnalyzer/Core/SValBuilder.cpp

NonLoc SValBuilder::makeNonLoc(const SymExpr *operand, UnaryOperator::Opcode op,
   assert(!Loc::isLocType(type));
   return nonloc::SymbolVal(SymMgr.getUnarySymExpr(operand, op, type));
 }
 
 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));
 }
 
 SVal SValBuilder::convertToArrayIndex(SVal val) {
   if (val.isUnknownOrUndef())
     return val;
 
   // Common case: we have an appropriately sized integer.

clang/test/Analysis/symbolcast-floatingpoint.cpp

+// 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)
+
+}