[PATCH] D128068: [analyzer] Do not emit redundant SymbolCasts

[Gabor Marton via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5d7fa481cf4d: [analyzer] Do not emit redundant SymbolCasts 
(authored by martong).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128068/new/

https://reviews.llvm.org/D128068

Files:
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/test/Analysis/symbolcast-floatingpoint.cpp


Index: clang/test/Analysis/symbolcast-floatingpoint.cpp
===
--- /dev/null
+++ 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 
+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) / 
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)
+
+}
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -113,6 +113,8 @@
   QualType fromTy, QualType toTy) {
   assert(operand);
   assert(!Loc::isLocType(toTy));
+  if (fromTy == toTy)
+return operand;
   return nonloc::SymbolVal(SymMgr.getCastSymbol(operand, fromTy, toTy));
 }
 


Index: clang/test/Analysis/symbolcast-floatingpoint.cpp
===
--- /dev/null
+++ 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 
+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) / 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)
+
+}
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -113,6 +113,8 @@
   QualType fromTy, QualType toTy) {
   assert(operand);
   assert(!Loc::isLocType(toTy));
+  if (fromTy == toTy)
+return operand;
   return nonloc::SymbolVal(SymMgr.getCastSymbol(operand, fromTy, toTy));
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128068: [analyzer] Do not emit redundant SymbolCasts

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128068/new/

https://reviews.llvm.org/D128068

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128068: [analyzer] Do not emit redundant SymbolCasts

[Balázs Benics via Phabricator via cfe-commits]

steakhal accepted this revision.
steakhal added a comment.
This revision is now accepted and ready to land.

> 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?




Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:116-117
   assert(!Loc::isLocType(toTy));
+  if (fromTy == toTy)
+return operand;
   return nonloc::SymbolVal(SymMgr.getCastSymbol(operand, fromTy, toTy));

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



Comment at: clang/test/Analysis/symbolcast-floatingpoint.cpp:12-24
+void test_no_redundant_floating_point_cast(int n) {
+
+  double D = n / 30;
+  clang_analyzer_dump(D); // expected-warning{{(double) ((reg_$0) / 
30)}}
+
+  // There are two cast operations evaluated above:
+  // 1. (n / 30) is cast to a double during the store of `D`.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128068/new/

https://reviews.llvm.org/D128068

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128068: [analyzer] Do not emit redundant SymbolCasts

[Gabor Marton via Phabricator via cfe-commits]

martong created this revision.
martong added reviewers: steakhal, NoQ, ASDenysPetrov.
Herald added subscribers: manas, gamesh411, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, rnkovacs, szepet, baloghadamsoftware, xazax.hun.
Herald added a reviewer: Szelethus.
Herald added a project: All.
martong requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

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 simplest solution is to check whether the `to`
and `from` type are the same in `makeNonLoc`. We can't just do that check
in `evalCast` because there are many additonal logic before we'd end up
in `makeNonLoc`.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D128068

Files:
  clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
  clang/test/Analysis/symbolcast-floatingpoint.cpp


Index: clang/test/Analysis/symbolcast-floatingpoint.cpp
===
--- /dev/null
+++ 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 
+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) / 
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)
+
+}
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -113,6 +113,8 @@
   QualType fromTy, QualType toTy) {
   assert(operand);
   assert(!Loc::isLocType(toTy));
+  if (fromTy == toTy)
+return operand;
   return nonloc::SymbolVal(SymMgr.getCastSymbol(operand, fromTy, toTy));
 }
 


Index: clang/test/Analysis/symbolcast-floatingpoint.cpp
===
--- /dev/null
+++ 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 
+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) / 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)
+
+}
Index: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
@@ -113,6 +113,8 @@
   QualType fromTy, QualType toTy) {
   assert(operand);
   assert(!Loc::isLocType(toTy));
+  if (fromTy == toTy)
+return operand;
   return nonloc::SymbolVal(SymMgr.getCastSymbol(operand, fromTy, toTy));
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits