[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.
Herald added a subscriber: dkrupp.

polite ping


Repository:
  rC Clang

https://reviews.llvm.org/D52730



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

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:164
+// double and possibly long double on some systems
+RepresentsUntilExp = 53; break;
+  case 32:

xazax.hun wrote:
> A link to the source of these number would be useful. How are these 
> calculated. Also,  as far as I know the current C++ standard does not require 
> anything about how floating points are represented in an implementation. So 
> it would be great to somehow refer to the representation used by clang rather 
> than hardcoding these values. (Note that I am perfectly fine with warning for 
> implementation defined behavior as the original version also warn for such in 
> case of integers.) 
I took these magic numbers from the IEEE 754 standard; I completely agree that 
their introduction is far from being elegant.

Unfortunately it seems that referring to the representation used by clang seems 
to be somewhat difficult, see e.g. this old [[ 
https://stackoverflow.com/questions/13780931/how-do-i-get-llvm-types-from-clang 
| stackoverflow answer ]].  In the Z3 solver a similar problem was solved by 
defining a static function ([[ 
https://clang.llvm.org/doxygen/Z3ConstraintManager_8cpp_source.html#l00269 | 
getFloatSemantics() ]]) which uses a switch to translate the bit width of a 
floating point type into an llvm::fltSemantics value (which contains the 
precision value as a field).

I could imagine three solutions: 

  - reimplementing the logic getFloatSemantics,
  - moving getFloatSemantics to some utility library and using it from there,
  - keeping the current code, with comments describing my assumptions and 
referencing the IEEE standard.

Which of these is the best?

Note: According to the documentation [[ 
https://releases.llvm.org/2.5/docs/LangRef.html#t_floating | the floating point 
types ]] supported by the LLVM IR are just float, double and some high 
precision extension types (which are handled by the `default:` branch of my 
code). Unfortunately, I do not know what happens to the [[ 
https://clang.llvm.org/docs/LanguageExtensions.html#half-precision-floating-point
 | `_Float16` ]] half-width float type.


Repository:
  rC Clang

https://reviews.llvm.org/D52730



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

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 170169.
donat.nagy added a comment.

Give a reference for the significand size values of the IEEE 754 floating point 
types; cleanup comments and formatting.


Repository:
  rC Clang

https://reviews.llvm.org/D52730

Files:
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  test/Analysis/conversion.c

Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -137,6 +137,12 @@
   U8 = S + 10;
 }
 
+char dontwarn6(long long x) {
+  long long y = 42;
+  y += x;
+  return y == 42;
+}
+
 
 // C library functions, handled via apiModeling.StdCLibraryFunctions
 
@@ -154,7 +160,7 @@
 # define EOF (-1)
 char reply_string[8192];
 FILE *cin;
-extern int dostuff (void);
+extern int dostuff(void);
 int libraryFunction2() {
   int c, n;
   int dig;
@@ -179,3 +185,26 @@
   }
 }
 
+double floating_point(long long a, int b) {
+  if (a > 1LL << 55) {
+double r = a; // expected-warning {{Loss of precision}}
+return r;
+  } else if (b > 1 << 25) {
+float f = b; // expected-warning {{Loss of precision}}
+return f;
+  }
+  return 137;
+}
+
+double floating_point2() {
+  int a = 1 << 24;
+  long long b = 1LL << 53;
+  double d = a; // no-warning
+  double f = b; // no-warning
+  return d - f;
+}
+
+int floating_point_3(unsigned long long a) {
+  double b = a; // no-warning
+  return 42;
+}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -14,8 +14,10 @@
 // of expressions. A warning is reported when:
 // * a negative value is implicitly converted to an unsigned value in an
 //   assignment, comparison or multiplication.
-// * assignment / initialization when source value is greater than the max
-//   value of target
+// * assignment / initialization when the source value is greater than the max
+//   value of the target integer type
+// * assignment / initialization when the source integer is above the range
+//   where the target floating point type can represent all integers
 //
 // Many compilers and tools have similar checks that are based on semantic
 // analysis. Those checks are sound but have poor precision. ConversionChecker
@@ -29,6 +31,8 @@
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
+#include 
+
 using namespace clang;
 using namespace ento;
 
@@ -40,11 +44,9 @@
 private:
   mutable std::unique_ptr BT;
 
-  // Is there loss of precision
   bool isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType,
  CheckerContext &C) const;
 
-  // Is there loss of sign
   bool isLossOfSign(const ImplicitCastExpr *Cast, CheckerContext &C) const;
 
   void reportBug(ExplodedNode *N, CheckerContext &C, const char Msg[]) const;
@@ -132,19 +134,72 @@
 
   QualType SubType = Cast->IgnoreParenImpCasts()->getType();
 
-  if (!DestType->isIntegerType() || !SubType->isIntegerType())
+  if (!DestType->isRealType() || !SubType->isIntegerType())
 return false;
 
-  if (C.getASTContext().getIntWidth(DestType) >=
-  C.getASTContext().getIntWidth(SubType))
+  const bool isInteger = DestType->isIntegerType();
+
+  const auto &AC = C.getASTContext();
+
+  // We will find the largest RepresentsUntilExp value such that the DestType
+  // can exactly represent all nonnegative integers below 2^RepresentsUntilExp.
+  unsigned RepresentsUntilExp;
+
+  if (isInteger) {
+RepresentsUntilExp = AC.getIntWidth(DestType);
+if (RepresentsUntilExp == 1) {
+  // This is just casting a number to bool, probably not a bug.
+  return false;
+}
+if (DestType->isSignedIntegerType())
+  RepresentsUntilExp--;
+  } else {
+unsigned FloatingSize = AC.getTypeSize(DestType);
+
+// We assume that we are dealing with IEEE 754 floating point types
+// (including e.g. the 80-bit long double). The constants in the following
+// code come from the standard, see e.g.: https://enwp.org/IEEE_754
+
+switch (FloatingSize) {
+  case 64:
+// double (and possibly long double on some systems)
+RepresentsUntilExp = 53;
+break;
+  case 32:
+// float
+RepresentsUntilExp = 24;
+break;
+  case 16:
+// _Float16
+RepresentsUntilExp = 12;
+break;
+  default:
+// A larger type, which can represent all integers below 2^64.
+return false;
+}
+  }
+
+  if (RepresentsUntilExp >= sizeof(unsigned long long) * CHAR_BIT) {
+// Avoid overflow in our later calculations.
 return false;
+  }
 
-  unsigned W = C.getASTContext().getIntWidth(DestType);
-  if (W == 1 || W >= 64U)
+  unsigned CorrectedSrcWidth = AC.getIntWidth(SubType);
+  if (SubType->isSignedIntegerType())
+Cor

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 2 inline comments as done.
donat.nagy added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:164
+// double and possibly long double on some systems
+RepresentsUntilExp = 53; break;
+  case 32:

donat.nagy wrote:
> xazax.hun wrote:
> > A link to the source of these number would be useful. How are these 
> > calculated. Also,  as far as I know the current C++ standard does not 
> > require anything about how floating points are represented in an 
> > implementation. So it would be great to somehow refer to the representation 
> > used by clang rather than hardcoding these values. (Note that I am 
> > perfectly fine with warning for implementation defined behavior as the 
> > original version also warn for such in case of integers.) 
> I took these magic numbers from the IEEE 754 standard; I completely agree 
> that their introduction is far from being elegant.
> 
> Unfortunately it seems that referring to the representation used by clang 
> seems to be somewhat difficult, see e.g. this old [[ 
> https://stackoverflow.com/questions/13780931/how-do-i-get-llvm-types-from-clang
>  | stackoverflow answer ]].  In the Z3 solver a similar problem was solved by 
> defining a static function ([[ 
> https://clang.llvm.org/doxygen/Z3ConstraintManager_8cpp_source.html#l00269 | 
> getFloatSemantics() ]]) which uses a switch to translate the bit width of a 
> floating point type into an llvm::fltSemantics value (which contains the 
> precision value as a field).
> 
> I could imagine three solutions: 
> 
>   - reimplementing the logic getFloatSemantics,
>   - moving getFloatSemantics to some utility library and using it from there,
>   - keeping the current code, with comments describing my assumptions and 
> referencing the IEEE standard.
> 
> Which of these is the best?
> 
> Note: According to the documentation [[ 
> https://releases.llvm.org/2.5/docs/LangRef.html#t_floating | the floating 
> point types ]] supported by the LLVM IR are just float, double and some high 
> precision extension types (which are handled by the `default:` branch of my 
> code). Unfortunately, I do not know what happens to the [[ 
> https://clang.llvm.org/docs/LanguageExtensions.html#half-precision-floating-point
>  | `_Float16` ]] half-width float type.
I updated the patch with a comment describing my assumptions, but I will 
implement a different solution if that would be better.


Repository:
  rC Clang

https://reviews.llvm.org/D52730



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

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:163
+
+switch (FloatingSize) {
+  case 64:

NoQ wrote:
> Continuing the float semantics discussion on the new revision - Did you 
> consider `llvm::APFloat`? (http://llvm.org/doxygen/classllvm_1_1APFloat.html) 
> This looks like something that you're trying to re-implement.
This switch statement essentially fulfills two functions: it maps QualTypes to 
standardized IEEE floating point types and it immediately maps the standardized 
IEEE types to  their precision values.

The difficulty is that I don't think that the first map is available as a 
public function in the clang libraries. This means that although a switch over 
the bit width of the floating point type is certainly inelegant, I cannot avoid 
it.

The second map is available in the APFloat library (via the llvm::fltSemantics 
constants, which describe the standard IEEE types). However, this map is 
extremely stable (e.g. I don't think that the binary structure of the IEEE 
double type will ever change), so I think that re-implementing it is justified 
by the fact that it yields shorter and cleaner code. However, as I had noted 
previously, I am open to using the llvm::fltSemantics constants to implement 
this mapping.

As an additional remark, my current code supports the _Float16 type, which is 
currently not supported by the APFloat/fltSemantics library. If we decide to 
use the llvm::fltSemantics constants, then we must either extend the APFloat 
library or apply some workarounds for this case.  



Repository:
  rC Clang

https://reviews.llvm.org/D52730



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

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 171678.
donat.nagy added a comment.

Use APFloat to determine precision of floating point type

Additionally, fix a typo in the tests.


Repository:
  rC Clang

https://reviews.llvm.org/D52730

Files:
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  test/Analysis/conversion.c

Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -137,6 +137,12 @@
   U8 = S + 10;
 }
 
+char dontwarn6(long long x) {
+  long long y = 42;
+  y += x;
+  return y == 42;
+}
+
 
 // C library functions, handled via apiModeling.StdCLibraryFunctions
 
@@ -154,7 +160,7 @@
 # define EOF (-1)
 char reply_string[8192];
 FILE *cin;
-extern int dostuff (void);
+extern int dostuff(void);
 int libraryFunction2() {
   int c, n;
   int dig;
@@ -179,3 +185,26 @@
   }
 }
 
+double floating_point(long long a, int b) {
+  if (a > 1LL << 55) {
+double r = a; // expected-warning {{Loss of precision}}
+return r;
+  } else if (b > 1 << 25) {
+float f = b; // expected-warning {{Loss of precision}}
+return f;
+  }
+  return 137;
+}
+
+double floating_point2() {
+  int a = 1 << 24;
+  long long b = 1LL << 53;
+  float f = a; // no-warning
+  double d = b; // no-warning
+  return d - f;
+}
+
+int floating_point_3(unsigned long long a) {
+  double b = a; // no-warning
+  return 42;
+}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -14,8 +14,10 @@
 // of expressions. A warning is reported when:
 // * a negative value is implicitly converted to an unsigned value in an
 //   assignment, comparison or multiplication.
-// * assignment / initialization when source value is greater than the max
-//   value of target
+// * assignment / initialization when the source value is greater than the max
+//   value of the target integer type
+// * assignment / initialization when the source integer is above the range
+//   where the target floating point type can represent all integers
 //
 // Many compilers and tools have similar checks that are based on semantic
 // analysis. Those checks are sound but have poor precision. ConversionChecker
@@ -28,6 +30,9 @@
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "llvm/ADT/APFloat.h"
+
+#include 
 
 using namespace clang;
 using namespace ento;
@@ -40,11 +45,9 @@
 private:
   mutable std::unique_ptr BT;
 
-  // Is there loss of precision
   bool isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType,
  CheckerContext &C) const;
 
-  // Is there loss of sign
   bool isLossOfSign(const ImplicitCastExpr *Cast, CheckerContext &C) const;
 
   void reportBug(ExplodedNode *N, CheckerContext &C, const char Msg[]) const;
@@ -132,19 +135,55 @@
 
   QualType SubType = Cast->IgnoreParenImpCasts()->getType();
 
-  if (!DestType->isIntegerType() || !SubType->isIntegerType())
+  if (!DestType->isRealType() || !SubType->isIntegerType())
 return false;
 
-  if (C.getASTContext().getIntWidth(DestType) >=
-  C.getASTContext().getIntWidth(SubType))
+  const bool isInteger = DestType->isIntegerType();
+
+  const auto &AC = C.getASTContext();
+
+  // We will find the largest RepresentsUntilExp value such that the DestType
+  // can exactly represent all nonnegative integers below 2^RepresentsUntilExp.
+  unsigned RepresentsUntilExp;
+
+  if (isInteger) {
+RepresentsUntilExp = AC.getIntWidth(DestType);
+if (RepresentsUntilExp == 1) {
+  // This is just casting a number to bool, probably not a bug.
+  return false;
+}
+if (DestType->isSignedIntegerType())
+  RepresentsUntilExp--;
+  } else {
+unsigned FloatingSize = AC.getTypeSize(DestType);
+// getAllOneValues returns an APFloat with semantics corresponding to the
+// bit size given as the first argument; this is the only function in
+// APFloat.h that maps bit width to semantics.
+llvm::APFloat Tmp = llvm::APFloat::getAllOnesValue(FloatingSize, true);
+RepresentsUntilExp = llvm::APFloat::semanticsPrecision(Tmp.getSemantics());
+  }
+
+  if (RepresentsUntilExp >= sizeof(unsigned long long) * CHAR_BIT) {
+// Avoid overflow in our later calculations.
 return false;
+  }
+
+  unsigned CorrectedSrcWidth = AC.getIntWidth(SubType);
+  if (SubType->isSignedIntegerType())
+CorrectedSrcWidth--;
 
-  unsigned W = C.getASTContext().getIntWidth(DestType);
-  if (W == 1 || W >= 64U)
+  if (RepresentsUntilExp >= CorrectedSrcWidth) {
+// Simple case: the destination can store all values of the source type.
 return false;
+  }
 
-  unsigned long long MaxVal = 1ULL << W;
+  unsigned long long MaxVal = 1ULL << Repr

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 7 inline comments as done.
donat.nagy added a comment.

I found a solution for determining the precision value of a floating point 
type. Is this acceptable?




Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:163
+
+switch (FloatingSize) {
+  case 64:

NoQ wrote:
> donat.nagy wrote:
> > NoQ wrote:
> > > Continuing the float semantics discussion on the new revision - Did you 
> > > consider `llvm::APFloat`? 
> > > (http://llvm.org/doxygen/classllvm_1_1APFloat.html) This looks like 
> > > something that you're trying to re-implement.
> > This switch statement essentially fulfills two functions: it maps QualTypes 
> > to standardized IEEE floating point types and it immediately maps the 
> > standardized IEEE types to  their precision values.
> > 
> > The difficulty is that I don't think that the first map is available as a 
> > public function in the clang libraries. This means that although a switch 
> > over the bit width of the floating point type is certainly inelegant, I 
> > cannot avoid it.
> > 
> > The second map is available in the APFloat library (via the 
> > llvm::fltSemantics constants, which describe the standard IEEE types). 
> > However, this map is extremely stable (e.g. I don't think that the binary 
> > structure of the IEEE double type will ever change), so I think that 
> > re-implementing it is justified by the fact that it yields shorter and 
> > cleaner code. However, as I had noted previously, I am open to using the 
> > llvm::fltSemantics constants to implement this mapping.
> > 
> > As an additional remark, my current code supports the _Float16 type, which 
> > is currently not supported by the APFloat/fltSemantics library. If we 
> > decide to use the llvm::fltSemantics constants, then we must either extend 
> > the APFloat library or apply some workarounds for this case.  
> > 
> > The difficulty is that I don't think that the first map is available as a 
> > public function in the clang libraries. This means that although a switch 
> > over the bit width of the floating point type is certainly inelegant, I 
> > cannot avoid it.
> 
> `fltSemantics` are not just enum constants, [[ 
> http://llvm.org/doxygen/structllvm_1_1fltSemantics.html | they contain a lot 
> of fancy data ]]:
> ```
>static const fltSemantics semIEEEhalf = {15, -14, 11, 16};
>static const fltSemantics semIEEEsingle = {127, -126, 24, 32};
>static const fltSemantics semIEEEdouble = {1023, -1022, 53, 64};
>static const fltSemantics semIEEEquad = {16383, -16382, 113, 128};
>static const fltSemantics semX87DoubleExtended = {16383, -16382, 64, 80};
> ```
I knew about the data members of fltSemantics, but I did not see a way to use 
them elegantly (although fltSemantics has sizeInBits as a member, the possible 
fltSemantics values are not collected into one data structure, so I could not 
look up the precision field).

However, as I examined the APFloat.h source again, I found the function 
llvm::APFloat::getAllOnesValue which allows me to map bit width to semantics. 
While this is a somewhat indirect solution, it avoids using a switch.

The `missing _Float16' problem mentioned in my previous comments was my 
mistake, somehow I managed to overlook semIEEEhalf.


Repository:
  rC Clang

https://reviews.llvm.org/D52730



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

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 172922.
donat.nagy marked an inline comment as done.
donat.nagy added a comment.

Use ASTContext::getFloatTypeSemantics()


Repository:
  rC Clang

https://reviews.llvm.org/D52730

Files:
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  test/Analysis/conversion.c

Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -137,6 +137,12 @@
   U8 = S + 10;
 }
 
+char dontwarn6(long long x) {
+  long long y = 42;
+  y += x;
+  return y == 42;
+}
+
 
 // C library functions, handled via apiModeling.StdCLibraryFunctions
 
@@ -154,7 +160,7 @@
 # define EOF (-1)
 char reply_string[8192];
 FILE *cin;
-extern int dostuff (void);
+extern int dostuff(void);
 int libraryFunction2() {
   int c, n;
   int dig;
@@ -179,3 +185,26 @@
   }
 }
 
+double floating_point(long long a, int b) {
+  if (a > 1LL << 55) {
+double r = a; // expected-warning {{Loss of precision}}
+return r;
+  } else if (b > 1 << 25) {
+float f = b; // expected-warning {{Loss of precision}}
+return f;
+  }
+  return 137;
+}
+
+double floating_point2() {
+  int a = 1 << 24;
+  long long b = 1LL << 53;
+  float f = a; // no-warning
+  double d = b; // no-warning
+  return d - f;
+}
+
+int floating_point_3(unsigned long long a) {
+  double b = a; // no-warning
+  return 42;
+}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -14,8 +14,10 @@
 // of expressions. A warning is reported when:
 // * a negative value is implicitly converted to an unsigned value in an
 //   assignment, comparison or multiplication.
-// * assignment / initialization when source value is greater than the max
-//   value of target
+// * assignment / initialization when the source value is greater than the max
+//   value of the target integer type
+// * assignment / initialization when the source integer is above the range
+//   where the target floating point type can represent all integers
 //
 // Many compilers and tools have similar checks that are based on semantic
 // analysis. Those checks are sound but have poor precision. ConversionChecker
@@ -28,6 +30,9 @@
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "llvm/ADT/APFloat.h"
+
+#include 
 
 using namespace clang;
 using namespace ento;
@@ -40,11 +45,9 @@
 private:
   mutable std::unique_ptr BT;
 
-  // Is there loss of precision
   bool isLossOfPrecision(const ImplicitCastExpr *Cast, QualType DestType,
  CheckerContext &C) const;
 
-  // Is there loss of sign
   bool isLossOfSign(const ImplicitCastExpr *Cast, CheckerContext &C) const;
 
   void reportBug(ExplodedNode *N, CheckerContext &C, const char Msg[]) const;
@@ -132,19 +135,51 @@
 
   QualType SubType = Cast->IgnoreParenImpCasts()->getType();
 
-  if (!DestType->isIntegerType() || !SubType->isIntegerType())
+  if (!DestType->isRealType() || !SubType->isIntegerType())
 return false;
 
-  if (C.getASTContext().getIntWidth(DestType) >=
-  C.getASTContext().getIntWidth(SubType))
+  const bool isFloat = DestType->isFloatingType();
+
+  const auto &AC = C.getASTContext();
+
+  // We will find the largest RepresentsUntilExp value such that the DestType
+  // can exactly represent all nonnegative integers below 2^RepresentsUntilExp.
+  unsigned RepresentsUntilExp;
+
+  if (isFloat) {
+const llvm::fltSemantics &Sema = AC.getFloatTypeSemantics(DestType);
+RepresentsUntilExp = llvm::APFloat::semanticsPrecision(Sema);
+  } else {
+RepresentsUntilExp = AC.getIntWidth(DestType);
+if (RepresentsUntilExp == 1) {
+  // This is just casting a number to bool, probably not a bug.
+  return false;
+}
+if (DestType->isSignedIntegerType())
+  RepresentsUntilExp--;
+  }
+
+  if (RepresentsUntilExp >= sizeof(unsigned long long) * CHAR_BIT) {
+// Avoid overflow in our later calculations.
 return false;
+  }
+
+  unsigned CorrectedSrcWidth = AC.getIntWidth(SubType);
+  if (SubType->isSignedIntegerType())
+CorrectedSrcWidth--;
 
-  unsigned W = C.getASTContext().getIntWidth(DestType);
-  if (W == 1 || W >= 64U)
+  if (RepresentsUntilExp >= CorrectedSrcWidth) {
+// Simple case: the destination can store all values of the source type.
 return false;
+  }
 
-  unsigned long long MaxVal = 1ULL << W;
+  unsigned long long MaxVal = 1ULL << RepresentsUntilExp;
+  if (isFloat) {
+// If this is a floating point type, it can also represent MaxVal exactly.
+MaxVal++;
+  }
   return C.isGreaterOrEqual(Cast->getSubExpr(), MaxVal);
+  // TODO: maybe also check negative values with too large magnitude.
 }
 
 bool ConversionChecker::i

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Could someone with commit rights commit this patch (if it is acceptable)? I 
don't have commit rights myself.




Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:158-162
+unsigned FloatingSize = AC.getTypeSize(DestType);
+// getAllOneValues returns an APFloat with semantics corresponding to the
+// bit size given as the first argument; this is the only function in
+// APFloat.h that maps bit width to semantics.
+llvm::APFloat Tmp = llvm::APFloat::getAllOnesValue(FloatingSize, true);

NoQ wrote:
> Hmm, so the remaining problem is how to extract float semantics from a float 
> `QualType`? Would `ASTContext::getFloatTypeSemantics(DestType)` make sense?
Thank you, that is the method I was looking for!


Repository:
  rC Clang

https://reviews.llvm.org/D52730



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy created this revision.
donat.nagy added reviewers: alexfh, hokein, aaron.ballman, xazax.hun, 
whisperity.
Herald added subscribers: cfe-commits, gamesh411, Szelethus, dkrupp, rnkovacs, 
mgorny.

Implement a check for detecting if/else if/else chains where two or more
branches are Type I clones of each other (that is, they contain identical code)
and for detecting switch statements where two or more consecutive branches are
Type I clones of each other.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54757

Files:
  clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tidy/bugprone/BranchCloneCheck.h
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-branch-clone.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-branch-clone.cpp

Index: test/clang-tidy/bugprone-branch-clone.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-branch-clone.cpp
@@ -0,0 +1,751 @@
+// RUN: %check_clang_tidy %s bugprone-branch-clone %t
+
+void test_basic1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic2(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+  }
+
+  out++;
+}
+
+void test_basic3(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic4(int in, int &out) {
+  if (in > 77) {
+out--;
+  }
+  else {
+out++;
+  }
+}
+
+void test_basic5(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++;
+out++;
+  }
+}
+
+void test_basic6(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++, out++;
+  }
+}
+
+void test_basic7(int in, int &out) {
+  if (in > 77) {
+out++;
+out++;
+  }
+  else
+out++;
+
+  out++;
+}
+
+void test_basic8(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+out++;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+out++;
+out++;
+  }
+
+  if (in % 2)
+out++;
+}
+
+void test_basic9(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+  }
+}
+
+// If we remove the braces from the previous example, the check recognizes it
+// as an `else if`.
+void test_basic10(int in, int &out) {
+  if (in > 77)
+if (in % 2)
+  out++;
+else
+  out--;
+  else
+if (in % 2)
+  out++;
+else
+  out--;
+
+}
+
+void test_basic11(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  }
+}
+
+void test_basic12(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+  }
+}
+
+void test_basic13(int in, int &out) {
+  if (in > 77) {
+// Empty compound statement is not identical to null statement.
+  } else;
+}
+
+
+void test_chain1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: warning: repeated branch in conditional chain [bugprone-branch-clone]
+out++;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: end of the original
+  else if (in > 55)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 1 starts here
+out++;
+
+  out++;
+}
+
+void test_chain2(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: warning: repeated branch in conditional chain [bugprone-branch-clone]
+out++;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: end of the original
+  else if (in > 55)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 1 starts here
+out++;
+  else if (in > 42)
+out--;
+  else

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 174943.
donat.nagy added a comment.

Implement suggested improvements


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54757

Files:
  clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tidy/bugprone/BranchCloneCheck.h
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-branch-clone.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-branch-clone.cpp

Index: test/clang-tidy/bugprone-branch-clone.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-branch-clone.cpp
@@ -0,0 +1,751 @@
+// RUN: %check_clang_tidy %s bugprone-branch-clone %t
+
+void test_basic1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic2(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+  }
+
+  out++;
+}
+
+void test_basic3(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic4(int in, int &out) {
+  if (in > 77) {
+out--;
+  }
+  else {
+out++;
+  }
+}
+
+void test_basic5(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++;
+out++;
+  }
+}
+
+void test_basic6(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++, out++;
+  }
+}
+
+void test_basic7(int in, int &out) {
+  if (in > 77) {
+out++;
+out++;
+  }
+  else
+out++;
+
+  out++;
+}
+
+void test_basic8(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+out++;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+out++;
+out++;
+  }
+
+  if (in % 2)
+out++;
+}
+
+void test_basic9(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+  }
+}
+
+// If we remove the braces from the previous example, the check recognizes it
+// as an `else if`.
+void test_basic10(int in, int &out) {
+  if (in > 77)
+if (in % 2)
+  out++;
+else
+  out--;
+  else
+if (in % 2)
+  out++;
+else
+  out--;
+
+}
+
+void test_basic11(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  }
+}
+
+void test_basic12(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+  }
+}
+
+void test_basic13(int in, int &out) {
+  if (in > 77) {
+// Empty compound statement is not identical to null statement.
+  } else;
+}
+
+
+void test_chain1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: warning: repeated branch in conditional chain [bugprone-branch-clone]
+out++;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: end of the original
+  else if (in > 55)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 1 starts here
+out++;
+
+  out++;
+}
+
+void test_chain2(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: warning: repeated branch in conditional chain [bugprone-branch-clone]
+out++;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: end of the original
+  else if (in > 55)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 1 starts here
+out++;
+  else if (in > 42)
+out--;
+  else if (in > 28)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 2 starts here
+out++;
+  else if (in > 12) {
+out++;
+out *= 7;
+  } else if (in > 7) {
+// CHECK-MESSAGES: :[[@LINE-1]]:22: note: clone 3 starts here
+out++;
+  }
+}
+
+void test_chain3(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: repeate

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 6 inline comments as done.
donat.nagy added a comment.

An example of duplicated code in Clang (it appears in 
llvm/tools/clang/lib/Frontend/Rewrite/RewriteMacros.cpp starting from line 128):

  // If this is a #warning directive or #pragma mark (GNU extensions),
  // comment the line out.
  if (RawTokens[CurRawTok].is(tok::identifier)) {
const IdentifierInfo *II = RawTokens[CurRawTok].getIdentifierInfo();
if (II->getName() == "warning") {
  // Comment out #warning.
  RB.InsertTextAfter(SM.getFileOffset(RawTok.getLocation()), "//");   
// THIS IS...
} else if (II->getName() == "pragma" &&
   RawTokens[CurRawTok+1].is(tok::identifier) &&
   (RawTokens[CurRawTok+1].getIdentifierInfo()->getName() ==
"mark")) {
  // Comment out #pragma mark.
  RB.InsertTextAfter(SM.getFileOffset(RawTok.getLocation()), "//");   
// IDENTICAL TO THIS
}
  }


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54757



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 175502.
donat.nagy added a comment.

Remove braces, move if parent checking to ASTMatcher, other minor improvements


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757

Files:
  clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tidy/bugprone/BranchCloneCheck.h
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-branch-clone.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-branch-clone.cpp

Index: test/clang-tidy/bugprone-branch-clone.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-branch-clone.cpp
@@ -0,0 +1,760 @@
+// RUN: %check_clang_tidy %s bugprone-branch-clone %t
+
+void test_basic1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic2(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+  }
+
+  out++;
+}
+
+void test_basic3(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic4(int in, int &out) {
+  if (in > 77) {
+out--;
+  }
+  else {
+out++;
+  }
+}
+
+void test_basic5(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++;
+out++;
+  }
+}
+
+void test_basic6(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++, out++;
+  }
+}
+
+void test_basic7(int in, int &out) {
+  if (in > 77) {
+out++;
+out++;
+  }
+  else
+out++;
+
+  out++;
+}
+
+void test_basic8(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+out++;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+out++;
+out++;
+  }
+
+  if (in % 2)
+out++;
+}
+
+void test_basic9(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+  }
+}
+
+// If we remove the braces from the previous example, the check recognizes it
+// as an `else if`.
+void test_basic10(int in, int &out) {
+  if (in > 77)
+if (in % 2)
+  out++;
+else
+  out--;
+  else
+if (in % 2)
+  out++;
+else
+  out--;
+
+}
+
+void test_basic11(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  }
+}
+
+void test_basic12(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+  }
+}
+
+void test_basic13(int in, int &out) {
+  if (in > 77) {
+// Empty compound statement is not identical to null statement.
+  } else;
+}
+
+
+void test_chain1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: warning: repeated branch in conditional chain [bugprone-branch-clone]
+out++;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: end of the original
+  else if (in > 55)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 1 starts here
+out++;
+
+  out++;
+}
+
+void test_chain2(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: warning: repeated branch in conditional chain [bugprone-branch-clone]
+out++;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: note: end of the original
+  else if (in > 55)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 1 starts here
+out++;
+  else if (in > 42)
+out--;
+  else if (in > 28)
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: clone 2 starts here
+out++;
+  else if (in > 12) {
+out++;
+out *= 7;
+  } else if (in > 7) {
+// CHECK-MESSAGES: :[[@LINE-1]]:22: note: clone 3 starts here
+out++;
+  }
+}

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 15 inline comments as done.
donat.nagy added a comment.

I will add tests for macro handling later.




Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:71
+// Check whether this `if` is part of an `else if`:
+if (const auto *IP =
+dyn_cast(Result.Nodes.getNodeAs("ifParent"))) {

JonasToth wrote:
> This if should always be true, no? The matcher will always bind `ifParent`
I moved this check to the AST matcher.



Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:116
+Branches.push_back(Else);
+// Note: We will exit the while loop here.
+  }

JonasToth wrote:
> This note is misplaced? The exit is at the `break`.
The note was placed correctly, but I replaced it with a break.



Comment at: docs/clang-tidy/checks/list.rst:13
abseil-str-cat-append
+   abseil-string-find-startswith
android-cloexec-accept

JonasToth wrote:
> spurious change
It was added automatically by the python script, which sorts this list 
alphabetically. If I remove it now, it will be automatically re-added later.



Comment at: test/clang-tidy/bugprone-branch-clone.cpp:4
+void test_basic1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else 
branches [bugprone-branch-clone]

JonasToth wrote:
> could you please add tests for ternary operators?
Currently the check does not handle ternary operators, but I added some checks 
reflecting this.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 175697.
donat.nagy marked 5 inline comments as done.
donat.nagy added a comment.

Minor simplifications, add tests for macro handling


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757

Files:
  clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tidy/bugprone/BranchCloneCheck.h
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-branch-clone.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-branch-clone.cpp

Index: test/clang-tidy/bugprone-branch-clone.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-branch-clone.cpp
@@ -0,0 +1,992 @@
+// RUN: %check_clang_tidy %s bugprone-branch-clone %t
+
+void test_basic1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic2(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+  }
+
+  out++;
+}
+
+void test_basic3(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic4(int in, int &out) {
+  if (in > 77) {
+out--;
+  }
+  else {
+out++;
+  }
+}
+
+void test_basic5(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++;
+out++;
+  }
+}
+
+void test_basic6(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++, out++;
+  }
+}
+
+void test_basic7(int in, int &out) {
+  if (in > 77) {
+out++;
+out++;
+  }
+  else
+out++;
+
+  out++;
+}
+
+void test_basic8(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+out++;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+out++;
+out++;
+  }
+
+  if (in % 2)
+out++;
+}
+
+void test_basic9(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+  }
+}
+
+// If we remove the braces from the previous example, the check recognizes it
+// as an `else if`.
+void test_basic10(int in, int &out) {
+  if (in > 77)
+if (in % 2)
+  out++;
+else
+  out--;
+  else
+if (in % 2)
+  out++;
+else
+  out--;
+
+}
+
+void test_basic11(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  }
+}
+
+void test_basic12(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+  }
+}
+
+void test_basic13(int in, int &out) {
+  if (in > 77) {
+// Empty compound statement is not identical to null statement.
+  } else;
+}
+
+// We use a comparison that ignores redundant parentheses:
+void test_basic14(int in, int &out) {
+  if (in > 77)
+out += 2;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+(out) += (2);
+}
+
+void test_basic15(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+((out += 2));
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out += 2;
+}
+
+// ..but does not apply additional simplifications: 
+void test_basic16(int in, int &out) {
+  if (in > 77)
+out += 2;
+  else
+out += 1 + 1;
+}
+
+// ..and does not forget important parentheses:
+int test_basic17(int a, int b, int c, int mode) {
+  if (mode>8)
+return (a + b) * c;
+  else
+return a + b * c;
+}

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 3 inline comments as done.
donat.nagy added a comment.

**AstDiff:**

I looked at the AstDiff library, but I think that it is overkill for my goals. 
The main feat of the AstDiff library is analyzing and presenting the 
differences between two AST subtrees, while this checker only needs a simple 
yes/no answer for "Are these branches identical?".

**Macros:**

The current implementation of the check only looks at the preprocessed code, 
therefore it detects the situations where the duplication is created by macros. 
I added some tests to highlight this behavior. I think that in some situations 
this would be useful for detecting redundant or incorrect parts of 
macro-infected code.




Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:31
+/// an if/else if/else chain is one statement (which may be a CompoundStmt).
+using SwitchBranch = llvm::SmallVector;
+} // anonymous namespace

JonasToth wrote:
> maybe plural for the typename would fit better, as its a vector of multiple 
> elements?
This type represents one branch in a switch statement (which consists of 
multiple statements). I cannot think of a descriptive, short name which also 
happens to be plural and I do not want to use a monstrosity like 
`StatementsInSwitchBranch`.

By the way, can anyone think of shorter, but still clear names for 
`areStatementsIdentical()` and `areSwitchBranchesIdentical()` ?



Comment at: clang-tidy/bugprone/BranchCloneCheck.cpp:187
+while (FamilyBegin < End) {
+  auto FamilyEnd = FamilyBegin + 1;
+  while (FamilyEnd < End &&

JonasToth wrote:
> i think the name `FamilyEnd` is not very descriptive, maybe 
> `SubsequentBranch` or so?
I have renamed FamilyBegin/FamilyEnd to BeginCurrent/EndCurrent. I kept `begin` 
and `end` in the names to emphasize that this is is an iterator range (and 
altered the comment to include the expression "iterator range"). Is this naming 
choice clear enough?



Comment at: test/clang-tidy/bugprone-branch-clone.cpp:200
+
+void test_macro1(int in, int &out) {
+  PASTE_CODE(

The tests for macro handling start here.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 175701.
donat.nagy added a comment.

Correct a typo (ELSE instead of else)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757

Files:
  clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tidy/bugprone/BranchCloneCheck.h
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-branch-clone.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-branch-clone.cpp

Index: test/clang-tidy/bugprone-branch-clone.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-branch-clone.cpp
@@ -0,0 +1,992 @@
+// RUN: %check_clang_tidy %s bugprone-branch-clone %t
+
+void test_basic1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic2(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+  }
+
+  out++;
+}
+
+void test_basic3(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic4(int in, int &out) {
+  if (in > 77) {
+out--;
+  }
+  else {
+out++;
+  }
+}
+
+void test_basic5(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++;
+out++;
+  }
+}
+
+void test_basic6(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++, out++;
+  }
+}
+
+void test_basic7(int in, int &out) {
+  if (in > 77) {
+out++;
+out++;
+  }
+  else
+out++;
+
+  out++;
+}
+
+void test_basic8(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+out++;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+out++;
+out++;
+  }
+
+  if (in % 2)
+out++;
+}
+
+void test_basic9(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+  }
+}
+
+// If we remove the braces from the previous example, the check recognizes it
+// as an `else if`.
+void test_basic10(int in, int &out) {
+  if (in > 77)
+if (in % 2)
+  out++;
+else
+  out--;
+  else
+if (in % 2)
+  out++;
+else
+  out--;
+
+}
+
+void test_basic11(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  }
+}
+
+void test_basic12(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+  }
+}
+
+void test_basic13(int in, int &out) {
+  if (in > 77) {
+// Empty compound statement is not identical to null statement.
+  } else;
+}
+
+// We use a comparison that ignores redundant parentheses:
+void test_basic14(int in, int &out) {
+  if (in > 77)
+out += 2;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+(out) += (2);
+}
+
+void test_basic15(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+((out += 2));
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out += 2;
+}
+
+// ..but does not apply additional simplifications: 
+void test_basic16(int in, int &out) {
+  if (in > 77)
+out += 2;
+  else
+out += 1 + 1;
+}
+
+// ..and does not forget important parentheses:
+int test_basic17(int a, int b, int c, int mode) {
+  if (mode>8)
+return (a + b) * c;
+  else
+return a + b * c;
+}
+
+#define PASTE_CODE(x) x
+
+void test_macro1(int in, int

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked an inline comment as done.
donat.nagy added inline comments.



Comment at: test/clang-tidy/bugprone-branch-clone.cpp:4
+void test_basic1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else 
branches [bugprone-branch-clone]

JonasToth wrote:
> donat.nagy wrote:
> > JonasToth wrote:
> > > could you please add tests for ternary operators?
> > Currently the check does not handle ternary operators, but I added some 
> > checks reflecting this.
> Thank you. Could you please add a short note to the user-facing documentation 
> that these are not handled.
In fact, I could easily extend the functionality of the checker to cover 
ternary operators. Would it be an useful addition?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 176066.
donat.nagy added a comment.

Handle ternary operators, improve documentation


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757

Files:
  clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tidy/bugprone/BranchCloneCheck.h
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-branch-clone.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-branch-clone.cpp

Index: test/clang-tidy/bugprone-branch-clone.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-branch-clone.cpp
@@ -0,0 +1,1017 @@
+// RUN: %check_clang_tidy %s bugprone-branch-clone %t
+
+void test_basic1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic2(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+  }
+
+  out++;
+}
+
+void test_basic3(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic4(int in, int &out) {
+  if (in > 77) {
+out--;
+  }
+  else {
+out++;
+  }
+}
+
+void test_basic5(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++;
+out++;
+  }
+}
+
+void test_basic6(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++, out++;
+  }
+}
+
+void test_basic7(int in, int &out) {
+  if (in > 77) {
+out++;
+out++;
+  }
+  else
+out++;
+
+  out++;
+}
+
+void test_basic8(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+out++;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+out++;
+out++;
+  }
+
+  if (in % 2)
+out++;
+}
+
+void test_basic9(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+  }
+}
+
+// If we remove the braces from the previous example, the check recognizes it
+// as an `else if`.
+void test_basic10(int in, int &out) {
+  if (in > 77)
+if (in % 2)
+  out++;
+else
+  out--;
+  else
+if (in % 2)
+  out++;
+else
+  out--;
+
+}
+
+void test_basic11(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  }
+}
+
+void test_basic12(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+  }
+}
+
+void test_basic13(int in, int &out) {
+  if (in > 77) {
+// Empty compound statement is not identical to null statement.
+  } else;
+}
+
+// We use a comparison that ignores redundant parentheses:
+void test_basic14(int in, int &out) {
+  if (in > 77)
+out += 2;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+(out) += (2);
+}
+
+void test_basic15(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+((out += 2));
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out += 2;
+}
+
+// ..but does not apply additional simplifications: 
+void test_basic16(int in, int &out) {
+  if (in > 77)
+out += 2;
+  else
+out += 1 + 1;
+}
+
+// ..and does not forget important parentheses:
+int test_basic17(int a, int b, int c, int mode) {
+  if (mode>8)
+return (a + b) * c;
+  else
+return a + b * c;
+}
+
+//===//

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 8 inline comments as done.
donat.nagy added a comment.

At the end of the documentation I stated that the check examines the code after 
macro expansion. Is this note clear enough?




Comment at: test/clang-tidy/bugprone-branch-clone.cpp:4
+void test_basic1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else 
branches [bugprone-branch-clone]

Eugene.Zelenko wrote:
> donat.nagy wrote:
> > JonasToth wrote:
> > > donat.nagy wrote:
> > > > JonasToth wrote:
> > > > > could you please add tests for ternary operators?
> > > > Currently the check does not handle ternary operators, but I added some 
> > > > checks reflecting this.
> > > Thank you. Could you please add a short note to the user-facing 
> > > documentation that these are not handled.
> > In fact, I could easily extend the functionality of the checker to cover 
> > ternary operators. Would it be an useful addition?
> Sure, it'll be great extension of functionality.
I added support for ternary operators and documented this fact.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 176408.
donat.nagy added a comment.

Minor correction in documentation


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757

Files:
  clang-tidy/bugprone/BranchCloneCheck.cpp
  clang-tidy/bugprone/BranchCloneCheck.h
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-branch-clone.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-branch-clone.cpp

Index: test/clang-tidy/bugprone-branch-clone.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-branch-clone.cpp
@@ -0,0 +1,1017 @@
+// RUN: %check_clang_tidy %s bugprone-branch-clone %t
+
+void test_basic1(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic2(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+  }
+
+  out++;
+}
+
+void test_basic3(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+  }
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out++;
+
+  out++;
+}
+
+void test_basic4(int in, int &out) {
+  if (in > 77) {
+out--;
+  }
+  else {
+out++;
+  }
+}
+
+void test_basic5(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++;
+out++;
+  }
+}
+
+void test_basic6(int in, int &out) {
+  if (in > 77) {
+out++;
+  }
+  else {
+out++, out++;
+  }
+}
+
+void test_basic7(int in, int &out) {
+  if (in > 77) {
+out++;
+out++;
+  }
+  else
+out++;
+
+  out++;
+}
+
+void test_basic8(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+out++;
+out++;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+out++;
+out++;
+  }
+
+  if (in % 2)
+out++;
+}
+
+void test_basic9(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+  }
+}
+
+// If we remove the braces from the previous example, the check recognizes it
+// as an `else if`.
+void test_basic10(int in, int &out) {
+  if (in > 77)
+if (in % 2)
+  out++;
+else
+  out--;
+  else
+if (in % 2)
+  out++;
+else
+  out--;
+
+}
+
+void test_basic11(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+if (in % 2)
+  out++;
+else
+  out--;
+if (in % 3)
+  out++;
+else
+  out--;
+  }
+}
+
+void test_basic12(int in, int &out) {
+  if (in > 77) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  } else {
+// CHECK-MESSAGES: :[[@LINE-1]]:5: note: else branch starts here
+  }
+}
+
+void test_basic13(int in, int &out) {
+  if (in > 77) {
+// Empty compound statement is not identical to null statement.
+  } else;
+}
+
+// We use a comparison that ignores redundant parentheses:
+void test_basic14(int in, int &out) {
+  if (in > 77)
+out += 2;
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+(out) += (2);
+}
+
+void test_basic15(int in, int &out) {
+  if (in > 77)
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: if with identical then and else branches [bugprone-branch-clone]
+((out += 2));
+  else
+// CHECK-MESSAGES: :[[@LINE-1]]:3: note: else branch starts here
+out += 2;
+}
+
+// ..but does not apply additional simplifications: 
+void test_basic16(int in, int &out) {
+  if (in > 77)
+out += 2;
+  else
+out += 1 + 1;
+}
+
+// ..and does not forget important parentheses:
+int test_basic17(int a, int b, int c, int mode) {
+  if (mode>8)
+return (a + b) * c;
+  else
+return a + b * c;
+}
+
+//===//
+
+#define PAS

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 2 inline comments as done.
donat.nagy added a comment.

I applied this check to the llvm + clang codebase, and I was surprised to see 
that it produced about 600 results (for comparison, the clang-tidy checks which 
are enabled by default produce approximately 6000 results together). I didn't 
go through the whole list, but after examining a few dozen random reports it 
seems that most of these are true positives: relatively short branches (usually 
one or two lines of code) are repeated for no obvious reason. I would guess 
that most of these fall into the "correct but too verbose" case, because 
otherwise the tests would have caught the problem, but I didn't try to 
understand their context.

I have seen a few false positives related to preprocessor trickery: when an 
`.inc` file is included to create a huge switch, sometimes it will become a 
huge switch with lots of identical branches. There were also some situations 
where the check found identical branches which are annotated with different 
comments; I don't know if this should be considered a false positive.

If this is acceptable, then I would be grateful if someone would commit this 
patch for me as I don't have commit rights.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757



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

[PATCH] D54757: [clang-tidy] new check: bugprone-branch-clone

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Macro-generated long switches: I cannot access the check results right now, but 
if I recall correctly, the check assigned these errors to certain lines in the 
`.inc` files. This means that (1) it is not very easy to to suppress them with 
`//NOLINT` comments, but (2) it should be relatively easy to filter them by 
location (ignore all reports coming from `.inc` file).

A quick, tangentially related idea: Would it be useful to implement multiline 
nolint sections (e.g. `//BEGINNOLINT` – `//ENDNOLINT` or something similar)? 
This would be helpful for using clang-tidy on projects that contain some 
automatically generated fragments.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54757



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

[PATCH] D52423: Make ConversionChecker load StdCLibraryFunctionsChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy created this revision.
donat.nagy added a reviewer: dergachev.a.
Herald added a subscriber: cfe-commits.

ConversionChecker produces false positives when it encounters the
idiomatic usage of certain well-known functions (e.g. getc() and the
character classification functions like isalpha()). To eliminate these
false positives, the analyzer needs some information about semantics of
these functions. This functionality have been implemented already in
StdCLibraryFunctionsChecker, so we simply load that automatically when
ConversionChecker is loaded.


Repository:
  rC Clang

https://reviews.llvm.org/D52423

Files:
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
  test/Analysis/conversion.c


Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -138,15 +138,14 @@
 }
 
 
-// false positives..
+// old false positives..
 
 int isascii(int c);
 void falsePositive1() {
   char kb2[5];
   int X = 1000;
   if (isascii(X)) {
-// FIXME: should not warn here:
-kb2[0] = X; // expected-warning {{Loss of precision}}
+kb2[0] = X; // no-warning
   }
 }
 
@@ -175,8 +174,7 @@
   if (c == EOF)
 return(4);
   if (cp < &reply_string[sizeof(reply_string) - 1])
-// FIXME: should not warn here:
-*cp++ = c; // expected-warning {{Loss of precision}}
+*cp++ = c; // no-warning
 }
   }
 }
Index: lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
===
--- lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
+++ lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
@@ -23,5 +23,8 @@
 /// Register the part of MallocChecker connected to InnerPointerChecker.
 void registerInnerPointerCheckerAux(CheckerManager &Mgr);
 
+/// Register evaluation of some basic C standard library functions.
+void registerStdCLibraryFunctionsChecker(CheckerManager &mgr);
+
 }}
 #endif /* INTERCHECKERAPI_H_ */
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -23,6 +23,7 @@
 //
 
//===--===//
 #include "ClangSACheckers.h"
+#include "InterCheckerAPI.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
@@ -159,5 +160,6 @@
 }
 
 void ento::registerConversionChecker(CheckerManager &mgr) {
+  registerStdCLibraryFunctionsChecker(mgr);
   mgr.registerChecker();
 }


Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -138,15 +138,14 @@
 }
 
 
-// false positives..
+// old false positives..
 
 int isascii(int c);
 void falsePositive1() {
   char kb2[5];
   int X = 1000;
   if (isascii(X)) {
-// FIXME: should not warn here:
-kb2[0] = X; // expected-warning {{Loss of precision}}
+kb2[0] = X; // no-warning
   }
 }
 
@@ -175,8 +174,7 @@
   if (c == EOF)
 return(4);
   if (cp < &reply_string[sizeof(reply_string) - 1])
-// FIXME: should not warn here:
-*cp++ = c; // expected-warning {{Loss of precision}}
+*cp++ = c; // no-warning
 }
   }
 }
Index: lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
===
--- lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
+++ lib/StaticAnalyzer/Checkers/InterCheckerAPI.h
@@ -23,5 +23,8 @@
 /// Register the part of MallocChecker connected to InnerPointerChecker.
 void registerInnerPointerCheckerAux(CheckerManager &Mgr);
 
+/// Register evaluation of some basic C standard library functions.
+void registerStdCLibraryFunctionsChecker(CheckerManager &mgr);
+
 }}
 #endif /* INTERCHECKERAPI_H_ */
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -23,6 +23,7 @@
 //
 //===--===//
 #include "ClangSACheckers.h"
+#include "InterCheckerAPI.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
@@ -159,5 +160,6 @@
 }
 
 void ento::registerConversionChecker(CheckerManager &mgr) {
+  registerStdCLibraryFunctionsChecker(mgr);
   mgr.registerChecker();
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52423: [analyzer] Make ConversionChecker load StdCLibraryFunctionsChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Yes, moving StdCLibraryFunctionsChecker to an always-loaded package is probably 
a better solution than adding this one particular dependency link. (Evaluating 
these functions may be useful for other checkers as well, although it does not 
seem to change the results of other regression tests.) As an alternative to 
moving this checker to either the core or the apiModeling package, we could add 
unix.StdCLibraryFunctions to the dozen or so loaded-by-default checkers listed 
in lib/Driver/ToolChains/Clang.cpp without moving it to a different package. 
Which of these options is the best?


Repository:
  rC Clang

https://reviews.llvm.org/D52423



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

[PATCH] D52722: [analyzer] Move StdCLibraryFunctions to apiModeling

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy created this revision.
donat.nagy added a reviewer: NoQ.
Herald added subscribers: cfe-commits, Szelethus, mikhail.ramalho, a.sidorin, 
szepet, eraman, xazax.hun.
Herald added a reviewer: george.karpenkov.

StdCLibraryFunctionsChecker models the evaluation of several well-known
functions of the C standard library. This commit moves it to the
apiModeling category, which is loaded by default (it was in the unix
category, despite the fact that it models platform-independent standard
functions). As we load this checker, ConversionChecker will no longer
emit false positives when it encunters certain functions (e.g. getc()
and the character classification functions); this commit updates its
regression test to reflect this fact.


Repository:
  rC Clang

https://reviews.llvm.org/D52722

Files:
  include/clang/StaticAnalyzer/Checkers/Checkers.td
  test/Analysis/conversion.c
  test/Analysis/std-c-library-functions-inlined.c
  test/Analysis/std-c-library-functions.c
  test/Analysis/std-c-library-functions.cpp

Index: test/Analysis/std-c-library-functions.cpp
===
--- test/Analysis/std-c-library-functions.cpp
+++ test/Analysis/std-c-library-functions.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=unix.StdCLibraryFunctions,debug.ExprInspection -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify %s
 
 // Test that we don't model functions with broken prototypes.
 // Because they probably work differently as well.
Index: test/Analysis/std-c-library-functions.c
===
--- test/Analysis/std-c-library-functions.c
+++ test/Analysis/std-c-library-functions.c
@@ -1,8 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=unix.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=unix.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=unix.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=unix.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=unix.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions,debug.ExprInspection -verify -analyzer-config eagerly-assume=false %s
 
 void clang_analyzer_eval(int);
 
Index: test/Analysis/std-c-library-functions-inlined.c
===
--- test/Analysis/std-c-library-functions-inlined.c
+++ test/Analysis/std-c-library-functions-inlined.c
@@ -1,8 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=unix.StdCLibraryFunctions -verify %s
-// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=unix.StdCLibraryFunctions -verify %s
-// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=unix.StdCLibraryFunctions -verify %s
-// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=unix.StdCLibraryFunctions -verify %s
-// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=unix.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=apiModeling.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple i686-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple x86_64-unknown-linux -analyzer-checker=apiModeling.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple armv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions -verify %s
+// RUN: %clang_analyze_cc1 -triple thumbv7-a15-linux -analyzer-checker=apiModeling.StdCLibraryFunctions -verify %s
 
 // This test tests crashes that occur when standard functions are available
 // for inlining.
Index: test/Analys

[PATCH] D52423: [analyzer] Make ConversionChecker load StdCLibraryFunctionsChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy abandoned this revision.
donat.nagy added a comment.

I submitted a new patch, which moves stdCLibraryFunctions to apiModeling 
(https://reviews.llvm.org/D52722).


Repository:
  rC Clang

https://reviews.llvm.org/D52423



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

[PATCH] D52730: [analysis] ConversionChecker: handle floating point

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy created this revision.
donat.nagy added reviewers: NoQ, george.karpenkov, rnkovacs, 
baloghadamsoftware, mikhail.ramalho.
Herald added subscribers: cfe-commits, Szelethus.

Extend the alpha.core.Conversion checker to handle implicit converions
where a too large integer value is converted to a floating point type. Each
floating point type has a range where it can exactly represent all integers; we
emit a warning when the integer value is above this range. Although it is
possible to exactly represent some integers which are outside of this range
(those that are divisible by a large enough power of 2); we still report cast
involving those, because their usage may lead to bugs. (For example, if 1<<24
is stored in a float variable x, then x==x+1 holds.)


Repository:
  rC Clang

https://reviews.llvm.org/D52730

Files:
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  test/Analysis/conversion.c

Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -137,6 +137,12 @@
   U8 = S + 10;
 }
 
+char dontwarn6(long long x) {
+  long long y=42;
+  y += x;
+  return y==42;
+}
+
 
 // false positives..
 
@@ -181,3 +187,28 @@
   }
 }
 
+
+double floating_point(long long a, int b) {
+  if (a>1LL<<55) {
+double r = a; // expected-warning {{Loss of precision}}
+return r;
+  } else if (b>1<<25) {
+float f = b; // expected-warning {{Loss of precision}}
+return f;
+  }
+  return 137;
+}
+
+double floating_point2() {
+  int a = 1<<24;
+  long long b = 1LL<<53;
+  double d = a; // no-warning
+  double f = b; // no-warning
+  return d-f;
+}
+
+int floating_point_3(unsigned long long a) {
+  double b = a; // no-warning
+  return 42;
+}
+
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -14,8 +14,10 @@
 // of expressions. A warning is reported when:
 // * a negative value is implicitly converted to an unsigned value in an
 //   assignment, comparison or multiplication.
-// * assignment / initialization when source value is greater than the max
-//   value of target
+// * assignment / initialization when the source value is greater than the max
+//   value of the target integer type
+// * assignment / initialization when the source integer is above the range
+//   where the target floating point type can represent all integers
 //
 // Many compilers and tools have similar checks that are based on semantic
 // analysis. Those checks are sound but have poor precision. ConversionChecker
@@ -132,19 +134,65 @@
 
   QualType SubType = Cast->IgnoreParenImpCasts()->getType();
 
-  if (!DestType->isIntegerType() || !SubType->isIntegerType())
+  if (!DestType->isRealType() || !SubType->isIntegerType())
 return false;
 
-  if (C.getASTContext().getIntWidth(DestType) >=
-  C.getASTContext().getIntWidth(SubType))
+  const bool isInteger = DestType->isIntegerType();
+
+  const auto &AC = C.getASTContext();
+
+  // We will find the largest RepresentsUntilExp value such that the DestType
+  // can exactly represent all integers 0 <= n < 2^RepresentsUntilExp
+  unsigned RepresentsUntilExp;
+
+  if (isInteger) {
+RepresentsUntilExp = AC.getIntWidth(DestType);
+if (RepresentsUntilExp == 1) {
+  // This is just casting a number to bool, probably not a bug.
+  return false;
+}
+if (DestType->isSignedIntegerType()) {
+  RepresentsUntilExp--;
+}
+  } else {
+unsigned FloatingSize = AC.getTypeSize(DestType);
+switch (FloatingSize) {
+  case 64:
+// double and possibly long double on some systems
+RepresentsUntilExp = 53; break;
+  case 32:
+// float
+RepresentsUntilExp = 24; break;
+  case 16:
+// _Float16
+RepresentsUntilExp = 12; break;
+  default:
+// larger types, which can represent all integers below 2^64
+return false;
+}
+  }
+
+  if (RepresentsUntilExp >= sizeof(unsigned long long)*8) {
 return false;
+  }
+
+  unsigned CorrectedSrcWidth = AC.getIntWidth(SubType);
+  if (SubType->isSignedIntegerType()) {
+CorrectedSrcWidth--;
+  }
 
-  unsigned W = C.getASTContext().getIntWidth(DestType);
-  if (W == 1 || W >= 64U)
+  if (RepresentsUntilExp >= CorrectedSrcWidth) {
+// Simple case: the destination can store all values of the source type.
 return false;
+  }
 
-  unsigned long long MaxVal = 1ULL << W;
+  unsigned long long MaxVal = 1ULL << RepresentsUntilExp;
+  if (!isInteger) {
+// if this is a floating point type, it can also represent MaxVal exactly
+MaxVal++;
+  }
   return C.isGreaterOrEqual(Cast->getSubExpr(), MaxVal);
+  // TODO: maybe also check negative values with too large magnitude
 }
 
 bool ConversionChecker::isLossOfSign(const ImplicitCastExpr *Cast,

[PATCH] D52722: [analyzer] Move StdCLibraryFunctions to apiModeling

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a subscriber: martong.
donat.nagy added a comment.

I don't have commit rights, please commit for me @NoQ or @martong


Repository:
  rC Clang

https://reviews.llvm.org/D52722



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

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 167929.
donat.nagy added a comment.

Fix formatting in tests; add a comment.


Repository:
  rC Clang

https://reviews.llvm.org/D52730

Files:
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  test/Analysis/conversion.c

Index: test/Analysis/conversion.c
===
--- test/Analysis/conversion.c
+++ test/Analysis/conversion.c
@@ -137,6 +137,12 @@
   U8 = S + 10;
 }
 
+char dontwarn6(long long x) {
+  long long y = 42;
+  y += x;
+  return y == 42;
+}
+
 
 // false positives..
 
@@ -155,7 +161,7 @@
 # define EOF (-1)
 char reply_string[8192];
 FILE *cin;
-extern int dostuff (void);
+extern int dostuff(void);
 int falsePositive2() {
   int c, n;
   int dig;
@@ -181,3 +187,26 @@
   }
 }
 
+double floating_point(long long a, int b) {
+  if (a > 1LL << 55) {
+double r = a; // expected-warning {{Loss of precision}}
+return r;
+  } else if (b > 1 << 25) {
+float f = b; // expected-warning {{Loss of precision}}
+return f;
+  }
+  return 137;
+}
+
+double floating_point2() {
+  int a = 1 << 24;
+  long long b = 1LL << 53;
+  double d = a; // no-warning
+  double f = b; // no-warning
+  return d - f;
+}
+
+int floating_point_3(unsigned long long a) {
+  double b = a; // no-warning
+  return 42;
+}
Index: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -14,8 +14,10 @@
 // of expressions. A warning is reported when:
 // * a negative value is implicitly converted to an unsigned value in an
 //   assignment, comparison or multiplication.
-// * assignment / initialization when source value is greater than the max
-//   value of target
+// * assignment / initialization when the source value is greater than the max
+//   value of the target integer type
+// * assignment / initialization when the source integer is above the range
+//   where the target floating point type can represent all integers
 //
 // Many compilers and tools have similar checks that are based on semantic
 // analysis. Those checks are sound but have poor precision. ConversionChecker
@@ -29,6 +31,8 @@
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 
+#include 
+
 using namespace clang;
 using namespace ento;
 
@@ -132,19 +136,66 @@
 
   QualType SubType = Cast->IgnoreParenImpCasts()->getType();
 
-  if (!DestType->isIntegerType() || !SubType->isIntegerType())
+  if (!DestType->isRealType() || !SubType->isIntegerType())
 return false;
 
-  if (C.getASTContext().getIntWidth(DestType) >=
-  C.getASTContext().getIntWidth(SubType))
+  const bool isInteger = DestType->isIntegerType();
+
+  const auto &AC = C.getASTContext();
+
+  // We will find the largest RepresentsUntilExp value such that the DestType
+  // can exactly represent all integers 0 <= n < 2^RepresentsUntilExp
+  unsigned RepresentsUntilExp;
+
+  if (isInteger) {
+RepresentsUntilExp = AC.getIntWidth(DestType);
+if (RepresentsUntilExp == 1) {
+  // This is just casting a number to bool, probably not a bug.
+  return false;
+}
+if (DestType->isSignedIntegerType()) {
+  RepresentsUntilExp--;
+}
+  } else {
+unsigned FloatingSize = AC.getTypeSize(DestType);
+switch (FloatingSize) {
+  case 64:
+// double and possibly long double on some systems
+RepresentsUntilExp = 53; break;
+  case 32:
+// float
+RepresentsUntilExp = 24; break;
+  case 16:
+// _Float16
+RepresentsUntilExp = 12; break;
+  default:
+// larger types, which can represent all integers below 2^64
+return false;
+}
+  }
+
+  if (RepresentsUntilExp >= sizeof(unsigned long long)*CHAR_BIT) {
+// Avoid overflow in our later calculations
 return false;
+  }
 
-  unsigned W = C.getASTContext().getIntWidth(DestType);
-  if (W == 1 || W >= 64U)
+  unsigned CorrectedSrcWidth = AC.getIntWidth(SubType);
+  if (SubType->isSignedIntegerType()) {
+CorrectedSrcWidth--;
+  }
+
+  if (RepresentsUntilExp >= CorrectedSrcWidth) {
+// Simple case: the destination can store all values of the source type.
 return false;
+  }
 
-  unsigned long long MaxVal = 1ULL << W;
+  unsigned long long MaxVal = 1ULL << RepresentsUntilExp;
+  if (!isInteger) {
+// if this is a floating point type, it can also represent MaxVal exactly
+MaxVal++;
+  }
   return C.isGreaterOrEqual(Cast->getSubExpr(), MaxVal);
+  // TODO: maybe also check negative values with too large magnitude
 }
 
 bool ConversionChecker::isLossOfSign(const ImplicitCastExpr *Cast,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52730: [analyzer] ConversionChecker: handle floating point

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 6 inline comments as done.
donat.nagy added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/ConversionChecker.cpp:175
+
+  if (RepresentsUntilExp >= sizeof(unsigned long long)*8) {
 return false;

NoQ wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > How about `AC.getSizeType(AC.UnsignedLongLongTy))`?
> > I'm actually not too sure about this. @whisperity?
> Yeah, i suspect it's a host machine check (to prevent our own overflow on 
> line 189) rather than a target machine check.
Yes, this is intended to be a host machine check (corresponding to the `W >= 
64U` in the old version). I will add a comment to clarify this.


Repository:
  rC Clang

https://reviews.llvm.org/D52730



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

[PATCH] D52742: [analyzer][WIP] Add macro expansions to the plist output

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Does this checker handle the expansion of variadic macros introduced in C++11 
(see syntax (3) and (4) here 
) and the `#` and `##` 
preprocessor operators?




Comment at: lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:755
+if (It->is(tok::l_paren)) {
+  while ((++It)->isNot(tok::r_paren)) {
+assert(It->isNot(tok::eof) &&

I fear that this does not handle nested parentheses correctly; e.g. in a case 
like `MACRO_ONE(foo, MACRO_TWO(bar, baz), spam)`, it does not skip `, spam)`.


https://reviews.llvm.org/D52742



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

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added inline comments.



Comment at: clang/test/AST/ast-dump-overloaded-operators.cpp:27
 // CHECK-NEXT: | `-ParmVarDecl {{.*}}  col:19{{( imported)?}} 'E'
-// CHECK-NEXT: `-FunctionDecl {{.*}}  line:14:6{{( 
imported)?}} test 'void ()'
+// CHECK-NEXT:  -FunctionDecl {{.*}}  line:14:6{{( 
imported)?}} test 'void ()'
 // CHECK-NEXT:   `-CompoundStmt {{.*}} 

Why did a backtick disappear here and in many other locations? Is this somehow 
related to va_list handling?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142822

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

[PATCH] D158156: [analyzer] Add C++ array delete checker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Looks good, thanks for the improvements!

One minor remark: your commit uses backticks (`) around the class names in the 
bug reports, which is commonly used to mark code fragments e.g. in commit 
messages; however, in the bug reports IIRC most other checkers use apostrophes 
(').


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

https://reviews.llvm.org/D158156

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

[PATCH] D159519: [clang][AST][ASTImporter] improve AST comparasion on VarDecl & GotoStmt

[Donát Nagy via Phabricator via cfe-commits]

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

LGTM. I'm not very familiar with this area, but the change seems to be a very 
clean improvement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159519

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

[PATCH] D145229: [analyzer] Improve the documentation of the alpha.security.taint.TaintPropagation checker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Mostly LGTM, but I'd like to ask you to ensure consistent line wrapping (to 
e.g. 72 or 80 characters) in the free-flowing text that was added or rewritten 
by this commit. (I know that this is a minor question because the linebreaks 
won't be visible in the final output, but the raw markdown is itself a 
human-readable format and we should keep it reasonably clean.)


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

https://reviews.llvm.org/D145229

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

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

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

This commit releases a checker that was developed to a stable level in the 
Ericsson-internal fork of Clang Static Analyzer.

Note that the functionality of this checker overlaps with 
core.UndefinedBinaryOperatorResult ("UBOR"), but there are several differences 
between them:
(1) UBOR is only triggered when the constant folding performed by the Clang 
Static Analyzer engine determines that the value of a binary
operator expression is undefined; this checker can report issues where the 
operands are not constants.
(2) UBOR has unrelated checks for handling other binary operators, this checker 
only examines bitwise shifts.
(3) This checker has a Pedantic flag and by default does not report expressions 
(e.g. -2 << 2) that're undefined by the standard but consistently supported in 
practice.
(4) UBOR exhibits buggy behavior in code that involves cast expressions, e.g.

  void foo(unsigned short s) {
if (s == 2) {
  (void) ((unsigned int) s) << 16;
}
  }

Later it would be good to eliminate this overlap (perhaps by deprecating and 
then eliminating the bitwise shift handling in UBOR), but in my opinion that 
that belongs to separate commits.

Co-authored-by: Endre Fulop 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D156312

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/bitwise-ops-nocrash.c
  clang/test/Analysis/bitwise-ops.c
  clang/test/Analysis/bitwise-shift-common.c
  clang/test/Analysis/bitwise-shift-pedantic.c
  clang/test/Analysis/bitwise-shift-sanity-checks.c
  clang/test/Analysis/bitwise-shift-state-update.c
  clang/test/Analysis/casts.c
  clang/test/Analysis/diagnostics/track_subexpressions.cpp
  clang/test/Analysis/left-shift-cxx2a.cpp
  clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
  clang/test/Analysis/symbol-simplification-nonloc-loc.cpp

Index: clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
===
--- clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
+++ clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
@@ -14,7 +14,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -29,7 +29,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -43,8 +43,6 @@
   nonloc_OP_loc(p, BINOP(-)); // no-crash
 
   // Bitwise operators:
-  // expected-warning@+2 {{The result of the left shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
-  // expected-warning@+2 {{The result of the right shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
   nonloc_OP_loc(p, BINOP(<<)); // no-crash
   nonloc_OP_loc(p, BINOP(>>)); // no-crash
   nonloc_OP_loc(p, BINOP(&));
Index: clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
===
--- clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -24,6 +24,7 @@
 // CHECK-NEXT: apiModeling.TrustReturnsNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
+// CHECK-NEXT: core.BitwiseShift
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonnilStringConstants
Index: clang/test/Analysis/left-shift-cxx2a.cpp
===
--- clang/test/Analysis/left-shift-cxx2a.cpp
+++ clang/test/Analysis/left-shift-cxx2a.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,de

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

I tested this commit on several open source projects (in the default mode 
Pedantic=false), the following table summarizes the results:

| vim_v8.2.1920_baseline_vs_vim_v8.2.1920_with_bitwise_shift
   | New reports 

   | Lost reports 

   | no differences 
|
| 
openssl_openssl-3.0.0-alpha7_baseline_vs_openssl_openssl-3.0.0-alpha7_with_bitwise_shift
 | New reports 

 | Lost reports 

 | +2 FPs from same invalid loop assumption [1]   |
| sqlite_version-3.33.0_baseline_vs_sqlite_version-3.33.0_with_bitwise_shift
   | New reports 

   | Lost reports 

   | one FP lost for unclear reasons
|
| ffmpeg_n4.3.1_baseline_vs_ffmpeg_n4.3.1_with_bitwise_shift
   | New reports 

   | Lost reports 

   | 21 new reports, 14 lost reports, 
mostly FPs [2]|
| postgres_REL_13_0_baseline_vs_postgres_REL_13_0_with_bitwise_shift
   | New reports 

   | Lost reports 

   | two FPs are "converted" to new checker, one 
unreadable report lost |
| 
libwebm_libwebm-1.0.0.27_baseline_vs_libwebm_libwebm-1.0.0.27_with_bitwise_shift
 | New reports 

 | Lost reports 

 | no differences   
  |
| xerces_v3.2.3_baseline_vs_xerces_v3.2.3_with_bitwise_shift
   | New reports 

   | Lost reports 

   | no differences 
|
| bitcoin_v0.20.1_baseline_vs_bitcoin_v0.20.1_with_bitwise_shift
   | New reports 

   | Lost reports 

   

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 544317.
donat.nagy added a comment.

(Fix incorrect filename mark in the license header.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/bitwise-ops-nocrash.c
  clang/test/Analysis/bitwise-ops.c
  clang/test/Analysis/bitwise-shift-common.c
  clang/test/Analysis/bitwise-shift-pedantic.c
  clang/test/Analysis/bitwise-shift-sanity-checks.c
  clang/test/Analysis/bitwise-shift-state-update.c
  clang/test/Analysis/casts.c
  clang/test/Analysis/diagnostics/track_subexpressions.cpp
  clang/test/Analysis/left-shift-cxx2a.cpp
  clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
  clang/test/Analysis/symbol-simplification-nonloc-loc.cpp

Index: clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
===
--- clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
+++ clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
@@ -14,7 +14,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -29,7 +29,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -43,8 +43,6 @@
   nonloc_OP_loc(p, BINOP(-)); // no-crash
 
   // Bitwise operators:
-  // expected-warning@+2 {{The result of the left shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
-  // expected-warning@+2 {{The result of the right shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
   nonloc_OP_loc(p, BINOP(<<)); // no-crash
   nonloc_OP_loc(p, BINOP(>>)); // no-crash
   nonloc_OP_loc(p, BINOP(&));
Index: clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
===
--- clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -24,6 +24,7 @@
 // CHECK-NEXT: apiModeling.TrustReturnsNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
+// CHECK-NEXT: core.BitwiseShift
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonnilStringConstants
Index: clang/test/Analysis/left-shift-cxx2a.cpp
===
--- clang/test/Analysis/left-shift-cxx2a.cpp
+++ clang/test/Analysis/left-shift-cxx2a.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
 
 int testNegativeShift(int a) {
   if (a == -5) {
Index: clang/test/Analysis/diagnostics/track_subexpressions.cpp
===
--- clang/test/Analysis/diagnostics/track_subexpressions.cpp
+++ clang/test/Analysis/diagnostics/track_subexpressions.cpp
@@ -12,10 +12,10 @@
 
 void shift_by_undefined_value() {
   uint8_t shift_amount = get_uint8_max(); // expected-note{{'shift_amount' initialized to 255}}
-// expected-note@-1{{Calling 'get_uint8_max'}}
-// expected-note@-2{{Returning from 'get_uint8_max'}}
-  (void)(TCP_MAXWIN << shift_amount); // expected-warning{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
-  // expected-note@-1{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
+  // expected-note@-1{{Calling 'get_uint8_max'}}
+  // expected-note@-2{{Returning from 'get_uint8_max'}}
+  (void)(TCP_MAXWIN << shift_amount); // expected-warning{{Left shift by '255' overflows the capacity of 'int'}}
+  // expe

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

In D156312#4537200 , @NoQ wrote:

>> (4) UBOR exhibits buggy behavior in code that involves cast expressions
>
> Hmm, what makes your check more resilient to our overall type-incorrectness?

The code of UBOR (UndefResultChecker.cpp) uses the method `LHS->countl_zero()` 
(count zeros on the left side of the binary representation) which is very 
convenient but uses the "cached" bit width that is stored in the APSInt and 
apparently not updated by the casts. The checker proposed in this commit avoids 
this pitfall because it calls `getActiveBits()` on the APSInt and subtracts the 
result from the bit width of the type of the LHS (which is queried from the AST 
node).




Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:94-98
+void BitwiseShiftValidator::run() {
+  if (isValidShift()) {
+Ctx.addTransition(State, createNoteTag());
+  }
+}

NoQ wrote:
> Because `addTransition()` is a very confusing API that's very easy to misuse 
> in subtle ways (causing unexpected state splits), I feel anxious when I see 
> functions like
> ```
>   bool isValidShift();
>   bool isOvershift();
>   bool isOperandNegative(OperandSide Side);
>   bool isLeftShiftOverflow();
> ```
> that look like boolean getters but in fact call `addTransition()` under the 
> hood. If we could at least call them `checkOvershift()` etc., that'd be much 
> better. Ideally I wish there was some safeguard against producing redundant 
> transitions, like make them return an `ExplodedNode` and chain them, or 
> something like that.
These functions were originally called `checkXXX()` but I felt that the meaning 
of their boolean return value was too confusing with that naming scheme.

I'll try to ensure that `emitReport` or `addTransition` are only called on the 
topmost layer (the `run()` method) and change the return types of internal 
subroutines to make them return nullable pointers (e.g. 
`std::unique_ptr`) instead of `bool`s. This would (1) 
eliminate the "what does `true` mean at this point?" issues (2) clarify that 
this checker never performs more than one transition. (If the shift is invaild, 
we transition to a sink node by emitting a bug; if the shift is vaild, we 
perform a transition to record the state update.)



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:301-302
+  pluralSuffix(MaximalAllowedShift));
+R->addNote(LeftNote, PathDiagnosticLocation{LHS, Ctx.getSourceManager(),
+Ctx.getLocationContext()});
+Ctx.emitReport(std::move(R));

NoQ wrote:
> Can we just append this to the warning? The `addNote()` is useful for notes 
> that need to be placed in code outside of the execution path, but if it's 
> always next to the warning, it probably doesn't make sense to display it 
> separately.
I didn't append this to the warning because I felt that the warning text is 
already too long. (By the way, an earlier internal variant of this checker 
produced two separate notes next to the warning message.)

I tweaked the messages of this checker before initiating this review, but I 
feel that there's still some room for improvements. (E.g. in this particular 
case perhaps we could omit some of the details that are not immediately useful 
and could be manually calculated from other parts of the message...) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312

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

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 21 inline comments as done.
donat.nagy added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:29-30
+
+using namespace clang;
+using namespace ento;
+

steakhal wrote:
> This is used quite often and hinders the readability by indenting the args a 
> lot. Making the spelling shorter, would somewhat mitigate this.
Good idea!



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:57-59
+  // Secondary mutable state, used only for note tag creation:
+  enum { NonNegLeft = 1, NonNegRight = 2 };
+  unsigned NonNegFlags = 0;

donat.nagy wrote:
> steakhal wrote:
> > How about using the same enum type for these two? It would signify that 
> > these are used together.
> > Also, by only looking at the possible values, it's not apparent that these 
> > are bitflag values. A suffix might help the reader.
> Using the enum type is a good idea, however I'd probably put the `Flag` 
> suffix into the name of the type:
> ```
> enum SignednessFlags : unsigned { NonNegLeft = 1, NonNegRight = 2 };
> SignednessFlags NonNegOperands = 0;
> ```
I rename the member `NonNegFlags` to `NonNegOperands` but its type remains a 
plain `unsigned` because otherwise using the or-assign operator like 
`NonNegOperands |= NonNegLeft` produces the error
> Assigning to 'enum SignednessFlags' from incompatible type 'unsigned int' 
> (clang typecheck_convert_incompatible)
(note that both operands were from the same enum type).



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:218
+   Side == OperandSide::Left ? "Left" : 
"Right",
+   isLeftShift() ? "left" : "right")
+ .str();

steakhal wrote:
> The `isLeftShift() ? "left" : "right"` expression appears 4 times in total.
> I'd also recommend to not distinguish the capitalized "left" and "right" 
> strings, and just post-process the messages to ensure that the first 
> character is capitalized inside the `createBugReport()`.
> 
> Also note that you can reuse the same placeholder, so you would only need to 
> pass the `side()` only once.
> ```lang=C++
> static void capitalizeBeginning(std::string &Str) {
>   if (Str.empty())
> return;
>   Str[0] = llvm::toUpper(Str[0]);
> }
> 
> StringRef side() const {
>   return isLeftShift() ? "left" : "right";
> }
> ```
I introduced a method for `isLeftShift() ? "left" : "right"` (which is indeed 
repeated several times), but I didn't define `capitalizeBeginning` because that 
would've been used only once.

Your remark that [with the capitalization function] "you can reuse the same 
placeholder, so you would only need to pass the side() only once" seems to be 
incorrect, because `Side == OperandSide::Left ? "Left" : "Right"` (which 
operand are we talking about?) and `isLeftShift() ? "left" : "right"` (what 
kind of shift operator are we talking about?) are two independent values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312

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

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 549091.
donat.nagy marked 2 inline comments as done.
donat.nagy added a comment.

Uploading a new version based on the reviews.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/bitwise-ops-nocrash.c
  clang/test/Analysis/bitwise-ops.c
  clang/test/Analysis/bitwise-shift-common.c
  clang/test/Analysis/bitwise-shift-pedantic.c
  clang/test/Analysis/bitwise-shift-sanity-checks.c
  clang/test/Analysis/bitwise-shift-state-update.c
  clang/test/Analysis/casts.c
  clang/test/Analysis/diagnostics/track_subexpressions.cpp
  clang/test/Analysis/left-shift-cxx2a.cpp
  clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
  clang/test/Analysis/symbol-simplification-nonloc-loc.cpp

Index: clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
===
--- clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
+++ clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
@@ -14,7 +14,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -29,7 +29,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -43,8 +43,6 @@
   nonloc_OP_loc(p, BINOP(-)); // no-crash
 
   // Bitwise operators:
-  // expected-warning@+2 {{The result of the left shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
-  // expected-warning@+2 {{The result of the right shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
   nonloc_OP_loc(p, BINOP(<<)); // no-crash
   nonloc_OP_loc(p, BINOP(>>)); // no-crash
   nonloc_OP_loc(p, BINOP(&));
Index: clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
===
--- clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -24,6 +24,7 @@
 // CHECK-NEXT: apiModeling.TrustReturnsNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
+// CHECK-NEXT: core.BitwiseShift
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonnilStringConstants
Index: clang/test/Analysis/left-shift-cxx2a.cpp
===
--- clang/test/Analysis/left-shift-cxx2a.cpp
+++ clang/test/Analysis/left-shift-cxx2a.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
 
 int testNegativeShift(int a) {
   if (a == -5) {
Index: clang/test/Analysis/diagnostics/track_subexpressions.cpp
===
--- clang/test/Analysis/diagnostics/track_subexpressions.cpp
+++ clang/test/Analysis/diagnostics/track_subexpressions.cpp
@@ -12,10 +12,10 @@
 
 void shift_by_undefined_value() {
   uint8_t shift_amount = get_uint8_max(); // expected-note{{'shift_amount' initialized to 255}}
-// expected-note@-1{{Calling 'get_uint8_max'}}
-// expected-note@-2{{Returning from 'get_uint8_max'}}
-  (void)(TCP_MAXWIN << shift_amount); // expected-warning{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
-  // expected-note@-1{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
+  // expected-note@-1{{Calling 'get_uint8_max'}}
+  // expected-note@-2{{Returning from 'get_uint8_max'}}
+  (void)(TCP_MAXWIN << shift_amount); // expected-warning{{Left shift by '255' overfl

[PATCH] D157104: [analyzer] Improve underflow handling in ArrayBoundV2

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 549881.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157104

Files:
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  clang/test/Analysis/out-of-bounds.c


Index: clang/test/Analysis/out-of-bounds.c
===
--- clang/test/Analysis/out-of-bounds.c
+++ clang/test/Analysis/out-of-bounds.c
@@ -151,11 +151,23 @@
 // Don't warn when indexing below the start of a symbolic region's whose
 // base extent we don't know.
 int *get_symbolic(void);
-void test_index_below_symboloc(void) {
+void test_underflow_symbolic(void) {
   int *buf = get_symbolic();
   buf[-1] = 0; // no-warning;
 }
 
+// But warn if we understand the internal memory layout of a symbolic region.
+typedef struct {
+  int id;
+  char name[256];
+} user_t;
+
+user_t *get_symbolic_user(void);
+char test_underflow_symbolic_2() {
+  user_t *user = get_symbolic_user();
+  return user->name[-1]; // expected-warning{{Out of bound memory access}}
+}
+
 void test_incomplete_struct(void) {
   extern struct incomplete incomplete;
   int *p = (int *)&incomplete;
Index: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -172,13 +172,18 @@
 return;
 
   NonLoc ByteOffset = RawOffset->getByteOffset();
+  const SubRegion *Reg = RawOffset->getRegion();
 
   // CHECK LOWER BOUND
-  const MemSpaceRegion *SR = RawOffset->getRegion()->getMemorySpace();
-  if (!llvm::isa(SR)) {
-// A pointer to UnknownSpaceRegion may point to the middle of
-// an allocated region.
-
+  const MemSpaceRegion *Space = Reg->getMemorySpace();
+  if (!(isa(Reg) && isa(Space))) {
+// A symbolic region in unknown space represents an unknown pointer that
+// may point into the middle of an array, so we don't look for underflows.
+// Both conditions are significant because we want to check underflows in
+// symbolic regions on the heap (which may be introduced by checkers like
+// MallocChecker that call SValBuilder::getConjuredHeapSymbolVal()) and
+// non-symbolic regions (e.g. a field subregion of a symbolic region) in
+// unknown space.
 auto [state_precedesLowerBound, state_withinLowerBound] =
 compareValueToThreshold(state, ByteOffset,
 svalBuilder.makeZeroArrayIndex(), svalBuilder);
@@ -195,7 +200,7 @@
 
   // CHECK UPPER BOUND
   DefinedOrUnknownSVal Size =
-  getDynamicExtent(state, RawOffset->getRegion(), svalBuilder);
+  getDynamicExtent(state, Reg, svalBuilder);
   if (auto KnownSize = Size.getAs()) {
 auto [state_withinUpperBound, state_exceedsUpperBound] =
 compareValueToThreshold(state, ByteOffset, *KnownSize, svalBuilder);


Index: clang/test/Analysis/out-of-bounds.c
===
--- clang/test/Analysis/out-of-bounds.c
+++ clang/test/Analysis/out-of-bounds.c
@@ -151,11 +151,23 @@
 // Don't warn when indexing below the start of a symbolic region's whose
 // base extent we don't know.
 int *get_symbolic(void);
-void test_index_below_symboloc(void) {
+void test_underflow_symbolic(void) {
   int *buf = get_symbolic();
   buf[-1] = 0; // no-warning;
 }
 
+// But warn if we understand the internal memory layout of a symbolic region.
+typedef struct {
+  int id;
+  char name[256];
+} user_t;
+
+user_t *get_symbolic_user(void);
+char test_underflow_symbolic_2() {
+  user_t *user = get_symbolic_user();
+  return user->name[-1]; // expected-warning{{Out of bound memory access}}
+}
+
 void test_incomplete_struct(void) {
   extern struct incomplete incomplete;
   int *p = (int *)&incomplete;
Index: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -172,13 +172,18 @@
 return;
 
   NonLoc ByteOffset = RawOffset->getByteOffset();
+  const SubRegion *Reg = RawOffset->getRegion();
 
   // CHECK LOWER BOUND
-  const MemSpaceRegion *SR = RawOffset->getRegion()->getMemorySpace();
-  if (!llvm::isa(SR)) {
-// A pointer to UnknownSpaceRegion may point to the middle of
-// an allocated region.
-
+  const MemSpaceRegion *Space = Reg->getMemorySpace();
+  if (!(isa(Reg) && isa(Space))) {
+// A symbolic region in unknown space represents an unknown pointer that
+// may point into the middle of an array, so we don't look for underflows.
+// Both conditions are significant because we want to check underflows in
+// symbolic regions on the heap (which may be introduced by checkers like
+// MallocChecker that call SValBuilder::get

[PATCH] D157104: [analyzer] Improve underflow handling in ArrayBoundV2

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 2 inline comments as done.
donat.nagy added a comment.

I didn't upload the test results yet because right now there is some 
incompatibility between our local fork and the upstream. I hope that it'll be 
fixed soon, but until then I can't use our automated tools to run the analysis 
with revisions that are close to the upstream main branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157104

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

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 13 inline comments as done.
donat.nagy added a comment.

I handled the inline comments, I'll check the example code and edit the release 
notes tomorrow.




Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:272
+  if (const auto ConcreteRight = Right.getAs()) {
+const std::int64_t RHS = ConcreteRight->getValue().getExtValue();
+assert(RHS > MaximalAllowedShift);

steakhal wrote:
> `getExtValue()`, I believe, asserts that it "fits", thus the concrete int is 
> at most 64 bits wide. Does it work for `_BigInt`s? (or whatever we have like 
> that :D)
This final check is executed when we already know that 0 <= ConcreteRight < 128 
//(or the bit width of the largest integer type)//. I added a comment that 
mentions this and replaced the type `std::int64_t` with a plain `unsigned`.

I also updated the testcase `large_right` to ensure that later changes don't 
introduce crashes related to absurdly oversized right arguments.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:335-339
+auto R =
+std::make_unique(BT, ShortMsg, Msg, ErrNode);
+bugreporter::trackExpressionValue(ErrNode, Op->getLHS(), *R);
+bugreporter::trackExpressionValue(ErrNode, Op->getRHS(), *R);
+return R;

steakhal wrote:
> AHA! Now I got you. You also call this `R`.
> Just use `R` at the callsite as well. Job done.
This is a code fragment that survived my refactoring efforts without 
significant changes... until now :)

Now that you highlighted it, I'm renaming `R` to `BR` just to be a contrarian :)



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:355-357
+if (!BTPtr)
+  BTPtr = std::make_unique(this, "Bitwise shift",
+"Suspicious operation");

steakhal wrote:
> How about eagerly inline initializing this field? And avoiding `mutable` as 
> well.
This pattern systematically appears in all checkers that I've seen because the 
constructor of `BugType` queries and stores the name of the checker, which is 
only initialized when the checker (`*this`) is registered. This dependency 
means that the `BugType` cannot be initialized in the constructor of the 
`Checker`.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:53
+class BitwiseShiftValidator {
+  // Primary mutable state:
+  CheckerContext &Ctx;

steakhal wrote:
> donat.nagy wrote:
> > steakhal wrote:
> > > Where does this comment corresponds to?
> > The two data members (`Ctx` and `State`) that are directly below the 
> > comment.
> > 
> > I tried to split the list of data members into three groups (primary 
> > mutable state, secondary mutable state, `const`  members), but now I 
> > reconsider this I feel that these header comments are actually superfluous.
> > 
> > I'll return to a variant of the earlier layout where I put 
> > `NonNegFlags`/`UpperBound` to the end and comment that they are only used 
> > for note tag creation; but don't emphasize the (immediately visible) 
> > `const`ness / non-`const`-ness of other members with comments.
> One way to emphasize this is by choosing names like `FoldedState`, or 
> anything else that has something to do with "motion" or "change".
> Given that if we have `ProgramStateRefs` in helper classes they usually 
> remain still. Think of BugReportVisitors for example. Consequently,it's 
> important to raise awareness when it's not like that. A different name would 
> achieve this.
My intuition is that the variable name "State" already implies that it will be 
regularly changed and updated as we are going forward; but you're right that in 
this project we've a tradition of storing snapshots of "old" states in various 
helper classes.

(By the way, I'm a bit worried when I read code that stores and passes around 
random stale snapshots of the state. Of course we cannot avoid the branching 
nature of the exploded graph, but other than that it'd be good to store the 
State in language constructs where you cannot accidentally lose information by 
building onto an older state variable.) 



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:60-63
+  // If there is an upper bound, it is <= the size of a primitive type
+  // (expressed in bits), so this is a very safe sentinel value:
+  enum { NoUpperBound = 999 };
+  unsigned UpperBound = NoUpperBound;

steakhal wrote:
> donat.nagy wrote:
> > steakhal wrote:
> > > Have you considered using `std::optional`?
> > > Given that the dimension of this variable is "bits", I think it would be 
> > > great to have that in its name.
> > I considered using `std::optional`, but using this "old-school" solution 
> > ensures that `NoUpperBound` is comparable to and larger than the "real" 
> > upper bound values, which lets me avoid some ugly boolean logic. I'll 
> > u

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 549978.
donat.nagy marked 7 inline comments as done.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/bitwise-ops-nocrash.c
  clang/test/Analysis/bitwise-ops.c
  clang/test/Analysis/bitwise-shift-common.c
  clang/test/Analysis/bitwise-shift-pedantic.c
  clang/test/Analysis/bitwise-shift-sanity-checks.c
  clang/test/Analysis/bitwise-shift-state-update.c
  clang/test/Analysis/casts.c
  clang/test/Analysis/diagnostics/track_subexpressions.cpp
  clang/test/Analysis/left-shift-cxx2a.cpp
  clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
  clang/test/Analysis/symbol-simplification-nonloc-loc.cpp

Index: clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
===
--- clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
+++ clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
@@ -14,7 +14,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -29,7 +29,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -43,8 +43,6 @@
   nonloc_OP_loc(p, BINOP(-)); // no-crash
 
   // Bitwise operators:
-  // expected-warning@+2 {{The result of the left shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
-  // expected-warning@+2 {{The result of the right shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
   nonloc_OP_loc(p, BINOP(<<)); // no-crash
   nonloc_OP_loc(p, BINOP(>>)); // no-crash
   nonloc_OP_loc(p, BINOP(&));
Index: clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
===
--- clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -24,6 +24,7 @@
 // CHECK-NEXT: apiModeling.TrustReturnsNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
+// CHECK-NEXT: core.BitwiseShift
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonnilStringConstants
Index: clang/test/Analysis/left-shift-cxx2a.cpp
===
--- clang/test/Analysis/left-shift-cxx2a.cpp
+++ clang/test/Analysis/left-shift-cxx2a.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
 
 int testNegativeShift(int a) {
   if (a == -5) {
Index: clang/test/Analysis/diagnostics/track_subexpressions.cpp
===
--- clang/test/Analysis/diagnostics/track_subexpressions.cpp
+++ clang/test/Analysis/diagnostics/track_subexpressions.cpp
@@ -12,10 +12,10 @@
 
 void shift_by_undefined_value() {
   uint8_t shift_amount = get_uint8_max(); // expected-note{{'shift_amount' initialized to 255}}
-// expected-note@-1{{Calling 'get_uint8_max'}}
-// expected-note@-2{{Returning from 'get_uint8_max'}}
-  (void)(TCP_MAXWIN << shift_amount); // expected-warning{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
-  // expected-note@-1{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
+  // expected-note@-1{{Calling 'get_uint8_max'}}
+  // expected-note@-2{{Returning from 'get_uint8_max'}}
+  (void)(TCP_MAXWIN << shift_amount); // expected-warning{{Left shift by '255' overflows the capacity of 'int'}}
+  // expected-note@-1{{The result of left shift

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 550318.
donat.nagy edited the summary of this revision.
donat.nagy added a comment.

I verified that the checker handles the examples in the documentation correctly 
(and added them to the test suite). However, as I was tweaking the examples in 
the documentation, I accidentally found a situation where the checker produces 
a very surprising and arguably incorrect error message.

After investigating this issue, I added the testcases 
`signed_aritmetic_{good,bad}` which document the current sub-optimal state. The 
root cause of this problem is a high-level property of the engine (that it 
assumes that signed overflows are always possible and acceptable) and I don't 
see a local workaround that would silence or fix these incorrect error messages.

@steakhal @NoQ What do you know about these signed overflow issues (I presume 
that analogous issues also appear in other checkers)? How should we handle this 
limitation of this checker?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/bitwise-ops-nocrash.c
  clang/test/Analysis/bitwise-ops.c
  clang/test/Analysis/bitwise-shift-common.c
  clang/test/Analysis/bitwise-shift-pedantic.c
  clang/test/Analysis/bitwise-shift-sanity-checks.c
  clang/test/Analysis/bitwise-shift-state-update.c
  clang/test/Analysis/casts.c
  clang/test/Analysis/diagnostics/track_subexpressions.cpp
  clang/test/Analysis/left-shift-cxx2a.cpp
  clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
  clang/test/Analysis/symbol-simplification-nonloc-loc.cpp

Index: clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
===
--- clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
+++ clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
@@ -14,7 +14,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -29,7 +29,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -43,8 +43,6 @@
   nonloc_OP_loc(p, BINOP(-)); // no-crash
 
   // Bitwise operators:
-  // expected-warning@+2 {{The result of the left shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
-  // expected-warning@+2 {{The result of the right shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
   nonloc_OP_loc(p, BINOP(<<)); // no-crash
   nonloc_OP_loc(p, BINOP(>>)); // no-crash
   nonloc_OP_loc(p, BINOP(&));
Index: clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
===
--- clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -24,6 +24,7 @@
 // CHECK-NEXT: apiModeling.TrustReturnsNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
+// CHECK-NEXT: core.BitwiseShift
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonnilStringConstants
Index: clang/test/Analysis/left-shift-cxx2a.cpp
===
--- clang/test/Analysis/left-shift-cxx2a.cpp
+++ clang/test/Analysis/left-shift-cxx2a.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
 
 int testNegativeShift(int a) {
   if (a == -5) {
Index: clang/test/Analysis/diagnostics/track_subexpressions.cpp
===
--- clang/test/Analysis/diagnostics/track_subexpressions.cpp
+++ clang/test/Analysis/diagnostics/track_subexpressions.cpp
@@ -12,10 +12,10 @@
 
 void shift_by_undefined_value() {
   uint

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

(The release notes entry is still missing, I'll try to add it tomorrow.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312

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

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

In D156312#4589251 , @steakhal wrote:

> I don't think we should do anything about it unless it's frequent enough.
> Try to come up with a heuristic to be able to measure how often this happens, 
> if you really care.
> Once you have a semi-working heuristic for determining if that bugpath 
> suffers from this, you can as well use it for marking the bugreport invalid 
> if that's the case to lean towards suppressing those FPs.

Minor clarification: these are not FPs, these are true positives with a bad 
error message. I was annoyed when I found this surprising bug on the 
almost-ready checker that I was working on; but I wouldn't say that this is an 
especially severe issue.

In D156312#4589251 , @steakhal wrote:

> I don't think it's completely necessary to fix those FPs to land this. I 
> think of that as an improvement, on top of this one.
> I hope I clarified my standpoint.

I agree, I'll create a new revision which mentions this checker in the release 
notes, and I hope that I'll be able to merge that. Later I'll try to return to 
this question with either an engine improvement or a heuristic mitigation.




Comment at: clang/test/Analysis/bitwise-shift-sanity-checks.c:70-74
+  if (right < 0)
+return 0;
+  return left << (right + 32);
+  // expected - warning@-1 {{Left shift overflows the capacity of 'int'}}
+  // expected-warning@-2 {{Right operand is negative in left shift}}

steakhal wrote:
> Let's be explicit about the actual assumed value range, and use 
> `clang_analyzer_value()`.
> I also recommend using an explicit FIXME for calling out what should be 
> there, instead of abusing a non-matching `expected` pattern. I know I used 
> that in the past, and probably seen it from me. I feel ashamed that I did 
> that. I know I did that to have cleaner diffs, once fixed, but I honestly 
> think it does more harm than good.
I already tried calling `clang_analyzer_value()` at that point when I was 
investigating, but unfortunately it won't say anything useful because the 
actual range corresponding to the symbolic expression is only calculated when 
the `evalBinOp()` calls examine it.

I'll change the "expected - warning" to a FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312

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

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 550703.
donat.nagy added a comment.

Updated the release notes. This could be the final version of this commit if 
you agree that it's good enough.


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

https://reviews.llvm.org/D156312

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/bitwise-ops-nocrash.c
  clang/test/Analysis/bitwise-ops.c
  clang/test/Analysis/bitwise-shift-common.c
  clang/test/Analysis/bitwise-shift-pedantic.c
  clang/test/Analysis/bitwise-shift-sanity-checks.c
  clang/test/Analysis/bitwise-shift-state-update.c
  clang/test/Analysis/casts.c
  clang/test/Analysis/diagnostics/track_subexpressions.cpp
  clang/test/Analysis/left-shift-cxx2a.cpp
  clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
  clang/test/Analysis/symbol-simplification-nonloc-loc.cpp

Index: clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
===
--- clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
+++ clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
@@ -14,7 +14,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -29,7 +29,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -43,8 +43,6 @@
   nonloc_OP_loc(p, BINOP(-)); // no-crash
 
   // Bitwise operators:
-  // expected-warning@+2 {{The result of the left shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
-  // expected-warning@+2 {{The result of the right shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
   nonloc_OP_loc(p, BINOP(<<)); // no-crash
   nonloc_OP_loc(p, BINOP(>>)); // no-crash
   nonloc_OP_loc(p, BINOP(&));
Index: clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
===
--- clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -24,6 +24,7 @@
 // CHECK-NEXT: apiModeling.TrustReturnsNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
+// CHECK-NEXT: core.BitwiseShift
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonnilStringConstants
Index: clang/test/Analysis/left-shift-cxx2a.cpp
===
--- clang/test/Analysis/left-shift-cxx2a.cpp
+++ clang/test/Analysis/left-shift-cxx2a.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
 
 int testNegativeShift(int a) {
   if (a == -5) {
Index: clang/test/Analysis/diagnostics/track_subexpressions.cpp
===
--- clang/test/Analysis/diagnostics/track_subexpressions.cpp
+++ clang/test/Analysis/diagnostics/track_subexpressions.cpp
@@ -12,10 +12,10 @@
 
 void shift_by_undefined_value() {
   uint8_t shift_amount = get_uint8_max(); // expected-note{{'shift_amount' initialized to 255}}
-// expected-note@-1{{Calling 'get_uint8_max'}}
-// expected-note@-2{{Returning from 'get_uint8_max'}}
-  (void)(TCP_MAXWIN << shift_amount); // expected-warning{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
-  // expected-note@-1{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
+  // expected-note@-1{{Calling 'get_uint8_max'}}
+  // expected-note@-2{{Returning from 'get_uint8_max'}}
+  (void)(TCP_MAXWIN << shift_amount); // expected-warning{{Left shift by '255' overflows the capacity of

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 2 inline comments as done.
donat.nagy added inline comments.



Comment at: clang/docs/analyzer/checkers.rst:81
+
+Ensure the shift operands are in proper range before shifting.
+

donat.nagy wrote:
> steakhal wrote:
> > We should exactly elaborate on what "proper" means here.
> What would you suggest? I could try to write a slightly longer suggestion, 
> but I don't want to repeat the same conditions that I listed above the code 
> example.
(I'm marking this request as "Done" because I couldn't find a better wording. 
Feel free to reopen the discussion if you wish.)


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

https://reviews.llvm.org/D156312

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

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 550706.
donat.nagy added a comment.

A few minutes ago I accidentally re-uploaded the previous version of the patch; 
now I'm fixing this by uploading the actually updated variant.


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

https://reviews.llvm.org/D156312

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/bitwise-ops-nocrash.c
  clang/test/Analysis/bitwise-ops.c
  clang/test/Analysis/bitwise-shift-common.c
  clang/test/Analysis/bitwise-shift-pedantic.c
  clang/test/Analysis/bitwise-shift-sanity-checks.c
  clang/test/Analysis/bitwise-shift-state-update.c
  clang/test/Analysis/casts.c
  clang/test/Analysis/diagnostics/track_subexpressions.cpp
  clang/test/Analysis/left-shift-cxx2a.cpp
  clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
  clang/test/Analysis/symbol-simplification-nonloc-loc.cpp

Index: clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
===
--- clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
+++ clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
@@ -14,7 +14,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -29,7 +29,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -43,8 +43,6 @@
   nonloc_OP_loc(p, BINOP(-)); // no-crash
 
   // Bitwise operators:
-  // expected-warning@+2 {{The result of the left shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
-  // expected-warning@+2 {{The result of the right shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
   nonloc_OP_loc(p, BINOP(<<)); // no-crash
   nonloc_OP_loc(p, BINOP(>>)); // no-crash
   nonloc_OP_loc(p, BINOP(&));
Index: clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
===
--- clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -24,6 +24,7 @@
 // CHECK-NEXT: apiModeling.TrustReturnsNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
+// CHECK-NEXT: core.BitwiseShift
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonnilStringConstants
Index: clang/test/Analysis/left-shift-cxx2a.cpp
===
--- clang/test/Analysis/left-shift-cxx2a.cpp
+++ clang/test/Analysis/left-shift-cxx2a.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
 
 int testNegativeShift(int a) {
   if (a == -5) {
Index: clang/test/Analysis/diagnostics/track_subexpressions.cpp
===
--- clang/test/Analysis/diagnostics/track_subexpressions.cpp
+++ clang/test/Analysis/diagnostics/track_subexpressions.cpp
@@ -12,10 +12,10 @@
 
 void shift_by_undefined_value() {
   uint8_t shift_amount = get_uint8_max(); // expected-note{{'shift_amount' initialized to 255}}
-// expected-note@-1{{Calling 'get_uint8_max'}}
-// expected-note@-2{{Returning from 'get_uint8_max'}}
-  (void)(TCP_MAXWIN << shift_amount); // expected-warning{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
-  // expected-note@-1{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
+  // expected-note@-1{{Calling 'get_uint8_max'}}
+  // expected-note@-2{{Returning from 'get_uint8_max'}}
+  (void)(TCP_MAXWIN << shift_amount); // 

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy 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 rG25b9696b61e5: [analyzer] Upstream BitwiseShiftChecker 
(authored by donat.nagy).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312

Files:
  clang/docs/ReleaseNotes.rst
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/bitwise-ops-nocrash.c
  clang/test/Analysis/bitwise-ops.c
  clang/test/Analysis/bitwise-shift-common.c
  clang/test/Analysis/bitwise-shift-pedantic.c
  clang/test/Analysis/bitwise-shift-sanity-checks.c
  clang/test/Analysis/bitwise-shift-state-update.c
  clang/test/Analysis/casts.c
  clang/test/Analysis/diagnostics/track_subexpressions.cpp
  clang/test/Analysis/left-shift-cxx2a.cpp
  clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
  clang/test/Analysis/symbol-simplification-nonloc-loc.cpp

Index: clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
===
--- clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
+++ clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
@@ -14,7 +14,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -29,7 +29,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -43,8 +43,6 @@
   nonloc_OP_loc(p, BINOP(-)); // no-crash
 
   // Bitwise operators:
-  // expected-warning@+2 {{The result of the left shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
-  // expected-warning@+2 {{The result of the right shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
   nonloc_OP_loc(p, BINOP(<<)); // no-crash
   nonloc_OP_loc(p, BINOP(>>)); // no-crash
   nonloc_OP_loc(p, BINOP(&));
Index: clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
===
--- clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -24,6 +24,7 @@
 // CHECK-NEXT: apiModeling.TrustReturnsNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
+// CHECK-NEXT: core.BitwiseShift
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonnilStringConstants
Index: clang/test/Analysis/left-shift-cxx2a.cpp
===
--- clang/test/Analysis/left-shift-cxx2a.cpp
+++ clang/test/Analysis/left-shift-cxx2a.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
 
 int testNegativeShift(int a) {
   if (a == -5) {
Index: clang/test/Analysis/diagnostics/track_subexpressions.cpp
===
--- clang/test/Analysis/diagnostics/track_subexpressions.cpp
+++ clang/test/Analysis/diagnostics/track_subexpressions.cpp
@@ -12,10 +12,10 @@
 
 void shift_by_undefined_value() {
   uint8_t shift_amount = get_uint8_max(); // expected-note{{'shift_amount' initialized to 255}}
-// expected-note@-1{{Calling 'get_uint8_max'}}
-// expected-note@-2{{Returning from 'get_uint8_max'}}
-  (void)(TCP_MAXWIN << shift_amount); // expected-warning{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
-  // expected-note@-1{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
+  // expected-note@-1{{Calling 'get_uint8_max'}}
+  // expected-note@-2{{Returning from 'get_uint8_max

[PATCH] D158156: [analyzer] Add C++ array delete checker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

This checker is a straightforward, clean implementation of the SEI-CERT rule 
EXP51-CPP, it'll be a good addition to Static Analyzer. I'm satisfied with the 
implementation, although there might be others who have a more refined taste in 
C++ coding style. The only issue that I found is the misleading name of the 
"common ancestor" hidden checker that I mentioned in the inline comment.

The test suite nicely covers all the basic situations, but I'd be happy to see 
some corner cases like virtual and non-virtual diamond inheritance or even a 
degenerate example like

  class Base { /*...*/ };
  class Derived: public Base {
Base basesAsMember[10];
/*...*/
  };
  void f() {
Derived *d = new Derived(...);
delete[] (d->basesAsMember);
  }




Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:757-760
+def CXXArrayModeling: Checker<"CXXArrayModeling">,
+  HelpText<"The base of a few CXX array related checkers.">,
+  Documentation,
+  Hidden;

The name and help text of this hidden dependency is very misleading: it's not 
particularly connected to arrays (the connection between the two child checkers 
is that they both handle the "delete" operator) and it's not a modeling checker 
(which would provide function evaluation, constraints and transitions) but an 
"empty shell" checker that does nothing by itself, but provides a singleton 
checker object where the registration of the child checkers can enable the two 
analysis modes.

Please rename this to `CXXDeleteHelper` or something similar.



Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:88
+  const auto *BaseClassRegion = MR->getAs();
+  const auto *DerivedClassRegion = 
MR->getBaseRegion()->getAs();
+  if (!BaseClassRegion || !DerivedClassRegion)

This logic (which is inherited from the earlier checker) is a bit surprising 
because it strips //all// Element, Field, BaseObject and DerivedObject layers 
from the MemRegion object.

I think it works in practice because
(1) there is an `isDerivedFrom() check after it and
(2) it's rare to write (buggy) code like
```lang=c++
struct Inner { /*...*/ };
struct Outer { Inner inner; /*...*/ };
void f() {
  Outer *o = new Outer(...);
  delete(o->inner);
}
```

Perhaps this could be explained by a comment if you feel that it's warranted; 
but I mostly wrote this as a note for myself and other reviewers. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158156

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

[PATCH] D157104: [analyzer] Improve underflow handling in ArrayBoundV2

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

The results on open-source projects are depressing, but acceptable. This 
checker is looking for a serious defect, so it doesn't find any true positives 
on stable versions of open-source projects; however it produces a steady 
trickle of false positives because the Clang SA engine regularly misinterprets 
complicated code. As this patch allows this checker to analyze more situations, 
it introduces no true positives and a manageable amount of false positives (on 
average ~1/project).

Table of raw results:

| memcached | New reports 

   | Lost reports 

   | no change  
 |
| tmux  | New reports 

 | Lost reports 

 | no change
   |
| twin  | New reports 

   | Lost reports 

   | no change  
 |
| vim   | New reports 

   | Lost reports 

   | no change  
 |
| openssl   | New reports 

 | Lost reports 

 | no change   |
| sqlite| New reports 

   | Lost reports 

   | no change  
 |
| ffmpeg| New reports 

   | Lost reports 

   | four new reports (probably FPs), two of them 
are from the same macro|
| postgres  | New reports 

   | Lost reports 

   | two new false positives

[PATCH] D157104: [analyzer] Improve underflow handling in ArrayBoundV2

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

A (somewhat delayed) ping to @steakhal because you asked me to tell you when I 
have the results. (If you've already seen the table that I uploaded on Friday, 
then you've nothing to do.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157104

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

[PATCH] D158156: [analyzer] Add C++ array delete checker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

By the way, what would you think about putting the implementation of the two 
checkers (DeleteWithNonVirtualDtor, ArrayDelete) into two separate checker 
classes and placing the shared code into a common base class of these classes? 
I think this could be a significant simplification: we could avoid the manual 
management of the two checker names and bug types and register two regular 
checker objects instead of the one dummy prerequisite checker + two "fake" 
checker registrations that just toggle boolean flags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158156

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

[PATCH] D157104: [analyzer] Improve underflow handling in ArrayBoundV2

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Yup, there was a superfluous line break that was corrected by git clang-format; 
thanks for bringing it to my attention. As this is a very trivial change, I'll 
merge the fixed commit directly, without uploading it into Phabricator.




Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:202-203
   // CHECK UPPER BOUND
   DefinedOrUnknownSVal Size =
-  getDynamicExtent(state, RawOffset->getRegion(), svalBuilder);
+  getDynamicExtent(state, Reg, svalBuilder);
   if (auto KnownSize = Size.getAs()) {

This will be a single line in the commit that I'll merge.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157104

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

[PATCH] D157104: [analyzer] Improve underflow handling in ArrayBoundV2

[Donát Nagy 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 rG3e014038b373: [analyzer] Improve underflow handling in 
ArrayBoundV2 (authored by donat.nagy).

Changed prior to commit:
  https://reviews.llvm.org/D157104?vs=549881&id=552025#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157104

Files:
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  clang/test/Analysis/out-of-bounds.c


Index: clang/test/Analysis/out-of-bounds.c
===
--- clang/test/Analysis/out-of-bounds.c
+++ clang/test/Analysis/out-of-bounds.c
@@ -151,11 +151,23 @@
 // Don't warn when indexing below the start of a symbolic region's whose
 // base extent we don't know.
 int *get_symbolic(void);
-void test_index_below_symboloc(void) {
+void test_underflow_symbolic(void) {
   int *buf = get_symbolic();
   buf[-1] = 0; // no-warning;
 }
 
+// But warn if we understand the internal memory layout of a symbolic region.
+typedef struct {
+  int id;
+  char name[256];
+} user_t;
+
+user_t *get_symbolic_user(void);
+char test_underflow_symbolic_2() {
+  user_t *user = get_symbolic_user();
+  return user->name[-1]; // expected-warning{{Out of bound memory access}}
+}
+
 void test_incomplete_struct(void) {
   extern struct incomplete incomplete;
   int *p = (int *)&incomplete;
Index: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -172,13 +172,18 @@
 return;
 
   NonLoc ByteOffset = RawOffset->getByteOffset();
+  const SubRegion *Reg = RawOffset->getRegion();
 
   // CHECK LOWER BOUND
-  const MemSpaceRegion *SR = RawOffset->getRegion()->getMemorySpace();
-  if (!llvm::isa(SR)) {
-// A pointer to UnknownSpaceRegion may point to the middle of
-// an allocated region.
-
+  const MemSpaceRegion *Space = Reg->getMemorySpace();
+  if (!(isa(Reg) && isa(Space))) {
+// A symbolic region in unknown space represents an unknown pointer that
+// may point into the middle of an array, so we don't look for underflows.
+// Both conditions are significant because we want to check underflows in
+// symbolic regions on the heap (which may be introduced by checkers like
+// MallocChecker that call SValBuilder::getConjuredHeapSymbolVal()) and
+// non-symbolic regions (e.g. a field subregion of a symbolic region) in
+// unknown space.
 auto [state_precedesLowerBound, state_withinLowerBound] =
 compareValueToThreshold(state, ByteOffset,
 svalBuilder.makeZeroArrayIndex(), svalBuilder);
@@ -194,8 +199,7 @@
   }
 
   // CHECK UPPER BOUND
-  DefinedOrUnknownSVal Size =
-  getDynamicExtent(state, RawOffset->getRegion(), svalBuilder);
+  DefinedOrUnknownSVal Size = getDynamicExtent(state, Reg, svalBuilder);
   if (auto KnownSize = Size.getAs()) {
 auto [state_withinUpperBound, state_exceedsUpperBound] =
 compareValueToThreshold(state, ByteOffset, *KnownSize, svalBuilder);


Index: clang/test/Analysis/out-of-bounds.c
===
--- clang/test/Analysis/out-of-bounds.c
+++ clang/test/Analysis/out-of-bounds.c
@@ -151,11 +151,23 @@
 // Don't warn when indexing below the start of a symbolic region's whose
 // base extent we don't know.
 int *get_symbolic(void);
-void test_index_below_symboloc(void) {
+void test_underflow_symbolic(void) {
   int *buf = get_symbolic();
   buf[-1] = 0; // no-warning;
 }
 
+// But warn if we understand the internal memory layout of a symbolic region.
+typedef struct {
+  int id;
+  char name[256];
+} user_t;
+
+user_t *get_symbolic_user(void);
+char test_underflow_symbolic_2() {
+  user_t *user = get_symbolic_user();
+  return user->name[-1]; // expected-warning{{Out of bound memory access}}
+}
+
 void test_incomplete_struct(void) {
   extern struct incomplete incomplete;
   int *p = (int *)&incomplete;
Index: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -172,13 +172,18 @@
 return;
 
   NonLoc ByteOffset = RawOffset->getByteOffset();
+  const SubRegion *Reg = RawOffset->getRegion();
 
   // CHECK LOWER BOUND
-  const MemSpaceRegion *SR = RawOffset->getRegion()->getMemorySpace();
-  if (!llvm::isa(SR)) {
-// A pointer to UnknownSpaceRegion may point to the middle of
-// an allocated region.
-
+  const MemSpaceRegion *Space = Reg->getMemorySpace();
+  if (!(isa(Reg) && isa(Space))) {
+// A symbolic region in unknown space represents a

[PATCH] D158156: [analyzer] Add C++ array delete checker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

In D158156#4604242 , @steakhal wrote:

> One more thing to mention. Its usually illadvised to rename files or do 
> changes that would seriously impact git blame, unless we have a really good 
> reason doing so.
> Aesthetics usually don't count one, especially if the name is not 
> user-facing. However, maintainability is another axis, thus as it's always, 
> not black and white.

Git (e.g. git blame) can be smart enough to recognize renamed files if there 
aren't too much changes, but this change may be too large for that (at least 
Phabricator doesn't recognize it as a move, which confused me at first when I 
looked at this review). I'd suggest keeping the old filename in this commit; if 
you wish (and the community accepts it) you could rename the file in a separate 
followup NFC commit that doesn't do anything else.

In D158156#4604221 , @steakhal wrote:

> PS: sorry if any of my comments are dups, or outdated. I only had a little 
> time last week, and things have progressed since then I noticed. I still 
> decided to submit my possibly outdated and partial review comments. I hope 
> you understand.
> Its quite difficult to allocate time to do reviews from day to to day work. I 
> unfortunately usually do this in my spare time, if I have.

Thanks for reviewing our commits, it's very helpful for us.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158156

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

[PATCH] D154603: [analyzer][clangsa] Add new option to alpha.security.cert.InvalidPtrChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

The change looks promising, I only have minor remarks.




Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:125
 
-  const NoteTag *Note =
-  C.getNoteTag([SymbolicEnvPtrRegion, FunctionName](
-   PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
-if (!BR.isInteresting(SymbolicEnvPtrRegion))
-  return;
-Out << '\'' << FunctionName
-<< "' call may invalidate the environment parameter of 'main'";
-  });
+  ExplodedNode *CurrentChainEnd = nullptr;
+

gamesh411 wrote:
> steakhal wrote:
> > donat.nagy wrote:
> > > Perhaps add a comment that clarifies that passing a `nullptr` as the 
> > > ExplodedNode to `addTransition` is equivalent to specifying the current 
> > > node. I remember this because I was studying its implementation recently, 
> > > but I would've been surprised and suspicious otherwise.
> > If `nullptr` is equivalent with `C.getPredecessor()` inside 
> > `addTransition()`, why not simply initialize it to that value instead of to 
> > `nullptr`?
> I ended up using C.getPredecessor() instead of explaining; this seems a bit 
> more intuitive (if such a thing even exists in CSA).
> if such a thing even exists in CSA 
😆 




Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:121-122
+const NoteTag *Note =
+C.getNoteTag([Region, FunctionName = SmallString<64>{FunctionName},
+  Message = SmallString<256>{Message}](
+ PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {

Minor nitpick: in situations like this, when we want to save an already 
composed string, `std::string` is better than `SmallString` because it doesn't 
occupy more memory than the actual length of the string. (OTOH `SmallString` is 
better for buffer variables, I've seen code that creates a SmallString, 
composes the message in it, then converts it to a `std::string` for longer-term 
storage.)

Of course these are all just inconsequential micro-optimization details...



Comment at: clang/test/Analysis/invalid-ptr-checker.c:10
+// RUN: alpha.security.cert.env.InvalidPtr:InvalidatingGetEnv=true \
+// RUN: -analyzer-output=text -verify=pedantic -Wno-unused %s
+

Use `-verify=expected,pedantic` here and then you can eliminate the 
`pedantic-warning` lines that duplicate the messages of `expected-warning`s.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154603

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

[PATCH] D158499: [analyzer] Compute FAM dynamic size

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

In D158499#4608974 , @danix800 wrote:

> One of the observable issue with inconsistent size type is
>
>   void clang_analyzer_eval(int);
>   
>   typedef unsigned long long size_t;
>   void *malloc(unsigned long size);
>   void free(void *);
>   
>   void symbolic_longlong_and_int0(long long len) {
> char *a = malloc(5);
> (void)a[len + 1]; // no-warning
> // len: [-1,3]
> clang_analyzer_eval(-1 <= len && len <= 3); // expected-warning {{TRUE}}
> clang_analyzer_eval(0 <= len);  // expected-warning 
> {{UNKNOWN}}
> clang_analyzer_eval(len <= 2);  // expected-warning 
> {{UNKNOWN}}
> free(a);
>   }
>
> which is extracted from 
> `clang/test/Analysis/array-bound-v2-constraint-check.c`,
> with `DynamicMemoryModeling` turned on,
> the second warning does not hold anymore: `clang_analyzer_eval(0 <= len);`
> will be reported as `TRUE` which is not expected.
>
> `DynamicMemoryModeling` will record the extent of allocated memory as `5ULL`,
> `ArrayBoundV2` will do `len + 1 < 5ULL` assumption, simplified to `len < 
> 4ULL`,
> which casts `len` to unsigned, dropping `-1`, similar to
>
>   void clang_analyzer_eval(int);
>   
>   void test(int len) {
> if (len >= -1 && len <= 4U) { // len is promoted into unsigned, thus can 
> never be negative
> clang_analyzer_eval(0 <= len);  // TRUE
> }
>   }

This issue is very interesting and scary -- I wouldn't have guessed that the 
core constraint handling algorithms are so buggy that things like this can 
happen 😦. I reproduced the issue and double-checked that the logic error is 
//not// in ArrayBoundV2 (despite that its simplification algorithm is messy and 
suspicious); I'd be very happy if this issue could be resolved (perhaps along 
with other inaccuracies of APSInt math...).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158499

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

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:301-302
+  pluralSuffix(MaximalAllowedShift));
+R->addNote(LeftNote, PathDiagnosticLocation{LHS, Ctx.getSourceManager(),
+Ctx.getLocationContext()});
+Ctx.emitReport(std::move(R));

gamesh411 wrote:
> donat.nagy wrote:
> > NoQ wrote:
> > > Can we just append this to the warning? The `addNote()` is useful for 
> > > notes that need to be placed in code outside of the execution path, but 
> > > if it's always next to the warning, it probably doesn't make sense to 
> > > display it separately.
> > I didn't append this to the warning because I felt that the warning text is 
> > already too long. (By the way, an earlier internal variant of this checker 
> > produced two separate notes next to the warning message.)
> > 
> > I tweaked the messages of this checker before initiating this review, but I 
> > feel that there's still some room for improvements. (E.g. in this 
> > particular case perhaps we could omit some of the details that are not 
> > immediately useful and could be manually calculated from other parts of the 
> > message...) 
> Just a thought on simplifying the diagnostics a bit:
> 
> Warning: "Right operand is negative in left shift"
> Note: "The result of left shift is undefined because the right operand is 
> negative"
> Shortened: "Undefined left shift due to negative right operand"
> 
> Warning: "Left shift by '34' overflows the capacity of 'int'"
> Note: "The result of left shift is undefined because the right operand '34' 
> is not smaller than 32, the capacity of 'int'"
> Shortened: "Undefined left shift: '34' exceeds 'int' capacity (32 bits)"
> 
> The more complex notes are a bit sketchy, as relevant information can be 
> lost, and the following solution is probably a bit too much, but could prove 
> to be an inspiration:
> 
> Warning: "Left shift of '1024' overflows the capacity of 'int'"
> CXX Note: "Left shift of '1024' is undefined because 'int' can hold only 32 
> bits (including the sign bit), so some bits overflow"
> CXX Note: "The value '1024' is represented by 11 bits, allowing at most 21 
> bits for bitshift"
> C Note: "Left shift of '1024' is undefined because 'int' can hold only 31 
> bits (excluding the sign bit), so some bits overflow"
> C Note: "The value '1024' is represented by 11 bits, allowing at most 20 bits 
> for bitshift"
> 
> Shortened:
> CXX Warning: "Undefined left shift: '1024' (11 bits) exceeds 'int' capacity 
> (32 bits, including sign)"
> C Warning: "Undefined left shift: '1024' (11 bits) exceeds 'int' capacity (31 
> bits, excluding sign)"
Clarification about the `Msg`/`ShortMsg` distinction:
I'm intentionally separating the short warning messages and the longer note 
messages because `createBugReport()` enforces the convention that it will 
always emit a warning and a note at the bug location.

According to the comments in the source code, the intention is that the note 
contains all the relevant information, while the warning is a brief summary 
that can be displayed in situations where the notes wouldn't fit the UI.

IIUC many checkers ignore this distinction and emit the same (often long and 
cumbersome) message both as a note and as a warning (`createBugReport()` has a 
variant which takes only one message parameter and passes it to both 
locations), but I tried to follow it because I think it's ugly when the same 
message is repeated twice and there is some sense in providing both a brief 
summary and a full description that doesn't use potentially-ambiguous 
abbreviations to save space.

Of course I could also accept a community decision that this "brief warning / 
full note" distinction is deprecated and will be eliminated (because e.g. 
front-end utilities are not prepared to handle it), but in that case I'd 
strongly suggest a redesign where we eliminate the redundantly printed 'note' 
message. (There is no reason to say the same thing twice! There is no reason to 
say the same thing twice!)

On the other hand, in addition to this `Msg`/`ShortMsg` distinction, this part 
of the code also adds the extra `LeftNote` (as a remnant from an earlier 
internal version of this checker), and that's that's what I'd like to merge 
into `Msg` (following NoQ's suggestion and keeping it distinct from the 
`ShortMsg`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312

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

[PATCH] D155715: [clang][analyzer] Improve StdCLibraryFunctions socket send/recv functions.

[Donát Nagy via Phabricator via cfe-commits]

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

LGTM, I don't have anything significant to add.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155715

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

[PATCH] D153954: [clang][analyzer] Fix empty enum handling in EnumCastOutOfRange checker

[Donát Nagy via Phabricator via cfe-commits]

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

LGTM, with a little bikeshedding ;-)




Comment at: clang/test/Analysis/enum-cast-out-of-range.cpp:203
+
+enum class empty_unfixed {};
+

Consider using "specified" and "unspecified" instead of "fixed" and "unfixed", 
because the rest of the test file uses them and in my opinion "unfixed" looks 
strange in this context.  (I know that e.g. 
https://en.cppreference.com/w/cpp/language/enum speaks about "fixed" underlying 
context, but it doesn't use "unfixed".) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153954

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

[PATCH] D153954: [clang][analyzer] Fix empty enum handling in EnumCastOutOfRange checker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added inline comments.



Comment at: clang/test/Analysis/enum-cast-out-of-range.cpp:203
+
+enum class empty_unfixed {};
+

steakhal wrote:
> donat.nagy wrote:
> > Consider using "specified" and "unspecified" instead of "fixed" and 
> > "unfixed", because the rest of the test file uses them and in my opinion 
> > "unfixed" looks strange in this context.  (I know that e.g. 
> > https://en.cppreference.com/w/cpp/language/enum speaks about "fixed" 
> > underlying context, but it doesn't use "unfixed".) 
> How about calling it "plain" or "common"?
"plain" is also a good alternative for "unfixed" (although it would still be 
inconsistent with the earlier cases); but "common" is confusing, because in 
addition to the "plain, regular, normal" meaning it can also mean "mutual; 
shared by more than one ".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153954

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

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked an inline comment as done.
donat.nagy added a comment.

@steakhal  Thanks for the review!

I answered some of your questions; and I'll handle the rest soon.




Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:150
+  SVB.evalBinOp(State, Comparison, Val, LimitVal, SVB.getConditionType());
+  if (auto DURes = ResultVal.getAs()) {
+auto [StTrue, StFalse] = State->assume(DURes.value());

steakhal wrote:
> How about swapping these branches on the `ResultVal.isUndef()` predicate?
The argument of `ProgramState::assume()` must be a `DefinedOrUnknownSVal`, so I 
need to cast the return value of `evalBinOp` (which may be Undefined) to this 
type.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:157-158
+}
+// The code may be valid, so let's assume that it's valid:
+State = StTrue;
+if (StFalse) {

steakhal wrote:
> I wonder if we should add a message here that we introduced a new constraint: 
> "X assumed to be non-negative".
> To add such a message, you should do something similar to what is done in the 
> `StdLibraryFunctionsChecker`.
> Take the `C.getPredecessor()` and chain all your NoteTags by calling the 
> `Pred = C.addTransition(NewState, Pred, Tag)` like this. This way. This is 
> how one "sequence of ExplodedNodes" can be made.
This is already done by `BitwiseShiftValidator::createNoteTag()`. It's a bit 
complex because the checker merges all its assumptions into a single note tag 
(because I didn't want to emit two or three notes directly after each other.



Comment at: clang/test/Analysis/analyzer-config.c:36
 // CHECK-NEXT: cfg-temporary-dtors = true
+// CHECK-NEXT: consider-single-element-arrays-as-flexible-array-members = true
+// CHECK-NEXT: core.BitwiseShift:Pedantic = false

steakhal wrote:
> I'm pretty sure I got rid of this one :D
> Consider rebasing on top of `llvm/main`.
Oops, this might be an error introduced during a rebase... Thanks for catching 
it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312

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

[PATCH] D157104: [analyzer] Improve underflow handling in ArrayBoundV2

[Donát Nagy via Phabricator via cfe-commits]

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

This minor change ensures that underflow errors are reported on arrays that are 
in unknown space but have well-defined size.

As a concrete example, the following test case did not produce a warning 
previously, but will produce a warning after this patch:

  typedef struct {
int id;
char name[256];
  } user_t;
  
  user_t *get_symbolic_user(void);
  char test_underflow_symbolic_2() {
user_t *user = get_symbolic_user();
return user->name[-1];
  }


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D157104

Files:
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  clang/test/Analysis/out-of-bounds.c


Index: clang/test/Analysis/out-of-bounds.c
===
--- clang/test/Analysis/out-of-bounds.c
+++ clang/test/Analysis/out-of-bounds.c
@@ -151,11 +151,23 @@
 // Don't warn when indexing below the start of a symbolic region's whose
 // base extent we don't know.
 int *get_symbolic(void);
-void test_index_below_symboloc(void) {
+void test_underflow_symbolic(void) {
   int *buf = get_symbolic();
   buf[-1] = 0; // no-warning;
 }
 
+// But warn if we understand the internal memory layout of a symbolic region.
+typedef struct {
+  int id;
+  char name[256];
+} user_t;
+
+user_t *get_symbolic_user(void);
+char test_underflow_symbolic_2() {
+  user_t *user = get_symbolic_user();
+  return user->name[-1]; // expected-warning{{Out of bound memory access}}
+}
+
 void test_incomplete_struct(void) {
   extern struct incomplete incomplete;
   int *p = (int *)&incomplete;
@@ -173,4 +185,3 @@
   buf[x] = 1;
   clang_analyzer_eval(x <= 99); // expected-warning{{TRUE}}
 }
-
Index: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -172,13 +172,17 @@
 return;
 
   NonLoc ByteOffset = RawOffset->getByteOffset();
+  const SubRegion *Reg = RawOffset->getRegion();
 
   // CHECK LOWER BOUND
-  const MemSpaceRegion *SR = RawOffset->getRegion()->getMemorySpace();
-  if (!llvm::isa(SR)) {
-// A pointer to UnknownSpaceRegion may point to the middle of
-// an allocated region.
-
+  const MemSpaceRegion *Space = Reg->getMemorySpace();
+  if (!(isa(Reg) && isa(Space))) {
+// A symbolic region in unknown space represents an unknown pointer that
+// may point into the middle of an array, so we don't look for underflows.
+// Both conditions are significant because we want to check underflows in
+// symbolic regions on the heap (which may be introduced by checkers that
+// call SValBuilder::getConjuredHeapSymbolVal()) and non-symbolic regions
+// (e.g. a field subregion of a symbolic region) in unknown space.
 auto [state_precedesLowerBound, state_withinLowerBound] =
 compareValueToThreshold(state, ByteOffset,
 svalBuilder.makeZeroArrayIndex(), svalBuilder);
@@ -195,7 +199,7 @@
 
   // CHECK UPPER BOUND
   DefinedOrUnknownSVal Size =
-  getDynamicExtent(state, RawOffset->getRegion(), svalBuilder);
+  getDynamicExtent(state, Reg, svalBuilder);
   if (auto KnownSize = Size.getAs()) {
 auto [state_withinUpperBound, state_exceedsUpperBound] =
 compareValueToThreshold(state, ByteOffset, *KnownSize, svalBuilder);


Index: clang/test/Analysis/out-of-bounds.c
===
--- clang/test/Analysis/out-of-bounds.c
+++ clang/test/Analysis/out-of-bounds.c
@@ -151,11 +151,23 @@
 // Don't warn when indexing below the start of a symbolic region's whose
 // base extent we don't know.
 int *get_symbolic(void);
-void test_index_below_symboloc(void) {
+void test_underflow_symbolic(void) {
   int *buf = get_symbolic();
   buf[-1] = 0; // no-warning;
 }
 
+// But warn if we understand the internal memory layout of a symbolic region.
+typedef struct {
+  int id;
+  char name[256];
+} user_t;
+
+user_t *get_symbolic_user(void);
+char test_underflow_symbolic_2() {
+  user_t *user = get_symbolic_user();
+  return user->name[-1]; // expected-warning{{Out of bound memory access}}
+}
+
 void test_incomplete_struct(void) {
   extern struct incomplete incomplete;
   int *p = (int *)&incomplete;
@@ -173,4 +185,3 @@
   buf[x] = 1;
   clang_analyzer_eval(x <= 99); // expected-warning{{TRUE}}
 }
-
Index: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
===
--- clang/lib/StaticAnalyzer/Check

[PATCH] D157104: [analyzer] Improve underflow handling in ArrayBoundV2

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Note: this commit is split off from the larger commit 
https://reviews.llvm.org/D150446, which combined this simple improvement with 
other, more complex changes that had problematic side-effects.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157104

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

[PATCH] D150446: [analyzer] Check ArraySubscriptExprs in ArrayBoundCheckerV2

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy abandoned this revision.
donat.nagy added a comment.
Herald added a subscriber: wangpc.

I'm abandoning this commit because it amalgamates several unrelated changes and 
I think it'd be better to handle them separately:

1. First, there is a very simple, independent improvement related to underflows 
and UnknownSpaces. I already created the separate commit D157104 
 for this (reviews are welcome ;) ).
2. As the title of this commit says, I wanted to turn this checker into a 
`Checker>` instead of a 
`Checker`. I'm still planning to do this transition in a 
separate commit because I feel that this will be needed to compose good warning 
messages and there are a few situations like the testcase `test_field` where 
the `check::Location` model produces counter-intuitive results. (When I 
implement this, I'll also ensure that `*p` and `p->field` are handled 
analogously to `p[0]`, but perhaps these will be in a separate commit.)
3. Finally there is the "simplify `RegionRawOffsetV2::computeOffset` and fix 
multidimensional array handling" change. This is the modification that was 
responsible for the multitude of false positives caused by this commit; and I 
don't have a quick fix for it (the engine abuses `ElementRegion` to record 
pointer arithmetic and I didn't find a clear way to distinguish it from real 
element access). On the short-term I think I'll need to accept that this 
checker will produce false negatives in situations when one element of a 
multi-dimensional array is over-indexed without overflowing the whole array 
(e.g. if `arr` is declared as `int arr[5][5]`, then `arr[1][10]` over-indexes 
`arr[1]`, but points inside the full area of the matrix); but fortunately this 
is not a fatal limitation (it only produces false negatives, multi-dimensional 
arrays are not too common).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150446

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

[PATCH] D157104: [analyzer] Improve underflow handling in ArrayBoundV2

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added reviewers: dkrupp, steakhal, Szelethus, gamesh411.
donat.nagy added a comment.

I'll try to upload results on open source projects, probably on Tuesday (I'm on 
vacation on Monday).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157104

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

[PATCH] D152436: [clang][analyzer] Move checker alpha.unix.StdCLibraryFunctions out of alpha.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Why is this checker placed in the "unix" group (instead of e.g. "core")? As far 
as I understand it can check some POSIX-specific functions with a flag (that's 
off by default) but the core functionality checks platform-independent standard 
library functions.

Moreover, if you can, please provide links to some test result examples on open 
source projects (either run some fresh tests or provide links to an earlier 
test run that was running with the same functionality).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152436

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

[PATCH] D156787: [analyzer][attr] Add docs to ownership_* attributes

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added inline comments.



Comment at: clang/include/clang/Basic/AttrDocs.td:1189
+``onwership_returns``: Functions with this annotation return dynamic memory.
+The second annotation parameter is the size of the returned memory in bytes.
+

Szelethus wrote:
> aaron.ballman wrote:
> > I'm a bit confused on this last bit; how is the user supposed to statically 
> > know that value? I read this as claiming `my_malloc` returns 1 byte of 
> > dynamic storage; can the user tie the allocation to a parameter value 
> > instead?
> I share your confusion, I guess. What I've written here is my best 
> understanding of how this works, unfortunately, these annotations were made 
> in ~2010 (when I was still in elementary school) and abandoned since.
> 
> > I read this as claiming my_malloc returns 1 byte of dynamic storage;
> 
> Thats what the corresponding code in the Static Analyzer leads me to believe 
> as well.
> 
> > can the user tie the allocation to a parameter value instead?
> 
> No. 
I'd guess that this annotation was designed for factory functions that 
construct an instance of a concrete struct type on the heap. Allowing "generic" 
malloc variants (where the allocation size is specified by a parameter) would 
be a good additional feature, but I think that the current annotation also has 
its own uses.

Perhaps it would be good to provide examples like
```
  void __attribute((ownership_takes(malloc, 1))) free_widget(struct widget *);
  struct widget *__attribute((ownership_returns(malloc, sizeof(struct 
widget create_widget(void);
  void __attribute((ownership_holds(malloc, 1))) hold_widget(struct widget *);
```
that correspond to this use case. (By the way, does the checker handle 
non-`void*` pointers?)


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

https://reviews.llvm.org/D156787

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

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy planned changes to this revision.
donat.nagy marked 5 inline comments as done.
donat.nagy added a comment.

I'll soon upload a refactored version.




Comment at: clang/docs/analyzer/checkers.rst:44-50
+Moreover, if the pedantic mode is activated by
+``-analyzer-config core.BitwiseShift:Pedantic=true``, then this checker also
+reports situations where the _left_ operand of a shift operator is negative or
+overflow occurs during the right shift of a signed value. (Most compilers
+handle these predictably, but the C standard and the C++ standards before C++20
+say that they're undefined behavior. In the C++20 standard these constructs are
+well-defined, so activating pedantic mode in C++20 has no effect.)

steakhal wrote:
> I believe shifting out the "sign bit" is UB because the representation of the 
> signed integer type was not defined.
> Prior C++20 (?), it was allowed to represent signed integer types as 
> `two's-complement` (virtually every platform uses this), `one's complement`, 
> `sign-magnitude`. [[ 
> https://en.wikipedia.org/wiki/Signed_number_representations | Wiki ]].
> And it turns out, all these three representations behave differently with 
> negative values.
> Check out [[ https://youtu.be/JhUxIVf1qok?t=2089 | JF's talk ]] from 2018 
> about this.
> 
> I'd need to think about the relevance of C++20 in this context, but here is 
> what I think of now:
> My idea is that the observed behavior is not influenced by the "standard", 
> rather by the time we released C++20, they realized that no actual platform 
> uses weird signed integer representations.
> Given this observation, I'd argue that we shouldn't have this flag at all.
Thanks for the background information! I knew the rough outlines of the 
situation, but your links provided interesting details.

I agree that the non-pedantic mode is the "sane mode" that reflects the real 
behavior of the hardware; but the standards (with the exception of C++20) 
explicitly declare that signed shift overflow and shifting negative values is 
UB and there may be secondary requirements like [[ 
https://wiki.sei.cmu.edu/confluence/display/c/INT32-C.+Ensure+that+operations+on+signed+integers+do+not+result+in+overflow
 | SEI-CERT rule INT32-C  ]] that demand compliance to the standards.

I don't think that I'd spend time on implementing pedantic mode because there 
are more important things to do, but as we already have this feature, I think 
we should release it as an off-by-default option.




Comment at: clang/docs/analyzer/checkers.rst:81
+
+Ensure the shift operands are in proper range before shifting.
+

steakhal wrote:
> We should exactly elaborate on what "proper" means here.
What would you suggest? I could try to write a slightly longer suggestion, but 
I don't want to repeat the same conditions that I listed above the code example.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:142
+  BinaryOperator::Opcode 
Comparison,
+  long long Limit) {
+  SValBuilder &SVB = Ctx.getSValBuilder();

steakhal wrote:
> One does not frequently see the `long long` type.
> Do you have a specific reason why `int` or `unsigned` wouldn't be good?
I inherited this type choice from old code and didn't think much about it. My 
guess is that `LimitVal` needs to be a signed integer (because e.g. `-1 < 0u` 
would be evaluated to false) and it's a `long long` because that's guaranteed 
to represent all `unsigned` values. (However, the actual values of `Limit` are 
very small.)

I'll probably use `int` for `Limit` and `LimitVal` with some comments to 
clarify the reason for this type choice.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:152-156
+if (!StTrue) {
+  // We detected undefined behavior (the caller will report it).
+  State = StFalse;
+  return false;
+}

steakhal wrote:
> Generally, I don't favor mutating member functions.
> It makes it harder to track how we ended up with a given State.
> 
I also agree that mutating member functions are often problematic and may make 
the code more difficult to understand.

However, in this particular case I think it's important to ensure that we're 
always using the most recent state, and that's difficult to guarantee in the 
"pass everything as arguments and return values" style. Moreover, I do some 
parallel bookkeeping because I want to summarize the state changes in a single 
note tag, and that would make the "naive functional" solution even more 
convoluted.

In a functional language like Haskell I'd use computations in a [[ 
https://en.wikibooks.org/wiki/Haskell/Understanding_monads/State | State monad 
]] to implement this logic; my C++ code is a direct translation of that 
solution. (As a secondary utility, my class also provid

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 2 inline comments as done.
donat.nagy added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:301-302
+  pluralSuffix(MaximalAllowedShift));
+R->addNote(LeftNote, PathDiagnosticLocation{LHS, Ctx.getSourceManager(),
+Ctx.getLocationContext()});
+Ctx.emitReport(std::move(R));

donat.nagy wrote:
> donat.nagy wrote:
> > gamesh411 wrote:
> > > donat.nagy wrote:
> > > > NoQ wrote:
> > > > > Can we just append this to the warning? The `addNote()` is useful for 
> > > > > notes that need to be placed in code outside of the execution path, 
> > > > > but if it's always next to the warning, it probably doesn't make 
> > > > > sense to display it separately.
> > > > I didn't append this to the warning because I felt that the warning 
> > > > text is already too long. (By the way, an earlier internal variant of 
> > > > this checker produced two separate notes next to the warning message.)
> > > > 
> > > > I tweaked the messages of this checker before initiating this review, 
> > > > but I feel that there's still some room for improvements. (E.g. in this 
> > > > particular case perhaps we could omit some of the details that are not 
> > > > immediately useful and could be manually calculated from other parts of 
> > > > the message...) 
> > > Just a thought on simplifying the diagnostics a bit:
> > > 
> > > Warning: "Right operand is negative in left shift"
> > > Note: "The result of left shift is undefined because the right operand is 
> > > negative"
> > > Shortened: "Undefined left shift due to negative right operand"
> > > 
> > > Warning: "Left shift by '34' overflows the capacity of 'int'"
> > > Note: "The result of left shift is undefined because the right operand 
> > > '34' is not smaller than 32, the capacity of 'int'"
> > > Shortened: "Undefined left shift: '34' exceeds 'int' capacity (32 bits)"
> > > 
> > > The more complex notes are a bit sketchy, as relevant information can be 
> > > lost, and the following solution is probably a bit too much, but could 
> > > prove to be an inspiration:
> > > 
> > > Warning: "Left shift of '1024' overflows the capacity of 'int'"
> > > CXX Note: "Left shift of '1024' is undefined because 'int' can hold only 
> > > 32 bits (including the sign bit), so some bits overflow"
> > > CXX Note: "The value '1024' is represented by 11 bits, allowing at most 
> > > 21 bits for bitshift"
> > > C Note: "Left shift of '1024' is undefined because 'int' can hold only 31 
> > > bits (excluding the sign bit), so some bits overflow"
> > > C Note: "The value '1024' is represented by 11 bits, allowing at most 20 
> > > bits for bitshift"
> > > 
> > > Shortened:
> > > CXX Warning: "Undefined left shift: '1024' (11 bits) exceeds 'int' 
> > > capacity (32 bits, including sign)"
> > > C Warning: "Undefined left shift: '1024' (11 bits) exceeds 'int' capacity 
> > > (31 bits, excluding sign)"
> > Clarification about the `Msg`/`ShortMsg` distinction:
> > I'm intentionally separating the short warning messages and the longer note 
> > messages because `createBugReport()` enforces the convention that it will 
> > always emit a warning and a note at the bug location.
> > 
> > According to the comments in the source code, the intention is that the 
> > note contains all the relevant information, while the warning is a brief 
> > summary that can be displayed in situations where the notes wouldn't fit 
> > the UI.
> > 
> > IIUC many checkers ignore this distinction and emit the same (often long 
> > and cumbersome) message both as a note and as a warning 
> > (`createBugReport()` has a variant which takes only one message parameter 
> > and passes it to both locations), but I tried to follow it because I think 
> > it's ugly when the same message is repeated twice and there is some sense 
> > in providing both a brief summary and a full description that doesn't use 
> > potentially-ambiguous abbreviations to save space.
> > 
> > Of course I could also accept a community decision that this "brief warning 
> > / full note" distinction is deprecated and will be eliminated (because e.g. 
> > front-end utilities are not prepared to handle it), but in that case I'd 
> > strongly suggest a redesign where we eliminate the redundantly printed 
> > 'note' message. (There is no reason to say the same thing twice! There is 
> > no reason to say the same thing twice!)
> > 
> > On the other hand, in addition to this `Msg`/`ShortMsg` distinction, this 
> > part of the code also adds the extra `LeftNote` (as a remnant from an 
> > earlier internal version of this checker), and that's that's what I'd like 
> > to merge into `Msg` (following NoQ's suggestion and keeping it distinct 
> > from the `ShortMsg`).
> Among notes, my only planned change is that instead of 
> 
> > Warning: "Left shift of '1024' overflows the capacity of 'int'"
> > CXX Note: "Left shif

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy updated this revision to Diff 548626.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312

Files:
  clang/docs/analyzer/checkers.rst
  clang/include/clang/StaticAnalyzer/Checkers/Checkers.td
  clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt
  clang/test/Analysis/analyzer-config.c
  clang/test/Analysis/analyzer-enabled-checkers.c
  clang/test/Analysis/bitwise-ops-nocrash.c
  clang/test/Analysis/bitwise-ops.c
  clang/test/Analysis/bitwise-shift-common.c
  clang/test/Analysis/bitwise-shift-pedantic.c
  clang/test/Analysis/bitwise-shift-sanity-checks.c
  clang/test/Analysis/bitwise-shift-state-update.c
  clang/test/Analysis/casts.c
  clang/test/Analysis/diagnostics/track_subexpressions.cpp
  clang/test/Analysis/left-shift-cxx2a.cpp
  clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
  clang/test/Analysis/symbol-simplification-nonloc-loc.cpp

Index: clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
===
--- clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
+++ clang/test/Analysis/symbol-simplification-nonloc-loc.cpp
@@ -14,7 +14,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -29,7 +29,7 @@
   if (p) {
 // no-crash
   }
-  if (p == (int *)0x404) {
+  if (p == (int *)0x1b) {
 // no-crash
   }
 }
@@ -43,8 +43,6 @@
   nonloc_OP_loc(p, BINOP(-)); // no-crash
 
   // Bitwise operators:
-  // expected-warning@+2 {{The result of the left shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
-  // expected-warning@+2 {{The result of the right shift is undefined due to shifting by '1028', which is greater or equal to the width of type 'int' [core.UndefinedBinaryOperatorResult]}}
   nonloc_OP_loc(p, BINOP(<<)); // no-crash
   nonloc_OP_loc(p, BINOP(>>)); // no-crash
   nonloc_OP_loc(p, BINOP(&));
Index: clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
===
--- clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
+++ clang/test/Analysis/std-c-library-functions-arg-enabled-checkers.c
@@ -24,6 +24,7 @@
 // CHECK-NEXT: apiModeling.TrustReturnsNonnull
 // CHECK-NEXT: apiModeling.llvm.CastValue
 // CHECK-NEXT: apiModeling.llvm.ReturnValue
+// CHECK-NEXT: core.BitwiseShift
 // CHECK-NEXT: core.DivideZero
 // CHECK-NEXT: core.DynamicTypePropagation
 // CHECK-NEXT: core.NonnilStringConstants
Index: clang/test/Analysis/left-shift-cxx2a.cpp
===
--- clang/test/Analysis/left-shift-cxx2a.cpp
+++ clang/test/Analysis/left-shift-cxx2a.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx17 -std=c++17 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-disable-checker core.BitwiseShift -triple x86_64-apple-darwin13 -Wno-shift-count-overflow -verify=expected,cxx2a -std=c++2a %s
 
 int testNegativeShift(int a) {
   if (a == -5) {
Index: clang/test/Analysis/diagnostics/track_subexpressions.cpp
===
--- clang/test/Analysis/diagnostics/track_subexpressions.cpp
+++ clang/test/Analysis/diagnostics/track_subexpressions.cpp
@@ -12,10 +12,10 @@
 
 void shift_by_undefined_value() {
   uint8_t shift_amount = get_uint8_max(); // expected-note{{'shift_amount' initialized to 255}}
-// expected-note@-1{{Calling 'get_uint8_max'}}
-// expected-note@-2{{Returning from 'get_uint8_max'}}
-  (void)(TCP_MAXWIN << shift_amount); // expected-warning{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
-  // expected-note@-1{{The result of the left shift is undefined due to shifting by '255', which is greater or equal to the width of type 'int'}}
+  // expected-note@-1{{Calling 'get_uint8_max'}}
+  // expected-note@-2{{Returning from 'get_uint8_max'}}
+  (void)(TCP_MAXWIN << shift_amount); // expected-warning{{Left shift by '255' overflows the capacity of 'int'}}
+  // expected-note@-1{{The result of left shift is undefined because the right operand '255'

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 6 inline comments as done.
donat.nagy added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:94-98
+void BitwiseShiftValidator::run() {
+  if (isValidShift()) {
+Ctx.addTransition(State, createNoteTag());
+  }
+}

donat.nagy wrote:
> NoQ wrote:
> > Because `addTransition()` is a very confusing API that's very easy to 
> > misuse in subtle ways (causing unexpected state splits), I feel anxious 
> > when I see functions like
> > ```
> >   bool isValidShift();
> >   bool isOvershift();
> >   bool isOperandNegative(OperandSide Side);
> >   bool isLeftShiftOverflow();
> > ```
> > that look like boolean getters but in fact call `addTransition()` under the 
> > hood. If we could at least call them `checkOvershift()` etc., that'd be 
> > much better. Ideally I wish there was some safeguard against producing 
> > redundant transitions, like make them return an `ExplodedNode` and chain 
> > them, or something like that.
> These functions were originally called `checkXXX()` but I felt that the 
> meaning of their boolean return value was too confusing with that naming 
> scheme.
> 
> I'll try to ensure that `emitReport` or `addTransition` are only called on 
> the topmost layer (the `run()` method) and change the return types of 
> internal subroutines to make them return nullable pointers (e.g. 
> `std::unique_ptr`) instead of `bool`s. This would (1) 
> eliminate the "what does `true` mean at this point?" issues (2) clarify that 
> this checker never performs more than one transition. (If the shift is 
> invaild, we transition to a sink node by emitting a bug; if the shift is 
> vaild, we perform a transition to record the state update.)
I eliminated `isValidShift()` by merging it with `run()` (which was very short) 
and renamed the rest of the functions to `checkXXX`.

Now the exploded graph is only modified in the top-level method `run()`.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:142
+  BinaryOperator::Opcode 
Comparison,
+  long long Limit) {
+  SValBuilder &SVB = Ctx.getSValBuilder();

donat.nagy wrote:
> steakhal wrote:
> > One does not frequently see the `long long` type.
> > Do you have a specific reason why `int` or `unsigned` wouldn't be good?
> I inherited this type choice from old code and didn't think much about it. My 
> guess is that `LimitVal` needs to be a signed integer (because e.g. `-1 < 0u` 
> would be evaluated to false) and it's a `long long` because that's guaranteed 
> to represent all `unsigned` values. (However, the actual values of `Limit` 
> are very small.)
> 
> I'll probably use `int` for `Limit` and `LimitVal` with some comments to 
> clarify the reason for this type choice.
I changed the type of the `Limit` argument to `unsigned` (because that's what 
this function is called with) and changed the type of `LimitVal` to `IntTy` 
with a comment that describes that it must be signed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156312

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

[PATCH] D156312: [analyzer] Upstream BitwiseShiftChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked 4 inline comments as done.
donat.nagy added a comment.

Thanks for the suggestions, I answered those where I had something to say and 
I'll implement the rest of them.

In D156312#4576120 , @steakhal wrote:

> Have you considered checking the shift operands for taint?
> I wonder if we could warn if we cannot prove the correct use of the shift 
> operator when either of the operands is tainted.

It would be straightforward to add taint handling; but I'd prefer to leave it 
for a follow-up commit. Some experimentation would be needed to see whether it 
produces useful results (do real-world projects use shifts on tainted numbers? 
do we encounter false positives?).




Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:53
+class BitwiseShiftValidator {
+  // Primary mutable state:
+  CheckerContext &Ctx;

steakhal wrote:
> Where does this comment corresponds to?
The two data members (`Ctx` and `State`) that are directly below the comment.

I tried to split the list of data members into three groups (primary mutable 
state, secondary mutable state, `const`  members), but now I reconsider this I 
feel that these header comments are actually superfluous.

I'll return to a variant of the earlier layout where I put 
`NonNegFlags`/`UpperBound` to the end and comment that they are only used for 
note tag creation; but don't emphasize the (immediately visible) `const`ness / 
non-`const`-ness of other members with comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:57-59
+  // Secondary mutable state, used only for note tag creation:
+  enum { NonNegLeft = 1, NonNegRight = 2 };
+  unsigned NonNegFlags = 0;

steakhal wrote:
> How about using the same enum type for these two? It would signify that these 
> are used together.
> Also, by only looking at the possible values, it's not apparent that these 
> are bitflag values. A suffix might help the reader.
Using the enum type is a good idea, however I'd probably put the `Flag` suffix 
into the name of the type:
```
enum SignednessFlags : unsigned { NonNegLeft = 1, NonNegRight = 2 };
SignednessFlags NonNegOperands = 0;
```



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:60-63
+  // If there is an upper bound, it is <= the size of a primitive type
+  // (expressed in bits), so this is a very safe sentinel value:
+  enum { NoUpperBound = 999 };
+  unsigned UpperBound = NoUpperBound;

steakhal wrote:
> Have you considered using `std::optional`?
> Given that the dimension of this variable is "bits", I think it would be 
> great to have that in its name.
I considered using `std::optional`, but using this "old-school" solution 
ensures that `NoUpperBound` is comparable to and larger than the "real" upper 
bound values, which lets me avoid some ugly boolean logic. I'll update the 
comment and probably also the variable name.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:106-109
+  if (auto BRP = checkOvershift()) {
+Ctx.emitReport(std::move(BRP));
+return;
+  }

steakhal wrote:
> I'd prefer `BR` or `Report`. Since we know already that we are in the 
> path-sensitive domain, `P` has less value there.
> I'd apply this concept everywhere in the patch.
The `P` stands for the "Ptr" in the name of the type alias `BugReportPtr`; but 
your comment clearly highlights that this variable name is confusing ;).

I'll probably avoid the `auto` and use `BugReportPtr Bug = ...` to remind the 
reader that this is a pointer (that can be null) and if it's non-null, then we 
found a bug.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:190-191
+  std::string Msg =
+  llvm::formatv("The result of {0} shift is undefined because the right "
+"operand{1} is not smaller than {2}, the capacity of 
'{3}'",
+isLeftShift() ? "left" : "right", RightOpStr, LHSBitWidth,

steakhal wrote:
> 
This is an important distinction, I use "not smaller than {2}" as a shorter 
synonym of "larger than or equal to {2}". I can choose some other wording if 
you feel that this is not clear enough.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:263
+
+  const SVal Right = Ctx.getSVal(Op->getRHS());
+

steakhal wrote:
> I'd move this closer to the first "use" place.
Good point, previously it was used by the calculation that is now performed by 
`assumeRequirement`.



Comment at: clang/lib/StaticAnalyzer/Checkers/BitwiseShiftChecker.cpp:315
+default:
+  llvm_unreachable("this checker does not use other comparison operators");
+  }

steakhal wrote:
> 
I'd say that the current wording is correct because the the checker "use"s the 
comparison op

[PATCH] D158156: [analyzer] Add C++ array delete checker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/CXXDeleteChecker.cpp:47
+protected:
+  class PtrCastVisitor : public BugReporterVisitor {
   public:

I like that yo switched to a more descriptive class name :)



Comment at: clang/test/Analysis/ArrayDelete.cpp:85-88
+Derived *d3 = new DoubleDerived[10];
+Base *b3 = d3; // expected-note{{Conversion from derived to base happened 
here}}
+delete[] b3; // expected-warning{{Deleting an array of polymorphic objects 
is undefined}}
+// expected-note@-1{{Deleting an array of polymorphic objects is 
undefined}}

steakhal wrote:
> Hmm, the static type of `d3` doesn't tell us much about how it refers to an 
> object of type `DoubleDerived`.
> To me, it would make sense to have multiple `Conversion from derived to base 
> happened here`, even telling us what static type it converted to what other 
> static type in the message.
> And it should add a new visitor of the same kind tracking the castee.
> 
> ```
> Derived *d3 = new DoubleDerived[10]; // note: `DoubleDerived` -> `Derived` 
> here
> Base *b3 = d3; // note: `Derived` -> `Base` here
> delete[] b3; // warn: Deleting `Derived` objects as `Base` objects.
> ```
> WDYT @donat.nagy ?
I agree that it would be good to be good to mention the class names in the 
message.



Comment at: clang/test/Analysis/ArrayDelete.cpp:90-93
+Base *b4 = new DoubleDerived[10];
+Derived *d4 = reinterpret_cast(b4);
+DoubleDerived *dd4 = reinterpret_cast(d4);
+delete[] dd4; // no-warning

steakhal wrote:
> I think in such cases a `static_cast` should suffice; unless you 
> intentionally wanted to test `reinterpret_cast` of course.
Note that the effects of `reinterpret_cast` and `static_cast` are different 
[1]: `static_cast(BasePtr)` adjusts the pointer value to ensure that 
it points to the derived object whose Base ancestor object would be located at 
BasePtr, while a `reinterpret_cast` keeps the raw pointer value (which is 
complete nonsense from an object-oriented point-of-view, but could be relevant 
in some low-level hacks).

I'd guess that this part is directly testing the strange behavior of 
`reinterpret_cast`; but it'd be good to add a comment that clarifies this.

[1]: 
https://stackoverflow.com/questions/9138730/why-is-it-important-to-use-static-cast-instead-of-reinterpret-cast-here


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

https://reviews.llvm.org/D158156

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

[PATCH] D158156: [analyzer] Add C++ array delete checker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added inline comments.



Comment at: clang/test/Analysis/ArrayDelete.cpp:85-88
+Derived *d3 = new DoubleDerived[10];
+Base *b3 = d3; // expected-note{{Conversion from derived to base happened 
here}}
+delete[] b3; // expected-warning{{Deleting an array of polymorphic objects 
is undefined}}
+// expected-note@-1{{Deleting an array of polymorphic objects is 
undefined}}

steakhal wrote:
> donat.nagy wrote:
> > steakhal wrote:
> > > Hmm, the static type of `d3` doesn't tell us much about how it refers to 
> > > an object of type `DoubleDerived`.
> > > To me, it would make sense to have multiple `Conversion from derived to 
> > > base happened here`, even telling us what static type it converted to 
> > > what other static type in the message.
> > > And it should add a new visitor of the same kind tracking the castee.
> > > 
> > > ```
> > > Derived *d3 = new DoubleDerived[10]; // note: `DoubleDerived` -> 
> > > `Derived` here
> > > Base *b3 = d3; // note: `Derived` -> `Base` here
> > > delete[] b3; // warn: Deleting `Derived` objects as `Base` objects.
> > > ```
> > > WDYT @donat.nagy ?
> > I agree that it would be good to be good to mention the class names in the 
> > message.
> Do you also agree that we should have all steps where such a conversion 
> happened?
> Notice the 2 `note:` markers in my example. @donat.nagy 
It would be a nice addition if it wouldn't seriously complicate the 
implementation.

If we want to report multiple/all conversions, then we would need to create 
messages for back-and-forth conversions (e.g. allocating Derived, converting it 
to Base, back to Derived, back to Base, then deleting it illegally).


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

https://reviews.llvm.org/D158156

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

[PATCH] D153776: [clang][analyzer] Display notes in StdLibraryFunctionsChecker only if interesting

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

In D153776#4467627 , @balazske wrote:

> The success/failure note tags are not added to all functions, `dup` is one of 
> these, this means at `dup` no note tag is shown. A next patch can be made to 
> add these messages to all functions. The other places look good, but 
> CodeChecker is a bit tricky, you must select 
> //*_stdclf_notetag_interesting_test_2// at the small arrow after the "found 
> in:" text (upper right corner). The link is good but not that instance of the 
> bug is displayed because only the note tags are different.

I think we should definitely add success/failure note tags to all functions 
where this checker can suddenly assume that "Oops, this failed". If this 
"Assuming that 'foo' fails" message is not there, it's a very natural reaction 
to search for some earlier note that would let the checker deduce that this 
function will fail here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153776

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

[PATCH] D154325: [analyzer][NFC] Move away from using raw-for loops inside StaticAnalyzer

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

I don't have concrete remarks, but I would like to note that I'm happy to see 
this cleanup :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154325

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

[PATCH] D154423: [clang][analyzer] Add all success/failure messages to StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

These code changes seem to be promising. Please upload results on the open 
source projects (like the ones that you uploaded previously), and I hope that 
after verifying those I we can finally merge this set of commits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154423

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

[PATCH] D154603: [analyzer][clangsa] Add new option to alpha.security.cert.InvalidPtrChecker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

The commit looks good in general, I have a few minor suggestions and a serious 
question about the state transition bureaucracy.




Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:65-66
+  // If set to true, consider getenv calls as invalidating operations on the
+  // environment variable buffer. This is implied in the standard, but can lead
+  // to practical false positives.
+  bool InvalidatingGetEnv = false;

Nitpick: "practical false positives" sounds strange for me, consider writing
[...but] "in practice does not cause problems (in the commonly used 
environments)"
or something similar.



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:125
 
-  const NoteTag *Note =
-  C.getNoteTag([SymbolicEnvPtrRegion, FunctionName](
-   PathSensitiveBugReport &BR, llvm::raw_ostream &Out) {
-if (!BR.isInteresting(SymbolicEnvPtrRegion))
-  return;
-Out << '\'' << FunctionName
-<< "' call may invalidate the environment parameter of 'main'";
-  });
+  ExplodedNode *CurrentChainEnd = nullptr;
+

Perhaps add a comment that clarifies that passing a `nullptr` as the 
ExplodedNode to `addTransition` is equivalent to specifying the current node. I 
remember this because I was studying its implementation recently, but I 
would've been surprised and suspicious otherwise.



Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:218
+State->add(Call.getReturnValue().getAsRegion());
+   C.addTransition(State);
+  }

I fear that this state transition will go "sideways" and the later state 
transitions (which add the note tags) will branch off instead of building onto 
this. IIUC calling `CheckerContext::addTransition` registers the transition 
without updating the "current ExplodedNode" field of `CheckerContext`, so you 
need to explicitly store and pass around the ExplodedNode returned by it if you 
want to build on it.

This is an ugly and counter-intuitive API, and I also ran into a very similar 
issue a few weeks ago (@Szelethus helped me).



Comment at: clang/test/Analysis/cert/env34-c.c:6
+//
+// TODO: write test cases that follow the pattern:
+//   "getenv -> store pointer -> setenv -> use stored pointer"

gamesh411 wrote:
> This test file is incomplete.
> I would welcome suggestions here as to how to test this.
> Should a new file be created for the config option with different test cases, 
> or is this file to be extended?
Personally I'd prefer putting those cases into a separate files, because this 
test file is already 340 lines long and it'd be difficult to understand if it 
was filled with conditional checks. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154603

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

[PATCH] D154509: [clang][analyzer] StdLibraryFunctionsChecker: Allow NULL buffer in `fread` and `fwrite` if size is zero.

[Donát Nagy via Phabricator via cfe-commits]

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

LGTM.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:413-414
+using ValueConstraint::ValueConstraint;
+ArgNo SizeArg1N;
+std::optional SizeArg2N;
+// This variable has a role when we negate the constraint.

What would you think about a `SmallVector<2>` for these? It would allow you to 
handle them with a (short) for loop instead of separate commands.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154509

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

[PATCH] D153954: [WIP][clang][analyzer] Relax alpha.cplusplus.EnumCastOutOfRange checker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

I think the old "report when the value stored in an enum type is not equal to 
one of the enumerators" behavior would be useful as an opt-in checker that 
could be adopted by some projects; while the new "relaxed" version is too bland 
to be really useful. I'd suggest keeping the old behavior in the general case, 
eliminating the "obvious" false positives like `std::byte` (don't report types 
that have no enumerators) and moving this checker towards the optin group.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153954

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

[PATCH] D153954: [WIP][clang][analyzer] Relax alpha.cplusplus.EnumCastOutOfRange checker

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

In D153954#4480296 , @steakhal wrote:

> [...] I'd not recommend moving the actual implementation anywhere.

I agree, I mostly spoke figuratively, suggesting that this checker could end up 
in the optin group when it's eventually brought out of alpha.




Comment at: clang/test/Analysis/enum-cast-out-of-range.cpp:44
+// Suppress unused warnings
+[](...){}(unscoped, scoped);
+  }

steakhal wrote:
> `void sink(...);` would have achieved the same and I'd say it's less complex.
> `sink(unscoped, scoped);`
Or just `(void)unscoped; (void)scoped;` if we're bikeshedding this test...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153954

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

[PATCH] D154509: [clang][analyzer] StdLibraryFunctionsChecker: Allow NULL buffer in `fread` and `fwrite` if size is zero.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

By the way here is a concrete example where this patch is needed to squash a 
false positive: 
https://codechecker-demo.eastus.cloudapp.azure.com/Default/report-detail?run=postgres_REL_13_0_stdclf_notetag_interesting_test_3&is-unique=on&report-hash=49939ae1896dff1ad782092b8534bd68&report-filepath=%2arelcache.c&report-id=1942889


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154509

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

[PATCH] D154423: [clang][analyzer] Add all success/failure messages to StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

I looked through most of the open source results, and they look promising.

However I've seen one questionable tendency: there are many reports (e.g. this 
one 
)
 where the checker assumes that `fileno(stdin)`, `fileno(stdout)` or 
`fileno(stderr)` fails. How realistic are these assumptions? Do we need to 
bother the programmer with these or should/can we silence them (and perhaps 
return the known fileno values corresponding to these standard streams)?

Another, concrete issue is this report 

 where the analyzer assumes that `ftell` returns `-1` (that gets converted to 
2**64-1 because unsigned numbers are crazy), but there is no note tag (not even 
in the _3 run) and I don't see where does this assumption come from (although I 
didn't do a deep analysis).

Apart from these, the false positives are coming from general limitations of 
the engine that cannot be reasonably avoided.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154423

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

[PATCH] D154423: [clang][analyzer] Add all success/failure messages to StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

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

Ok, then I think you can finally merge this commit and its two predecessors. 
I'm happy to see that this improvement is complete.

I hope that you can later add a special case for the standard streams, but I 
agree that it should be a separate commit.

Thanks for clarifying the `ftell` situation, that's clearly an engine issue 
that should not block this checker improvement. However, I'd be really glad to 
see a cleanup of the interestingness system (as a separate project), because 
its inconsistency leads to lots of very confusing bug reports.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154423

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

[PATCH] D153776: [clang][analyzer] Display notes in StdLibraryFunctionsChecker only if interesting

[Donát Nagy via Phabricator via cfe-commits]

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

LGTM, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153776

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

[PATCH] D154423: [clang][analyzer] Add all success/failure messages to StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

In D154423#4485009 , @balazske wrote:

> It would be more simple to handle the standard streams in `StreamChecker` 
> only. [...]

That's a reasonable plan, especially if we can bring that checker out of alpha 
in the foreseeable future. I like that it avoids code duplication.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154423

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

[PATCH] D153612: [clang][analyzer] Add and change NoteTags in StdLibraryFunctionsChecker.

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

(Just added some side remarks about string handling annoyances.)

@balazske Please (1) handle the `dyn_cast` request of  @steakhal and also (2) 
do something with this "how to convert `StringRef` to `char *`" question that 
we're bikeshedding. I hope that after those we could finally merge this commit 
sequence.




Comment at: 
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:1305
+  std::string Note =
+  llvm::formatv(Case.getNote().str().c_str(),
+cast(Call.getDecl())->getDeclName());

steakhal wrote:
> donat.nagy wrote:
> > Consider using the method `StringRef::data()` which directly converts a 
> > `StringRef` to a `const char *`. Your two-step conversion has the advantage 
> > that it adds a `\0` terminator even if the `StringRef` isn't 
> > null-terminated, but I cannot imagine a "natural" code change that would 
> > introduce references to non-null-terminated char arrays as note message 
> > templates.
> I would prefer not to rely on that `StringRef`s (here) are expected to be 
> null-terminated, unless benchmarks demonstrate that this is important. If 
> that would turn out to be the case, then we would need to enforce this using 
> some sort of `assert` expression.
I think the way of expressing this concrete conversion is a very low priority 
question (a.k.a. bikeshedding), so returning to the previously used two-step 
conversion is also fine for me.

The primary goal of this ".data()" shortcut was not performance, but clarity: 
those chained conversions triggered my "detect suspicious code" instincts and 
this was the best alternative that I could find. In general I feel that the 
LLVM codebase has way too many string types, and the back-and-forth conversions 
between them add lots of unnecessary noise to the code.

In this particular case I think it's the fault of `llvm::formatv` that it 
doesn't accept `StringRef` (the most commonly used non-owning string) and in 
general it's problematic design that there is no "good" conversion between 
`StringRef` and C-style strings in either direction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153612

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

[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added a comment.

Thanks for creating this commit, it's a useful improvement!

I added some inline comments on minor implementation details; moreover, note 
that "Releted checkers:" (instead of "related") is a typo in the commit message.

I also have a less concrete question about the root cause of these bugs. Does 
this commit fix the "root" of the issue by eliminating some misuse of correctly 
implemented (but perhaps surprising) `SVal` / `APSInt` arithmetic; or is there 
an underlying bug in the `SVal` / `APSInt` arithmetic which is just avoided 
(and not eliminated) by this commit? In the latter case, what obstacles prevent 
us from fixing the root cause?




Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:202-203
   // CHECK UPPER BOUND
-  DefinedOrUnknownSVal Size = getDynamicExtent(state, Reg, svalBuilder);
+  SVal Size = svalBuilder.convertToArrayIndex(
+  getDynamicExtent(state, Reg, svalBuilder));
   if (auto KnownSize = Size.getAs()) {

I wonder whether it would be better to move this conversion into the definition 
of `getDynamicExtent` to ensure that it has a consistent return type. Are there 
any situations where it's useful that `getDynamicExtent` can return something 
that's not an `ArrayIndexTy`?



Comment at: clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:180-183
+  SVal CountReached =
+  SVB.evalBinOp(State, BO_GE, Idx, Count, ASTCtx.BoolTy);
+  if (!CountReached.isUndef() &&
+  State->assume(*CountReached.getAs(), true))

I think checking the nullness of `getAs()` is more elegant than using a 
separate `isUndef()` check.

On a longer term / as a separate improvement, I'd also think about allowing 
`UndefinedVal` as the argument of the `assert()`-like functions, because the 
`evalBinOp` -> `assert` combination is very common in checkers and IIRC in most 
checkers the branch of `UndefinedVal` will produce the same result as 
`UnknownVal`.



Comment at: clang/lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp:186-192
   const ElementRegion *const ER = RegionManager.getElementRegion(
-  CE.getArgExpr(1)->getType()->getPointeeType(), Idx, SuperRegion,
-  Ctx.getASTContext());
+  ElemType,
+  SVB.evalBinOp(State, BO_Add, Idx, MROffset, SVB.getArrayIndexType())
+  .castAs(),
+  SuperRegion, Ctx.getASTContext());
 
   ReqRegions.push_back(ER->getAs());

I'd use `getAs()` and a conditional to avoid crashes in the 
(theoretical) case that `evalBinOp` returns `UnknownVal`; and I suspect that 
`getAs()` is superfluous because `MemRegion` is a base class of 
`ElementRegion`.



Comment at: clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp:92
+
+  return ElementCount.castAs();
+}

Are you sure that this cannot cause crashes? (E.g. did you check that there is 
no corner case when `getElementExtent` returns 0?)

I can accept this cast, especially if you have a clear proof that it's valid, 
but I'd prefer a more defensive solution that turns `UndefinedVal` into 
`UnknownVal` either here or preferably in the `assert()` function family that 
consumes the results from functions like this. 



Comment at: clang/test/Analysis/array-bound-v2-constraint-check.c:1
-// RUN: %clang_analyze_cc1 
-analyzer-checker=core,alpha.security.ArrayBoundV2,debug.ExprInspection \
+// RUN: %clang_analyze_cc1 
-analyzer-checker=core,unix,alpha.security.ArrayBoundV2,debug.ExprInspection \
 // RUN:   -analyzer-config eagerly-assume=false -verify %s

Perhaps only enable `unix.Malloc` to ensure that this test is not affected by 
changes to other checkers in the `unix` group. (It's enough for the testcase 
that you added.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158707

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

[PATCH] D158855: [analyzer][NFC] Remove useless class BuiltinBug

[Donát Nagy via Phabricator via cfe-commits]

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

...because it provides no useful functionality compared to its base class 
`BugType`.

A long time ago there were substantial differences between `BugType` and 
`BuiltinBug`, but they were eliminated by commit 1bd58233 in 2009 (!).  Since 
then the only functionality provided by `BuiltinBug` was that it specified 
`categories::LogicError` as the bug category and it stored an extra data member 
`desc`.

This commit sets `categories::LogicError` as the default value of the third 
argument (bug category) in the constructors of BugType and replaces use of the 
`desc` field with simpler logic.

Note that `BugType` has a data member `Description` and a non-virtual method 
`BugType::getDescription()` which queries it; these are distinct from the 
member `desc` of `BuiltinBug` and the shadowing method 
`BuiltinBug::getDescription()` which queries it. This confusing name collision 
was a major motivation for the elimination of `BuiltinBug`.

As this commit touches many files, I avoided functional changes and left behind 
several FIXME notes on messages that should be improved later.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D158855

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/unittests/StaticAnalyzer/CallEventTest.cpp
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -89,8 +89,8 @@
 
 class CheckerRegistrationOrderPrinter
 : public Checker> {
-  std::unique_ptr BT =
-  std::make_unique(this, "Registration order");
+  std::unique_ptr BT =
+  std::make_unique(this, "Registration order");
 
 public:
   void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
@@ -125,8 +125,7 @@
 
 #define UNITTEST_CHECKER(CHECKER_NAME, DIAG_MSG)   \
   class CHECKER_NAME : public Checker> {  \
-std::unique_ptr BT =   \
-std::make_unique(this, DIAG_MSG);  \
+std::unique_ptr BT = std::make_unique(this, DIAG_MSG);   \
\
   public:  \
 void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {}  \
Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -26,7 +26,7 @@
 
 class FalsePositiveGenerator 

[PATCH] D158855: [analyzer][NFC] Remove useless class BuiltinBug

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy added inline comments.
Herald added a subscriber: ormris.



Comment at: clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp:35-36
 public Checker {
-  mutable std::unique_ptr BT;
+  mutable std::unique_ptr BT;
   mutable std::unique_ptr TaintBT;
 

This inconsistency is the "original target" of this commit. I wanted to unify 
the two types of bug report creation ("regular" and tainted) and as I examined 
the situation I noticed that `BuiltinBug` isa confusing mess and it's always 
easy to replace it with `BugType`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158855

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

[PATCH] D158855: [analyzer][NFC] Remove useless class BuiltinBug

[Donát Nagy via Phabricator via cfe-commits]

donat.nagy marked an inline comment as done.
donat.nagy added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp:27
   class BoolAssignmentChecker : public Checker< check::Bind > {
-mutable std::unique_ptr BT;
+mutable std::unique_ptr BT;
 void emitReport(ProgramStateRef state, CheckerContext &C,

xazax.hun wrote:
> In case you are down to some additional cleanup efforts, we no longer need 
> lazy initialization for `BugType`s. This is an old pattern, we used to need 
> it due to some initialization order problems, but those have been worked 
> around in engine. See the FuchsiaHandleChecker as an example how we do it in 
> newer checks: 
> https://github.com/llvm/llvm-project/blob/ea82a822d990824c58c6fa4b503ca84c4870c940/clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp#L190
> 
> Feel free to ignore this or do it in a follow-up PR. 
Great news! I'll keep it in mind and I'll try to do this clean-up if I have 
enough free time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158855

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

[PATCH] D158968: [analyzer] Fix assertion on casting SVal to NonLoc inside the IteratorRange checker

[Donát Nagy via Phabricator via cfe-commits]

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

LGTM. I'm not familiar with the Iterator checker family, but this is a very 
straightforward change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158968

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

[PATCH] D158707: [analyzer] Fix a few size-type signedness inconsistency related to DynamicExtent

[Donát Nagy via Phabricator via cfe-commits]

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

Thanks for the updates! I didn't spot any important issue, so I'm accepting 
this commit, but let's wait for the opinion of @steakhal before merging this.

In D158707#4613955 , @danix800 wrote:

> In D158707#4613743 , @donat.nagy 
> wrote:
>
>> [...] Does this commit fix the "root" of the issue by eliminating some 
>> misuse of correctly implemented (but perhaps surprising) `SVal` / `APSInt` 
>> arithmetic; or is there an underlying bug in the `SVal` / `APSInt` 
>> arithmetic which is just avoided (and not eliminated) by this commit? In the 
>> latter case, what obstacles prevent us from fixing the root cause?
>
> For the observed signed compared to unsigned unexpectation from ArrayBoundV2,
> the constraint manager does give the CORRECT result.
>
> It's a misuse of the constraint API, mainly caused by unexpected unsigned 
> extent
> set by memory modeling. Actually `ArrayBoundV2::getSimplifiedOffsets()` has 
> clear
> comment that this `signed <-> unsigned` comparison is problematic.
>
>   // TODO: once the constraint manager is smart enough to handle non 
> simplified
>   // symbolic expressions remove this function. Note that this can not be 
> used in
>   // the constraint manager as is, since this does not handle overflows. It is
>   // safe to assume, however, that memory offsets will not overflow.
>   // NOTE: callers of this function need to be aware of the effects of 
> overflows
>   // and signed<->unsigned conversions!
>   static std::pair
>   getSimplifiedOffsets(NonLoc offset, nonloc::ConcreteInt extent,
>SValBuilder &svalBuilder) {

In fact, the "NOTE:" comment was added by my patch D135375 
 (which eliminated lots of false positives 
caused by a situation when the argument `offset` was unsigned), but I was still 
confused by this new bug. (Where the other argument `extent` was unsigned and 
this led to incorrect conclusions like "`len` cannot be larger than `3u`, so 
`len` cannot be `-1`, because `-1` is larger than `3u`" 🙃 .) Thanks for 
troubleshooting this issue!




Comment at: clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp:34
+if (auto SSize =
+SVB.convertToArrayIndex(*Size).getAs())
+  return *SSize;

I think it's a good convention if `getDynamicExtent()` will always return 
concrete values as `ArrayIndexTy`. (If I didn't miss some very obscure case, 
then this will be true when this commit is merged.) 



Comment at: clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp:49
+return UnknownVal();
+  MR = MR->StripCasts();
+

Note that `StripCasts()` is overzealous and strips array indexing when the 
index is zero. For example if you have a rectangular matrix `int m[6][8];` then 
this function would (correctly) say that the element count of `m[1]` is 8, but 
it would claim that the element count of `m[0]` is 6 (because it strips the 
'cast' and calculates the element count of `m`).

This is caused by the hacky "solution" that casts are represented by 
`ElementRegion{original region, 0, new type}` which cannot be distinguished 
from a real element access where the index happens to be 0. (This is just a 
public service announcement, this already affects e.g. `getDynamicExtent` and I 
don't think that you can find a local solution. For more information, see also 
https://github.com/llvm/llvm-project/issues/42709 which plans to refactor the 
memory model.)



Comment at: clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp:72-75
+  auto ElementCount =
+  SVB.evalBinOp(State, BO_Div, Size, ElementSize, SVB.getArrayIndexType())
+  .getAs();
+  return ElementCount ? *ElementCount : UnknownVal();

Perhaps use the method `value_or` of std::optional?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158707

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

[PATCH] D158855: [analyzer][NFC] Remove useless class BuiltinBug

[Donát Nagy via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
donat.nagy marked an inline comment as done.
Closed by commit rG8a5cfdf7851d: [analyzer][NFC] Remove useless class 
BuiltinBug (authored by donat.nagy).

Changed prior to commit:
  https://reviews.llvm.org/D158855?vs=553498&id=553909#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158855

Files:
  clang/include/clang/StaticAnalyzer/Core/BugReporter/BugType.h
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  clang/lib/StaticAnalyzer/Checkers/BoolAssignmentChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/CastSizeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ChrootChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/DebugCheckers.cpp
  clang/lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/FixedAddressChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/NonNullParamChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ObjCAtSyncChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/PointerArithChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/PointerSubChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnPointerRangeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/ReturnUndefChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/TestAfterDivZeroChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefCapturedBlockVarChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefinedArraySubscriptChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefinedAssignmentChecker.cpp
  
clang/lib/StaticAnalyzer/Checkers/UninitializedObject/UninitializedObjectChecker.cpp
  clang/lib/StaticAnalyzer/Checkers/VforkChecker.cpp
  clang/lib/StaticAnalyzer/Core/BugReporter.cpp
  clang/unittests/StaticAnalyzer/CallEventTest.cpp
  clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
  clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp

Index: clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
===
--- clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
+++ clang/unittests/StaticAnalyzer/RegisterCustomCheckersTest.cpp
@@ -89,8 +89,8 @@
 
 class CheckerRegistrationOrderPrinter
 : public Checker> {
-  std::unique_ptr BT =
-  std::make_unique(this, "Registration order");
+  std::unique_ptr BT =
+  std::make_unique(this, "Registration order");
 
 public:
   void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
@@ -125,8 +125,7 @@
 
 #define UNITTEST_CHECKER(CHECKER_NAME, DIAG_MSG)   \
   class CHECKER_NAME : public Checker> {  \
-std::unique_ptr BT =   \
-std::make_unique(this, DIAG_MSG);  \
+std::unique_ptr BT = std::make_unique(this, DIAG_MSG);   \
\
   public:  \
 void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {}  \
Index: clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
===
--- clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
+++ clang/unittests/StaticAnalyzer/FalsePositiveRefutationBRVisitorTest.cpp
@@ -26,7 +26,7 @@
 
 class FalsePositiveGenerator : public Checker {
   using Self = FalsePositiveGenerator;
-  const BuiltinBug FalsePositiveGeneratorBug{this, "FalsePositiveGenerator"};
+  const BugType FalsePositiveGeneratorBug{this, "FalsePositiveGenerator"};
   using HandlerFn = bool (Self::*)(const CallEvent &Call,
CheckerContext &) const;
   CallDescriptionMap Callbacks = {
Index: clang/unittests/StaticAnalyzer/CallEventTest.cpp
===
--- clang/unittests/StaticAnalyzer/CallEventTest.cpp
+++ clang/unittests/StaticAnalyzer/CallEventTest.cpp
@@ -36,11 +36,11 @@
 }
 
 class CXXDeallocatorChecker : public Checker {
-  std::unique_ptr BT_uninitField;
+  std::unique_ptr BT_uninitField;
 
 public:
   CXXDeallocatorChecker()
-  : BT_uninitField(new BuiltinBug(this, "CXXDeallocator")) {}
+  : BT_uninitField(new BugType(this, "CXXDeallocator")) {}
 
   void checkPreCall(const CallEvent &Call, CheckerContext &C) const {
 const auto *DC = dyn_cast(&Call);
Index: clang/lib/StaticAnalyzer/Core/BugReporter.cpp
===