[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

In D92634#2484404 , @steakhal wrote:

> Here is a link for our results on a few more projects. It might be useful for 
> you.
> https://codechecker-demo.eastus.cloudapp.azure.com/Default/runs?run=D92634&items-per-page=50&sort-by=name&sort-desc=false
> Note: use the diff to filter only for the new reports.

Thanks! Looks great.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92634

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

[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

> Typically in such cases bug visitors should be added/improved until it is 
> clear from the user-facing report why does the analyzer think so. They'd 
> highlight the important events, prevent path pruning, and potentially 
> suppress reports if the reason is discovered to not be valid.

Thanks! But in this case I do not think that my code works well.

I have reduced the test case:

  #include 
  
  #define LIBCERROR_MESSAGE_INCREMENT_SIZE64
  #define LIBCERROR_MESSAGE_MAXIMUM_SIZE  4096
  
  int get_print_count();
  
  void foo( )
  {
size_t message_size= 0;
size_t next_message_size   = 
LIBCERROR_MESSAGE_INCREMENT_SIZE;
int print_count= 0;
  
while (1)
{
if( next_message_size >= LIBCERROR_MESSAGE_MAXIMUM_SIZE )
{
next_message_size = LIBCERROR_MESSAGE_MAXIMUM_SIZE;
}
  
message_size = next_message_size;
  
  
print_count = get_print_count();
  
if( print_count <= -1 )
{
next_message_size += LIBCERROR_MESSAGE_INCREMENT_SIZE;  
// <- Assigned value is garbage or undefined
}
else if( ( (size_t) print_count >= message_size ) )
{
next_message_size = (size_t) ( print_count + 1 );
print_count   = -1;
}
else
{
int error_string_size = (size_t) print_count + 1; // <- 
The result of the '+' expression is undefined
}
if( message_size >= LIBCERROR_MESSAGE_MAXIMUM_SIZE )
{
break;
}
}
  }

I guess it's best that I rewrite my logic in a new check.

But well I have the feeling it could take a couple of days before I have 
something new..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92634

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

[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

I have run clang static analysis on random open source projects. The very first 
finding that I look at seems (to me) to be a false positive. :-(

My code seems to think that a variable `print_count` has the value INT_MAX for 
some reason and to me that seems impossible. I'll investigate this..

analyzed package:
ftp://ftp.de.debian.org/debian/pool/main/libm/libmsiecf/libmsiecf_20181227.orig.tar.gz

>
=

libcerror_error.c:426:45: warning: The result of the '+' expression is 
undefined [core.UndefinedBinaryOperatorResult]

error_string_size = (size_t) print_count + 1;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92634

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

[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

> I also agree with @NoQ's D92634#2478703 
>  comment.

well.. maybe it's better that I stop programming then and take this code I had 
and test it out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92634

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

[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

> BTW, I cannot optimize function f to returning zero directly with GCC-10.2.1 
> and Clang-10.0.1 under -O3. Should I add any other flags? Or it is version 
> specific?

Yeah I can't reproduce that with latest gcc/clang neither. Try gcc 4.x and gcc 
5.x.

> But for the example you mentioned, I prefer implementing this check with a 
> coding style checker for Clang-Tidy, although the integer overflow checker in 
> the CSA is also necessary.

Yes if that was all we wanted to detect. I'd rather also flag more complex 
examples.. whenever the optimizer can see that there is overflow and has an 
opportunity to destroy the code. Here is a more complex example: 
https://stackoverflow.com/questions/57650026/optimized-clang-handling-overflow

> This patch adds a new core checker. I wouldn't be comfortable enabling it by 
> default without a much more careful evaluation (than a single lit test).

I agree 1000%. The patch I provided so far was only a proof of concept. Thanks 
for all comments I feel I will remake this.

> Not necessarily a new checker but bug reports from this checker are already 
> lacking a lot of information. In this new check both operands should be 
> tracked with visitors. Also in other checks there's a bailout path that 
> produces generic diagnostics ("the result... is undefined" without explaining 
> why); we should probably silence those instead because it's clear that we 
> can't explain what's going on.

Yes I really agree about that. The current warning message is unacceptable and 
it must be clarified.

Sorry I don't make much progress right now.. I hope to have some new code to 
show soon..


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92634

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

[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

> However, the mainstream compilers like GCC and Clang implement this as the 
> overflowed value, and some programmers also use this feature to do some 
> tricky things.

hmm.. you mean if some -fwrapv flag is used right. yes I should disable this 
checking then.

For information, I am not very worried about signed integer overflow. I am 
mostly worried about the compiler optimisations. If the compiler determines 
that there is signed integer overflow in a execution path and removes all code 
in that path.

I was inspired by this blog post:  https://www.airs.com/blog/archives/120

This function:

  int f(int x) { return 0x7ff0 < x && x + 32 < 0x7fff; }

might be optimized to:

  int f(int x) { return 0; }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92634

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

[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

> Besides, the return value should be the exact value computed from the two 
> integers, even unknown, rather than undefined. As the developers may overflow 
> an integer on purpose.

I am not sure what you mean. If there is undefined behavior then the value 
should be undefined and nothing else.. right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92634

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

[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki reclaimed this revision.
danielmarjamaki added a comment.

ok.. thanks for the reviews. I will see if I can make some new check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92634

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

[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki abandoned this revision.
danielmarjamaki added a comment.

No reviews => I will not contribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92634

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

[PATCH] D92634: [Analyzer] Diagnose signed integer overflow

[Daniel Marjamäki via Phabricator via cfe-commits]

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

Diagnose signed integer overflows. The core.UndefResultChecker will warn.

This is a bit unfinished.. I wonder if you like the approach. It only checks 
addition right now. and not underflow.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D92634

Files:
  clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
  clang/test/Analysis/integer-overflow.c

Index: clang/test/Analysis/integer-overflow.c
===
--- /dev/null
+++ clang/test/Analysis/integer-overflow.c
@@ -0,0 +1,7 @@
+// RUN: %clang_analyze_cc1 -analyze -analyzer-checker core.UndefinedBinaryOperatorResult -verify %s
+
+void f(int x) {
+if (x > 0x7f00 && x + 32 < 0x7fff){}
+if (x > 0x7ff0 && x + 32 < 0x7fff){} // expected-warning {{The result of the '+' expression is undefined}}
+}
+
Index: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
===
--- clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
+++ clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
@@ -42,6 +42,8 @@
   SVal evalBinOpLN(ProgramStateRef state, BinaryOperator::Opcode op,
Loc lhs, NonLoc rhs, QualType resultTy) override;
 
+  bool isGreater(ProgramStateRef State, const SymExpr *LHS, int64_t RHS);
+
   /// getKnownValue - evaluates a given SVal. If the SVal has only one possible
   ///  (integer) value, that value is returned. Otherwise, returns NULL.
   const llvm::APSInt *getKnownValue(ProgramStateRef state, SVal V) override;
@@ -51,7 +53,8 @@
   SVal simplifySVal(ProgramStateRef State, SVal V) override;
 
   SVal MakeSymIntVal(const SymExpr *LHS, BinaryOperator::Opcode op,
- const llvm::APSInt &RHS, QualType resultTy);
+ const llvm::APSInt &RHS, QualType resultTy,
+ ProgramStateRef State);
 };
 } // end anonymous namespace
 
@@ -210,14 +213,29 @@
   }
 }
 
+bool SimpleSValBuilder::isGreater(ProgramStateRef State, const SymExpr *LHS,
+  int64_t RHS) {
+  ProgramStateManager &Mgr = State->getStateManager();
+  SValBuilder &Bldr = Mgr.getSValBuilder();
+  SVal Eval =
+  Bldr.evalBinOp(State, BO_GT, nonloc::SymbolVal(LHS),
+ makeIntVal(RHS, LHS->getType()), Bldr.getConditionType());
+  if (Eval.isUnknownOrUndef())
+return false;
+  ProgramStateRef StTrue, StFalse;
+  std::tie(StTrue, StFalse) = State->assume(Eval.castAs());
+  return (StTrue && !StFalse);
+}
+
 //===--===//
 // Transfer function for binary operators.
 //===--===//
 
 SVal SimpleSValBuilder::MakeSymIntVal(const SymExpr *LHS,
-BinaryOperator::Opcode op,
-const llvm::APSInt &RHS,
-QualType resultTy) {
+  BinaryOperator::Opcode op,
+  const llvm::APSInt &RHS,
+  QualType resultTy,
+  ProgramStateRef State) {
   bool isIdempotent = false;
 
   // Check for a few special cases with known reductions first.
@@ -281,6 +299,15 @@
   if (isIdempotent)
   return evalCastFromNonLoc(nonloc::SymbolVal(LHS), resultTy);
 
+  // Is there a signed integer overflow
+  if (LHS->getType()->isSignedIntegerType() && op == BO_Add) {
+int64_t ResultTyMaxVal =
+(1ULL << (getContext().getTypeSize(resultTy) - 1)) - 1;
+if (RHS > 0 && RHS < ResultTyMaxVal &&
+isGreater(State, LHS, ResultTyMaxVal - RHS.getExtValue()))
+  return UndefinedVal();
+  }
+
   // If we reach this point, the expression cannot be simplified.
   // Make a SymbolVal for the entire expression, after converting the RHS.
   const llvm::APSInt *ConvertedRHS = &RHS;
@@ -748,7 +775,7 @@
   }
 
   // Otherwise, make a SymIntExpr out of the expression.
-  return MakeSymIntVal(symIntExpr, op, *RHSValue, resultTy);
+  return MakeSymIntVal(symIntExpr, op, *RHSValue, resultTy, state);
 }
   }
 
@@ -764,7 +791,7 @@
 
   // Is the RHS a constant?
   if (const llvm::APSInt *RHSValue = getKnownValue(state, rhs))
-return MakeSymIntVal(Sym, op, *RHSValue, resultTy);
+return MakeSymIntVal(Sym, op, *RHSValue, resultTy, state);
 
   if (Optional V = tryRearrange(state, op, lhs, rhs, resultTy))
 return *V;
@@ -938,7 +965,7 @@
   

[PATCH] D38718: [Sema] No -Wtautological-pointer-compare warning on variables within parentheses

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki abandoned this revision.
danielmarjamaki added a comment.

Erik and I will not continue working on this. Feel free to take over the patch 
or write a new patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D38718



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

[PATCH] D39049: [analyzer] Fix wrong calculation of offset in ArrayBoundsV2

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki abandoned this revision.
danielmarjamaki added a comment.
Herald added subscribers: llvm-commits, a.sidorin, rnkovacs.

I will not continue working on this. Feel free to take over the patch or write 
a new patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D39049



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

[PATCH] D30489: [analyzer] catch out of bounds for VLA

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki abandoned this revision.
danielmarjamaki added a comment.
Herald added subscribers: llvm-commits, a.sidorin, rnkovacs.

I will not continue working on this. Feel free to take over the patch or write 
a new patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D30489



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

[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki abandoned this revision.
danielmarjamaki added a comment.
Herald added subscribers: llvm-commits, a.sidorin, rnkovacs.

I will not continue working on this. Feel free to take over the patch or write 
a new patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D36471



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

[PATCH] D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki abandoned this revision.
danielmarjamaki added a comment.
Herald added subscribers: llvm-commits, a.sidorin, rnkovacs.

I will not continue working on this. Feel free to take over the patch or write 
a new patch.


Repository:
  rL LLVM

https://reviews.llvm.org/D37897



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

[PATCH] D39049: [analyzer] Fix wrong calculation of offset in ArrayBoundsV2

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

> So what are the arguments that are passed to getSimplifiedOffset() in that 
> case? 0? That does not seem to be correct.

yes.

so the conclusion is:

- this code does not work
- this code is untested
- this code is not even used in the use cases it was intended for because of 
bugs elsewhere

therefore it should be removed.


Repository:
  rL LLVM

https://reviews.llvm.org/D39049



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

[PATCH] D39049: [analyzer] Fix wrong calculation of offset in ArrayBoundsV2

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

> Could you do a similar analysis that I did above to check why does this not 
> work for the multidimensional case? (I.e.: checking what constraints are 
> generated and what the analyzer does with them.)

the "location.dump()" will just say "x".

the ProgramState is:

Expressions:
 (0x2acd12a78a0,0x2acd1478b80) buf : &buf
 (0x2acd12a78a0,0x2acd1478bf8) buf : &element{buf,0 S64b,int [3]}
 (0x2acd12a78a0,0x2acd1478c10) buf[1] : &element{buf,1 S64b,int [3]}
 (0x2acd12a78a0,0x2acd1478c38) x : &x
 (0x2acd12a78a0,0x2acd1478c88) buf[1] : &element{element{buf,1 S64b,int [3]},0 
S64b,int}
Ranges of symbol values:
 reg_$0 : { [4, 10] }

rawOffsetVal => 0

extentBegin => 0

For getSimplifiedOffset() , the offset is not a SymIntExpr it will just return 
0.


Repository:
  rL LLVM

https://reviews.llvm.org/D39049



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

[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 121726.
danielmarjamaki added a comment.
Herald added a subscriber: szepet.

I have updated the patch so it uses evalBinOpNN. This seems to work properly.

I have a number of TODOs in the test cases that should be fixed. Truncations 
are not handled properly.

Here is a short example code:

  void f(unsigned char X) {
if (X >= 10 && X <= 50) {
  unsigned char Y = X + 0x100; // truncation
  clang_analyzer_eval(Y >= 10 && Y <= 50); // expected-warning{{FALSE}}
}
  }

The expected-warning should be TRUE but currently FALSE is written.

When the "Y >= 10" condition is evaluated the ProgramState is:

  Store (direct and default bindings), 0x222ab0fe5f8 :
   (Y,0,direct) : (unsigned char) ((reg_$0) + 256)
  
  Expressions:
   (0x222a96d6050,0x222ab0eb930) X + 256 : (unsigned char) ((reg_$0) + 256)
   (0x222a96d6050,0x222ab0eb960) clang_analyzer_eval : 
&code{clang_analyzer_eval}
   (0x222a96d6050,0x222ab0eb988) Y : &Y
   (0x222a96d6050,0x222ab0eb9d8) Y : (unsigned char) ((reg_$0) 
+ 256)
   (0x222a96d6050,0x222ab0eb9f0) Y : (unsigned char) ((reg_$0) 
+ 256)
   (0x222a96d6050,0x222ab0eba08) Y >= 10 : ((unsigned char) ((reg_$0) + 256)) >= 10
   (0x222a96d6050,0x222ab0ebb28) clang_analyzer_eval : 
&code{clang_analyzer_eval}
  Ranges of symbol values:
   reg_$0 : { [10, 50] }
   (reg_$0) + 256 : { [10, 50] }

It seems to me that the symbol initialization does not handle the range 
properly. Imho there is nothing wrong with the calculation. What you think 
about adding a range like below?

  (unsigned char) ((reg_$0) + 256) : { [10, 50] }




Repository:
  rL LLVM

https://reviews.llvm.org/D36471

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
  include/clang/StaticAnalyzer/Core/PathSensitive/SimpleConstraintManager.h
  lib/StaticAnalyzer/Core/ExprEngineC.cpp
  lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  test/Analysis/range_calc.c

Index: test/Analysis/range_calc.c
===
--- test/Analysis/range_calc.c
+++ test/Analysis/range_calc.c
@@ -0,0 +1,143 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+#define INT_MAX((signed int)((~0U)>>1))
+#define INT_MIN((signed int)(~((~0U)>>1)))
+
+void addInts(int X)
+{
+  if (X >= 10 && X <= 50) {
+int Y = X + 2;
+clang_analyzer_eval(Y >= 12 && Y <= 52); // expected-warning{{TRUE}}
+  }
+  
+  if (X < 5) {
+int Y = X + 1;
+clang_analyzer_eval(Y < 6); // expected-warning{{TRUE}}
+  }
+
+  if (X >= 1000) {
+int Y = X + 1; // might overflow
+clang_analyzer_eval(Y >= 1001); // expected-warning{{UNKNOWN}}
+clang_analyzer_eval(Y == INT_MIN); // expected-warning{{UNKNOWN}}
+	clang_analyzer_eval(Y == INT_MIN || Y >= 1001); // expected-warning{{TRUE}}
+  }
+}
+
+void addU8(unsigned char X)
+{
+  if (X >= 10 && X <= 50) {
+unsigned char Y = X + 2;
+clang_analyzer_eval(Y >= 12 && Y <= 52); // expected-warning{{TRUE}}
+  }
+  
+  if (X < 5) {
+unsigned char Y = X + 1;
+clang_analyzer_eval(Y < 6); // expected-warning{{TRUE}}
+  }
+
+  // TODO
+  if (X >= 10 && X <= 50) {
+unsigned char Y = X + (-256); // truncation
+clang_analyzer_eval(Y >= 10 && Y <= 50); // expected-warning{{FALSE}}
+  }
+
+  // TODO
+  if (X >= 10 && X <= 50) {
+unsigned char Y = X + 256; // truncation
+clang_analyzer_eval(Y >= 10 && Y <= 50); // expected-warning{{FALSE}} expected-warning{{UNKNOWN}}
+  }
+
+  // TODO
+  if (X >= 100) {
+unsigned char Y = X + 1; // truncation
+	clang_analyzer_eval(Y == 0); // expected-warning{{FALSE
+	clang_analyzer_eval(Y >= 101); // expected-warning{{TRUE}}
+clang_analyzer_eval(Y == 0 || Y >= 101); // expected-warning{{TRUE}}
+  }
+
+  if (X >= 100) {
+unsigned short Y = X + 1;
+clang_analyzer_eval(Y >= 101 && Y <= 256); // expected-warning{{TRUE}}
+  }
+}
+
+void sub1(int X)
+{
+  if (X >= 10 && X <= 50) {
+int Y = X - 2;
+clang_analyzer_eval(Y >= 8 && Y <= 48); // expected-warning{{TRUE}}
+  }
+
+  if (X >= 10 && X <= 50) {
+unsigned char Y = (unsigned int)X - 20; // truncation
+clang_analyzer_eval(Y <= 30 || Y >= 246); // expected-warning{{TRUE}}
+  }
+
+  // TODO
+  if (X >= 10 && X <= 50) {
+unsigned char Y = (unsigned int)X - 256; // truncation
+clang_analyzer_eval(Y >= 10 && Y <= 50); // expected-warning{{FALSE}} expected-warning{{UNKNOWN}}
+  }
+
+  if (X < 5) {
+int Y = X - 1; // might overflow
+clang_analyzer_eval(Y < 4); // expected-warning{{UNKNOWN}}
+clang_analyzer_eval(Y == INT_MAX); // expected-warning{{UNKNOWN}}
+	clang_analyzer_eval(Y == INT_MAX || Y < 4); // expected-warning{{TRUE}}
+  }
+
+  if (X >= 1000) {
+int Y = X - 1;
+clang_analyzer_eval(Y >= 999); // expected-warning{{TRUE}}
+  }
+}
+
+void subU8(unsigned char X)
+{
+  if (X >= 10 && X <= 50) {
+unsigned char Y = X - 2;
+clang_analyzer_eval(Y >= 

[PATCH] D39049: [analyzer] Fix wrong calculation of offset in ArrayBoundsV2

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

> Do you mind writing some tests with multidimensional arrays to check what do 
> we lose if we remove that code?

I have spent a few hours trying to write a test case that shows there is false 
negatives caused by this change. And fail.

I see lots of false negatives for multidimensional arrays with or without this 
code.

For instance:

  void f(int x) {
int buf[2][3];
if (x < 4 || x>10)
  return;
buf[1][x] = 0;
  }


Repository:
  rL LLVM

https://reviews.llvm.org/D39049



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

[PATCH] D30489: [analyzer] catch out of bounds for VLA

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 119590.
danielmarjamaki added a comment.
Herald added a subscriber: szepet.

As suggested, use a ProgramState trait to detect VLA overflows.

I did not yet manage to get a SubRegion from the DeclStmt that matches the 
location SubRegion. Therefore I am using VariableArrayType in the trait for now.


Repository:
  rL LLVM

https://reviews.llvm.org/D30489

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

Index: test/Analysis/out-of-bounds.c
===
--- test/Analysis/out-of-bounds.c
+++ test/Analysis/out-of-bounds.c
@@ -174,3 +174,7 @@
   clang_analyzer_eval(x <= 99); // expected-warning{{TRUE}}
 }
 
+void vla(int X) {
+  char buf[X];
+  buf[X] = 0; // expected-warning {{Out of bound memory access (access exceeds upper limit of memory block)}}
+}
Index: lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
===
--- lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -27,17 +27,18 @@
 using namespace ento;
 
 namespace {
-class ArrayBoundCheckerV2 :
-public Checker {
+class ArrayBoundCheckerV2
+: public Checker> {
   mutable std::unique_ptr BT;
 
   enum OOB_Kind { OOB_Precedes, OOB_Excedes, OOB_Tainted };
 
   void reportOOB(CheckerContext &C, ProgramStateRef errorState,
  OOB_Kind kind) const;
 
 public:
-  void checkLocation(SVal l, bool isLoad, const Stmt*S,
+  void checkPreStmt(const DeclStmt *DS, CheckerContext &C) const;
+  void checkLocation(SVal l, bool isLoad, const Stmt *S,
  CheckerContext &C) const;
 };
 
@@ -64,7 +65,10 @@
   void dump() const;
   void dumpToStream(raw_ostream &os) const;
 };
-}
+} // namespace
+
+REGISTER_MAP_WITH_PROGRAMSTATE(VariableLengthArrayExtent,
+   const VariableArrayType *, NonLoc);
 
 static SVal computeExtentBegin(SValBuilder &svalBuilder,
const MemRegion *region) {
@@ -111,8 +115,46 @@
   return std::pair(offset, extent);
 }
 
+void ArrayBoundCheckerV2::checkPreStmt(const DeclStmt *DS,
+   CheckerContext &checkerContext) const {
+  const VarDecl *VD = dyn_cast(DS->getSingleDecl());
+  if (!VD)
+return;
+
+  ASTContext &Ctx = checkerContext.getASTContext();
+  const VariableArrayType *VLA = Ctx.getAsVariableArrayType(VD->getType());
+  if (!VLA)
+return;
+
+  SVal VLASize = checkerContext.getSVal(VLA->getSizeExpr());
+
+  Optional Sz = VLASize.getAs();
+  if (!Sz)
+return;
+
+  ProgramStateRef state = checkerContext.getState();
+  checkerContext.addTransition(
+  state->set(VLA, Sz.getValue()));
+}
+
+static const VariableArrayType *getVLAFromExtent(DefinedOrUnknownSVal extentVal,
+ ASTContext &Ctx) {
+  if (extentVal.getSubKind() != nonloc::SymbolValKind)
+return nullptr;
+
+  SymbolRef SR = extentVal.castAs().getSymbol();
+  const SymbolExtent *SE = dyn_cast(SR);
+  const MemRegion *SEMR = SE->getRegion();
+  if (SEMR->getKind() != MemRegion::VarRegionKind)
+return nullptr;
+
+  const VarRegion *VR = cast(SEMR);
+  QualType T = VR->getDecl()->getType();
+  return Ctx.getAsVariableArrayType(T);
+}
+
 void ArrayBoundCheckerV2::checkLocation(SVal location, bool isLoad,
-const Stmt* LoadS,
+const Stmt *LoadS,
 CheckerContext &checkerContext) const {
 
   // NOTE: Instead of using ProgramState::assumeInBound(), we are prototyping
@@ -175,13 +217,21 @@
   }
 
   do {
+
 // CHECK UPPER BOUND: Is byteOffset >= extent(baseRegion)?  If so,
 // we are doing a load/store after the last valid offset.
 DefinedOrUnknownSVal extentVal =
-  rawOffset.getRegion()->getExtent(svalBuilder);
+rawOffset.getRegion()->getExtent(svalBuilder);
 if (!extentVal.getAs())
   break;
 
+if (const VariableArrayType *VLA =
+getVLAFromExtent(extentVal, checkerContext.getASTContext())) {
+  const NonLoc *V = state->get(VLA);
+  if (V)
+extentVal = *V;
+}
+
 if (extentVal.getAs()) {
   std::pair simplifiedOffsets =
   getSimplifiedOffsets(rawOffset.getByteOffset(),
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39015: [Analyzer] Always use non-reference types when creating expressions in BodyFarm, removes std::call_once crash

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added inline comments.



Comment at: cfe/trunk/lib/Analysis/BodyFarm.cpp:139
 
-DeclRefExpr *ASTMaker::makeDeclRefExpr(const VarDecl *D,
-   bool RefersToEnclosingVariableOrCapture,
-   bool GetNonReferenceType) {
-  auto Type = D->getType();
-  if (GetNonReferenceType)
-Type = Type.getNonReferenceType();
+DeclRefExpr *ASTMaker::makeDeclRefExpr(
+const VarDecl *D,

this looks strange, did clang-format do this?


Repository:
  rL LLVM

https://reviews.llvm.org/D39015



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

[PATCH] D38986: [Analyzer] Better unreachable message in enumeration

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

> I think it is much better when the assert failure tells the developer _what_ 
> value is failing, rather than saying "oops we are dead".

yes of course, more informative assert messages is better.


https://reviews.llvm.org/D38986



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

[PATCH] D35109: [Analyzer] SValBuilder Comparison Rearrangement

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

I like this patch overall.. here are some stylistic nits.




Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:610
+} else {
+  if (*lInt >= *rInt) {
+newRhsExt = lInt->getExtValue() - rInt->getExtValue();

you can use `else if` here



Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:595
+
+  if (origWidth < 128) {
+auto newWidth = std::max(2 * origWidth, (uint32_t) 8);

I would like that "128" is rewritten somehow. Using expression instead.



Comment at: lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:596
+  if (origWidth < 128) {
+auto newWidth = std::max(2 * origWidth, (uint32_t) 8);
+auto newAPSIntType = APSIntType(newWidth, false);

Is `origWidth < 4` possible?

I wonder about "8". Is that CHAR_BIT hardcoded?


https://reviews.llvm.org/D35109



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

[PATCH] D39031: [Analyzer] Correctly handle parameters passed by reference when bodyfarming std::call_once

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a reviewer: danielmarjamaki.
danielmarjamaki added a comment.

Stylistically this looks pretty good to me. Just a minor nit.




Comment at: lib/Analysis/BodyFarm.cpp:389
+  for (unsigned int i = 2; i < D->getNumParams(); i++) {
+
+const ParmVarDecl *PDecl = D->getParamDecl(i);

ehm.. I would remove this blank


https://reviews.llvm.org/D39031



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

[PATCH] D39049: [analyzer] Fix wrong calculation of offset in ArrayBoundsV2

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki created this revision.
Herald added a subscriber: szepet.

Example code:

  void test3_simplified_offset(int x, unsigned long long y) {
int buf[100];
if (x < 0)
  x = 0;
for (int i = y - x; i > 0 && i < 100; i++)
  buf[i] = 0; // no-warning
  }

Without this patch Clang will wrongly report this FP:

  File out-of-bounds.c Line 144: Out of bound memory access (accessed memory 
precedes memory block)

There is some bug in the getSimplifiedOffsets() calculations. I removed the 
wrong calculations and this does not break any existing tests so either no 
tests were written in the first place or these calculations got redundant 
sometime. If somebody wants to readd the calculations that I remove.. I am not 
against that if some tests are added and it does not break my test.


Repository:
  rL LLVM

https://reviews.llvm.org/D39049

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


Index: test/Analysis/out-of-bounds.c
===
--- test/Analysis/out-of-bounds.c
+++ test/Analysis/out-of-bounds.c
@@ -136,6 +136,14 @@
 buf[x] = 1; // expected-warning{{Out of bound memory access}}
 }
 
+void test3_simplified_offset(int x, unsigned long long y) {
+  int buf[100];
+  if (x < 0)
+x = 0;
+  for (int i = y - x; i > 0 && i < 100; i++)
+buf[i] = 0; // no-warning
+}
+
 void test4(int x) {
   int buf[100];
   if (x > 99)
Index: lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
===
--- lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -98,10 +98,6 @@
   nonloc::SymbolVal(SIE->getLHS()),
   svalBuilder.makeIntVal(extent.getValue() / constant),
   svalBuilder);
-  case BO_Add:
-return getSimplifiedOffsets(
-nonloc::SymbolVal(SIE->getLHS()),
-svalBuilder.makeIntVal(extent.getValue() - constant), svalBuilder);
   default:
 break;
   }


Index: test/Analysis/out-of-bounds.c
===
--- test/Analysis/out-of-bounds.c
+++ test/Analysis/out-of-bounds.c
@@ -136,6 +136,14 @@
 buf[x] = 1; // expected-warning{{Out of bound memory access}}
 }
 
+void test3_simplified_offset(int x, unsigned long long y) {
+  int buf[100];
+  if (x < 0)
+x = 0;
+  for (int i = y - x; i > 0 && i < 100; i++)
+buf[i] = 0; // no-warning
+}
+
 void test4(int x) {
   int buf[100];
   if (x > 99)
Index: lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
===
--- lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -98,10 +98,6 @@
   nonloc::SymbolVal(SIE->getLHS()),
   svalBuilder.makeIntVal(extent.getValue() / constant),
   svalBuilder);
-  case BO_Add:
-return getSimplifiedOffsets(
-nonloc::SymbolVal(SIE->getLHS()),
-svalBuilder.makeIntVal(extent.getValue() - constant), svalBuilder);
   default:
 break;
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38801: [analyzer] In getSVal() API, disable auto-detection of void type as char type.

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

LGTM


https://reviews.llvm.org/D38801



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

[PATCH] D38921: [analyzer] LoopUnrolling: update the matched assignment operators

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

LGTM.. however I would like approval from somebody else also.


https://reviews.llvm.org/D38921



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

[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

ping


Repository:
  rL LLVM

https://reviews.llvm.org/D36471



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

[PATCH] D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added inline comments.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:123
+  // Is variable changed anywhere in TU?
+  for (const Decl *D : AMgr.getASTContext().getTranslationUnitDecl()->decls()) 
{
+if (isChanged(D, VD))

danielmarjamaki wrote:
> danielmarjamaki wrote:
> > dcoughlin wrote:
> > > Since you are calling `getInitialStateForGlobalStaticVar()` in 
> > > `getInitialState()` for each static variable declaration and 
> > > `getInitialState()` is called for each top-level function, you are doing 
> > > an n^3 operation in the size of the translation unit, which is going to 
> > > be very, very expensive for large translation units.
> > > 
> > > Have you considered doing the analysis for static variables that are 
> > > never changed during call-graph construction? This should be a linear 
> > > operation and doing it during call-graph construction would avoid an 
> > > extra walk of the entire translation unit.
> > hmm... could you tell me where the call-graph construction is that I can 
> > tweak?
> I think I found it: `clang/lib/Analysis/CallGraph.cpp`
I now track variable modifications in call-graph construction instead.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:155
+  Children.push(FuncBody);
+  while (!Children.empty()) {
+const Stmt *Child = Children.top();

szepet wrote:
> I think instead of this logic it would be better to use ConstStmtVisitor. In 
> this case it does quite the same thing in a (maybe?) more structured manner. 
> What do you think?
As far as I see ConstStmtVisitor is also recursive. Imho let's have readable 
code instead..


Repository:
  rL LLVM

https://reviews.llvm.org/D37897



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

[PATCH] D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 118947.
danielmarjamaki added a comment.

Track modification of global static variables in CallGraph construction


Repository:
  rL LLVM

https://reviews.llvm.org/D37897

Files:
  include/clang/AST/Decl.h
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/AST/Decl.cpp
  lib/Analysis/CallGraph.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  test/Analysis/global-vars.c

Index: test/Analysis/global-vars.c
===
--- test/Analysis/global-vars.c
+++ test/Analysis/global-vars.c
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha,core -verify %s
+
+// Avoid false positive.
+static char *allv[] = {"rpcgen", "-s", "udp", "-s", "tcp"};
+static int allc = sizeof(allv) / sizeof(allv[0]);
+static void falsePositive1(void) {
+  int i;
+  for (i = 1; i < allc; i++) {
+const char *p = allv[i]; // no-warning
+i++;
+  }
+}
+
+// Detect division by zero.
+static int zero = 0;
+void truePositive1() {
+  int x = 1000 / zero; // expected-warning{{Division by zero}}
+}
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -103,8 +103,71 @@
 // Utility methods.
 //===--===//
 
+/** Get initial state for global static variable */
+ProgramStateRef ExprEngine::getInitialStateForGlobalStaticVar(
+const LocationContext *LCtx, ProgramStateRef State, const VarDecl *VD) {
+  // Is variable changed anywhere in TU?
+  if (VD->isModified())
+return State;
+
+  // What is the initialized value?
+  llvm::APSInt InitVal;
+  if (const Expr *I = VD->getInit()) {
+if (!I->EvaluateAsInt(InitVal, getContext()))
+  return State;
+  } else {
+InitVal = 0;
+  }
+
+  const MemRegion *R = State->getRegion(VD, LCtx);
+  if (!R)
+return State;
+  SVal V = State->getSVal(loc::MemRegionVal(R));
+  SVal Constraint_untested =
+  evalBinOp(State, BO_EQ, V, svalBuilder.makeIntVal(InitVal),
+svalBuilder.getConditionType());
+  Optional Constraint =
+  Constraint_untested.getAs();
+  if (!Constraint)
+return State;
+  return State->assume(*Constraint, true);
+}
+
+static void getGlobalStaticVars(const Stmt *FuncBody,
+llvm::SmallSet *Vars) {
+  std::stack Children;
+  Children.push(FuncBody);
+  while (!Children.empty()) {
+const Stmt *Child = Children.top();
+Children.pop();
+if (!Child)
+  continue;
+for (const Stmt *C : Child->children()) {
+  Children.push(C);
+}
+if (const DeclRefExpr *D = dyn_cast(Child)) {
+  const VarDecl *VD = dyn_cast(D->getDecl());
+  if (VD && VD->isDefinedOutsideFunctionOrMethod() &&
+  VD->getType()->isIntegerType() &&
+  VD->getStorageClass() == SC_Static &&
+  !VD->getType()->isPointerType()) {
+Vars->insert(VD);
+  }
+}
+  }
+}
+
 ProgramStateRef ExprEngine::getInitialState(const LocationContext *InitLoc) {
   ProgramStateRef state = StateMgr.getInitialState(InitLoc);
+  // Get initial states for static global variables.
+  if (const auto *FD = dyn_cast(InitLoc->getDecl())) {
+llvm::SmallSet Vars;
+getGlobalStaticVars(FD->getBody(), &Vars);
+for (const VarDecl *VD : Vars) {
+  state = getInitialStateForGlobalStaticVar(InitLoc, state, VD);
+}
+  }
+
   const Decl *D = InitLoc->getDecl();
 
   // Preconditions.
Index: lib/Analysis/CallGraph.cpp
===
--- lib/Analysis/CallGraph.cpp
+++ lib/Analysis/CallGraph.cpp
@@ -33,10 +33,12 @@
   CallGraphNode *CallerNode;
 
 public:
-  CGBuilder(CallGraph *g, CallGraphNode *N)
-: G(g), CallerNode(N) {}
+  CGBuilder(CallGraph *g, CallGraphNode *N) : G(g), CallerNode(N) {}
 
-  void VisitStmt(Stmt *S) { VisitChildren(S); }
+  void VisitStmt(Stmt *S) {
+markModifiedVars(S);
+VisitChildren(S);
+  }
 
   Decl *getDeclFromCall(CallExpr *CE) {
 if (FunctionDecl *CalleeDecl = CE->getDirectCallee())
@@ -63,13 +65,14 @@
 if (Decl *D = getDeclFromCall(CE))
   addCalledDecl(D);
 VisitChildren(CE);
+markModifiedVars(CE);
   }
 
   // Adds may-call edges for the ObjC message sends.
   void VisitObjCMessageExpr(ObjCMessageExpr *ME) {
 if (ObjCInterfaceDecl *IDecl = ME->getReceiverInterface()) {
   Selector Sel = ME->getSelector();
-  
+
   // Find the callee definition within the same translation unit.
   Decl *D = nullptr;
   if (ME->isInstanceMessage())
@@ -88,6 +91,53 @@
   if (SubStmt)
 this->Visit(SubStmt);
   }
+
+  void modifyVar(Expr *E) {
+auto *D = dyn_cast(E->IgnoreParenCasts());
+if (!D)
+  return;
+VarDecl *VD = dyn_cast(D->getDecl());
+if (VD)
+  VD->setModified();
+  }
+
+  void markModifiedVars(Stmt *

[PATCH] D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added inline comments.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:123
+  // Is variable changed anywhere in TU?
+  for (const Decl *D : AMgr.getASTContext().getTranslationUnitDecl()->decls()) 
{
+if (isChanged(D, VD))

danielmarjamaki wrote:
> dcoughlin wrote:
> > Since you are calling `getInitialStateForGlobalStaticVar()` in 
> > `getInitialState()` for each static variable declaration and 
> > `getInitialState()` is called for each top-level function, you are doing an 
> > n^3 operation in the size of the translation unit, which is going to be 
> > very, very expensive for large translation units.
> > 
> > Have you considered doing the analysis for static variables that are never 
> > changed during call-graph construction? This should be a linear operation 
> > and doing it during call-graph construction would avoid an extra walk of 
> > the entire translation unit.
> hmm... could you tell me where the call-graph construction is that I can 
> tweak?
I think I found it: `clang/lib/Analysis/CallGraph.cpp`


Repository:
  rL LLVM

https://reviews.llvm.org/D37897



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

[PATCH] D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added inline comments.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:123
+  // Is variable changed anywhere in TU?
+  for (const Decl *D : AMgr.getASTContext().getTranslationUnitDecl()->decls()) 
{
+if (isChanged(D, VD))

dcoughlin wrote:
> Since you are calling `getInitialStateForGlobalStaticVar()` in 
> `getInitialState()` for each static variable declaration and 
> `getInitialState()` is called for each top-level function, you are doing an 
> n^3 operation in the size of the translation unit, which is going to be very, 
> very expensive for large translation units.
> 
> Have you considered doing the analysis for static variables that are never 
> changed during call-graph construction? This should be a linear operation and 
> doing it during call-graph construction would avoid an extra walk of the 
> entire translation unit.
hmm... could you tell me where the call-graph construction is that I can tweak?


Repository:
  rL LLVM

https://reviews.llvm.org/D37897



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

[PATCH] D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

In https://reviews.llvm.org/D37897#892667, @dcoughlin wrote:

> Apologies for the delay reviewing! As I noted inline, I'm pretty worried 
> about the performance impact of this. Is it possible to do the analysis in a 
> single traversal of the translation unit?


I agree. I first tried more like that but ran into problems. Don't remember the 
details. I will try again.. however as far as I see this will mean the 
LoopUnroller AST matchers can't be reused unless I change them.


Repository:
  rL LLVM

https://reviews.llvm.org/D37897



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

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL315462: [Analyzer] Clarify error messages for undefined 
result (authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D30295?vs=116865&id=118620#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D30295

Files:
  cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  cfe/trunk/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  cfe/trunk/lib/StaticAnalyzer/Core/CheckerContext.cpp
  cfe/trunk/test/Analysis/bitwise-ops.c

Index: cfe/trunk/lib/StaticAnalyzer/Core/CheckerContext.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/CheckerContext.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/CheckerContext.cpp
@@ -99,3 +99,35 @@
   return Lexer::getSpelling(Loc, buf, getSourceManager(), getLangOpts());
 }
 
+/// Evaluate comparison and return true if it's known that condition is true
+static bool evalComparison(SVal LHSVal, BinaryOperatorKind ComparisonOp,
+   SVal RHSVal, ProgramStateRef State) {
+  if (LHSVal.isUnknownOrUndef())
+return false;
+  ProgramStateManager &Mgr = State->getStateManager();
+  if (!LHSVal.getAs()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+  LHSVal.castAs());
+if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+  return false;
+  }
+
+  SValBuilder &Bldr = Mgr.getSValBuilder();
+  SVal Eval = Bldr.evalBinOp(State, ComparisonOp, LHSVal, RHSVal,
+ Bldr.getConditionType());
+  if (Eval.isUnknownOrUndef())
+return false;
+  ProgramStateRef StTrue, StFalse;
+  std::tie(StTrue, StFalse) = State->assume(Eval.castAs());
+  return StTrue && !StFalse;
+}
+
+bool CheckerContext::isGreaterOrEqual(const Expr *E, unsigned long long Val) {
+  DefinedSVal V = getSValBuilder().makeIntVal(Val, getASTContext().LongLongTy);
+  return evalComparison(getSVal(E), BO_GE, V, getState());
+}
+
+bool CheckerContext::isNegative(const Expr *E) {
+  DefinedSVal V = getSValBuilder().makeIntVal(0, false);
+  return evalComparison(getSVal(E), BO_LT, V, getState());
+}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
@@ -123,57 +123,6 @@
   C.emitReport(std::move(R));
 }
 
-// Is E value greater or equal than Val?
-static bool isGreaterEqual(CheckerContext &C, const Expr *E,
-   unsigned long long Val) {
-  ProgramStateRef State = C.getState();
-  SVal EVal = C.getSVal(E);
-  if (EVal.isUnknownOrUndef())
-return false;
-  if (!EVal.getAs() && EVal.getAs()) {
-ProgramStateManager &Mgr = C.getStateManager();
-EVal =
-Mgr.getStoreManager().getBinding(State->getStore(), EVal.castAs());
-  }
-  if (EVal.isUnknownOrUndef() || !EVal.getAs())
-return false;
-
-  SValBuilder &Bldr = C.getSValBuilder();
-  DefinedSVal V = Bldr.makeIntVal(Val, C.getASTContext().LongLongTy);
-
-  // Is DefinedEVal greater or equal with V?
-  SVal GE = Bldr.evalBinOp(State, BO_GE, EVal, V, Bldr.getConditionType());
-  if (GE.isUnknownOrUndef())
-return false;
-  ConstraintManager &CM = C.getConstraintManager();
-  ProgramStateRef StGE, StLT;
-  std::tie(StGE, StLT) = CM.assumeDual(State, GE.castAs());
-  return StGE && !StLT;
-}
-
-// Is E value negative?
-static bool isNegative(CheckerContext &C, const Expr *E) {
-  ProgramStateRef State = C.getState();
-  SVal EVal = State->getSVal(E, C.getLocationContext());
-  if (EVal.isUnknownOrUndef() || !EVal.getAs())
-return false;
-  DefinedSVal DefinedEVal = EVal.castAs();
-
-  SValBuilder &Bldr = C.getSValBuilder();
-  DefinedSVal V = Bldr.makeIntVal(0, false);
-
-  SVal LT =
-  Bldr.evalBinOp(State, BO_LT, DefinedEVal, V, Bldr.getConditionType());
-
-  // Is E value greater than MaxVal?
-  ConstraintManager &CM = C.getConstraintManager();
-  ProgramStateRef StNegative, StPositive;
-  std::tie(StNegative, StPositive) =
-  CM.assumeDual(State, LT.castAs());
-
-  return StNegative && !StPositive;
-}
-
 bool ConversionChecker::isLossOfPrecision(const ImplicitCastExpr *Cast,
   QualType DestType,
   CheckerContext &C) const {
@@ -195,18 +144,18 @@
 return false;
 
   unsigned long long MaxVal = 1ULL << W;
-  return isGreaterEqual(C, Cast->getSubExpr(), MaxVal);
+  return C.isGreaterOrEqual(Cast->getSubExpr(), MaxVal);
 }
 
 bool ConversionChecker::isLossOfSign(const ImplicitCastExpr *Cast,
-   CheckerContext &C) const {
+ CheckerContext &C) const {
   QualType CastType = Cast->getType();
   Q

[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

ping


Repository:
  rL LLVM

https://reviews.llvm.org/D36471



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

[PATCH] D38718: [Sema] No -Wtautological-pointer-compare warning on variables within parentheses

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

LGTM! However I would like to see a review from somebody else also.

There are a number of diagnostics that might be affected. The 
Sema::DiagnoseAlwaysNonNullPointer diagnoses these:

diag::warn_this_null_compare
diag::warn_this_bool_conversion
diag::warn_address_of_reference_null_compare
diag::warn_address_of_reference_bool_conversion
diag::warn_nonnull_expr_compare
diag::warn_cast_nonnull_to_bool
diag::warn_null_pointer_compare   <-- I think this is the one bug 20951 is about
diag::warn_impcast_pointer_to_bool

It seems to me that it is an improvement for all these warnings to skip the 
parentheses. However there is a danger that parentheses should hide some 
warnings to make it possible for users to hide unwanted warnings. But if that 
was the design decision then some regression test should complain when we skip 
the parentheses.


Repository:
  rL LLVM

https://reviews.llvm.org/D38718



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

[PATCH] D38718: Patch to Bugzilla 20951

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a reviewer: danielmarjamaki.
danielmarjamaki added a comment.

I think a test for -Wtautological-pointer-compare should be added that shows 
that the bug is fixed.




Comment at: test/Sema/conditional-expr.c:84
+  //char x;
+  return (((x != ((void *) 0)) ? (*x = ((char) 1)) : (void) ((void *) 0)), 
(unsigned long) ((void *) 0)); // expected-warning {{C99 forbids conditional 
expressions with only one void side}}
 }

lebedev.ri wrote:
> Please don't just remove previous tests.
> E.g. does the old test no longer warns?
> 
no test is removed. The expected-warning is unchanged.

the problem with the test was that this comparison is always true:
```
(&x) != ((void *)0)
```
the address of x is never 0!

Fixing https://bugs.llvm.org/show_bug.cgi?id=20951 means Clang will warn:
```
warning: comparison of address of 'x' not equal to a null pointer is always 
true [-Wtautological-pointer-compare]
```

We changed the test so the -Wtautological-pointer-compare is not reported... 
but the original warning is still reported.



https://reviews.llvm.org/D38718



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

[PATCH] D38675: [analyzer] MisusedMovedObjectChecker: Moving the checker out of alpha state

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

In https://reviews.llvm.org/D38675#891912, @xazax.hun wrote:

> In https://reviews.llvm.org/D38675#891750, @danielmarjamaki wrote:
>
> > > However, the checker seems to work with a low false positive rate.  (<15 
> > > on the LLVM, 6 effectively different)
> >
> > This does not sound like a low false positive rate to me. Could you 
> > describe what the false positives are? Is it possible to fix them?
>
>
> Note that the unique findings are 6. I think there are non-alpha checks with 
> more false positives.


This does not answer the important questions. What are the false positives, and 
is it possible to fix them?

In https://reviews.llvm.org/D38675#891750, @danielmarjamaki wrote:

>




>> Is it enough or should I check it on other open source projects?
> 
> you should check a number of different projects. There might be idioms/usages 
> in other projects that are not seen in LLVM.
> 
> However I don't know what other open source C++11 projects there are.
> 
> But I have a script that runs clang on various Debian projects and I can run 
> that and provide you with the results.

Something didn't go well. I started my script yesterday afternoon. It has 
checked 426 projects so far. I do not see a single MisusedMovedObject warning. 
I am thinking that my script doesn't work well in this case. A wild idea is 
that maybe my script must somehow tell scan-build to use -std=c++11.

For information here are the cppcheck results for a similar checker:
http://cppcheck.sourceforge.net/cgi-bin/daca2-search.cgi?id=accessMoved

Maybe you could pick a few of those projects and check those. That should 
exercise this checker.

Please tell me if you think some of the accessMoved warnings are false 
positives. These warnings are "inconclusive" which indicates they might not 
always be correct.


https://reviews.llvm.org/D38675



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

[PATCH] D38674: [analyzer] MisusedMovedObjectChecker: More precise warning message

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

LGTM


https://reviews.llvm.org/D38674



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

[PATCH] D38675: [analyzer] MisusedMovedObjectChecker: Moving the checker out of alpha state

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a reviewer: danielmarjamaki.
danielmarjamaki added a comment.

> However, the checker seems to work with a low false positive rate.  (<15 on 
> the LLVM, 6 effectively different)

This does not sound like a low false positive rate to me. Could you describe 
what the false positives are? Is it possible to fix them?

> Is it enough or should I check it on other open source projects?

you should check a number of different projects. There might be idioms/usages 
in other projects that are not seen in LLVM.

However I don't know what other open source C++11 projects there are.

But I have a script that runs clang on various Debian projects and I can run 
that and provide you with the results.


https://reviews.llvm.org/D38675



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

[PATCH] D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki marked an inline comment as done.
danielmarjamaki added inline comments.



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:107
+/** Recursively check if variable is changed in code. */
+static bool isChanged(const Stmt *S, const VarDecl *VD, bool Write) {
+  if (!S)

xazax.hun wrote:
> Usually, we do not like bug recursions since it might eat up the stack. 
> Also, do you consider the case when the variable is passed by reference to a 
> function in another translation unit? 
I rewrote the code to reuse the ast matchers in LoopUnroll.. and that is not 
recursive. I rewrote the getGlobalStaticVars (this code is longer and in my 
opinion less readable but it's not recursive).



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:169
+  SVal Constraint_untested =
+  evalBinOp(State, BO_EQ, V, svalBuilder.makeIntVal(InitVal),
+svalBuilder.getConditionType());

xazax.hun wrote:
> Does this work for non-integer typed e.g. structs? 
Currently static struct objects are unaffected by these changes. There is no 
regression.

Reviews are taking too long time. I am not against getting reviews but I am 
against waiting for reviews for months, ideally people would only need to wait 
for at most a week to get a review. This is why I don't want to handle structs 
now -- I am not eager to prolong the review process for this patch.



Repository:
  rL LLVM

https://reviews.llvm.org/D37897



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

[PATCH] D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 117956.
danielmarjamaki added a comment.
Herald added a subscriber: szepet.

Fixes according to review comments. Reuse ast matchers in LoopUnrolling.cpp. 
Avoid some recursion (however the isChanged() is still recursive but it is very 
small and simple).


Repository:
  rL LLVM

https://reviews.llvm.org/D37897

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  include/clang/StaticAnalyzer/Core/PathSensitive/LoopUnrolling.h
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  test/Analysis/global-vars.c

Index: test/Analysis/global-vars.c
===
--- test/Analysis/global-vars.c
+++ test/Analysis/global-vars.c
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha,core -verify %s
+
+// Avoid false positive.
+static char *allv[] = {"rpcgen", "-s", "udp", "-s", "tcp"};
+static int allc = sizeof(allv) / sizeof(allv[0]);
+static void falsePositive1(void) {
+  int i;
+  for (i = 1; i < allc; i++) {
+const char *p = allv[i]; // no-warning
+i++;
+  }
+}
+
+// Detect division by zero.
+static int zero = 0;
+void truePositive1() {
+  int x = 1000 / zero; // expected-warning{{Division by zero}}
+}
Index: lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -99,7 +99,10 @@
 declRefExpr(to(varDecl(VarNodeMatcher)),
   binaryOperator(anyOf(hasOperatorName("="), hasOperatorName("+="),
hasOperatorName("/="), hasOperatorName("*="),
-   hasOperatorName("-=")),
+   hasOperatorName("-="), hasOperatorName("%="),
+   hasOperatorName("<<="), hasOperatorName(">>="),
+   hasOperatorName("&="), hasOperatorName("|="),
+   hasOperatorName("^=")),
  hasLHS(ignoringParenImpCasts(
  declRefExpr(to(varDecl(VarNodeMatcher)));
 }
@@ -283,5 +286,16 @@
 return false;
   return true;
 }
+
+bool isVarChanged(const FunctionDecl *FD, const VarDecl *VD) {
+  if (!FD->getBody())
+return false;
+  auto Match = match(
+  stmt(hasDescendant(stmt(anyOf(
+  callByRef(equalsNode(VD)), getAddrTo(equalsNode(VD)),
+  assignedToRef(equalsNode(VD)), changeIntBoundNode(equalsNode(VD)),
+  *FD->getBody(), FD->getASTContext());
+  return !Match.empty();
 }
-}
+} // namespace ento
+} // namespace clang
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -103,8 +103,86 @@
 // Utility methods.
 //===--===//
 
+static bool isChanged(const Decl *D, const VarDecl *VD) {
+  if (const auto *FD = dyn_cast(D)) {
+return isVarChanged(FD, VD);
+  }
+  if (const auto *Rec = dyn_cast(D)) {
+for (const auto *RecChild : Rec->decls()) {
+  if (isChanged(RecChild, VD))
+return true;
+}
+  }
+  return false;
+}
+
+/** Get initial state for global static variable */
+ProgramStateRef ExprEngine::getInitialStateForGlobalStaticVar(
+const LocationContext *LCtx, ProgramStateRef State, const VarDecl *VD) {
+  // Is variable changed anywhere in TU?
+  for (const Decl *D : AMgr.getASTContext().getTranslationUnitDecl()->decls()) {
+if (isChanged(D, VD))
+  return State;
+  }
+
+  // What is the initialized value?
+  llvm::APSInt InitVal;
+  if (const Expr *I = VD->getInit()) {
+if (!I->EvaluateAsInt(InitVal, getContext()))
+  return State;
+  } else {
+InitVal = 0;
+  }
+
+  const MemRegion *R = State->getRegion(VD, LCtx);
+  if (!R)
+return State;
+  SVal V = State->getSVal(loc::MemRegionVal(R));
+  SVal Constraint_untested =
+  evalBinOp(State, BO_EQ, V, svalBuilder.makeIntVal(InitVal),
+svalBuilder.getConditionType());
+  Optional Constraint =
+  Constraint_untested.getAs();
+  if (!Constraint)
+return State;
+  return State->assume(*Constraint, true);
+}
+
+static void getGlobalStaticVars(const Stmt *FuncBody,
+llvm::SmallSet *Vars) {
+  std::stack Children;
+  Children.push(FuncBody);
+  while (!Children.empty()) {
+const Stmt *Child = Children.top();
+Children.pop();
+if (!Child)
+  continue;
+for (const Stmt *C : Child->children()) {
+  Children.push(C);
+}
+if (const DeclRefExpr *D = dyn_cast(Child)) {
+  const VarDecl *VD = dyn_cast(D->getDecl());
+  if (VD && VD->isDefinedOutsideFunctionOrMethod() &&
+  VD->getType()->isIntegerType() &&
+  VD->getStorageClass() == SC_Static &&
+  !VD->getType()->isPointe

[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

ping


Repository:
  rL LLVM

https://reviews.llvm.org/D36471



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

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

ping


Repository:
  rL LLVM

https://reviews.llvm.org/D30295



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

[PATCH] D28148: [Sema] Suppress warnings for C's zero initializer

[Daniel Marjamäki via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL314499: [Sema] Suppress warnings for C's zero initializer 
(authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D28148?vs=82849&id=117107#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D28148

Files:
  cfe/trunk/include/clang/AST/Expr.h
  cfe/trunk/lib/AST/Expr.cpp
  cfe/trunk/lib/Sema/SemaInit.cpp
  cfe/trunk/test/Sema/zero-initializer.c

Index: cfe/trunk/lib/AST/Expr.cpp
===
--- cfe/trunk/lib/AST/Expr.cpp
+++ cfe/trunk/lib/AST/Expr.cpp
@@ -1951,6 +1951,17 @@
  getInit(0)->getType().getCanonicalType();
 }
 
+bool InitListExpr::isIdiomaticZeroInitializer(const LangOptions &LangOpts) const {
+  assert(isSyntacticForm() && "only test syntactic form as zero initializer");
+
+  if (LangOpts.CPlusPlus || getNumInits() != 1) {
+return false;
+  }
+
+  const IntegerLiteral *Lit = dyn_cast(getInit(0));
+  return Lit && Lit->getValue() == 0;
+}
+
 SourceLocation InitListExpr::getLocStart() const {
   if (InitListExpr *SyntacticForm = getSyntacticForm())
 return SyntacticForm->getLocStart();
Index: cfe/trunk/lib/Sema/SemaInit.cpp
===
--- cfe/trunk/lib/Sema/SemaInit.cpp
+++ cfe/trunk/lib/Sema/SemaInit.cpp
@@ -886,7 +886,8 @@
 }
 
 // Complain about missing braces.
-if (T->isArrayType() || T->isRecordType()) {
+if ((T->isArrayType() || T->isRecordType()) &&
+!ParentIList->isIdiomaticZeroInitializer(SemaRef.getLangOpts())) {
   SemaRef.Diag(StructuredSubobjectInitList->getLocStart(),
diag::warn_missing_braces)
   << StructuredSubobjectInitList->getSourceRange()
@@ -1833,7 +1834,9 @@
   // worthwhile to skip over the rest of the initializer, though.
   RecordDecl *RD = DeclType->getAs()->getDecl();
   RecordDecl::field_iterator FieldEnd = RD->field_end();
-  bool CheckForMissingFields = true;
+  bool CheckForMissingFields =
+!IList->isIdiomaticZeroInitializer(SemaRef.getLangOpts());
+
   while (Index < IList->getNumInits()) {
 Expr *Init = IList->getInit(Index);
 
Index: cfe/trunk/include/clang/AST/Expr.h
===
--- cfe/trunk/include/clang/AST/Expr.h
+++ cfe/trunk/include/clang/AST/Expr.h
@@ -4011,6 +4011,10 @@
   /// initializer)?
   bool isTransparent() const;
 
+  /// Is this the zero initializer {0} in a language which considers it
+  /// idiomatic?
+  bool isIdiomaticZeroInitializer(const LangOptions &LangOpts) const;
+
   SourceLocation getLBraceLoc() const { return LBraceLoc; }
   void setLBraceLoc(SourceLocation Loc) { LBraceLoc = Loc; }
   SourceLocation getRBraceLoc() const { return RBraceLoc; }
@@ -4020,6 +4024,9 @@
   InitListExpr *getSemanticForm() const {
 return isSemanticForm() ? nullptr : AltForm.getPointer();
   }
+  bool isSyntacticForm() const {
+return !AltForm.getInt() || !AltForm.getPointer();
+  }
   InitListExpr *getSyntacticForm() const {
 return isSemanticForm() ? AltForm.getPointer() : nullptr;
   }
Index: cfe/trunk/test/Sema/zero-initializer.c
===
--- cfe/trunk/test/Sema/zero-initializer.c
+++ cfe/trunk/test/Sema/zero-initializer.c
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -std=c99 -Wmissing-field-initializers -Wmissing-braces -verify %s
+
+// Tests that using {0} in struct initialization or assignment is supported
+struct foo { int x; int y; };
+struct bar { struct foo a; struct foo b; };
+struct A { int a; };
+struct B { struct A a; };
+struct C { struct B b; };
+
+int main(void)
+{
+  struct foo f = { 0 }; // no-warning
+  struct foo g = { 9 }; // expected-warning {{missing field 'y' initializer}}
+  struct foo h = { 9, 9 }; // no-warning
+  struct bar i = { 0 }; // no-warning
+  struct bar j = { 0, 0 }; // expected-warning {{suggest braces around initialization of subobject}} expected-warning {{missing field 'b' initializer}}
+  struct bar k = { { 9, 9 }, { 9, 9 } }; // no-warning
+  struct bar l = { { 9, 9 }, { 0 } }; // no-warning
+  struct bar m = { { 0 }, { 0 } }; // no-warning
+  struct bar n = { { 0 }, { 9, 9 } }; // no-warning
+  struct bar o = { { 9 }, { 9, 9 } }; // expected-warning {{missing field 'y' initializer}}
+  struct C p = { 0 }; // no-warning
+  struct C q = { 9 }; // expected-warning {{suggest braces around initialization of subobject}} expected-warning {{suggest braces around initialization of subobject}}
+  f = (struct foo ) { 0 }; // no-warning
+  g = (struct foo ) { 9 }; // expected-warning {{missing field 'y' initializer}}
+  h = (struct foo ) { 9, 9 }; // no-warning
+  i = (struct bar) { 0 }; // no-warning
+  j = (struct bar) { 0, 0 }; // expected-warning {{suggest braces around initialization of subobject}} expected-warning {{missing field 'b' initializer}}
+  k = (struct bar) { { 9, 9 }, { 9, 9 } };

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 116865.
danielmarjamaki added a comment.

fixed review comments


Repository:
  rL LLVM

https://reviews.llvm.org/D30295

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  lib/StaticAnalyzer/Core/CheckerContext.cpp
  test/Analysis/bitwise-ops.c

Index: test/Analysis/bitwise-ops.c
===
--- test/Analysis/bitwise-ops.c
+++ test/Analysis/bitwise-ops.c
@@ -22,11 +22,25 @@
   case 1:
 return 0ULL << 63; // no-warning
   case 2:
-return 0ULL << 64; // expected-warning{{The result of the '<<' expression is undefined}}
+return 0ULL << 64; // expected-warning{{The result of the left shift is undefined due to shifting by '64', which is larger or equal to the width of type 'unsigned long long'}}
   case 3:
-return 0ULL << 65; // expected-warning{{The result of the '<<' expression is undefined}}
+return 0ULL << 65; // expected-warning{{The result of the left shift is undefined due to shifting by '65', which is larger or equal to the width of type 'unsigned long long'}}
 
   default:
 return 0;
   }
-}
\ No newline at end of file
+}
+
+int testOverflowShift(int a) {
+  if (a == 323) {
+return 1 << a; // expected-warning{{The result of the left shift is undefined due to shifting by '323', which is larger or equal to the width of type 'int'}}
+  }
+  return 0;
+}
+
+int testNegativeShift(int a) {
+  if (a == -5) {
+return 1 << a; // expected-warning{{The result of the left shift is undefined because the right operand is negative}}
+  }
+  return 0;
+}
Index: lib/StaticAnalyzer/Core/CheckerContext.cpp
===
--- lib/StaticAnalyzer/Core/CheckerContext.cpp
+++ lib/StaticAnalyzer/Core/CheckerContext.cpp
@@ -99,3 +99,35 @@
   return Lexer::getSpelling(Loc, buf, getSourceManager(), getLangOpts());
 }
 
+/// Evaluate comparison and return true if it's known that condition is true
+static bool evalComparison(SVal LHSVal, BinaryOperatorKind ComparisonOp,
+   SVal RHSVal, ProgramStateRef State) {
+  if (LHSVal.isUnknownOrUndef())
+return false;
+  ProgramStateManager &Mgr = State->getStateManager();
+  if (!LHSVal.getAs()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+  LHSVal.castAs());
+if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+  return false;
+  }
+
+  SValBuilder &Bldr = Mgr.getSValBuilder();
+  SVal Eval = Bldr.evalBinOp(State, ComparisonOp, LHSVal, RHSVal,
+ Bldr.getConditionType());
+  if (Eval.isUnknownOrUndef())
+return false;
+  ProgramStateRef StTrue, StFalse;
+  std::tie(StTrue, StFalse) = State->assume(Eval.castAs());
+  return StTrue && !StFalse;
+}
+
+bool CheckerContext::isGreaterOrEqual(const Expr *E, unsigned long long Val) {
+  DefinedSVal V = getSValBuilder().makeIntVal(Val, getASTContext().LongLongTy);
+  return evalComparison(getSVal(E), BO_GE, V, getState());
+}
+
+bool CheckerContext::isNegative(const Expr *E) {
+  DefinedSVal V = getSValBuilder().makeIntVal(0, false);
+  return evalComparison(getSVal(E), BO_LT, V, getState());
+}
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -59,6 +59,11 @@
   return StOutBound && !StInBound;
 }
 
+static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) {
+  return C.isGreaterOrEqual(
+  B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType()));
+}
+
 void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
   ProgramStateRef state = C.getState();
@@ -97,18 +102,46 @@
 }
 
 if (Ex) {
-  OS << "The " << (isLeft ? "left" : "right")
- << " operand of '"
+  OS << "The " << (isLeft ? "left" : "right") << " operand of '"
  << BinaryOperator::getOpcodeStr(B->getOpcode())
  << "' is a garbage value";
   if (isArrayIndexOutOfBounds(C, Ex))
 OS << " due to array index out of bounds";
-}
-else {
+} else {
   // Neither operand was undefined, but the result is undefined.
-  OS << "The result of the '"
- << BinaryOperator::getOpcodeStr(B->getOpcode())
- << "' expression is undefined";
+  if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+   B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+  C.isNegative(B->getRHS())) {
+OS << "The result of the "
+   << ((B->getOpcode() == BinaryOperatorKind::BO_Shl) ? "left"
+  : "right")
+   << " shi

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

ping


Repository:
  rL LLVM

https://reviews.llvm.org/D30295



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

[PATCH] D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

In https://reviews.llvm.org/D37897#871858, @xazax.hun wrote:

> Out of curiosity, does the false positive disappear after making the static 
> variables const?


Yes that fixes the false positive


Repository:
  rL LLVM

https://reviews.llvm.org/D37897



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

[PATCH] D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 115385.
danielmarjamaki added a comment.

Minor cleanups. Changed names. Updated comments.


Repository:
  rL LLVM

https://reviews.llvm.org/D37897

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  test/Analysis/global-vars.c

Index: test/Analysis/global-vars.c
===
--- test/Analysis/global-vars.c
+++ test/Analysis/global-vars.c
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha,core -verify %s
+
+// Avoid false positive.
+static char *allv[] = {"rpcgen", "-s", "udp", "-s", "tcp"};
+static int allc = sizeof(allv) / sizeof(allv[0]);
+static void falsePositive1(void) {
+  int i;
+  for (i = 1; i < allc; i++) {
+const char *p = allv[i]; // no-warning
+i++;
+  }
+}
+
+// Detect division by zero.
+static int zero = 0;
+void truePositive1() {
+  int x = 1000 / zero; // expected-warning{{Division by zero}}
+}
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -103,8 +103,107 @@
 // Utility methods.
 //===--===//
 
+/** Recursively check if variable is changed in code. */
+static bool isChanged(const Stmt *S, const VarDecl *VD, bool Write) {
+  if (!S)
+return false;
+  if (const BinaryOperator *B = dyn_cast(S)) {
+if (B->isAssignmentOp())
+  return isChanged(B->getLHS(), VD, true) ||
+ isChanged(B->getRHS(), VD, Write);
+  } else if (const UnaryOperator *U = dyn_cast(S)) {
+// Operators that mean operand is written.
+// AddrOf is here because the address might be used to write the operand
+// later indirectly.
+if (U->getOpcode() == UO_AddrOf || U->getOpcode() == UO_PreInc ||
+U->getOpcode() == UO_PreDec || U->getOpcode() == UO_PostInc ||
+U->getOpcode() == UO_PostDec)
+  return isChanged(U->getSubExpr(), VD, true);
+  } else if (const DeclRefExpr *D = dyn_cast(S)) {
+return Write && D->getDecl() == VD;
+  }
+
+  for (const Stmt *Child : S->children()) {
+if (isChanged(Child, VD, Write))
+  return true;
+  }
+  return false;
+}
+
+/** Is variable changed in function or method? */
+static bool isChanged(const Decl *D, const VarDecl *VD) {
+  if (isa(D))
+return isChanged(D->getBody(), VD, false);
+  if (const auto *Rec = dyn_cast(D)) {
+for (const auto *RecChild : Rec->decls()) {
+  if (isChanged(RecChild, VD))
+return true;
+}
+  }
+  return false;
+}
+
+/** Get initial state for global static variable */
+ProgramStateRef ExprEngine::getInitialStateForGlobalStaticVar(
+const LocationContext *LCtx, ProgramStateRef State, const VarDecl *VD) {
+  // Is variable changed anywhere in TU?
+  for (const Decl *D : AMgr.getASTContext().getTranslationUnitDecl()->decls()) {
+if (isChanged(D, VD))
+  return State;
+  }
+
+  // What is the initialized value?
+  llvm::APSInt InitVal;
+  if (const Expr *I = VD->getInit()) {
+if (!I->EvaluateAsInt(InitVal, getContext()))
+  return State;
+  } else {
+InitVal = 0;
+  }
+
+  const MemRegion *R = State->getRegion(VD, LCtx);
+  if (!R)
+return State;
+  SVal V = State->getSVal(loc::MemRegionVal(R));
+  SVal Constraint_untested =
+  evalBinOp(State, BO_EQ, V, svalBuilder.makeIntVal(InitVal),
+svalBuilder.getConditionType());
+  Optional Constraint =
+  Constraint_untested.getAs();
+  if (!Constraint)
+return State;
+  return State->assume(*Constraint, true);
+}
+
+static void getGlobalStaticVars(const Stmt *S,
+llvm::SmallSet *Vars) {
+  if (!S)
+return;
+  if (const DeclRefExpr *D = dyn_cast(S)) {
+const VarDecl *VD = dyn_cast(D->getDecl());
+if (VD && VD->isDefinedOutsideFunctionOrMethod() &&
+VD->getType()->isIntegerType() && VD->getStorageClass() == SC_Static &&
+!VD->getType()->isPointerType()) {
+  Vars->insert(VD);
+}
+  }
+  for (const Stmt *Child : S->children()) {
+getStaticVars(Child, Vars);
+  }
+}
+
 ProgramStateRef ExprEngine::getInitialState(const LocationContext *InitLoc) {
   ProgramStateRef state = StateMgr.getInitialState(InitLoc);
+
+  // Get initial states for static global variables.
+  if (const auto *FD = dyn_cast(InitLoc->getDecl())) {
+llvm::SmallSet Vars;
+getGlobalStaticVars(FD->getBody(), &Vars);
+for (const VarDecl *VD : Vars) {
+  state = getInitialState(InitLoc, state, VD);
+}
+  }
+
   const Decl *D = InitLoc->getDecl();
 
   // Preconditions.
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine

[PATCH] D37897: [StaticAnalyzer] Fix ProgramState for static variables that are not written

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki created this revision.

I saw a false positive where the analyzer made wrong conclusions about a static 
variable.

Static variables that are not written have known values (initialized values).

This is the (simplified) code that motivated me to create this patch:

  static char *allv[] = {
"rpcgen", "-s", "udp", "-s", "tcp",
  
  };
  static int allc = sizeof(allv) / sizeof(allv[0]);
  
  static void f(void) {
int i;
  
for (i = 1; i < allc; i++) {
const char *p = allv[i];  // <- line 28
i++;
}
  }

Clang output:

  array-fp3.c:28:19: warning: Access out-of-bound array element (buffer 
overflow)
  const char *p = allv[i];
  ^~~

I added testcases that shows this patch solves both false positives and false 
negatives


Repository:
  rL LLVM

https://reviews.llvm.org/D37897

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  test/Analysis/global-vars.c

Index: test/Analysis/global-vars.c
===
--- test/Analysis/global-vars.c
+++ test/Analysis/global-vars.c
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=alpha,core -verify %s
+
+// Avoid false positive.
+static char *allv[] = {"rpcgen", "-s", "udp", "-s", "tcp"};
+static int allc = sizeof(allv) / sizeof(allv[0]);
+static void falsePositive1(void) {
+  int i;
+  for (i = 1; i < allc; i++) {
+const char *p = allv[i]; // no-warning
+i++;
+  }
+}
+
+// Detect division by zero.
+static int zero = 0;
+void truePositive1() {
+  int x = 1000 / zero; // expected-warning{{Division by zero}}
+}
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -103,8 +103,106 @@
 // Utility methods.
 //===--===//
 
+static bool isChanged(const Stmt *S, const VarDecl *VD, bool W) {
+  if (!S)
+return false;
+  if (const BinaryOperator *B = dyn_cast(S)) {
+if (B->isAssignmentOp())
+  return isChanged(B->getLHS(), VD, true) || isChanged(B->getRHS(), VD, W);
+  } else if (const UnaryOperator *U = dyn_cast(S)) {
+if (U->getOpcode() == UO_AddrOf || U->getOpcode() == UO_PreInc ||
+U->getOpcode() == UO_PreDec || U->getOpcode() == UO_PostInc ||
+U->getOpcode() == UO_PostDec)
+  return isChanged(U->getSubExpr(), VD, true);
+  } else if (const DeclRefExpr *D = dyn_cast(S)) {
+return W && D->getDecl() == VD;
+  }
+
+  for (const Stmt *Child : S->children()) {
+if (isChanged(Child, VD, W))
+  return true;
+  }
+  return false;
+}
+
+static bool isChanged(const Decl *D, const VarDecl *VD) {
+  if (isa(D))
+return isChanged(D->getBody(), VD, false);
+  if (const auto *Rec = dyn_cast(D)) {
+for (const auto *RecChild : Rec->decls()) {
+  if (isChanged(RecChild, VD))
+return true;
+}
+  }
+  return false;
+}
+
+ProgramStateRef ExprEngine::getInitialState(const LocationContext *LCtx,
+ProgramStateRef State,
+const VarDecl *VD) {
+  // Is variable used in location context?
+  const FunctionDecl *FD = dyn_cast(LCtx->getDecl());
+  if (!FD)
+return State;
+
+  // Is variable changed anywhere in TU?
+  for (const Decl *D : AMgr.getASTContext().getTranslationUnitDecl()->decls()) {
+if (isChanged(D, VD))
+  return State;
+  }
+
+  // What is the initialized value?
+  llvm::APSInt InitVal;
+  if (const Expr *I = VD->getInit()) {
+if (!I->EvaluateAsInt(InitVal, getContext()))
+  return State;
+  } else {
+InitVal = 0;
+  }
+
+  const MemRegion *R = State->getRegion(VD, LCtx);
+  if (!R)
+return State;
+  SVal V = State->getSVal(loc::MemRegionVal(R));
+  SVal Constraint_untested =
+  evalBinOp(State, BO_EQ, V, svalBuilder.makeIntVal(InitVal),
+svalBuilder.getConditionType());
+  Optional Constraint =
+  Constraint_untested.getAs();
+  if (!Constraint)
+return State;
+  return State->assume(*Constraint, true);
+}
+
+static void getStaticVars(const Stmt *S,
+  llvm::SmallSet *Vars) {
+  if (!S)
+return;
+  if (const DeclRefExpr *D = dyn_cast(S)) {
+const VarDecl *VD = dyn_cast(D->getDecl());
+if (VD && VD->isDefinedOutsideFunctionOrMethod() &&
+VD->getType()->isIntegerType() && VD->getStorageClass() == SC_Static &&
+!VD->getType()->isPointerType()) {
+  Vars->insert(VD);
+}
+  }
+  for (const Stmt *Child : S->children()) {
+getStaticVars(Child, Vars);
+  }
+}
+
 ProgramStateRef ExprEngine::getInitialState(const LocationContext *InitLoc) {
   ProgramStateRef state = StateMgr.getInitialState(InitLoc);
+
+  // Get initial states for static global variab

[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

ping


Repository:
  rL LLVM

https://reviews.llvm.org/D36471



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

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

ping


Repository:
  rL LLVM

https://reviews.llvm.org/D30295



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

[PATCH] D28148: [Sema] Suppress warnings for C's zero initializer

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

This is not committed as far as I see.. do you have write permission or do you 
want that I commit it?


https://reviews.llvm.org/D28148



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

[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 113969.
danielmarjamaki added a comment.

minor code cleanup


Repository:
  rL LLVM

https://reviews.llvm.org/D36471

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
  lib/StaticAnalyzer/Core/ExprEngineC.cpp
  lib/StaticAnalyzer/Core/RangeConstraintManager.cpp

Index: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -304,6 +304,8 @@
   void print(ProgramStateRef State, raw_ostream &Out, const char *nl,
  const char *sep) override;
 
+  ProgramStateRef evalRangeOp(ProgramStateRef state, SVal V) override;
+
   //===--===//
   // Implementation for interface from RangedConstraintManager.
   //===--===//
@@ -741,3 +743,56 @@
   }
   Out << nl;
 }
+
+ProgramStateRef RangeConstraintManager::evalRangeOp(ProgramStateRef St,
+SVal V) {
+  const SymExpr *SE = V.getAsSymExpr();
+  if (!SE)
+return nullptr;
+
+  const SymIntExpr *SIE = dyn_cast(SE);
+  if (!SIE)
+return nullptr;
+
+  const clang::BinaryOperatorKind Opc = SIE->getOpcode();
+
+  if (Opc != BO_Add && Opc != BO_Sub && Opc != BO_Div)
+return nullptr;
+
+  const SymExpr *LHS = SIE->getLHS();
+  const llvm::APSInt &RHS = SIE->getRHS();
+
+  ConstraintRangeTy Ranges = St->get();
+  for (ConstraintRangeTy::iterator I = Ranges.begin(), E = Ranges.end(); I != E;
+   ++I) {
+if (LHS != I.getKey())
+  continue;
+const auto D = I.getData();
+for (auto I = D.begin(); I != D.end(); ++I) {
+  if (I->From().isUnsigned() != RHS.isUnsigned())
+// TODO: Handle sign conversions.
+return nullptr;
+  if (I->From().getBitWidth() != RHS.getBitWidth())
+// TODO: Promote values.
+return nullptr;
+  if (I->From().isNegative())
+// TODO: Handle negative range values
+return nullptr;
+
+  BasicValueFactory &BVF = getBasicVals();
+  const llvm::APSInt *Lower = BVF.evalAPSInt(Opc, I->From(), RHS);
+  if (!Lower)
+return nullptr;
+  const llvm::APSInt *Upper = BVF.evalAPSInt(Opc, I->To(), RHS);
+  if (!Upper)
+return nullptr;
+
+  SymbolRef Sym = V.getAsSymbol();
+  RangeSet RS =
+  getRange(St, Sym).Intersect(getBasicVals(), F, *Lower, *Upper);
+  // TODO: This only evaluates the first range. Evaluate all ranges.
+  return RS.isEmpty() ? nullptr : St->set(Sym, RS);
+}
+  }
+  return nullptr;
+}
Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -98,7 +98,9 @@
   }
 
   state = state->BindExpr(B, LCtx, Result);
-  Bldr.generateNode(B, *it, state);
+  ProgramStateRef state2 =
+  getConstraintManager().evalRangeOp(state, Result);
+  Bldr.generateNode(B, *it, state2 ? state2 : state);
   continue;
 }
 
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
@@ -142,13 +142,15 @@
   /// Scan all symbols referenced by the constraints. If the symbol is not
   /// alive, remove it.
   virtual ProgramStateRef removeDeadBindings(ProgramStateRef state,
- SymbolReaper& SymReaper) = 0;
+ SymbolReaper &SymReaper) = 0;
 
-  virtual void print(ProgramStateRef state,
- raw_ostream &Out,
- const char* nl,
+  virtual void print(ProgramStateRef state, raw_ostream &Out, const char *nl,
  const char *sep) = 0;
 
+  virtual ProgramStateRef evalRangeOp(ProgramStateRef state, SVal V) {
+return nullptr;
+  }
+
   virtual void EndPath(ProgramStateRef state) {}
 
   /// Convenience method to query the state to see if a symbol is null or
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

ping


Repository:
  rL LLVM

https://reviews.llvm.org/D30295



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

[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki accepted this revision.
danielmarjamaki added inline comments.
This revision is now accepted and ready to land.



Comment at: tools/scan-build-py/libscanbuild/analyze.py:165
+with open(filename, 'r') as in_file:
+for line in in_file:
+yield line

whisperity wrote:
> danielmarjamaki wrote:
> > I believe you can write:
> > 
> > for line in open(filename, 'r'):
> Do we want to rely on the interpreter implementation on when the file is 
> closed.
> 
> If 
> 
> ```
>   for line in open(filename, 'r'):
>  something()
> ```
> 
> is used, the file handle will be closed based on garbage collection rules. 
> Having this handle disposed after the iteration is true for the stock CPython 
> implementation, but it is still nontheless an implementation specific 
> approach.
> 
> Whereas using `with` will explicitly close the file handle on the spot, no 
> matter what.
ok I did not know that. feel free to ignore my comment.



Comment at: tools/scan-build-py/libscanbuild/analyze.py:172
+extern_fns_map_file = os.path.join(ctudir, CTU_FUNCTION_MAP_FILENAME)
+with open(extern_fns_map_file, 'w') as out_file:
+for mangled_name, ast_file in mangled_ast_pairs:

whisperity wrote:
> danielmarjamaki wrote:
> > this 'with' seems redundant. I suggest an assignment and then less 
> > indentation will be needed below
> I don't seem to understand what do you want to assign to what.
I did not consider the garbage collection. I assumed that out_file would Always 
be closed when it Went out of scope and then this would require less 
indentation:

out_file = open(extern_fns_map_file, 'w')
for mangled_name, ast_file in mangled_ast_pairs:
out_file.write('%s %s\n' % (mangled_name, ast_file))




Comment at: tools/scan-build-py/libscanbuild/analyze.py:223
+ctu_config = get_ctu_config(args)
+if ctu_config.collect:
+shutil.rmtree(ctu_config.dir, ignore_errors=True)

danielmarjamaki wrote:
> not a big deal but I would use early exits in this function
with "not a big deal" I mean; feel free to ignore my comment if you want to 
have it this way.


https://reviews.llvm.org/D30691



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

[PATCH] D34512: Add preliminary Cross Translation Unit support library

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

small nits




Comment at: include/clang/CrossTU/CrossTranslationUnit.h:58
+
+/// \brief This function can parse an index file that determines which
+///translation unit contains which definition.

I suggest that "can" is removed.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:62
+/// The index file format is the following:
+/// each line consists of an USR separated by a filepath.
+llvm::Expected>

there is no \return or \returns here.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:62
+/// The index file format is the following:
+/// each line consists of an USR separated by a filepath.
+llvm::Expected>

danielmarjamaki wrote:
> there is no \return or \returns here.
maybe: each line consists of an USR and a filepath separated by a space



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:68
+
+/// \brief This class can be used for tools that requires cross translation
+///unit capability.

Maybe /can be/is/ unless you envision that tools that require cross translation 
unit capability might use some other classes.



Comment at: include/clang/CrossTU/CrossTranslationUnit.h:92
+  /// definition of the function will be merged into the original AST using
+  /// the AST Importer. The declaration with the definition will be returned.
+  /// If no suitable definition is found in the index file, null will be

you should split out some of this information to a \return or \returns



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:60
+
+char IndexError::ID = 0;
+

redundant assignment



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:79
+  std::string Line;
+  unsigned LineNo = 0;
+  while (std::getline(ExternalFnMapFile, Line)) {

I suggest that LineNo is 1 on the first line.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:81
+  while (std::getline(ExternalFnMapFile, Line)) {
+size_t Pos = Line.find(" ");
+StringRef LineRef{Line};

Pos can be const



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:82
+size_t Pos = Line.find(" ");
+StringRef LineRef{Line};
+if (Pos > 0 && Pos != std::string::npos) {

LineRef can be const



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:84
+if (Pos > 0 && Pos != std::string::npos) {
+  FunctionName = LineRef.substr(0, Pos);
+  if (Result.count(FunctionName))

FunctionName and FileName can be moved here and it is possible to make these 
variables const.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:85
+  FunctionName = LineRef.substr(0, Pos);
+  if (Result.count(FunctionName))
+return llvm::make_error(

I would like to see some FunctionName validation. For instance "123" should not 
be a valid function name.



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:89
+  FileName = LineRef.substr(Pos + 1);
+  SmallString<256> FilePath = CrossTUDir;
+  llvm::sys::path::append(FilePath, FileName);

Stupid question .. how will this work if the path is longer than 256 bytes?



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:102
+createCrossTUIndexString(const llvm::StringMap &Index) {
+  std::stringstream Result;
+  for (const auto &E : Index) {

how about std::ostringstream , imho that is cleaner



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:121
+
+/// Recursively visits the funtion decls of a DeclContext, and looks up a
+/// function based on USRs.

/funtion/function/



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:125
+CrossTranslationUnitContext::findFunctionInDeclContext(const DeclContext *DC,
+   StringRef LookupFnName) 
{
+  assert(DC && "Declaration Context must not be null");

LookupFnName could be const right?



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:148
+
+  std::string LookupFnName = getLookupName(FD);
+  if (LookupFnName.empty())

I believe LookupFnName can be const



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:189
+  return nullptr; // No definition found even in some other build unit.
+ASTFileName = It->second;
+auto ASTCacheEntry = FileASTUnitMap.find(ASTFileName);

It is possible to make ASTFileName const



Comment at: lib/CrossTU/CrossTranslationUnit.cpp:223
+assert(ToDecl->hasBody());
+assert(FD->hasBody() && "Functions already imported should have body.");
+return ToDecl;

sorry I am probably missing something here.. you first assert !FD->hasBod

[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added inline comments.



Comment at: tools/scan-build-py/libscanbuild/analyze.py:165
+with open(filename, 'r') as in_file:
+for line in in_file:
+yield line

I believe you can write:

for line in open(filename, 'r'):



Comment at: tools/scan-build-py/libscanbuild/analyze.py:172
+extern_fns_map_file = os.path.join(ctudir, CTU_FUNCTION_MAP_FILENAME)
+with open(extern_fns_map_file, 'w') as out_file:
+for mangled_name, ast_file in mangled_ast_pairs:

this 'with' seems redundant. I suggest an assignment and then less indentation 
will be needed below



Comment at: tools/scan-build-py/libscanbuild/analyze.py:223
+ctu_config = get_ctu_config(args)
+if ctu_config.collect:
+shutil.rmtree(ctu_config.dir, ignore_errors=True)

not a big deal but I would use early exits in this function



Comment at: tools/scan-build-py/libscanbuild/clang.py:177
+arch = ""
+i = 0
+while i < len(cmd) and cmd[i] != "-triple":

I am guessing that you can use cmd.find() instead of the loop


https://reviews.llvm.org/D30691



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

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

ping


Repository:
  rL LLVM

https://reviews.llvm.org/D30295



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

[PATCH] D36670: misc-misplaced-widening-cast: fix assertion

[Daniel Marjamäki via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL311984: [clang-tidy] Fix 'misc-misplaced-widening-cast' 
assertion error. (authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D36670?vs=110940&id=113026#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D36670

Files:
  clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp
  
clang-tools-extra/trunk/test/clang-tidy/misc-misplaced-widening-cast-explicit-only.cpp


Index: 
clang-tools-extra/trunk/test/clang-tidy/misc-misplaced-widening-cast-explicit-only.cpp
===
--- 
clang-tools-extra/trunk/test/clang-tidy/misc-misplaced-widening-cast-explicit-only.cpp
+++ 
clang-tools-extra/trunk/test/clang-tidy/misc-misplaced-widening-cast-explicit-only.cpp
@@ -56,3 +56,9 @@
 return (long)a * 1000;
   }
 }
+
+// Shall not generate an assert. https://bugs.llvm.org/show_bug.cgi?id=33660
+template  class A {
+  enum Type {};
+  static char *m_fn1() { char p = (Type)(&p - m_fn1()); }
+};
Index: clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp
@@ -192,6 +192,10 @@
   if (Calc->getLocStart().isMacroID())
 return;
 
+  if (Cast->isTypeDependent() || Cast->isValueDependent() ||
+  Calc->isTypeDependent() || Calc->isValueDependent())
+return;
+
   ASTContext &Context = *Result.Context;
 
   QualType CastType = Cast->getType();


Index: clang-tools-extra/trunk/test/clang-tidy/misc-misplaced-widening-cast-explicit-only.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/misc-misplaced-widening-cast-explicit-only.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/misc-misplaced-widening-cast-explicit-only.cpp
@@ -56,3 +56,9 @@
 return (long)a * 1000;
   }
 }
+
+// Shall not generate an assert. https://bugs.llvm.org/show_bug.cgi?id=33660
+template  class A {
+  enum Type {};
+  static char *m_fn1() { char p = (Type)(&p - m_fn1()); }
+};
Index: clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp
@@ -192,6 +192,10 @@
   if (Calc->getLocStart().isMacroID())
 return;
 
+  if (Cast->isTypeDependent() || Cast->isValueDependent() ||
+  Calc->isTypeDependent() || Calc->isValueDependent())
+return;
+
   ASTContext &Context = *Result.Context;
 
   QualType CastType = Cast->getType();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

ping


Repository:
  rL LLVM

https://reviews.llvm.org/D30295



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

[PATCH] D36672: [clang-tidy] readability-non-const-parameter: fixit on all function declarations

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

LGTM. But others should approve.


Repository:
  rL LLVM

https://reviews.llvm.org/D36672



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

[PATCH] D36670: misc-misplaced-widening-cast: fix assertion

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

LGTM. I let others approve this.


Repository:
  rL LLVM

https://reviews.llvm.org/D36670



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

[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

Should evalAPSInt() have machinery to do standard sign/type promotions? I 
suggest that I add one more argument `bool promote = false`, do you think that 
sounds good?


Repository:
  rL LLVM

https://reviews.llvm.org/D36471



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

[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 110378.
danielmarjamaki added a comment.

Refactoring, use BasicValueFactory::evalAPSInt


Repository:
  rL LLVM

https://reviews.llvm.org/D36471

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
  lib/StaticAnalyzer/Core/ExprEngineC.cpp
  lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  test/Analysis/eval-range.c

Index: test/Analysis/eval-range.c
===
--- test/Analysis/eval-range.c
+++ test/Analysis/eval-range.c
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+void test1(int a) {
+  if (a >= 10 && a <= 50) {
+int b;
+
+b = a + 2;
+clang_analyzer_eval(b >= 12 && b <= 52); // expected-warning{{TRUE}}
+
+b = a - 2;
+clang_analyzer_eval(b >= 8 && b <= 48); // expected-warning{{TRUE}}
+
+b = a / 2;
+clang_analyzer_eval(b >= 5 && b <= 25); // expected-warning{{TRUE}}
+  }
+}
Index: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -304,6 +304,8 @@
   void print(ProgramStateRef State, raw_ostream &Out, const char *nl,
  const char *sep) override;
 
+  ProgramStateRef evalRangeOp(ProgramStateRef state, SVal V) override;
+
   //===--===//
   // Implementation for interface from RangedConstraintManager.
   //===--===//
@@ -741,3 +743,56 @@
   }
   Out << nl;
 }
+
+ProgramStateRef RangeConstraintManager::evalRangeOp(ProgramStateRef St,
+SVal V) {
+  const SymExpr *SE = V.getAsSymExpr();
+  if (!SE)
+return nullptr;
+
+  const SymIntExpr *SIE = dyn_cast(SE);
+  if (!SIE)
+return nullptr;
+
+  const clang::BinaryOperatorKind Opc = SIE->getOpcode();
+
+  if (Opc != BO_Add && Opc != BO_Sub && Opc != BO_Div)
+return nullptr;
+
+  const SymExpr *LHS = SIE->getLHS();
+  const llvm::APSInt &RHS = SIE->getRHS();
+
+  ConstraintRangeTy Ranges = St->get();
+  for (ConstraintRangeTy::iterator I = Ranges.begin(), E = Ranges.end(); I != E;
+   ++I) {
+if (LHS == I.getKey()) {
+  const auto D = I.getData();
+  for (auto I = D.begin(); I != D.end(); ++I) {
+if (I->From().isUnsigned() != RHS.isUnsigned())
+  // TODO: Handle sign conversions.
+  return nullptr;
+if (I->From().getBitWidth() != RHS.getBitWidth())
+  // TODO: Promote values.
+  return nullptr;
+if (I->From().isNegative())
+  // TODO: Handle negative range values
+  return nullptr;
+
+BasicValueFactory &BVF = getBasicVals();
+const llvm::APSInt *Lower = BVF.evalAPSInt(Opc, I->From(), RHS);
+if (!Lower)
+  return nullptr;
+const llvm::APSInt *Upper = BVF.evalAPSInt(Opc, I->To(), RHS);
+if (!Upper)
+  return nullptr;
+
+SymbolRef Sym = V.getAsSymbol();
+RangeSet RS =
+getRange(St, Sym).Intersect(getBasicVals(), F, *Lower, *Upper);
+// TODO: This only evaluates the first range. Evaluate all ranges.
+return RS.isEmpty() ? nullptr : St->set(Sym, RS);
+  }
+}
+  }
+  return nullptr;
+}
Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -98,7 +98,9 @@
   }
 
   state = state->BindExpr(B, LCtx, Result);
-  Bldr.generateNode(B, *it, state);
+  ProgramStateRef state2 =
+  getConstraintManager().evalRangeOp(state, Result);
+  Bldr.generateNode(B, *it, state2 ? state2 : state);
   continue;
 }
 
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
@@ -142,13 +142,15 @@
   /// Scan all symbols referenced by the constraints. If the symbol is not
   /// alive, remove it.
   virtual ProgramStateRef removeDeadBindings(ProgramStateRef state,
- SymbolReaper& SymReaper) = 0;
+ SymbolReaper &SymReaper) = 0;
 
-  virtual void print(ProgramStateRef state,
- raw_ostream &Out,
- const char* nl,
+  virtual void print(ProgramStateRef state, raw_ostream &Out, const char *nl,
  const char *sep) = 0;
 
+  virtual ProgramStateRef evalRangeOp(ProgramStateRef state, SVal V) {
+return nullptr;
+  }
+
   virtual

[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

In https://reviews.llvm.org/D36471#835410, @xazax.hun wrote:

> Can't you reuse somehow some machinery already available to evaluate the 
> arithmetic operators? Those should already handle most of your TODOs and 
> overflows.


Sounds good.. I have not seen that machinery.. I will look around.

To me it seems it would be nice if this machinery was builtin in APSInt so I 
could calculate (x+y) even if x and y did not have the same signedness and that 
the result would be unsigned.


Repository:
  rL LLVM

https://reviews.llvm.org/D36471



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

[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 110220.
danielmarjamaki added a comment.

A minor code cleanup. No functional change.


Repository:
  rL LLVM

https://reviews.llvm.org/D36471

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
  lib/StaticAnalyzer/Core/ExprEngineC.cpp
  lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  test/Analysis/eval-range.c

Index: test/Analysis/eval-range.c
===
--- test/Analysis/eval-range.c
+++ test/Analysis/eval-range.c
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+void test1(int a) {
+  if (a >= 10 && a <= 50) {
+int b;
+
+b = a + 2;
+clang_analyzer_eval(b >= 12 && b <= 52); // expected-warning{{TRUE}}
+
+b = a - 2;
+clang_analyzer_eval(b >= 8 && b <= 48); // expected-warning{{TRUE}}
+
+b = a / 2;
+clang_analyzer_eval(b >= 5 && b <= 25); // expected-warning{{TRUE}}
+  }
+}
Index: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -304,6 +304,8 @@
   void print(ProgramStateRef State, raw_ostream &Out, const char *nl,
  const char *sep) override;
 
+  ProgramStateRef evalRangeOp(ProgramStateRef state, SVal V) override;
+
   //===--===//
   // Implementation for interface from RangedConstraintManager.
   //===--===//
@@ -741,3 +743,65 @@
   }
   Out << nl;
 }
+
+ProgramStateRef RangeConstraintManager::evalRangeOp(ProgramStateRef St,
+SVal V) {
+  const SymExpr *SE = V.getAsSymExpr();
+  if (!SE)
+return nullptr;
+
+  const SymIntExpr *SIE = dyn_cast(SE);
+  if (!SIE)
+return nullptr;
+
+  const clang::BinaryOperatorKind Opc = SIE->getOpcode();
+
+  if (Opc != BO_Add && Opc != BO_Sub && Opc != BO_Div)
+return nullptr;
+
+  const SymExpr *LHS = SIE->getLHS();
+  const llvm::APSInt &RHS = SIE->getRHS();
+
+  if (RHS.isNegative())
+// TODO: Handle negative values.
+return nullptr;
+
+  ConstraintRangeTy Ranges = St->get();
+  for (ConstraintRangeTy::iterator I = Ranges.begin(), E = Ranges.end(); I != E;
+   ++I) {
+if (LHS == I.getKey()) {
+  const auto D = I.getData();
+  for (auto I = D.begin(); I != D.end(); ++I) {
+if (I->From().isUnsigned() != RHS.isUnsigned())
+  // TODO: Handle sign conversions.
+  return nullptr;
+if (I->From().getBitWidth() != RHS.getBitWidth())
+  // TODO: Promote values.
+  return nullptr;
+if (I->From().isNegative())
+  // TODO: Handle negative range values
+  return nullptr;
+llvm::APSInt Lower;
+llvm::APSInt Upper;
+if (Opc == BO_Add) {
+  Lower = I->From() + RHS;
+  Upper = I->To() + RHS;
+} else if (Opc == BO_Sub) {
+  if (RHS > I->From())
+return nullptr;
+  Lower = I->From() - RHS;
+  Upper = I->To() - RHS;
+} else if (Opc == BO_Div) {
+  Lower = I->From() / RHS;
+  Upper = I->To() / RHS;
+}
+SymbolRef Sym = V.getAsSymbol();
+RangeSet RS =
+getRange(St, Sym).Intersect(getBasicVals(), F, Lower, Upper);
+// TODO: This only evaluates the first range. Evaluate all ranges.
+return RS.isEmpty() ? nullptr : St->set(Sym, RS);
+  }
+}
+  }
+  return nullptr;
+}
Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -98,7 +98,9 @@
   }
 
   state = state->BindExpr(B, LCtx, Result);
-  Bldr.generateNode(B, *it, state);
+  ProgramStateRef state2 =
+  getConstraintManager().evalRangeOp(state, Result);
+  Bldr.generateNode(B, *it, state2 ? state2 : state);
   continue;
 }
 
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
@@ -142,13 +142,15 @@
   /// Scan all symbols referenced by the constraints. If the symbol is not
   /// alive, remove it.
   virtual ProgramStateRef removeDeadBindings(ProgramStateRef state,
- SymbolReaper& SymReaper) = 0;
+ SymbolReaper &SymReaper) = 0;
 
-  virtual void print(ProgramStateRef state,
- raw_ostream &Out,
- const char* nl,
+  vir

[PATCH] D36471: [StaticAnalyzer] Try to calculate arithmetic result when operand has a range of possible values

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki created this revision.

In the code below the division result should be a value between 5 and 25.

  if (a >= 10 && a <= 50) {
int b = a / 2;
  }

This patch will calculate results for additions, subtractions and divisions.

I intentionally do not try to handle all possible cases that can be handled. I 
want to know if my approach is ok.


Repository:
  rL LLVM

https://reviews.llvm.org/D36471

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
  lib/StaticAnalyzer/Core/ExprEngineC.cpp
  lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  test/Analysis/eval-range.c

Index: test/Analysis/eval-range.c
===
--- test/Analysis/eval-range.c
+++ test/Analysis/eval-range.c
@@ -0,0 +1,18 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+void clang_analyzer_eval(int);
+
+void test1(int a) {
+  if (a >= 10 && a <= 50) {
+int b;
+
+b = a + 2;
+clang_analyzer_eval(b >= 12 && b <= 52); // expected-warning{{TRUE}}
+
+b = a - 2;
+clang_analyzer_eval(b >= 8 && b <= 48); // expected-warning{{TRUE}}
+
+b = a / 2;
+clang_analyzer_eval(b >= 5 && b <= 25); // expected-warning{{TRUE}}
+  }
+}
Index: lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
===
--- lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
+++ lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
@@ -304,6 +304,8 @@
   void print(ProgramStateRef State, raw_ostream &Out, const char *nl,
  const char *sep) override;
 
+  ProgramStateRef evalRangeOp(ProgramStateRef state, SVal V) override;
+
   //===--===//
   // Implementation for interface from RangedConstraintManager.
   //===--===//
@@ -741,3 +743,65 @@
   }
   Out << nl;
 }
+
+ProgramStateRef RangeConstraintManager::evalRangeOp(ProgramStateRef St,
+SVal V) {
+  const SymExpr *SE = V.getAsSymExpr();
+  if (!SE)
+return nullptr;
+
+  const SymIntExpr *SIE = dyn_cast(SE);
+  if (!SIE)
+return nullptr;
+
+  const clang::BinaryOperatorKind Opc = SIE->getOpcode();
+
+  if (Opc != BO_Add && Opc != BO_Sub && Opc != BO_Div)
+return nullptr;
+
+  const SymExpr *LHS = SIE->getLHS();
+  const llvm::APSInt &RHS = SIE->getRHS();
+
+  if (RHS.isNegative())
+// TODO: Handle negative values.
+return nullptr;
+
+  ConstraintRangeTy Ranges = St->get();
+  for (ConstraintRangeTy::iterator I = Ranges.begin(), E = Ranges.end(); I != E;
+   ++I) {
+if (LHS == I.getKey()) {
+  const auto D = I.getData();
+  for (auto I = D.begin(); I != D.end(); ++I) {
+if (I->From().isUnsigned() != RHS.isUnsigned())
+  // TODO: Handle sign conversions.
+  return nullptr;
+if (I->From().getBitWidth() != RHS.getBitWidth())
+  // TODO: Promote values.
+  return nullptr;
+if (I->From().isNegative())
+  // TODO: Handle negative range values
+  return nullptr;
+llvm::APSInt Lower;
+llvm::APSInt Upper;
+if (Opc == BO_Add) {
+  Lower = I->From() + RHS;
+  Upper = I->To() + RHS;
+} else if (Opc == BO_Sub) {
+  if (RHS > I->From())
+return nullptr;
+  Lower = I->From() - RHS;
+  Upper = I->To() - RHS;
+} else if (Opc == BO_Div) {
+  Lower = I->From() / RHS;
+  Upper = I->To() / RHS;
+}
+SymbolRef Sym = V.getAsSymbol();
+RangeSet RS =
+getRange(St, Sym).Intersect(getBasicVals(), F, Lower, Upper);
+// TODO: This only evaluates the first range. Evaluate all ranges.
+return RS.isEmpty() ? nullptr : St->set(Sym, RS);
+  }
+}
+  }
+  return nullptr;
+}
Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -98,6 +98,14 @@
   }
 
   state = state->BindExpr(B, LCtx, Result);
+
+  {
+ProgramStateRef St2 = getConstraintManager().evalRangeOp(state, Result);
+if (St2) {
+  Bldr.generateNode(B, *it, St2);
+  continue;
+}
+  }
   Bldr.generateNode(B, *it, state);
   continue;
 }
Index: include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/ConstraintManager.h
@@ -142,13 +142,15 @@
   /// Scan all symbols referenced by the constraints. If the symbol is not
   /// alive, remove it.
   virtual ProgramStateRef removeDeadBindings(ProgramStateRef state,
-   

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 109590.
danielmarjamaki added a comment.

Cleaned up the patch a little. Thanks Gabor for telling me about 
SValBuilder::getKnownValue()


Repository:
  rL LLVM

https://reviews.llvm.org/D30295

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  lib/StaticAnalyzer/Core/CheckerContext.cpp
  test/Analysis/bitwise-ops.c

Index: test/Analysis/bitwise-ops.c
===
--- test/Analysis/bitwise-ops.c
+++ test/Analysis/bitwise-ops.c
@@ -22,11 +22,25 @@
   case 1:
 return 0ULL << 63; // no-warning
   case 2:
-return 0ULL << 64; // expected-warning{{The result of the '<<' expression is undefined}}
+return 0ULL << 64; // expected-warning{{The result of the left shift is undefined due to shifting by 64, which is larger or equal with the width of type 'unsigned long long'}}
   case 3:
-return 0ULL << 65; // expected-warning{{The result of the '<<' expression is undefined}}
+return 0ULL << 65; // expected-warning{{The result of the left shift is undefined due to shifting by 65, which is larger or equal with the width of type 'unsigned long long'}}
 
   default:
 return 0;
   }
-}
\ No newline at end of file
+}
+
+int testOverflowShift(int a) {
+  if (a == 323) {
+return 1 << a; // expected-warning{{The result of the left shift is undefined due to shifting by 323, which is larger or equal with the width of type 'int'}}
+  }
+  return 0;
+}
+
+int testNegativeShift(int a) {
+  if (a == -5) {
+return 1 << a; // expected-warning{{The result of the left shift is undefined because the right operand is negative}}
+  }
+  return 0;
+}
Index: lib/StaticAnalyzer/Core/CheckerContext.cpp
===
--- lib/StaticAnalyzer/Core/CheckerContext.cpp
+++ lib/StaticAnalyzer/Core/CheckerContext.cpp
@@ -99,3 +99,35 @@
   return Lexer::getSpelling(Loc, buf, getSourceManager(), getLangOpts());
 }
 
+/// Evaluate comparison and return true if it's known that condition is true
+static bool evalComparison(SVal LHSVal, BinaryOperatorKind ComparisonOp,
+   SVal RHSVal, ProgramStateRef State) {
+  if (LHSVal.isUnknownOrUndef())
+return false;
+  ProgramStateManager &Mgr = State->getStateManager();
+  if (!LHSVal.getAs()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+  LHSVal.castAs());
+if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+  return false;
+  }
+
+  SValBuilder &Bldr = Mgr.getSValBuilder();
+  SVal Eval = Bldr.evalBinOp(State, ComparisonOp, LHSVal, RHSVal,
+ Bldr.getConditionType());
+  if (Eval.isUnknownOrUndef())
+return false;
+  ProgramStateRef StTrue, StFalse;
+  std::tie(StTrue, StFalse) = State->assume(Eval.castAs());
+  return StTrue && !StFalse;
+}
+
+bool CheckerContext::isGreaterOrEqual(const Expr *E, unsigned long long Val) {
+  DefinedSVal V = getSValBuilder().makeIntVal(Val, getASTContext().LongLongTy);
+  return evalComparison(getSVal(E), BO_GE, V, getState());
+}
+
+bool CheckerContext::isNegative(const Expr *E) {
+  DefinedSVal V = getSValBuilder().makeIntVal(0, false);
+  return evalComparison(getSVal(E), BO_LT, V, getState());
+}
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -59,6 +59,11 @@
   return StOutBound && !StInBound;
 }
 
+static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) {
+  return C.isGreaterOrEqual(
+  B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType()));
+}
+
 void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
   ProgramStateRef state = C.getState();
@@ -97,18 +102,46 @@
 }
 
 if (Ex) {
-  OS << "The " << (isLeft ? "left" : "right")
- << " operand of '"
+  OS << "The " << (isLeft ? "left" : "right") << " operand of '"
  << BinaryOperator::getOpcodeStr(B->getOpcode())
  << "' is a garbage value";
   if (isArrayIndexOutOfBounds(C, Ex))
 OS << " due to array index out of bounds";
-}
-else {
+} else {
   // Neither operand was undefined, but the result is undefined.
-  OS << "The result of the '"
- << BinaryOperator::getOpcodeStr(B->getOpcode())
- << "' expression is undefined";
+  if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+   B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+  C.isNegative(B->getRHS())) {
+OS << "The result of the "
+   << ((B->getOpcode() == BinaryOperatorKind::BO_Shl) ? "left"
+

[PATCH] D36141: [StaticAnalyzer] Fix FP in UnreachableCodeChecker

[Daniel Marjamäki via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL309799: [StaticAnalyzer] Fix false positives for unreachable 
code in macros. (authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D36141?vs=109086&id=109294#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D36141

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
  cfe/trunk/test/Analysis/unreachable-code-path.c


Index: cfe/trunk/test/Analysis/unreachable-code-path.c
===
--- cfe/trunk/test/Analysis/unreachable-code-path.c
+++ cfe/trunk/test/Analysis/unreachable-code-path.c
@@ -213,3 +213,13 @@
   RETURN(1); // no-warning
 }
 
+// Avoid FP when macro argument is known
+void writeSomething(int *x);
+#define MACRO(C)\
+  if (!C) { \
+static int x;   \
+writeSomething(&x); \
+  }
+void macro2(void) {
+  MACRO(1);
+}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -112,7 +112,7 @@
   continue;
 
 // Check for false positives
-if (CB->size() > 0 && isInvalidPath(CB, *PM))
+if (isInvalidPath(CB, *PM))
   continue;
 
 // It is good practice to always have a "default" label in a "switch", even


Index: cfe/trunk/test/Analysis/unreachable-code-path.c
===
--- cfe/trunk/test/Analysis/unreachable-code-path.c
+++ cfe/trunk/test/Analysis/unreachable-code-path.c
@@ -213,3 +213,13 @@
   RETURN(1); // no-warning
 }
 
+// Avoid FP when macro argument is known
+void writeSomething(int *x);
+#define MACRO(C)\
+  if (!C) { \
+static int x;   \
+writeSomething(&x); \
+  }
+void macro2(void) {
+  MACRO(1);
+}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -112,7 +112,7 @@
   continue;
 
 // Check for false positives
-if (CB->size() > 0 && isInvalidPath(CB, *PM))
+if (isInvalidPath(CB, *PM))
   continue;
 
 // It is good practice to always have a "default" label in a "switch", even
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36141: [StaticAnalyzer] Fix FP in UnreachableCodeChecker

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki created this revision.
Herald added a subscriber: whisperity.

This fixes a FP. Without the fix, the checker says that "static int x;" is 
unreachable.


Repository:
  rL LLVM

https://reviews.llvm.org/D36141

Files:
  lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
  test/Analysis/unreachable-code-path.c


Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -213,3 +213,13 @@
   RETURN(1); // no-warning
 }
 
+// Avoid FP when macro argument is known
+void writeSomething(int *x);
+#define MACRO(C)\
+  if (!C) { \
+static int x;   \
+writeSomething(&x); \
+  }
+void macro2(void) {
+  MACRO(1);
+}
Index: lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -112,7 +112,7 @@
   continue;
 
 // Check for false positives
-if (CB->size() > 0 && isInvalidPath(CB, *PM))
+if (isInvalidPath(CB, *PM))
   continue;
 
 // It is good practice to always have a "default" label in a "switch", even


Index: test/Analysis/unreachable-code-path.c
===
--- test/Analysis/unreachable-code-path.c
+++ test/Analysis/unreachable-code-path.c
@@ -213,3 +213,13 @@
   RETURN(1); // no-warning
 }
 
+// Avoid FP when macro argument is known
+void writeSomething(int *x);
+#define MACRO(C)\
+  if (!C) { \
+static int x;   \
+writeSomething(&x); \
+  }
+void macro2(void) {
+  MACRO(1);
+}
Index: lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UnreachableCodeChecker.cpp
@@ -112,7 +112,7 @@
   continue;
 
 // Check for false positives
-if (CB->size() > 0 && isInvalidPath(CB, *PM))
+if (isInvalidPath(CB, *PM))
   continue;
 
 // It is good practice to always have a "default" label in a "switch", even
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki marked 4 inline comments as done.
danielmarjamaki added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp:126
+   << BinaryOperator::getOpcodeStr(B->getOpcode())
+   << "' expression is undefined due to shift count >= width of type";
+  } else {

zaks.anna wrote:
> It's best not to use ">=" in diagnostic messages.
> Suggestions: "due to shift count >= width of type" ->
> - "due to shifting by a value larger than the width of type"
> - "due to shifting by 5, which is larger than the width of type 'int'" // 
> Providing the exact value and the type would be very useful and this 
> information is readily available to us. Note that the users might not see the 
> type or the value because of macros and such.
I used "due to shifting by 5, which is larger than the width of type 'int'"

However I did not see an easy way to show the exact value. So I added 
getConcreteValue(). Maybe you have a better suggestion. If it's a ConcreteInt I 
show the exact value, but if it's some range etc then I write "due to shifting 
by a value that is larger..." instead.

The message "due to shifting by 64, which is larger than the width of type 
'unsigned long long'" is a bit weird imho. Because 64 is not larger than the 
width. Not sure how this can be rephrazed better though.


Repository:
  rL LLVM

https://reviews.llvm.org/D30295



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

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 103585.
danielmarjamaki added a comment.

Fix review comments


Repository:
  rL LLVM

https://reviews.llvm.org/D30295

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  lib/StaticAnalyzer/Core/CheckerContext.cpp
  test/Analysis/bitwise-ops.c

Index: test/Analysis/bitwise-ops.c
===
--- test/Analysis/bitwise-ops.c
+++ test/Analysis/bitwise-ops.c
@@ -22,11 +22,25 @@
   case 1:
 return 0ULL << 63; // no-warning
   case 2:
-return 0ULL << 64; // expected-warning{{The result of the '<<' expression is undefined}}
+return 0ULL << 64; // expected-warning{{The result of the left shift is undefined due to shifting by 64, which is larger than the width of type 'unsigned long long'}}
   case 3:
-return 0ULL << 65; // expected-warning{{The result of the '<<' expression is undefined}}
+return 0ULL << 65; // expected-warning{{The result of the left shift is undefined due to shifting by 65, which is larger than the width of type 'unsigned long long'}}
 
   default:
 return 0;
   }
-}
\ No newline at end of file
+}
+
+int testOverflowShift(int a) {
+  if (a == 323) {
+return 1 << a; // expected-warning{{The result of the left shift is undefined due to shifting by 323, which is larger than the width of type 'int'}}
+  }
+  return 0;
+}
+
+int testNegativeShift(int a) {
+  if (a == -5) {
+return 1 << a; // expected-warning{{The result of the left shift is undefined because the right operand is negative}}
+  }
+  return 0;
+}
Index: lib/StaticAnalyzer/Core/CheckerContext.cpp
===
--- lib/StaticAnalyzer/Core/CheckerContext.cpp
+++ lib/StaticAnalyzer/Core/CheckerContext.cpp
@@ -99,3 +99,35 @@
   return Lexer::getSpelling(Loc, buf, getSourceManager(), getLangOpts());
 }
 
+/// Evaluate comparison and return true if it's known that condition is true
+static bool evalComparison(SVal LHSVal, BinaryOperatorKind ComparisonOp,
+   SVal RHSVal, ProgramStateRef State) {
+  if (LHSVal.isUnknownOrUndef())
+return false;
+  ProgramStateManager &Mgr = State->getStateManager();
+  if (!LHSVal.getAs()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+  LHSVal.castAs());
+if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+  return false;
+  }
+
+  SValBuilder &Bldr = Mgr.getSValBuilder();
+  SVal Eval = Bldr.evalBinOp(State, ComparisonOp, LHSVal, RHSVal,
+ Bldr.getConditionType());
+  if (Eval.isUnknownOrUndef())
+return false;
+  ProgramStateRef StTrue, StFalse;
+  std::tie(StTrue, StFalse) = State->assume(Eval.castAs());
+  return StTrue && !StFalse;
+}
+
+bool CheckerContext::isGreaterOrEqual(const Expr *E, unsigned long long Val) {
+  DefinedSVal V = getSValBuilder().makeIntVal(Val, getASTContext().LongLongTy);
+  return evalComparison(getSVal(E), BO_GE, V, getState());
+}
+
+bool CheckerContext::isNegative(const Expr *E) {
+  DefinedSVal V = getSValBuilder().makeIntVal(0, false);
+  return evalComparison(getSVal(E), BO_LT, V, getState());
+}
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -59,6 +59,26 @@
   return StOutBound && !StInBound;
 }
 
+static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) {
+  return C.isGreaterOrEqual(
+  B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType()));
+}
+
+static const llvm::APSInt *getConcreteValue(const Expr *E, CheckerContext &C) {
+  SVal V = C.getSVal(E);
+  if (!V.getAs()) {
+ProgramStateRef State = C.getState();
+ProgramStateManager &Mgr = State->getStateManager();
+V = Mgr.getStoreManager().getBinding(State->getStore(), V.castAs());
+  }
+  if (V.getSubKind() == nonloc::ConcreteIntKind) {
+const auto &CI = V.castAs().castAs();
+const llvm::APSInt &I = CI.getValue();
+return &I;
+  }
+  return nullptr;
+}
+
 void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
   ProgramStateRef state = C.getState();
@@ -97,18 +117,44 @@
 }
 
 if (Ex) {
-  OS << "The " << (isLeft ? "left" : "right")
- << " operand of '"
+  OS << "The " << (isLeft ? "left" : "right") << " operand of '"
  << BinaryOperator::getOpcodeStr(B->getOpcode())
  << "' is a garbage value";
   if (isArrayIndexOutOfBounds(C, Ex))
 OS << " due to array index out of bounds";
-}
-else {
+} else {
   // Neither operand was undefined, but the result is undefined.
-  OS << "The result of the '"

[PATCH] D32346: [clang-tidy] New readability check for strlen argument

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki abandoned this revision.
danielmarjamaki added a comment.

I will not continue working on this checker


Repository:
  rL LLVM

https://reviews.llvm.org/D32346



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

[PATCH] D31029: [analyzer] Fix logical not for pointers with different bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
danielmarjamaki marked an inline comment as done.
Closed by commit rL305669: [analyzer] Fix logical not for pointers with 
different bit width (authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D31029?vs=99114&id=102999#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D31029

Files:
  cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp


Index: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -980,10 +980,9 @@
   //transfer functions as "0 == E".
   SVal Result;
   if (Optional LV = V.getAs()) {
-Loc X = svalBuilder.makeNull();
+Loc X = svalBuilder.makeNullWithType(Ex->getType());
 Result = evalBinOp(state, BO_EQ, *LV, X, U->getType());
-  }
-  else if (Ex->getType()->isFloatingType()) {
+  } else if (Ex->getType()->isFloatingType()) {
 // FIXME: handle floating point types.
 Result = UnknownVal();
   } else {
Index: 
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- 
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ 
cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -180,6 +180,11 @@
 return getValue(X);
   }
 
+  inline const llvm::APSInt& getZeroWithTypeSize(QualType T) {
+assert(T->isScalarType());
+return getValue(0, Ctx.getTypeSize(T), true);
+  }
+
   inline const llvm::APSInt& getZeroWithPtrWidth(bool isUnsigned = true) {
 return getValue(0, Ctx.getTypeSize(Ctx.VoidPtrTy), isUnsigned);
   }
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -315,6 +315,13 @@
 return nonloc::ConcreteInt(BasicVals.getTruthValue(b));
   }
 
+  /// Create NULL pointer, with proper pointer bit-width for given address
+  /// space.
+  /// \param type pointer type.
+  Loc makeNullWithType(QualType type) {
+return loc::ConcreteInt(BasicVals.getZeroWithTypeSize(type));
+  }
+
   Loc makeNull() {
 return loc::ConcreteInt(BasicVals.getZeroWithPtrWidth());
   }


Index: cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -980,10 +980,9 @@
   //transfer functions as "0 == E".
   SVal Result;
   if (Optional LV = V.getAs()) {
-Loc X = svalBuilder.makeNull();
+Loc X = svalBuilder.makeNullWithType(Ex->getType());
 Result = evalBinOp(state, BO_EQ, *LV, X, U->getType());
-  }
-  else if (Ex->getType()->isFloatingType()) {
+  } else if (Ex->getType()->isFloatingType()) {
 // FIXME: handle floating point types.
 Result = UnknownVal();
   } else {
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -180,6 +180,11 @@
 return getValue(X);
   }
 
+  inline const llvm::APSInt& getZeroWithTypeSize(QualType T) {
+assert(T->isScalarType());
+return getValue(0, Ctx.getTypeSize(T), true);
+  }
+
   inline const llvm::APSInt& getZeroWithPtrWidth(bool isUnsigned = true) {
 return getValue(0, Ctx.getTypeSize(Ctx.VoidPtrTy), isUnsigned);
   }
Index: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -315,6 +315,13 @@
 return nonloc::ConcreteInt(BasicVals.getTruthValue(b));
   }
 
+  /// Create NULL pointer, with proper pointer bit-width for given address
+  /// space.
+  /// \param type pointer type.
+  Loc makeNullWithType(QualType type) {
+return loc::ConcreteInt(BasicVals.getZeroWithTypeSize(type));
+  }
+
   Loc makeNull() {
 return loc::ConcreteInt(BasicVals.getZeroWithPtrWidth());
   }
___
cfe

[PATCH] D31029: [analyzer] Fix logical not for pointers with different bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

ping


Repository:
  rL LLVM

https://reviews.llvm.org/D31029



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

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

ping


Repository:
  rL LLVM

https://reviews.llvm.org/D30295



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

[PATCH] D31029: [analyzer] Fix logical not for pointers with different bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 99114.
danielmarjamaki added a comment.

Fix review comments


Repository:
  rL LLVM

https://reviews.llvm.org/D31029

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
  lib/StaticAnalyzer/Core/ExprEngineC.cpp


Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -980,10 +980,9 @@
   //transfer functions as "0 == E".
   SVal Result;
   if (Optional LV = V.getAs()) {
-Loc X = svalBuilder.makeNull();
+Loc X = svalBuilder.makeNullWithType(Ex->getType());
 Result = evalBinOp(state, BO_EQ, *LV, X, U->getType());
-  }
-  else if (Ex->getType()->isFloatingType()) {
+  } else if (Ex->getType()->isFloatingType()) {
 // FIXME: handle floating point types.
 Result = UnknownVal();
   } else {
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -315,10 +315,15 @@
 return nonloc::ConcreteInt(BasicVals.getTruthValue(b));
   }
 
-  Loc makeNull() {
-return loc::ConcreteInt(BasicVals.getZeroWithPtrWidth());
+  /// Create NULL pointer, with proper pointer bit-width for given address
+  /// space.
+  /// \param type pointer type.
+  Loc makeNullWithType(QualType type) {
+return loc::ConcreteInt(BasicVals.getZeroWithTypeSize(type));
   }
 
+  Loc makeNull() { return loc::ConcreteInt(BasicVals.getZeroWithPtrWidth()); }
+
   Loc makeLoc(SymbolRef sym) {
 return loc::MemRegionVal(MemMgr.getSymbolicRegion(sym));
   }
Index: include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -180,7 +180,12 @@
 return getValue(X);
   }
 
-  inline const llvm::APSInt& getZeroWithPtrWidth(bool isUnsigned = true) {
+  inline const llvm::APSInt &getZeroWithTypeSize(QualType T) {
+assert(T->isScalarType());
+return getValue(0, Ctx.getTypeSize(T), true);
+  }
+
+  inline const llvm::APSInt &getZeroWithPtrWidth(bool isUnsigned = true) {
 return getValue(0, Ctx.getTypeSize(Ctx.VoidPtrTy), isUnsigned);
   }
 


Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -980,10 +980,9 @@
   //transfer functions as "0 == E".
   SVal Result;
   if (Optional LV = V.getAs()) {
-Loc X = svalBuilder.makeNull();
+Loc X = svalBuilder.makeNullWithType(Ex->getType());
 Result = evalBinOp(state, BO_EQ, *LV, X, U->getType());
-  }
-  else if (Ex->getType()->isFloatingType()) {
+  } else if (Ex->getType()->isFloatingType()) {
 // FIXME: handle floating point types.
 Result = UnknownVal();
   } else {
Index: include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
@@ -315,10 +315,15 @@
 return nonloc::ConcreteInt(BasicVals.getTruthValue(b));
   }
 
-  Loc makeNull() {
-return loc::ConcreteInt(BasicVals.getZeroWithPtrWidth());
+  /// Create NULL pointer, with proper pointer bit-width for given address
+  /// space.
+  /// \param type pointer type.
+  Loc makeNullWithType(QualType type) {
+return loc::ConcreteInt(BasicVals.getZeroWithTypeSize(type));
   }
 
+  Loc makeNull() { return loc::ConcreteInt(BasicVals.getZeroWithPtrWidth()); }
+
   Loc makeLoc(SymbolRef sym) {
 return loc::MemRegionVal(MemMgr.getSymbolicRegion(sym));
   }
Index: include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -180,7 +180,12 @@
 return getValue(X);
   }
 
-  inline const llvm::APSInt& getZeroWithPtrWidth(bool isUnsigned = true) {
+  inline const llvm::APSInt &getZeroWithTypeSize(QualType T) {
+assert(T->isScalarType());
+return getValue(0, Ctx.getTypeSize(T), true);
+  }
+
+  inline const llvm::APSInt &getZeroWithPtrWidth(bool isUnsigned = true) {
 return getValue(0, Ctx.getTypeSize(Ctx.VoidPtrTy

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 99022.
danielmarjamaki added a comment.

renamed exprComparesTo to svalComparesTo


Repository:
  rL LLVM

https://reviews.llvm.org/D30295

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  lib/StaticAnalyzer/Core/CheckerHelpers.cpp
  test/Analysis/bitwise-ops.c

Index: test/Analysis/bitwise-ops.c
===
--- test/Analysis/bitwise-ops.c
+++ test/Analysis/bitwise-ops.c
@@ -29,4 +29,18 @@
   default:
 return 0;
   }
-}
\ No newline at end of file
+}
+
+int testOverflowShift(int a) {
+  if (a == 323) {
+return 1 << a; // expected-warning{{The result of the '<<' expression is undefined due to shift count >= width of type}}
+  }
+  return 0;
+}
+
+int testNegativeShift(int a) {
+  if (a == -5) {
+return 1 << a; // expected-warning{{The result of the '<<' expression is undefined due to negative value on the right side of operand}}
+  }
+  return 0;
+}
Index: lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -94,3 +94,40 @@
 
   return std::make_pair(VD, RHS);
 }
+
+bool clang::ento::svalComparesTo(SVal LHSVal, BinaryOperatorKind ComparisonOp,
+ SVal RHSVal, ProgramStateRef State) {
+
+  if (LHSVal.isUnknownOrUndef())
+return false;
+  ProgramStateManager &Mgr = State->getStateManager();
+  if (!LHSVal.getAs() && LHSVal.getAs()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+  LHSVal.castAs());
+  }
+  if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+return false;
+
+  SValBuilder &Bldr = Mgr.getSValBuilder();
+  SVal Eval = Bldr.evalBinOp(State, ComparisonOp, LHSVal, RHSVal,
+ Bldr.getConditionType());
+  if (Eval.isUnknownOrUndef())
+return false;
+  ProgramStateRef StTrue, StFalse;
+  std::tie(StTrue, StFalse) = State->assume(Eval.castAs());
+  return StTrue && !StFalse;
+}
+
+// Is E value greater or equal than Val?
+bool clang::ento::isGreaterOrEqual(const Expr *E, unsigned long long Val,
+   CheckerContext &C) {
+  DefinedSVal V =
+  C.getSValBuilder().makeIntVal(Val, C.getASTContext().LongLongTy);
+  return svalComparesTo(C.getSVal(E), BO_GE, V, C.getState());
+}
+
+// Is E value negative?
+bool clang::ento::isNegative(const Expr *E, CheckerContext &C) {
+  DefinedSVal V = C.getSValBuilder().makeIntVal(0, false);
+  return svalComparesTo(C.getSVal(E), BO_LT, V, C.getState());
+}
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -17,6 +17,7 @@
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/raw_ostream.h"
@@ -59,6 +60,11 @@
   return StOutBound && !StInBound;
 }
 
+static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) {
+  return isGreaterOrEqual(
+  B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType()), C);
+}
+
 void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
   ProgramStateRef state = C.getState();
@@ -103,12 +109,26 @@
  << "' is a garbage value";
   if (isArrayIndexOutOfBounds(C, Ex))
 OS << " due to array index out of bounds";
-}
-else {
+} else {
   // Neither operand was undefined, but the result is undefined.
-  OS << "The result of the '"
- << BinaryOperator::getOpcodeStr(B->getOpcode())
- << "' expression is undefined";
+  if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+   B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+  isNegative(B->getRHS(), C)) {
+OS << "The result of the '"
+   << BinaryOperator::getOpcodeStr(B->getOpcode())
+   << "' expression is undefined due to negative value on the right "
+  "side of operand";
+  } else if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+  B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+ isShiftOverflow(B, C)) {
+OS << "The result of the '"
+   << BinaryOperator::getOpcodeStr(B->getOpcode())
+   << "' expression is undefined due to shift count >= width of type";
+  } else {
+

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 98970.
danielmarjamaki added a comment.

minor tweak


Repository:
  rL LLVM

https://reviews.llvm.org/D30295

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  lib/StaticAnalyzer/Core/CheckerHelpers.cpp
  test/Analysis/bitwise-ops.c

Index: test/Analysis/bitwise-ops.c
===
--- test/Analysis/bitwise-ops.c
+++ test/Analysis/bitwise-ops.c
@@ -29,4 +29,18 @@
   default:
 return 0;
   }
-}
\ No newline at end of file
+}
+
+int testOverflowShift(int a) {
+  if (a == 323) {
+return 1 << a; // expected-warning{{The result of the '<<' expression is undefined due to shift count >= width of type}}
+  }
+  return 0;
+}
+
+int testNegativeShift(int a) {
+  if (a == -5) {
+return 1 << a; // expected-warning{{The result of the '<<' expression is undefined due to negative value on the right side of operand}}
+  }
+  return 0;
+}
Index: lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -94,3 +94,40 @@
 
   return std::make_pair(VD, RHS);
 }
+
+bool clang::ento::exprComparesTo(SVal LHSVal, BinaryOperatorKind ComparisonOp,
+ SVal RHSVal, ProgramStateRef State) {
+
+  if (LHSVal.isUnknownOrUndef())
+return false;
+  ProgramStateManager &Mgr = State->getStateManager();
+  if (!LHSVal.getAs() && LHSVal.getAs()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+  LHSVal.castAs());
+  }
+  if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+return false;
+
+  SValBuilder &Bldr = Mgr.getSValBuilder();
+  SVal Eval = Bldr.evalBinOp(State, ComparisonOp, LHSVal, RHSVal,
+ Bldr.getConditionType());
+  if (Eval.isUnknownOrUndef())
+return false;
+  ProgramStateRef StTrue, StFalse;
+  std::tie(StTrue, StFalse) = State->assume(Eval.castAs());
+  return StTrue && !StFalse;
+}
+
+// Is E value greater or equal than Val?
+bool clang::ento::isGreaterOrEqual(const Expr *E, unsigned long long Val,
+   CheckerContext &C) {
+  DefinedSVal V =
+  C.getSValBuilder().makeIntVal(Val, C.getASTContext().LongLongTy);
+  return exprComparesTo(C.getSVal(E), BO_GE, V, C.getState());
+}
+
+// Is E value negative?
+bool clang::ento::isNegative(const Expr *E, CheckerContext &C) {
+  DefinedSVal V = C.getSValBuilder().makeIntVal(0, false);
+  return exprComparesTo(C.getSVal(E), BO_LT, V, C.getState());
+}
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -17,6 +17,7 @@
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/raw_ostream.h"
@@ -59,6 +60,11 @@
   return StOutBound && !StInBound;
 }
 
+static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) {
+  return isGreaterOrEqual(
+  B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType()), C);
+}
+
 void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
   ProgramStateRef state = C.getState();
@@ -106,9 +112,24 @@
 }
 else {
   // Neither operand was undefined, but the result is undefined.
-  OS << "The result of the '"
- << BinaryOperator::getOpcodeStr(B->getOpcode())
- << "' expression is undefined";
+  if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+   B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+  isNegative(B->getRHS(), C)) {
+OS << "The result of the '"
+   << BinaryOperator::getOpcodeStr(B->getOpcode())
+   << "' expression is undefined due to negative value on the right "
+  "side of operand";
+  } else if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+  B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+ isShiftOverflow(B, C)) {
+OS << "The result of the '"
+   << BinaryOperator::getOpcodeStr(B->getOpcode())
+   << "' expression is undefined due to shift count >= width of type";
+  } else {
+OS << "The result of the '"
+   << BinaryOperator::getOpcodeStr(B->getOpcode())
+   << "' expression is undefined";
+  }
 }
 auto report = llvm::mak

[PATCH] D31650: [Analyzer] Detect when function pointer is freed

[Daniel Marjamäki via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL301913: [analyzer] Detect bad free of function pointers 
(authored by danielmarjamaki).

Changed prior to commit:
  https://reviews.llvm.org/D31650?vs=95929&id=97432#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D31650

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  cfe/trunk/test/Analysis/malloc.c


Index: cfe/trunk/test/Analysis/malloc.c
===
--- cfe/trunk/test/Analysis/malloc.c
+++ cfe/trunk/test/Analysis/malloc.c
@@ -1774,6 +1774,16 @@
return ok; // no warning
 }
 
+void (*fnptr)(int);
+void freeIndirectFunctionPtr() {
+  void *p = (void *)fnptr;
+  free(p); // expected-warning {{Argument to free() is a function pointer}}
+}
+
+void freeFunctionPtr() {
+  free((void *)fnptr); // expected-warning {{Argument to free() is a function 
pointer}}
+}
+
 // 
 // False negatives.
 
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -401,6 +401,9 @@
   void ReportUseZeroAllocated(CheckerContext &C, SourceRange Range,
   SymbolRef Sym) const;
 
+  void ReportFunctionPointerFree(CheckerContext &C, SVal ArgVal,
+ SourceRange Range, const Expr *FreeExpr) 
const;
+
   /// Find the location of the allocation for Sym on the path leading to the
   /// exploded node N.
   LeakInfo getAllocationSite(const ExplodedNode *N, SymbolRef Sym,
@@ -1564,6 +1567,11 @@
 }
   }
 
+  if (SymBase->getType()->isFunctionPointerType()) {
+ReportFunctionPointerFree(C, ArgVal, ArgExpr->getSourceRange(), 
ParentExpr);
+return nullptr;
+  }
+
   ReleasedAllocated = (RsBase != nullptr) && (RsBase->isAllocated() ||
   RsBase->isAllocatedOfSizeZero());
 
@@ -2024,10 +2032,45 @@
   }
 }
 
+void MallocChecker::ReportFunctionPointerFree(CheckerContext &C, SVal ArgVal,
+  SourceRange Range,
+  const Expr *FreeExpr) const {
+  if (!ChecksEnabled[CK_MallocChecker])
+return;
+
+  Optional CheckKind = getCheckIfTracked(C, 
FreeExpr);
+  if (!CheckKind.hasValue())
+return;
+
+  if (ExplodedNode *N = C.generateErrorNode()) {
+if (!BT_BadFree[*CheckKind])
+  BT_BadFree[*CheckKind].reset(
+  new BugType(CheckNames[*CheckKind], "Bad free", "Memory Error"));
+
+SmallString<100> Buf;
+llvm::raw_svector_ostream Os(Buf);
+
+const MemRegion *MR = ArgVal.getAsRegion();
+while (const ElementRegion *ER = dyn_cast_or_null(MR))
+  MR = ER->getSuperRegion();
+
+Os << "Argument to ";
+if (!printAllocDeallocName(Os, C, FreeExpr))
+  Os << "deallocator";
+
+Os << " is a function pointer";
+
+auto R = llvm::make_unique(*BT_BadFree[*CheckKind], Os.str(), 
N);
+R->markInteresting(MR);
+R->addRange(Range);
+C.emitReport(std::move(R));
+  }
+}
+
 ProgramStateRef MallocChecker::ReallocMemAux(CheckerContext &C,
  const CallExpr *CE,
  bool FreesOnFail,
- ProgramStateRef State, 
+ ProgramStateRef State,
  bool SuffixWithN) const {
   if (!State)
 return nullptr;


Index: cfe/trunk/test/Analysis/malloc.c
===
--- cfe/trunk/test/Analysis/malloc.c
+++ cfe/trunk/test/Analysis/malloc.c
@@ -1774,6 +1774,16 @@
return ok; // no warning
 }
 
+void (*fnptr)(int);
+void freeIndirectFunctionPtr() {
+  void *p = (void *)fnptr;
+  free(p); // expected-warning {{Argument to free() is a function pointer}}
+}
+
+void freeFunctionPtr() {
+  free((void *)fnptr); // expected-warning {{Argument to free() is a function pointer}}
+}
+
 // 
 // False negatives.
 
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -401,6 +401,9 @@
   void ReportUseZeroAllocated(CheckerContext &C, SourceRange Range,
   SymbolRef Sym) const;
 
+  void ReportFunctionPointerFree(CheckerContext &C, SVal ArgVal,
+ SourceRange Range, const Expr *FreeExpr) const;
+
   /// Find the location of the allocation for Sym on the path leading to the
   /// exploded node N.
   LeakInfo g

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added inline comments.



Comment at: lib/StaticAnalyzer/Core/CheckerHelpers.cpp:116
+return false;
+  ConstraintManager &CM = State->getConstraintManager();
+  ProgramStateRef StTrue, StFalse;

xazax.hun wrote:
> Any reason why do you get the constraint manager and not using 
> ProgramState::assume?
Mostly that it's just 1 call instead of 2. assumeDual() has some extra logic 
(early return , assertion). are there some special reason to use assume()?


Repository:
  rL LLVM

https://reviews.llvm.org/D30295



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

[PATCH] D30771: [analyzer] Teach the MallocChecker about Glib API for two arguments

[Daniel Marjamäki via Phabricator via cfe-commits]

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

If you have svn write permission then please do it.

If you need svn write permission, please see 
http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



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

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki marked an inline comment as done.
danielmarjamaki added a comment.

Ping.


Repository:
  rL LLVM

https://reviews.llvm.org/D30295



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

[PATCH] D30489: [analyzer] catch out of bounds for VLA

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.
This revision now requires changes to proceed.

Ping. Any comments?


Repository:
  rL LLVM

https://reviews.llvm.org/D30489



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

[PATCH] D31029: [analyzer] Fix logical not for pointers with different bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

Ping


Repository:
  rL LLVM

https://reviews.llvm.org/D31029



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

[PATCH] D30691: [analyzer] Support for naive cross translational unit analysis

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added inline comments.



Comment at: lib/AST/ASTContext.cpp:1495
+  ASTUnit *Unit = nullptr;
+  StringRef ASTFileName;
+  auto FnUnitCacheEntry = FunctionAstUnitMap.find(MangledFnName);

As far as I see you can move this ASTFileName declaration down.



Comment at: lib/AST/ASTContext.cpp:1497
+  auto FnUnitCacheEntry = FunctionAstUnitMap.find(MangledFnName);
+  if (FnUnitCacheEntry == FunctionAstUnitMap.end()) {
+if (FunctionFileMap.empty()) {

How about replacing FunctionAstUnitMap.find() with FunctionAstUnitMap.count()?




Comment at: lib/AST/ASTContext.cpp:1511
+auto It = FunctionFileMap.find(MangledFnName);
+if (It != FunctionFileMap.end())
+  ASTFileName = It->second;

As far as I see this can be changed to:
```
  if (It == FunctionFileMap.end())
return nullptr;
  const StringRef ASTFileName = It->second;
```



Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:48
+  : Ctx(Context), ItaniumCtx(MangleCtx) {}
+  std::string CurrentFileName;
+

As far I see CurrentFileName is only used in 1 method and should therefore be 
private.



Comment at: tools/clang-func-mapping/ClangFnMapGen.cpp:86
+  SM.getFileEntryForID(SM.getMainFileID())->getName();
+  char *Path = realpath(SMgrName.str().c_str(), nullptr);
+  CurrentFileName = Path;

I believe realpath is a posix function.


https://reviews.llvm.org/D30691



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

[PATCH] D32346: [clang-tidy] New readability check for strlen argument

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added inline comments.



Comment at: docs/clang-tidy/checks/readability-strlen-argument.rst:8
+
+In the example code below the developer probably wanted to make room for an 
extra char in the allocation but misplaced the addition.
+

JonasToth wrote:
> when the intend was to allocate one more char, he would need to do `strlen(s) 
> + 1`, why is it changed to subtraction then?
If I change it to strlen(s) + 1 then the logic of the program is changed.

If I change it to subtraction then the logic is the same and the program is 
still wrong, but imho it is easier to see.



Comment at: docs/clang-tidy/checks/readability-strlen-argument.rst:20
+char *p = new char[(strlen(s) - 1)]
+strcpy(p, s);
+

JonasToth wrote:
> isnt that an overflow?
> an example:
> `strlen(s) == 10` -> `p` will be 9 characters long, since its substracted 
> with `1`.
> 
> the copy operation will then copy the content of `s` into `p`, therefore 
> copying 10 characters into a buffer of length 9.
> 
> as i understand it `strcpy(p, s + 1)` would be correct with the sizes.
yes it is overflow. My intention was to show that strlen(s+1) syntax is 
dangerous.



Comment at: test/clang-tidy/readability-strlen-argument.cpp:11
+  X = strlen(Str + 10);
+  // CHECK-FIXES: {{^}}  X = (strlen(Str) - 10);{{$}}
+

JonasToth wrote:
> sberg wrote:
> > but if any of the first ten chars in Str can be NUL, then the change is 
> > bogus?  (I guess I fail to see the reason for this check)
> intersting point. If the Array holds a long list of string concatenated and 
> seperated by `0`, the offsetting would be a valid usecase.
> (are strings in programm executables stored like this?)
> 
> But i have no idea if that is actually a scenario. usually i use 
> `std::string` :)
I think that in theory, you have a point. It would be best that such users 
don't use this check. I doubt that is a big problem in practice. We don't need 
to speculate much, I will test this on all debian source code..

It's possible that strings in program executables are stored like that, but I'd 
say it's ub to calculate the address Str+10 and then dereference that if Str is 
a string that is 10 bytes long.



Repository:
  rL LLVM

https://reviews.llvm.org/D32346



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

[PATCH] D32346: [clang-tidy] New readability check for strlen argument

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

I am thinking about making my check more strict so it only warns in 
allocations. I believe the example code is much more motivating when there is 
allocation.

In https://reviews.llvm.org/D32346#733430, @JonasToth wrote:

> My thoughts on the check added.
>  Have you run it over a big codebase? What is the turnout?
>
> And please add the check to the ReleaseNotes.


I have a script that runs clang/clang-tidy on all debian source code. It 
basically grabs all packages and if it has a configure script it runs:  
./configure && bear make && clang-tidy  .. Running that right now.

It goes slowly I have run clang-tidy on 22 packages (735 files) so far and got 
13 warnings.

Unfortunately most warnings so far are false positives as in this example code:

  #include 
  void dostuff(const char *P) {
if (strncmp(P+2,"xyz",3)==0) {}
  }

Without -O2 I get no warning. With -O2 I get a false positive:

  danielm@debian:~$ ~/llvm/build/bin/clang-tidy 
-checks=-*,readability-strlen-argument strlen.c -- -O2
  1 warning generated.
  /home/danielm/strlen.c:3:16: warning: strlen() argument has pointer addition, 
it is recommended to subtract the result instead. [readability-strlen-argument]
if (strncmp(P+2,"xyz",3)==0) {}
 ^

When the -O2 flag is used then on my machine the strncmp() function call is 
expanded to lots of code. and in that code, there are calls to strlen().

I should probably avoid these, I guess skipping all warnings in macro code 
sounds ok to me.


Repository:
  rL LLVM

https://reviews.llvm.org/D32346



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

[PATCH] D32346: [clang-tidy] New readability check for strlen argument

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 96526.
danielmarjamaki added a comment.

Fixed review comments. Made code examples and documentation more motivational.


Repository:
  rL LLVM

https://reviews.llvm.org/D32346

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/StrlenArgumentCheck.cpp
  clang-tidy/readability/StrlenArgumentCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-strlen-argument.rst
  test/clang-tidy/readability-strlen-argument.cpp

Index: test/clang-tidy/readability-strlen-argument.cpp
===
--- test/clang-tidy/readability-strlen-argument.cpp
+++ test/clang-tidy/readability-strlen-argument.cpp
@@ -0,0 +1,31 @@
+// RUN: %check_clang_tidy %s readability-strlen-argument %t
+
+namespace std {
+typedef __typeof(sizeof(int)) size_t;
+size_t strlen(const char *s);
+}; // namespace std
+using namespace std;
+
+void warn1(const char *Str) {
+  char *P;
+
+  // CHECK-MESSAGES: :[[@LINE+1]]:27: warning: strlen() argument has pointer addition, it is recommended to subtract the result instead. [readability-strlen-argument]
+  P = new char[strlen(Str + 1)];
+  // CHECK-FIXES: {{^}}  P = new char[(strlen(Str) - 1)];{{$}}
+  delete[] P;
+
+  // CHECK-MESSAGES: :[[@LINE+1]]:30: warning: strlen() argument has pointer addition, it is recommended to subtract the result instead. [readability-strlen-argument]
+  P = new char[std::strlen(1 + Str)];
+  delete[] P;
+}
+
+struct S {
+  char *P;
+};
+void warn2(const struct S &Par) {
+  char *P;
+  // CHECK-MESSAGES: :[[@LINE+1]]:34: warning: strlen() argument has pointer addition, it is recommended to subtract the result instead. [readability-strlen-argument]
+  P = new char[std::strlen(Par.P + 1)];
+  // CHECK-FIXES: {{^}}  P = new char[(std::strlen(Par.P) - 1)];{{$}}
+  delete[] P;
+}
Index: docs/clang-tidy/checks/readability-strlen-argument.rst
===
--- docs/clang-tidy/checks/readability-strlen-argument.rst
+++ docs/clang-tidy/checks/readability-strlen-argument.rst
@@ -0,0 +1,22 @@
+.. title:: clang-tidy - readability-strlen-argument
+
+readability-strlen-argument
+===
+
+This check will detect addition in `strlen()` argument.
+
+In the example code below the developer probably wanted to make room for an extra char in the allocation but misplaced the addition.
+
+.. code-block:: c++
+
+char *p = new char[strlen(s + 1)];
+strcpy(p, s);
+
+With -fix that becomes:
+
+.. code-block:: c++
+
+char *p = new char[(strlen(s) - 1)]
+strcpy(p, s);
+
+For readability it is considered better to subtract the result outside the `strlen()`.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -169,4 +169,5 @@
readability-redundant-string-init
readability-simplify-boolean-expr
readability-static-definition-in-anonymous-namespace
+   readability-strlen-argument
readability-uniqueptr-delete-release
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -76,6 +76,12 @@
 
   Finds functions that have more then `ParameterThreshold` parameters and emits a warning.
 
+- New `readability-strlen-argument
+  `_ check
+
+  Finds misleading `strlen()` argument. If addition is used in the argument then the effect is that
+  the strlen() result is subtracted. 
+
 - New `hicpp` module
 
   Adds checks that implement the `High Integrity C++ Coding Standard `_ and other safety
Index: clang-tidy/readability/StrlenArgumentCheck.h
===
--- clang-tidy/readability/StrlenArgumentCheck.h
+++ clang-tidy/readability/StrlenArgumentCheck.h
@@ -0,0 +1,35 @@
+//===--- StrlenArgumentCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRLEN_ARGUMENT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRLEN_ARGUMENT_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Detect pointer addition in strlen() argument.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-strlen-argument.html
+class StrlenArgumentCheck : public ClangTidyCheck {
+public:
+  StrlenArgume

[PATCH] D32346: [clang-tidy] New readability check for strlen argument

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

Thanks for all comments. I am working on fixing them. Updated patch will be 
uploaded soon.

> Have you run it over a big codebase? What is the turnout?

I did not see this warning yet. Maybe after fixing the comments (::strlen) 
there will be a difference.

In https://reviews.llvm.org/D32346#733906, @Eugene.Zelenko wrote:

> It may be good idea to add check for arguments which taken from C++ 
> containers like std::string, std::string_view, etc.


Not sure how you want. Do you envision something like:

  std::string Par;
  strlen(Par.c_str() + 1);




Comment at: test/clang-tidy/readability-strlen-argument.cpp:1
+// RUN: %check_clang_tidy %s readability-strlen-argument %t
+

JonasToth wrote:
> Same as documentation, maybe a little more telling examples to test on.
> What happens with `char**`, an array of strings? Accessing those one by one 
> would be possible with an addition or subscriptoperation.
how do you mean with char**. If you access those one by one it will look 
something like this right?

char **A;
strlen(*(A+N));

such code is not matched as far as I see.


Repository:
  rL LLVM

https://reviews.llvm.org/D32346



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

[PATCH] D32346: [clang-tidy] New readability check for strlen argument

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki created this revision.
Herald added a subscriber: mgorny.

I propose this new readability checker.


Repository:
  rL LLVM

https://reviews.llvm.org/D32346

Files:
  clang-tidy/readability/CMakeLists.txt
  clang-tidy/readability/ReadabilityTidyModule.cpp
  clang-tidy/readability/StrlenArgumentCheck.cpp
  clang-tidy/readability/StrlenArgumentCheck.h
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/readability-strlen-argument.rst
  test/clang-tidy/readability-strlen-argument.cpp

Index: test/clang-tidy/readability-strlen-argument.cpp
===
--- test/clang-tidy/readability-strlen-argument.cpp
+++ test/clang-tidy/readability-strlen-argument.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s readability-strlen-argument %t
+
+typedef __typeof(sizeof(int)) size_t;
+size_t strlen(const char *s);
+
+void warn1(const char *Str) {
+  int X;
+
+  // CHECK-MESSAGES: :[[@LINE+1]]:18: warning: strlen() argument has pointer addition, it is recommended to subtract the result instead. [readability-strlen-argument]
+  X = strlen(Str + 10);
+  // CHECK-FIXES: {{^}}  X = (strlen(Str) - 10);{{$}}
+
+  // CHECK-MESSAGES: :[[@LINE+1]]:17: warning: strlen() argument has pointer addition, it is recommended to subtract the result instead. [readability-strlen-argument]
+  X = strlen(10 + Str);
+}
+
+struct S {
+  char *P;
+};
+void warn2(const struct S &Par) {
+  int X;
+  // CHECK-MESSAGES: :[[@LINE+1]]:20: warning: strlen() argument has pointer addition, it is recommended to subtract the result instead. [readability-strlen-argument]
+  X = strlen(Par.P + 10);
+  // CHECK-FIXES: {{^}}  X = (strlen(Par.P) - 10);{{$}}
+}
Index: docs/clang-tidy/checks/readability-strlen-argument.rst
===
--- docs/clang-tidy/checks/readability-strlen-argument.rst
+++ docs/clang-tidy/checks/readability-strlen-argument.rst
@@ -0,0 +1,18 @@
+.. title:: clang-tidy - readability-strlen-argument
+
+readability-strlen-argument
+===
+
+This checker will detect addition in strlen() argument. Example code:
+
+.. code-block:: c++
+
+strlen(str + 10)
+
+With -fix that becomes:
+
+.. code-block:: c++
+
+(strlen(str) - 10)
+
+For readability it is considered better to subtract the result outside the strlen().
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -169,4 +169,5 @@
readability-redundant-string-init
readability-simplify-boolean-expr
readability-static-definition-in-anonymous-namespace
+   readability-strlen-argument
readability-uniqueptr-delete-release
Index: clang-tidy/readability/StrlenArgumentCheck.h
===
--- clang-tidy/readability/StrlenArgumentCheck.h
+++ clang-tidy/readability/StrlenArgumentCheck.h
@@ -0,0 +1,35 @@
+//===--- StrlenArgumentCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRLEN_ARGUMENT_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRLEN_ARGUMENT_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace readability {
+
+/// Detect pointer addition in strlen() argument.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/readability-strlen-argument.html
+class StrlenArgumentCheck : public ClangTidyCheck {
+public:
+  StrlenArgumentCheck(StringRef Name, ClangTidyContext *Context)
+  : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace readability
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_READABILITY_STRLEN_ARGUMENT_H
Index: clang-tidy/readability/StrlenArgumentCheck.cpp
===
--- clang-tidy/readability/StrlenArgumentCheck.cpp
+++ clang-tidy/readability/StrlenArgumentCheck.cpp
@@ -0,0 +1,48 @@
+//===--- StrlenArgumentCheck.cpp - clang-tidy--===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "StrlenArgumentCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+u

[PATCH] D30771: [analyzer] Teach the MallocChecker about Glib API for two arguments

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

you can ignore my comment ... LGTM

I don't want to accept this revision. Hopefully NoQ or somebody else can do 
that.


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



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

[PATCH] D30771: [analyzer] Teach the MallocChecker about Glib API for two arguments

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

I don't have further comments except that I would personally rewrite:

  // Get the value of the size argument.
  SVal TotalSize = State->getSVal(Arg1, LCtx);
  if (SuffixWithN) {
const Expr *Arg2 = CE->getArg(2);
TotalSize = evalMulForBufferSize(C, Arg1, Arg2);
  }

to:

  // Get the value of the size argument.
  SVal TotalSize;
  if (!SuffixWithN) {
TotalSize = State->getSVal(Arg1, LCtx);
  } else {
TotalSize = evalMulForBufferSize(C, Arg1, CE->getArg(2));
  }


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



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

[PATCH] D30771: [analyzer] Teach the MallocChecker about Glib API for two arguments

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:337
+   const Expr *BlockBytes,
+   ProgramStateRef State);
   static ProgramStateRef CallocMem(CheckerContext &C, const CallExpr *CE,

Thanks for renaming the function I am happy now with that name. :-)

hmm.. if you have CheckerContext parameter already then ProgramStateRef 
parameter seems redundant. You should be able to use C.getState().

However looking at surrounding code it seems you can provide ProgramStateRef 
for consistency.

I don't have a strong opinion, but I would remove this State.



Repository:
  rL LLVM

https://reviews.llvm.org/D30771



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

[PATCH] D30771: [analyzer] Teach the MallocChecker about Glib API for two arguments

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:788
 
+SVal MallocChecker::SValBinMulOp(CheckerContext &C, const Expr *Blocks,
+ const Expr *BlockBytes, ProgramStateRef 
State) {

danielmarjamaki wrote:
> I have the feeling this should be renamed. Since its purpose is to calculate 
> the total size maybe MallocChecker::calculateBufferSize()
please respond to comments by clicking on the "Reply" button for the comment. 
So your responce will be shown near my comment.

yes it can be hard to figure out a good name.

alright, you didn't like my suggestion then let's skip that.

do you need to start the name with "sval". does that indicate that a "sval" is 
returned? then that would be unusual imho.

I guess I don't have a strong opinion but I would also remove "Bin". The "Mul" 
says that imho.

how about evalMulForBufferSize?



Repository:
  rL LLVM

https://reviews.llvm.org/D30771



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

[PATCH] D30771: [analyzer] Teach the MallocChecker about Glib API for two arguments

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki added a comment.

> I hold the view that I need to respect original developers' code, and it need 
> a Global Patch for Capital variable, just like KDE's Use nullptr everywhere

Yes I was too picky. Please respect the original developers' code.




Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2040
 
   const Expr *arg0Expr = CE->getArg(0);
   const LocationContext *LCtx = C.getLocationContext();

danielmarjamaki wrote:
> Capital variable: arg0Expr, arg0Val
sorry this is not your code. I remove my comment.


Repository:
  rL LLVM

https://reviews.llvm.org/D30771



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

[PATCH] D30771: [analyzer] Teach the MallocChecker about Glib API for two arguments

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki requested changes to this revision.
danielmarjamaki added inline comments.
This revision now requires changes to proceed.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:788
 
+SVal MallocChecker::SValBinMulOp(CheckerContext &C, const Expr *Blocks,
+ const Expr *BlockBytes, ProgramStateRef 
State) {

I have the feeling this should be renamed. Since its purpose is to calculate 
the total size maybe MallocChecker::calculateBufferSize()



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:790
+ const Expr *BlockBytes, ProgramStateRef 
State) {
+  SValBuilder &svalBuilder = C.getSValBuilder();
+  SVal nBlocks = C.getSVal(Blocks);

Please begin variables with Capitals. svalBuilder,nBlocks,nBlockBytes



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:911
+  if (FunI == II_g_malloc0_n || FunI == II_g_try_malloc0_n) {
+SValBuilder &svalBuilder = C.getSValBuilder();
+Init = svalBuilder.makeZeroVal(svalBuilder.getContext().CharTy);

Capital. svalBuilder



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2040
 
   const Expr *arg0Expr = CE->getArg(0);
   const LocationContext *LCtx = C.getLocationContext();

Capital variable: arg0Expr, arg0Val



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2054
   const Expr *Arg1 = CE->getArg(1);
   if (!Arg1)
 return nullptr;

is this "if (!Arg1)" needed? It seems to me that if CE->getNumArgs() returns >= 
2 then CE->getArg(1) should be non-NULL. Does it crash/assert if you remove 
this condition?



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2061
+const Expr *Arg2 = CE->getArg(2);
+if (!Arg2)
+  return nullptr;

hmm.. seems weird if CE->getArg(2) returns nullptr even though CE->getNumArgs() 
>= 3 .. is it possible to remove this check or does that cause some crash etc?

> Generally, i'd prefer the code for computing the buffer size to be structured 
> as

I would also prefer such code.



Repository:
  rL LLVM

https://reviews.llvm.org/D30771



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

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki marked 4 inline comments as done.
danielmarjamaki added inline comments.



Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h:46
 
-} // end GR namespace
+bool isExprResultConformsComparisonRule(CheckerContext &C,
+BinaryOperatorKind CompRule,

zaks.anna wrote:
> NoQ wrote:
> > Because you are making these functions public, i think it's worth it to 
> > make it obvious what they do without looking at the particular checker 
> > code. Comments are definitely worth it. I think function names could be 
> > worded better; i suggest `exprComparesTo(const Expr *LHSExpr, 
> > BinaryOperatorKind ComparisonOp, SVal RHSVal, CheckerContext &C);` and 
> > `isGreaterOrEqual(...)`; i'm personally fond of having CheckerContext at 
> > the back because that's where we have it in checker callbacks, but that's 
> > not important.
> + 1 on Artem's comment of making the names more clear and providing 
> documentation. Also, should these be part of CheckerContext?
> Also, should these be part of CheckerContext?

I chose not to. Then as NoQ suggested it's not possible to use this when 
CheckerContext is not available:

"The good thing about requiring only State is that we'd be able to use these 
functions in checker callbacks that don't provide the CheckerContext object, 
eg. checkEndAnalysis or checkRegionChanges."


Repository:
  rL LLVM

https://reviews.llvm.org/D30295



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

[PATCH] D30295: [analyzer] clarify undef shift result when shift count is negative or exceeds the bit width

[Daniel Marjamäki via Phabricator via cfe-commits]

danielmarjamaki updated this revision to Diff 95740.
danielmarjamaki added a comment.

Fix review comments

- renamed
- reorder function arguments (CheckerContext last)


Repository:
  rL LLVM

https://reviews.llvm.org/D30295

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
  lib/StaticAnalyzer/Checkers/ConversionChecker.cpp
  lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
  lib/StaticAnalyzer/Core/CheckerHelpers.cpp
  test/Analysis/bitwise-ops.c

Index: test/Analysis/bitwise-ops.c
===
--- test/Analysis/bitwise-ops.c
+++ test/Analysis/bitwise-ops.c
@@ -29,4 +29,18 @@
   default:
 return 0;
   }
-}
\ No newline at end of file
+}
+
+int testOverflowShift(int a) {
+  if (a == 323) {
+return 1 << a; // expected-warning{{The result of the '<<' expression is undefined due to shift count >= width of type}}
+  }
+  return 0;
+}
+
+int testNegativeShift(int a) {
+  if (a == -5) {
+return 1 << a; // expected-warning{{The result of the '<<' expression is undefined due to negative value on the right side of operand}}
+  }
+  return 0;
+}
Index: lib/StaticAnalyzer/Core/CheckerHelpers.cpp
===
--- lib/StaticAnalyzer/Core/CheckerHelpers.cpp
+++ lib/StaticAnalyzer/Core/CheckerHelpers.cpp
@@ -94,3 +94,41 @@
 
   return std::make_pair(VD, RHS);
 }
+
+bool clang::ento::exprComparesTo(SVal LHSVal, BinaryOperatorKind ComparisonOp,
+ SVal RHSVal, ProgramStateRef State) {
+
+  if (LHSVal.isUnknownOrUndef())
+return false;
+  ProgramStateManager &Mgr = State->getStateManager();
+  if (!LHSVal.getAs() && LHSVal.getAs()) {
+LHSVal = Mgr.getStoreManager().getBinding(State->getStore(),
+  LHSVal.castAs());
+  }
+  if (LHSVal.isUnknownOrUndef() || !LHSVal.getAs())
+return false;
+
+  SValBuilder &Bldr = Mgr.getSValBuilder();
+  SVal Eval = Bldr.evalBinOp(State, ComparisonOp, LHSVal, RHSVal,
+ Bldr.getConditionType());
+  if (Eval.isUnknownOrUndef())
+return false;
+  ConstraintManager &CM = State->getConstraintManager();
+  ProgramStateRef StTrue, StFalse;
+  std::tie(StTrue, StFalse) = CM.assumeDual(State, Eval.castAs());
+  return StTrue && !StFalse;
+}
+
+// Is E value greater or equal than Val?
+bool clang::ento::isGreaterOrEqual(const Expr *E, unsigned long long Val,
+   CheckerContext &C) {
+  DefinedSVal V =
+  C.getSValBuilder().makeIntVal(Val, C.getASTContext().LongLongTy);
+  return exprComparesTo(C.getSVal(E), BO_GE, V, C.getState());
+}
+
+// Is E value negative?
+bool clang::ento::isNegative(const Expr *E, CheckerContext &C) {
+  DefinedSVal V = C.getSValBuilder().makeIntVal(0, false);
+  return exprComparesTo(C.getSVal(E), BO_LT, V, C.getState());
+}
Index: lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
+++ lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
@@ -17,6 +17,7 @@
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/CheckerManager.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/raw_ostream.h"
@@ -59,6 +60,11 @@
   return StOutBound && !StInBound;
 }
 
+static bool isShiftOverflow(const BinaryOperator *B, CheckerContext &C) {
+  return isGreaterOrEqual(
+  B->getRHS(), C.getASTContext().getIntWidth(B->getLHS()->getType()), C);
+}
+
 void UndefResultChecker::checkPostStmt(const BinaryOperator *B,
CheckerContext &C) const {
   ProgramStateRef state = C.getState();
@@ -106,9 +112,24 @@
 }
 else {
   // Neither operand was undefined, but the result is undefined.
-  OS << "The result of the '"
- << BinaryOperator::getOpcodeStr(B->getOpcode())
- << "' expression is undefined";
+  if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+   B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+  isNegative(B->getRHS(), C)) {
+OS << "The result of the '"
+   << BinaryOperator::getOpcodeStr(B->getOpcode())
+   << "' expression is undefined due to negative value on the right "
+  "side of operand";
+  } else if ((B->getOpcode() == BinaryOperatorKind::BO_Shl ||
+  B->getOpcode() == BinaryOperatorKind::BO_Shr) &&
+ isShiftOverflow(B, C)) {
+OS << "The result of the '"
+   << BinaryOperator::getOpcodeStr(B->getOpcode())
+   << "' expression is undefined due to shift count >= width of type";
+  } else {
+OS << "The result of the '"
+   <