[PATCH] D67149: Fix argument order for BugType instation for

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

Good catch, the fix is reasonable to me. However, I'm no longer wok on the 
analyzer now, maybe you can add @NoQ  Or some other active reviewers to review 
the code.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67149



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

[PATCH] D55388: [analyzer] MoveChecker Pt.8: Add checks for dereferencing a smart pointer after move.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In D55388#1322601 , @xazax.hun wrote:

> Hm. I wonder if it would also make sense to model e.g. the get method to 
> return nullptr for moved from smart ptrs. This could help null dereference 
> checker and also aid false path prunning.


Great idea!

IIRC, we haven't model smart-pointer yet. There are no warnings fired in the 
simple code below.

  int foo() {
  auto ptr = std::make_unique(0);
  ptr = nullptr;
  return *ptr;  // <- Should emit a warning here
  }

If we want to model the `get()` method, not only the move action but also the 
assignment should be considered. `std::unique_ptr` may be fine, but is the 
reference count stuff too complex for `std::shared_ptr` to get the information 
available for analysis? Can the `lifetime` analysis cover the smart pointers?

  int unique_ptr_bad() {
  auto ptr = std::make_unique(0);
  ptr = nullptr;
  int *raw_p = ptr.get();   // <- 'raw_p' is definitely null here
  return *raw_p;  // <- Should emit a warning here
  }

Related dev-mail about smart pointer checkers, see 
http://lists.llvm.org/pipermail/cfe-dev/2012-January/019446.html


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

https://reviews.llvm.org/D55388



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

[PATCH] D55388: [analyzer] MoveChecker Pt.8: Add checks for dereferencing a smart pointer after move.

[Henry Wong via Phabricator via cfe-commits]

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



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:90
   // in a valid manner.
   // TODO: We can still try to identify *unsafe* use after move, such as
   // dereference of a moved-from smart pointer (which is guaranteed to be 
null).

Outdated `TODO`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D55388



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

[PATCH] D55226: [Fix][StaticAnalyzer] Bug 39792 - False positive on strcpy targeting struct member

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

Please add more context using the `-U` option, like `git diff -U9 ...`.

In D55226#1317119 , @Pierre-vh wrote:

> Sadly I'm not experienced enough to think of every case that should pass this 
> check, so I limited myself to just fixing the bug.
>  Can't we totally remove the outer if so we allow every `Target` expression 
> that has a `ConstantArrayType` to pass this check?


IMHO, it's fine to remove the outer check!


Repository:
  rC Clang

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

https://reviews.llvm.org/D55226



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

[PATCH] D54878: [clangd] NFC: Prefer `isa<>` to `dyn_cast<>` to do the checking.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In D54878#1307726 , @ilya-biryukov 
wrote:

> But given that `isa<>` are still better than `dyn_cast<>`, this change might 
> still be worth landing.


We can land this change this time or do the cleaning job in other patches in 
the future, it's all up to you guys, the active clangd contributors.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54878



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

[PATCH] D54878: [clangd] NFC: Eliminate the unused variable warning.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 175232.
MTC added a comment.

Use more concise form.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54878

Files:
  clangd/AST.cpp


Index: clangd/AST.cpp
===
--- clangd/AST.cpp
+++ clangd/AST.cpp
@@ -95,11 +95,11 @@
 return Out.str();
   }
   // The name was empty, so present an anonymous entity.
-  if (llvm::dyn_cast())
+  if (isa(ND))
 return "(anonymous namespace)";
   if (auto *Cls = llvm::dyn_cast())
 return ("(anonymous " + Cls->getKindName() + ")").str();
-  if (llvm::dyn_cast())
+  if (isa(ND))
 return "(anonymous enum)";
   return "(anonymous)";
 }


Index: clangd/AST.cpp
===
--- clangd/AST.cpp
+++ clangd/AST.cpp
@@ -95,11 +95,11 @@
 return Out.str();
   }
   // The name was empty, so present an anonymous entity.
-  if (llvm::dyn_cast())
+  if (isa(ND))
 return "(anonymous namespace)";
   if (auto *Cls = llvm::dyn_cast())
 return ("(anonymous " + Cls->getKindName() + ")").str();
-  if (llvm::dyn_cast())
+  if (isa(ND))
 return "(anonymous enum)";
   return "(anonymous)";
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54878: [clangd] NFC: Eliminate the unused variable warning.

[Henry Wong via Phabricator via cfe-commits]

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



Comment at: clangd/AST.cpp:98
   // The name was empty, so present an anonymous entity.
-  if (auto *NS = llvm::dyn_cast())
+  if (isa())
 return "(anonymous namespace)";

MaskRay wrote:
> `isa(ND)` also works.
Thanks for the tips! I will update it latter.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54878



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

[PATCH] D54878: [clangd] NFC: Eliminate the unused variable warning.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay, 
ioeric.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54878

Files:
  clangd/AST.cpp


Index: clangd/AST.cpp
===
--- clangd/AST.cpp
+++ clangd/AST.cpp
@@ -95,11 +95,11 @@
 return Out.str();
   }
   // The name was empty, so present an anonymous entity.
-  if (auto *NS = llvm::dyn_cast())
+  if (isa())
 return "(anonymous namespace)";
   if (auto *Cls = llvm::dyn_cast())
 return ("(anonymous " + Cls->getKindName() + ")").str();
-  if (auto *En = llvm::dyn_cast())
+  if (isa())
 return "(anonymous enum)";
   return "(anonymous)";
 }


Index: clangd/AST.cpp
===
--- clangd/AST.cpp
+++ clangd/AST.cpp
@@ -95,11 +95,11 @@
 return Out.str();
   }
   // The name was empty, so present an anonymous entity.
-  if (auto *NS = llvm::dyn_cast())
+  if (isa())
 return "(anonymous namespace)";
   if (auto *Cls = llvm::dyn_cast())
 return ("(anonymous " + Cls->getKindName() + ")").str();
-  if (auto *En = llvm::dyn_cast())
+  if (isa())
 return "(anonymous enum)";
   return "(anonymous)";
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342
+  DefaultBool IsOptimistic;
+  MemFunctionInfoTy MemFunctionInfo;
 public:

Szelethus wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > MTC wrote:
> > > > I can't say that abstracting `MemFunctionInfo` is a perfect solution, 
> > > > for example `IsOptimistic` and 
> > > > `ShouldIncludeOwnershipAnnotatedFunctions ` are close to each other in 
> > > > the object layout, but they do the same thing, which makes me feel 
> > > > weird. And there are so many `MemFunctionInfo.` :)
> > > Well the thinking here was that `MallocChecker` is seriously overcrowded 
> > > -- it's a one-tool-to-solve-everything class, and moving these 
> > > `IdentifierInfos` in their separate class was pretty much the first step 
> > > I took: I think it encapsulates a particular task well and eases a lot on 
> > > `MallocChecker`. Sure `MemFunctionInfo.` is reduntant, but each time you 
> > > see it, it indicates that we're calling a function or using a data member 
> > > that has no effect on the actual analysis, that we're inquiring about 
> > > more information about a given function. and nothing more. For a checker 
> > > that emits warning at seemingly random places wherever, I think this is 
> > > valuable.
> > > 
> > > By the way, it also forces almost all similar conditions in a separate 
> > > line, which is an unexpected, but pleasant help to the untrained eye:
> > > ```
> > > if (Fun == MemFunctionInfo.II_malloc ||
> > > Fun == MemFunctionInfo.II_whatever)
> > > ```
> > > Nevertheless, this is the only change I'm not 100% confident about, if 
> > > you and/or others disagree, I'll happily revert it.
> > (leaving a separate inline for a separate topic)
> > The was this checker abuses checker options isn't nice at all, I agree. I 
> > could just rename `MallocChecker::IsOptimistic` to 
> > `MallocChecker::ShouldIncludeOwnershipAnnotatedFunctions`, and have it 
> > passed around as a parameter in `MemFunctionInfoTy`. Would you prefer that, 
> > should you be okay with `MemFunctionInfoTy` in general?
> The way* this checker abuses 
Easier said than done :). I have no objection to this change, but I think merge 
`IsOptimistic` and `ShouldIncludeOwnershipAnnotatedFunctions` together is a 
good idea. This option seems from gabor, he may have new ideas.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
   if (FD->getKind() == Decl::Function) {
-initIdentifierInfo(C.getASTContext());
+MemFunctionInfo.initIdentifierInfo(C.getASTContext());
 IdentifierInfo *FunI = FD->getIdentifier();

Szelethus wrote:
> MTC wrote:
> > If I not wrong, `MemFunctionInfo` is mainly a class member of 
> > `MallocChecker` that hold a bunch of memory function identifiers and 
> > provide corresponding helper APIs. And we should pick a proper time to 
> > initialize it.
> > 
> > I want to known why you call `initIdentifierInfo()` in `checkPostStmt(const 
> > CallExpr *E,)`, this callback is called after `checkPreCall(const CallEvent 
> > , )` in which we need `MemFunctionInfo`.
> Well, I'd like to know too! I think lazy initializing this struct creates 
> more problems that it solves, but I wanted to draw a line somewhere in terms 
> of how invasive I'd like to make this patch.
How about put the initialization stuff into constructor? Let the constructor do 
the initialization that it should do, let `register*()` do its registration, 
like setting `setOptimism` and so on.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1247
+CheckerContext , const Expr *E, const unsigned IndexOfSizeArg,
+ProgramStateRef State, Optional RetVal) {
   if (!State)

Szelethus wrote:
> MTC wrote:
> > `const` qualifier missing?
> This method was made `static`.
You are right, sorry for my oversight!



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1323
+/// pointer to the type of the constructed record or one of its bases.
+static bool hasNonTrivialConstructorCall(const CXXNewExpr *NE) {
 

Szelethus wrote:
> MTC wrote:
> > I'm not sure `hasNonTrivialConstructorCall()` is a good name although you 
> > explained it in details in the comments below. And the comments for this 
> > function is difficult for me to understand, which is maybe my problem. 
> > 
> > And I also think this function doesn't do as much as described in the 
> > comments, it is just `true if the invoked constructor in \p NE has a 
> > parameter - pointer or reference to a record`, right?
> I admit that I copy-pasted this from the source I provided below, and I 
> overlooked it before posting it here. I //very// much prefer what you 
> proposed :)
The comments you provided is from the summary of the patch [[ 
https://reviews.llvm.org/D4025 | D4025 ]], ayartsev has not updated the summary 
information in time to adapt to his patch, 

[PATCH] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

Thank you for doing the cleaning that no one else is interested in! Comments is 
inline below.




Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:342
+  DefaultBool IsOptimistic;
+  MemFunctionInfoTy MemFunctionInfo;
 public:

I can't say that abstracting `MemFunctionInfo` is a perfect solution, for 
example `IsOptimistic` and `ShouldIncludeOwnershipAnnotatedFunctions ` are 
close to each other in the object layout, but they do the same thing, which 
makes me feel weird. And there are so many `MemFunctionInfo.` :)



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:422
+  ///
+  /// \param [in] CE The expression that allocates memory.
+  /// \param [in] IndexOfSizeArg Index of the argument that specifies the size

`CE` typo?



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:428
+  ///   if unspecified, the value of expression \p E is used.
+  static ProgramStateRef ProcessZeroAllocation(CheckerContext , const Expr 
*E,
+   const unsigned IndexOfSizeArg,

Personally, `ProcessZeroAllocation` is like half the story, maybe 
`ProcessZeroAllocCheck` or `ProcessZeroAllocationCheck` is better.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:448
+  /// \param [in] State The \c ProgramState right before allocation.
+  /// \returns The programstate right after allocation.
   ProgramStateRef MallocMemReturnsAttr(CheckerContext ,

Maybe `program state` or just `ProgramState` is better?
Same as 462, 476, 507, 575, 593.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:592
+  /// \param [in] CE The expression that reallocated memory
+  /// \param [in] State The \c ProgramState right before reallocation.
+  /// \returns The programstate right after allocation.

`before realloction` typo?



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1087
   if (FD->getKind() == Decl::Function) {
-initIdentifierInfo(C.getASTContext());
+MemFunctionInfo.initIdentifierInfo(C.getASTContext());
 IdentifierInfo *FunI = FD->getIdentifier();

If I not wrong, `MemFunctionInfo` is mainly a class member of `MallocChecker` 
that hold a bunch of memory function identifiers and provide corresponding 
helper APIs. And we should pick a proper time to initialize it.

I want to known why you call `initIdentifierInfo()` in `checkPostStmt(const 
CallExpr *E,)`, this callback is called after `checkPreCall(const CallEvent 
, )` in which we need `MemFunctionInfo`.



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1247
+CheckerContext , const Expr *E, const unsigned IndexOfSizeArg,
+ProgramStateRef State, Optional RetVal) {
   if (!State)

`const` qualifier missing?



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1323
+/// pointer to the type of the constructed record or one of its bases.
+static bool hasNonTrivialConstructorCall(const CXXNewExpr *NE) {
 

I'm not sure `hasNonTrivialConstructorCall()` is a good name although you 
explained it in details in the comments below. And the comments for this 
function is difficult for me to understand, which is maybe my problem. 

And I also think this function doesn't do as much as described in the comments, 
it is just `true if the invoked constructor in \p NE has a parameter - pointer 
or reference to a record`, right?



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2407
  const CallExpr *CE,
- bool FreesOnFail,
+ bool ShouldFreeOnFail,
  ProgramStateRef State,

The parameter name `ShouldFreeOnFail` is not consistent with the declaration, 
see line 577.


Repository:
  rC Clang

https://reviews.llvm.org/D54823



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

[PATCH] D54560: [analyzer] MoveChecker Pt.3: Improve warning messages a bit.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

> The "moved-from" terminology we adopt here still feels a bit weird to me, but 
> i don't have a better suggestion, so i just removed the single-quotes so that 
> to at least feel proud about what we have.

I am personally fine with this terminology, this checker corresponds to the 
cert rule **EXP63-CPP. Do not rely on the value of a moved-from object**, and 
**moved from** is also used in many places in CppCoreGuidelines.


https://reviews.llvm.org/D54560



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

[PATCH] D54401: [analyzer] Prefer returns values to out-params in CheckerRegistry.cpp

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

I'm totally fine with this patch personally. However I am not familiar with 
this part, so can't give substantial help :).




Comment at: lib/StaticAnalyzer/Core/CheckerRegistry.cpp:51
 
-/// Collects the checkers for the supplied \p opt option into \p collected.
-static void collectCheckers(const CheckerRegistry::CheckerInfoList ,
-const llvm::StringMap ,
-CheckerOptInfo , CheckerInfoSet ) {
-  // Use a binary search to find the possible start of the package.
-  CheckerRegistry::CheckerInfo packageInfo(nullptr, opt.getName(), "");
-  auto end = checkers.cend();
-  auto i = std::lower_bound(checkers.cbegin(), end, packageInfo, 
checkerNameLT);
-
-  // If we didn't even find a possible package, give up.
-  if (i == end)
-return;
-
-  // If what we found doesn't actually start the package, give up.
-  if (!isInPackage(*i, opt.getName()))
-return;
-
-  // There is at least one checker in the package; claim the option.
-  opt.claim();
-
-  // See how large the package is.
-  // If the package doesn't exist, assume the option refers to a single 
checker.
-  size_t size = 1;
-  llvm::StringMap::const_iterator packageSize =
-packageSizes.find(opt.getName());
-  if (packageSize != packageSizes.end())
-size = packageSize->getValue();
-
-  // Step through all the checkers in the package.
-  for (auto checkEnd = i+size; i != checkEnd; ++i)
-if (opt.isEnabled())
-  collected.insert(&*i);
-else
-  collected.remove(&*i);
+static CheckerInfoSet getEnabledCheckers(
+const CheckerRegistry::CheckerInfoList ,

I just curious about the reason why you move the `CheckerInfoSet` from the 
parameter passing by reference to the return value, keep the number of 
parameter at a lower value? Or make the code at the caller cite cleaner?


https://reviews.llvm.org/D54401



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

[PATCH] D52949: [Diagnostics] Implement -Wsizeof-pointer-div

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

Sorry for the long delay for this patch! The implementation is fine for me. 
However, I'm the newbie on clang diagnostics and have no further insight into 
this checker. @aaron.ballman may have more valuable insights into this checker.




Comment at: test/Sema/div-sizeof-ptr.c:13
+int b3 = sizeof(*q) / sizeof(q);
+int b4 = sizeof(p) / sizeof(char);
+}

My bad! I didn't read the code carefully.


https://reviews.llvm.org/D52949



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

[PATCH] D53856: [analyzer] Put llvm.Conventions back in alpha

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In addition, `clang/lib/StaticAnalyzer/README.txt`  and other related docs in 
`clang/lib/docs/analyzer` are also out of date.




Comment at: www/analyzer/alpha_checks.html:570
+
+  A StringRef should not be bound to a temporary std::string
+  whose lifetime is shorter than the StringRef's.

Like `StringRef`, `ArrayRef` may have similar issues. After this patch landed, 
maybe we can enhance this checker step by step. By that time, we may need the 
ideas of the heavy users of the LLVM library.


https://reviews.llvm.org/D53856



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

[PATCH] D53543: [analyzer] MallocChecker: pr39348: Realize that sized delete isn't a custom delete.

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: test/Analysis/NewDelete-sized-deallocation.cpp:1
+// RUN: %clang_analyze_cc1 -std=c++17 -analyzer-checker=core,cplusplus -verify 
-analyzer-output=text %s
+// RUN: %clang_analyze_cc1 -std=c++17 -analyzer-checker=core,cplusplus -verify 
-analyzer-output=text %s -fsized-deallocation

george.karpenkov wrote:
> MTC wrote:
> > I believe `sized deallocation` is the feature since C++14, see 
> > https://en.cppreference.com/w/cpp/memory/new/operator_delete. 
> specifying 17 is also fine
Yea, that's true.  


https://reviews.llvm.org/D53543



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

[PATCH] D52949: [Diagnostics] Implement -Wsizeof-pointer-div

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D52949#1268640, @xbolva00 wrote:

> Second thought, I don't think we should recommend std::size here (maybe it 
> should be okay for clang static analyzers)
>
> uint32_t data[] = {10, 20, 30, 40};
>  len = sizeof(data)/sizeof(*data); // "warn" on valid code to recommend 
> std::size? I dont agree with such behaviour.


IMHO, a clang-tidy checker is more  suitable than clang static analyzer to 
indicate to the developers that we should prefer `std::size` to 
`sizeof(data)/sizeof(*data)`.

What I want to know most is whether we should emit a warning for the code 
below. I haven't been able to test the patch yet, but intuitively, this patch 
will emit a warning.

  int foo(int *p) {
  int d = sizeof(p) / sizeof(char); // Should we emit a warning here?
  return d;
  }

GCC keep silent about this case, and I agree with it, see 
https://gcc.godbolt.org/z/ZToFqq. What do you think about this case?


https://reviews.llvm.org/D52949



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

[PATCH] D53543: [analyzer] MallocChecker: pr39348: Realize that sized delete isn't a custom delete.

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: test/Analysis/NewDelete-custom.cpp:7
 
-#if !(LEAKS && !ALLOCATOR_INLINING)
 // expected-no-diagnostics
 

Should we continue to keep this line?



Comment at: test/Analysis/NewDelete-sized-deallocation.cpp:1
+// RUN: %clang_analyze_cc1 -std=c++17 -analyzer-checker=core,cplusplus -verify 
-analyzer-output=text %s
+// RUN: %clang_analyze_cc1 -std=c++17 -analyzer-checker=core,cplusplus -verify 
-analyzer-output=text %s -fsized-deallocation

I believe `sized deallocation` is the feature since C++14, see 
https://en.cppreference.com/w/cpp/memory/new/operator_delete. 


https://reviews.llvm.org/D53543



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

[PATCH] D53024: [analyzer][www] Add more open projects

[Henry Wong via Phabricator via cfe-commits]

MTC added subscribers: teemperor, baloghadamsoftware, blitz.opensource.
MTC added a comment.
Herald added a subscriber: donat.nagy.

In https://reviews.llvm.org/D53024#1258976, @Szelethus wrote:

> Also, a lot of items on this list is outdated, but I joined the project 
> relatively recently, so I'm not sure what's the state on all of them.


AFAIK, the items below is outdated.

- Enhance CFG to model C++ temporaries properly (This problem has basically  
been fixed by @NoQ.)
- Enhance CFG to model C++ new more precisely (This problem has basically  been 
fixed by @NoQ.)
- Implement iterators invalidation checker (IIRC, @baloghadamsoftware has 
solved this, see `IteratorChecker.cpp`.)
- Write checkers which catch Copy and Paste errors (IIRC, @teemperor has solved 
this, see `CloneChecker.cpp`)
- Enhance CFG to model C++ delete more precisely (@blitz.opensource's focus is 
no longer on clang static analyzer, so we should not keep him as `current 
contact`.).

And there are items, I'm not sure what the current state is. Like:

- Explicitly model standard library functions with BodyFarm. (This item is 
marked as **ongoing**, it doesn't look very active nowadays.)

If I'm wrong, @NoQ and @george.karpenkov, please correct me. In addition `2018 
Bay Area LLVM Developers' Meetings` may bring some new open projects :), see 
http://llvm.org/devmtg/2018-10/talk-abstracts.html#bof6.

At the end, there are some punctuation problems, yea, I browsed this page 
through the browser :).




Comment at: www/analyzer/open_projects.html:98
+efficient. For instance, if the function is pure, then a single bit of
+information “this function is pure” would already be much better than
+conservative evaluation, and sometimes good enough to make inlining not

`“this function is pure”` -> `"this function is pure"`



Comment at: www/analyzer/open_projects.html:100
+conservative evaluation, and sometimes good enough to make inlining not
+worth the effort. Gathering such snippets of information - “partial
+summaries" - automatically, from the more simple to the more complex

`“partial` -> `"partial`



Comment at: www/analyzer/open_projects.html:259
+One of the more annoying parts in this is handling state splits for 
error
+return values. A “Schrödinger state” technique that was first implemented 
in
+the PthreadLockChecker (where a mutex was destroyed and not destroyed at 
the

Also here? `“Schrödinger state”` -> `"Schrödinger state"`



Comment at: www/analyzer/open_projects.html:267
+Many alpha checks can be turned into opt-in lint-like checks
+Path-sensitive lint checks are interesting and they can’t be implemented
+in clang-tidy and there’s clearly an interest in them, but we here aren’t

`can‘t` -> `can't`



Comment at: www/analyzer/open_projects.html:268
+Path-sensitive lint checks are interesting and they can’t be implemented
+in clang-tidy and there’s clearly an interest in them, but we here aren’t
+having enough maintenance power to respond to bugs and false positives. If

`there‘s` -> `there's`
`aren‘t` -> `aren't`


Repository:
  rC Clang

https://reviews.llvm.org/D53024



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

[PATCH] D52984: [analyzer] Checker reviewer's checklist

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

Greate idea! If we can enrich this list, it will not only help the reviewer, 
but also help beginners, like me, avoid some pitfalls when developing a new 
checker.

I agree with NoQ to classify the lists. In addition to the two categories 
proposed by NoQ, there is another classof issues, see below, we don't have to 
take them into account.

- Develop a checker(proposed by NoQ). Like:
  1. Does it have to be in the path-sensitive manner?
  2. Should we move it into `clang-tidy`?
  3. Prefer `ASTMatcher` to `StmtVisitor`?
  4. Use `CallDescription` to get a stricter filter.
  5. Could we use `checkPreCall()` instead of `checkPreStmt(const CallExpr *, 
)`?
  6. Could we put this function model into `StdLibraryFunctionsChecker.cpp`?
- Make a good checke(proposed by NoQ).
- More general coding standards. Like:
  1. Use `isa<>` instead of `dyn_cast<>` if we don't need the casted result.
  2. Use `dyn_cast_or_null` instead of `dyn_cast` to enable the null-check.
  3. Use `llvm::make_unique` instead of `std::make_unique` because 
`std::make_unique` is not included in C++11.
  4. Avoid unnecessary null-check
  5. ... and so on.

The third list can be long, and maybe we don't need to take it into account, 
because developers will follow this standards whether they are developing 
checkers or contribute to other projects, like LLDB. We need to dig deeper into 
the analyzer-specific coding standards.


https://reviews.llvm.org/D52984



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

[PATCH] D51390: [analyzer] CallDescription: Improve array management.

[Henry Wong via Phabricator via cfe-commits]

MTC accepted this revision.
MTC added a comment.

Thank you for digging in to delete that meaningless constructor, NoQ! And sorry 
for my technology debt : ).


Repository:
  rC Clang

https://reviews.llvm.org/D51390



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

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D48027#1209844, @NoQ wrote:

> So i believe that one of the important remaining problems with 
> `CallDescription` is to teach it to discriminate between global functions and 
> methods. We can do it in a number of ways:
>
> 1. Make a special sub-class for `CallDescription` depending on the sort of 
> the function (too verbose),
> 2. Tag all items in the list as "record" vs. "namespace" (too many tags),
> 3. Dunno, tons of other boring and verbose approaches,
> 4. Change our `PreCall`/`PostCall` callbacks to callback templates that let 
> allow the user subscribe to specific sub-classes of `CallEvent` similarly to 
> how he can subscribe to different kinds of statements in 
> `PreStmt`/`PostStmt`, and then the user doesn't need to write 
> any code to check it dynamically.


Personally, I prefer `4`. It is more checker developer friendly!


Repository:
  rC Clang

https://reviews.llvm.org/D48027



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

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D48027#1207645, @xazax.hun wrote:

> Sorry for the delay, I think this is OK to commit.
>  As a possible improvement, I can imagine making it slightly stricter, e.g. 
> you could only skip QualifiedNames for inline namespaces and the beginning. 
> Maybe add support for staring with `""` or `::` to even disable skipping 
> namespaces at the beginning?
>  But these are just nice to have features, I am perfectly ok with not having 
> them or doing it in a followup patch.


Thanks, Gábor!
I will land it first and do the improvement according to the mismatch case in 
the followup patch!


https://reviews.llvm.org/D48027



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

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

@xazax.hun What do you think?


https://reviews.llvm.org/D48027



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

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D48027#1201248, @xazax.hun wrote:

> Generally looks good, I only wonder if this works well with inline 
> namespaces. Could you test? Probably it will.


Thank you for reminding me! Yea, this can handle inline namespaces.

However this approach has limit. Given the code below, we cannot distinguish 
whether the `basic_string` is user-defined struct or namespace. That's means 
when the user provide {"std", "basic_string", "append"}, we can only know the 
qualified name of the call sequentially contains `std`, `basic_string`, 
`append`. We don't know if these names come from `RecordDecl` or 
`NamespaceDecl`.

  namespace  std {
namespace basic_string {
  struct A {
void append() {}
  };
}
  }
  
  void foo() {
std::basic_string::A a;
a.append(); // Match
  }

@rnkovacs What do you think? Can this approach meet `InnerPointerChecker`'s 
needs?


https://reviews.llvm.org/D48027



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

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 161217.
MTC added a comment.

- rebase
- Since we have enhanced the ability of `CallDescription`, remove the helper 
method `isCalledOnStringObject()`.


https://reviews.llvm.org/D48027

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  lib/StaticAnalyzer/Core/CallEvent.cpp

Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -359,11 +359,38 @@
 return false;
   if (!CD.IsLookupDone) {
 CD.IsLookupDone = true;
-CD.II = ()->getStateManager().getContext().Idents.get(CD.FuncName);
+CD.II = ()->getStateManager().getContext().Idents.get(
+CD.getFunctionName());
   }
   const IdentifierInfo *II = getCalleeIdentifier();
   if (!II || II != CD.II)
 return false;
+
+  const Decl *D = getDecl();
+  // If CallDescription provides prefix names, use them to improve matching
+  // accuracy.
+  if (CD.QualifiedName.size() > 1 && D) {
+const DeclContext *Ctx = D->getDeclContext();
+std::vector QualifiedName = CD.QualifiedName;
+QualifiedName.pop_back();
+for (; Ctx && isa(Ctx); Ctx = Ctx->getParent()) {
+  if (const auto *ND = dyn_cast(Ctx)) {
+if (!QualifiedName.empty() && ND->getName() == QualifiedName.back())
+  QualifiedName.pop_back();
+continue;
+  }
+
+  if (const auto *RD = dyn_cast(Ctx)) {
+if (!QualifiedName.empty() && RD->getName() == QualifiedName.back())
+  QualifiedName.pop_back();
+continue;
+  }
+}
+
+if (!QualifiedName.empty())
+  return false;
+  }
+
   return (CD.RequiredArgs == CallDescription::NoArgRequirement ||
   CD.RequiredArgs == getNumArgs());
 }
Index: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -86,14 +86,20 @@
   };
 
   InnerPointerChecker()
-  : AppendFn("append"), AssignFn("assign"), ClearFn("clear"),
-CStrFn("c_str"), DataFn("data"), EraseFn("erase"), InsertFn("insert"),
-PopBackFn("pop_back"), PushBackFn("push_back"), ReplaceFn("replace"),
-ReserveFn("reserve"), ResizeFn("resize"),
-ShrinkToFitFn("shrink_to_fit"), SwapFn("swap") {}
-
-  /// Check if the object of this member function call is a `basic_string`.
-  bool isCalledOnStringObject(const CXXInstanceCall *ICall) const;
+  : AppendFn({"std", "basic_string", "append"}),
+AssignFn({"std", "basic_string", "assign"}),
+ClearFn({"std", "basic_string", "clear"}),
+CStrFn({"std", "basic_string", "c_str"}),
+DataFn({"std", "basic_string", "data"}),
+EraseFn({"std", "basic_string", "erase"}),
+InsertFn({"std", "basic_string", "insert"}),
+PopBackFn({"std", "basic_string", "pop_back"}),
+PushBackFn({"std", "basic_string", "push_back"}),
+ReplaceFn({"std", "basic_string", "replace"}),
+ReserveFn({"std", "basic_string", "reserve"}),
+ResizeFn({"std", "basic_string", "resize"}),
+ShrinkToFitFn({"std", "basic_string", "shrink_to_fit"}),
+SwapFn({"std", "basic_string", "swap"}) {}
 
   /// Check whether the called member function potentially invalidates
   /// pointers referring to the container object's inner buffer.
@@ -122,21 +128,6 @@
 
 } // end anonymous namespace
 
-bool InnerPointerChecker::isCalledOnStringObject(
-const CXXInstanceCall *ICall) const {
-  const auto *ObjRegion =
-dyn_cast_or_null(ICall->getCXXThisVal().getAsRegion());
-  if (!ObjRegion)
-return false;
-
-  QualType ObjTy = ObjRegion->getValueType();
-  if (ObjTy.isNull())
-return false;
-
-  CXXRecordDecl *Decl = ObjTy->getAsCXXRecordDecl();
-  return Decl && Decl->getName() == "basic_string";
-}
-
 bool InnerPointerChecker::isInvalidatingMemberFunction(
 const CallEvent ) const {
   if (const auto *MemOpCall = dyn_cast()) {
@@ -220,33 +211,31 @@
   ProgramStateRef State = C.getState();
 
   if (const auto *ICall = dyn_cast()) {
-if (isCalledOnStringObject(ICall)) {
-  const auto *ObjRegion = dyn_cast_or_null(
-  ICall->getCXXThisVal().getAsRegion());
-
-  if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
-SVal RawPtr = Call.getReturnValue();
-if (SymbolRef Sym = RawPtr.getAsSymbol(/*IncludeBaseRegions=*/true)) {
-  // Start tracking this raw pointer by adding it to the set of symbols
-  // associated with this container object in the program state map.
-
-  PtrSet::Factory  = State->getStateManager().get_context();
-  const PtrSet *SetPtr = State->get(ObjRegion);
-  PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet();
-  assert(C.wasInlined || 

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

kindly ping!


Repository:
  rC Clang

https://reviews.llvm.org/D48027



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

[PATCH] D50382: [analyzer] Fix a typo in `RegionStore.txt`.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: NoQ, george.karpenkov.
Herald added subscribers: mikhail.ramalho, a.sidorin, szepet, xazax.hun.

The typo of the description for default bindings can be confusing.


Repository:
  rC Clang

https://reviews.llvm.org/D50382

Files:
  docs/analyzer/RegionStore.txt


Index: docs/analyzer/RegionStore.txt
===
--- docs/analyzer/RegionStore.txt
+++ docs/analyzer/RegionStore.txt
@@ -118,7 +118,7 @@
   int manyInts[10];
   manyInts[1] = 42;   // Creates a Direct binding for manyInts[1].
   print(manyInts[1]); // Retrieves the Direct binding for manyInts[1];
-  print(manyInts[0]); // There is no Direct binding for manyInts[1].
+  print(manyInts[0]); // There is no Direct binding for manyInts[0].
   // Is there a Default binding for the entire array?
   // There is not, but it is a stack variable, so we use
   // "uninitialized" as the default value (and emit a
@@ -166,6 +166,6 @@
 return p2.x;// The binding for FieldRegion 'p2.x' is requested.
 // There is no Direct binding, so we look for a Default
 // binding to 'p2' and find the LCV.
-// Because it's an LCV, we look at our requested region
+// Because it's a LCV, we look at our requested region
 // and see that it's the '.x' field. We ask for the value
 // of 'p.x' within the snapshot, and get back 42.


Index: docs/analyzer/RegionStore.txt
===
--- docs/analyzer/RegionStore.txt
+++ docs/analyzer/RegionStore.txt
@@ -118,7 +118,7 @@
   int manyInts[10];
   manyInts[1] = 42;   // Creates a Direct binding for manyInts[1].
   print(manyInts[1]); // Retrieves the Direct binding for manyInts[1];
-  print(manyInts[0]); // There is no Direct binding for manyInts[1].
+  print(manyInts[0]); // There is no Direct binding for manyInts[0].
   // Is there a Default binding for the entire array?
   // There is not, but it is a stack variable, so we use
   // "uninitialized" as the default value (and emit a
@@ -166,6 +166,6 @@
 return p2.x;// The binding for FieldRegion 'p2.x' is requested.
 // There is no Direct binding, so we look for a Default
 // binding to 'p2' and find the LCV.
-// Because it's an LCV, we look at our requested region
+// Because it's a LCV, we look at our requested region
 // and see that it's the '.x' field. We ask for the value
 // of 'p.x' within the snapshot, and get back 42.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 157499.
MTC added a comment.

@xazax.hun Thanks for your tips! After some investigation, `MatchFinder::match` 
just traverse one ASTNode, that means `match(namedDecl(HasNameMatcher()))` and 
`match(namedDecl(matchesName()))` both not traverse children. So there are 
three ways to match the specified AST node.

1. `match(namedDecl(HasNameMatcher()))` matches the full qualified name that 
contains template parameter and that's not what we want to see.
2. `match(namedDecl(matchesName()))` uses llvm regex to  match the full 
qualified name.
3. Unwrap the declaration and match each layer, similar to 
`HasNameMatcher::matchesNodeFullFast()`.

In this update, I use the third way to match the specified AST node. This is 
simpler than I thought.

In addition, add a redundant constructor that is only used to construct a 
`CallDescription` with one name, this avoids the modification of the existing 
checker, like `BlockInCriticalSectionChecker.cpp`. Any suggestions?


Repository:
  rC Clang

https://reviews.llvm.org/D48027

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
  lib/StaticAnalyzer/Core/CallEvent.cpp

Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -256,11 +256,38 @@
 return false;
   if (!CD.IsLookupDone) {
 CD.IsLookupDone = true;
-CD.II = ()->getStateManager().getContext().Idents.get(CD.FuncName);
+CD.II = ()->getStateManager().getContext().Idents.get(
+CD.getFunctionName());
   }
   const IdentifierInfo *II = getCalleeIdentifier();
   if (!II || II != CD.II)
 return false;
+
+  const Decl *D = getDecl();
+  // If CallDescription provides prefix names, use them to improve matching
+  // accuracy.
+  if (CD.QualifiedName.size() > 1 && D) {
+const DeclContext *Ctx = D->getDeclContext();
+std::vector QualifiedName = CD.QualifiedName;
+QualifiedName.pop_back();
+for (; Ctx && isa(Ctx); Ctx = Ctx->getParent()) {
+  if (const auto *ND = dyn_cast(Ctx)) {
+if (!QualifiedName.empty() && ND->getName() == QualifiedName.back())
+  QualifiedName.pop_back();
+continue;
+  }
+
+  if (const auto *RD = dyn_cast(Ctx)) {
+if (!QualifiedName.empty() && RD->getName() == QualifiedName.back())
+  QualifiedName.pop_back();
+continue;
+  }
+}
+
+if (!QualifiedName.empty())
+  return false;
+  }
+
   return (CD.RequiredArgs == CallDescription::NoArgRequirement ||
   CD.RequiredArgs == getNumArgs());
 }
Index: lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
+++ lib/StaticAnalyzer/Checkers/InnerPointerChecker.cpp
@@ -85,11 +85,20 @@
   };
 
   InnerPointerChecker()
-  : AppendFn("append"), AssignFn("assign"), ClearFn("clear"),
-CStrFn("c_str"), DataFn("data"), EraseFn("erase"), InsertFn("insert"),
-PopBackFn("pop_back"), PushBackFn("push_back"), ReplaceFn("replace"),
-ReserveFn("reserve"), ResizeFn("resize"),
-ShrinkToFitFn("shrink_to_fit"), SwapFn("swap") {}
+  : AppendFn({"std", "basic_string", "append"}),
+AssignFn({"std", "basic_string", "assign"}),
+ClearFn({"std", "basic_string", "clear"}),
+CStrFn({"std", "basic_string", "c_str"}),
+DataFn({"std", "basic_string", "data"}),
+EraseFn({"std", "basic_string", "erase"}),
+InsertFn({"std", "basic_string", "insert"}),
+PopBackFn({"std", "basic_string", "pop_back"}),
+PushBackFn({"std", "basic_string", "push_back"}),
+ReplaceFn({"std", "basic_string", "replace"}),
+ReserveFn({"std", "basic_string", "reserve"}),
+ResizeFn({"std", "basic_string", "resize"}),
+ShrinkToFitFn({"std", "basic_string", "shrink_to_fit"}),
+SwapFn({"std", "basic_string", "swap"}) {}
 
   /// Check whether the function called on the container object is a
   /// member function that potentially invalidates pointers referring
@@ -148,10 +157,6 @@
   if (!ObjRegion)
 return;
 
-  auto *TypeDecl = ObjRegion->getValueType()->getAsCXXRecordDecl();
-  if (TypeDecl->getName() != "basic_string")
-return;
-
   ProgramStateRef State = C.getState();
 
   if (Call.isCalled(CStrFn) || Call.isCalled(DataFn)) {
Index: include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -79,24 +79,41 @@
 
   mutable IdentifierInfo *II = nullptr;
   mutable bool IsLookupDone = false;
-  StringRef FuncName;
+  // The list of the qualified names used to identify the specified CallEvent,

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

Thanks for your review, NoQ!




Comment at: lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp:68
 : IILockGuard(nullptr), IIUniqueLock(nullptr),
-  LockFn("lock"), UnlockFn("unlock"), SleepFn("sleep"), GetcFn("getc"),
-  FgetsFn("fgets"), ReadFn("read"), RecvFn("recv"),
-  PthreadLockFn("pthread_mutex_lock"),
-  PthreadTryLockFn("pthread_mutex_trylock"),
-  PthreadUnlockFn("pthread_mutex_unlock"),
-  MtxLock("mtx_lock"),
-  MtxTimedLock("mtx_timedlock"),
-  MtxTryLock("mtx_trylock"),
-  MtxUnlock("mtx_unlock"),
+  LockFn({"lock"}), UnlockFn({"unlock"}), SleepFn({"sleep"}), 
GetcFn({"getc"}),
+  FgetsFn({"fgets"}), ReadFn({"read"}), RecvFn({"recv"}),

NoQ wrote:
> I wish the old syntax for simple C functions was preserved.
> 
> This could be accidentally achieved by using `ArrayRef` instead of 
> `std::vector` for your constructor's argument.
`ArrayRef` can't achieve this goal. The only way I can figure out is 
changing the declaration to the following form.

`CallDescription(StringRef FuncName, unsigned RequiredArgs = NoArgRequirement, 
ArrayRef QualifiedName = None) {}`



Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:273-280
+std::string Regex = "^::(.*)?";
+Regex += llvm::join(CD.QualifiedName, "(.*)?::(.*)?") + "(.*)?$";
+
+auto Matches = match(namedDecl(matchesName(Regex)).bind("Regex"), *ND,
+ LCtx->getAnalysisDeclContext()->getASTContext());
+
+if (Matches.empty())

NoQ wrote:
> Uhm, i think we don't need to flatten the class name to a string and then 
> regex to do this.
> 
> We can just unwrap the declaration and see if the newly unwrapped layer 
> matches the expected name on every step, until all expected names have been 
> found.
Is the way you mentioned above is similar `NamedDecl::printQualifiedName()`? 
Get the full name through the `DeclContext` chain. See 
https://github.com/llvm-mirror/clang/blob/master/lib/AST/Decl.cpp#L1511.

Or ignore namespace ,like `std`, just only consider the `CXXRecordDecl`? If so, 
`dyn_cast<>` might be enough.


Repository:
  rC Clang

https://reviews.llvm.org/D48027



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

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

kindly ping!


Repository:
  rC Clang

https://reviews.llvm.org/D48027



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

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 152702.
MTC added a comment.

Sorry for the long long delay, I was on the Dragon Boat Festival a few days ago.

This update has two parts:

- Use the `matchesName` to match the AST node with the specified name, 
`matchesName` use regex to match the specified name.
- Use `std::vector` to record the list of qualified name, user can provide 
information as much as possible to improve matching accuracy. But can also 
provide only the function name.

There are two remain issues:

- There is possible match the wrong AST node, e.g. for the NamedDecl `foo()`, 
if the function body has the `a::b::x`, when we match `a::b::X()`, we will 
match `foo()` . Because I'm not familiar with ASTMatcher, my bad, I can't think 
of a perfect way to match only the specified nodes.
- The `std::vector` to record the information provide by the users may be not 
the best data structure.
- I'm not the regex expert,  the regex used in this patch definitely has the 
chance to improve.

  And I am not good at English writing, if any comments are not very clear, 
please correct me. Thanks for the help!




Repository:
  rC Clang

https://reviews.llvm.org/D48027

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  lib/StaticAnalyzer/Checkers/BlockInCriticalSectionChecker.cpp
  lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
  lib/StaticAnalyzer/Checkers/MmapWriteExecChecker.cpp
  lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp
  lib/StaticAnalyzer/Checkers/ValistChecker.cpp
  lib/StaticAnalyzer/Core/CallEvent.cpp

Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -28,6 +28,7 @@
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/ProgramPoint.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
@@ -65,6 +66,7 @@
 
 using namespace clang;
 using namespace ento;
+using namespace clang::ast_matchers;
 
 QualType CallEvent::getResultType() const {
   ASTContext  = getState()->getStateManager().getContext();
@@ -256,11 +258,28 @@
 return false;
   if (!CD.IsLookupDone) {
 CD.IsLookupDone = true;
-CD.II = ()->getStateManager().getContext().Idents.get(CD.FuncName);
+CD.II = ()->getStateManager().getContext().Idents.get(
+CD.getFunctionName());
   }
   const IdentifierInfo *II = getCalleeIdentifier();
   if (!II || II != CD.II)
 return false;
+
+  if (CD.QualifiedName.size() > 1) {
+const NamedDecl *ND = dyn_cast_or_null(getDecl());
+if (!ND)
+  return false;
+
+std::string Regex = "^::(.*)?";
+Regex += llvm::join(CD.QualifiedName, "(.*)?::(.*)?") + "(.*)?$";
+
+auto Matches = match(namedDecl(matchesName(Regex)).bind("Regex"), *ND,
+ LCtx->getAnalysisDeclContext()->getASTContext());
+
+if (Matches.empty())
+  return false;
+  }
+
   return (CD.RequiredArgs == CallDescription::NoArgRequirement ||
   CD.RequiredArgs == getNumArgs());
 }
Index: lib/StaticAnalyzer/Checkers/ValistChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -103,24 +103,24 @@
 
 const SmallVector
 ValistChecker::VAListAccepters = {
-{{"vfprintf", 3}, 2},
-{{"vfscanf", 3}, 2},
-{{"vprintf", 2}, 1},
-{{"vscanf", 2}, 1},
-{{"vsnprintf", 4}, 3},
-{{"vsprintf", 3}, 2},
-{{"vsscanf", 3}, 2},
-{{"vfwprintf", 3}, 2},
-{{"vfwscanf", 3}, 2},
-{{"vwprintf", 2}, 1},
-{{"vwscanf", 2}, 1},
-{{"vswprintf", 4}, 3},
+{{{"vfprintf"}, 3}, 2},
+{{{"vfscanf"}, 3}, 2},
+{{{"vprintf"}, 2}, 1},
+{{{"vscanf"}, 2}, 1},
+{{{"vsnprintf"}, 4}, 3},
+{{{"vsprintf"}, 3}, 2},
+{{{"vsscanf"}, 3}, 2},
+{{{"vfwprintf"}, 3}, 2},
+{{{"vfwscanf"}, 3}, 2},
+{{{"vwprintf"}, 2}, 1},
+{{{"vwscanf"}, 2}, 1},
+{{{"vswprintf"}, 4}, 3},
 // vswprintf is the wide version of vsnprintf,
 // vsprintf has no wide version
-{{"vswscanf", 3}, 2}};
-const CallDescription ValistChecker::VaStart("__builtin_va_start", 2),
-ValistChecker::VaCopy("__builtin_va_copy", 2),
-ValistChecker::VaEnd("__builtin_va_end", 1);
+{{{"vswscanf"}, 3}, 2}};
+const CallDescription ValistChecker::VaStart({"__builtin_va_start"}, 2),
+ValistChecker::VaCopy({"__builtin_va_copy"}, 2),
+ValistChecker::VaEnd({"__builtin_va_end"}, 1);
 } // end anonymous namespace
 
 void ValistChecker::checkPreCall(const CallEvent ,
Index: lib/StaticAnalyzer/Checkers/SimpleStreamChecker.cpp

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:32
  check::PostCall> {
-  CallDescription CStrFn;
+  const llvm::SmallVector CStrFnFamily = {
+{"std::basic_string::c_str"}, {"std::basic_string::c_str"},

NoQ wrote:
> xazax.hun wrote:
> > I am not sure if this is the right solution in case of this check. We 
> > should track `c_str` calls regardless of what the template parameter is, so 
> > supporting any instantiation of `basic_string` is desired. This might not 
> > be the case, however, for other checks. 
> > 
> > If we think it is more likely we do not care about the template arguments, 
> > maybe a separate API could be used, where we pass the qualified name of the 
> > class separately without the template arguments.
> > Alternatively, we could use matches name so the users could use regexps.
> > 
> > At this point I also wonder what isCalled API gives us compared to 
> > matchers? Maybe it is more convenient to use than calling a `match`. Also, 
> > isCalled API has an IdentifierInfo cached which could be used for 
> > relatively efficient checks.
> > 
> > @NoQ what do you think?
> > 
> > 
> I agree that it's better to match the chain of classes and namespaces (in as 
> much detail as the user cares to provide) and discard template parameters.
> 
> For example, i wish that a `CallDescription` that's defined as `{"std", 
> "basic_string", "c_str"}` would be able to match both `std::string::c_str()` 
> and `std::__1::string::c_str()`.
Yea, checker writers may can't provide all the template arguments, and it's not 
convenient to use!

I will try to give a better solution!



Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:65
   auto *TypeDecl = TypedR->getValueType()->getAsCXXRecordDecl();
   if (TypeDecl->getName() != "basic_string")
 return;

xazax.hun wrote:
> If we check for fully qualified names, this check might be redundant.
Right, thanks!



Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:273
+  auto Matches =
+  match(namedDecl(hasName(CD.FuncName)).bind("match_qualified_name"), *ND,
+LCtx->getAnalysisDeclContext()->getASTContext());

xazax.hun wrote:
> Doesn't match also traverse the subtree of the AST? We only need to check if 
> that particular node matches the qualified name. I wonder if we could, during 
> the traversal, find another node that is a match unintentionally.
Yea, this maybe a problem, I will test whether this is going to happen and give 
a fine-grained match. Thanks!



Repository:
  rC Clang

https://reviews.llvm.org/D48027



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

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 151166.
MTC added a comment.

- Use `hasName` matcher to match the qualified name.
- Use the full name, like `std::basic_string::c_str` instead of `c_str` 
to match the `c_str` method in `DanglingInternalBufferChecker.cpp`.




Repository:
  rC Clang

https://reviews.llvm.org/D48027

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
  lib/StaticAnalyzer/Core/CallEvent.cpp

Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -28,6 +28,7 @@
 #include "clang/Analysis/AnalysisDeclContext.h"
 #include "clang/Analysis/CFG.h"
 #include "clang/Analysis/ProgramPoint.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/CrossTU/CrossTranslationUnit.h"
 #include "clang/Basic/IdentifierTable.h"
 #include "clang/Basic/LLVM.h"
@@ -65,6 +66,7 @@
 
 using namespace clang;
 using namespace ento;
+using namespace clang::ast_matchers;
 
 QualType CallEvent::getResultType() const {
   ASTContext  = getState()->getStateManager().getContext();
@@ -256,11 +258,24 @@
 return false;
   if (!CD.IsLookupDone) {
 CD.IsLookupDone = true;
-CD.II = ()->getStateManager().getContext().Idents.get(CD.FuncName);
+CD.II = ()->getStateManager().getContext().Idents.get(
+CD.getFunctionName());
   }
   const IdentifierInfo *II = getCalleeIdentifier();
   if (!II || II != CD.II)
 return false;
+
+  const NamedDecl *ND = dyn_cast_or_null(getDecl());
+  if (!ND)
+return false;
+
+  auto Matches =
+  match(namedDecl(hasName(CD.FuncName)).bind("match_qualified_name"), *ND,
+LCtx->getAnalysisDeclContext()->getASTContext());
+
+  if (Matches.empty())
+return false;
+
   return (CD.RequiredArgs == CallDescription::NoArgRequirement ||
   CD.RequiredArgs == getNumArgs());
 }
Index: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp
@@ -13,26 +13,28 @@
 //
 //===--===//
 
+#include "AllocationState.h"
 #include "ClangSACheckers.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
 #include "clang/StaticAnalyzer/Core/BugReporter/CommonBugCategories.h"
 #include "clang/StaticAnalyzer/Core/Checker.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
-#include "AllocationState.h"
+#include "llvm/ADT/SmallVector.h"
 
 using namespace clang;
 using namespace ento;
 
 namespace {
 
 class DanglingInternalBufferChecker : public Checker {
-  CallDescription CStrFn;
+  const llvm::SmallVector CStrFnFamily = {
+{"std::basic_string::c_str"}, {"std::basic_string::c_str"},
+{"std::basic_string::c_str"},
+{"std::basic_string::c_str"}};
 
 public:
-  DanglingInternalBufferChecker() : CStrFn("c_str") {}
-
   /// Record the connection between the symbol returned by c_str() and the
   /// corresponding string object region in the ProgramState. Mark the symbol
   /// released if the string object is destroyed.
@@ -65,7 +67,15 @@
 
   ProgramStateRef State = C.getState();
 
-  if (Call.isCalled(CStrFn)) {
+  auto isCStrFnFamilyCall = [&](const CallEvent ) -> bool {
+for (auto CStrFn : CStrFnFamily) {
+  if (Call.isCalled(CStrFn))
+return true;
+}
+return false;
+  };
+
+  if (isCStrFnFamilyCall(Call)) {
 SVal RawPtr = Call.getReturnValue();
 if (!RawPtr.isUnknown()) {
   State = State->set(TypedR, RawPtr.getAsSymbol());
Index: include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -79,6 +79,7 @@
 
   mutable IdentifierInfo *II = nullptr;
   mutable bool IsLookupDone = false;
+  // Represent the function name or method name, like "X" or "a::b::X".
   StringRef FuncName;
   unsigned RequiredArgs;
 
@@ -96,7 +97,11 @@
   : FuncName(FuncName), RequiredArgs(RequiredArgs) {}
 
   /// Get the name of the function that this object matches.
-  StringRef getFunctionName() const { return FuncName; }
+  StringRef getFunctionName() const {
+auto QualifierNamePair = FuncName.rsplit("::");
+return QualifierNamePair.second.empty() ? QualifierNamePair.first
+: QualifierNamePair.second;
+  }
 };
 
 template
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47616: [CFG] [analyzer] Explain copy elision through construction contexts.

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.
Herald added a subscriber: mikhail.ramalho.



Comment at: include/clang/Analysis/ConstructionContext.h:351
+
+  explicit SimpleTemporaryObjectConstructionContext(
+  const CXXBindTemporaryExpr *BTE, const MaterializeTemporaryExpr *MTE)

MTC wrote:
> `explicit` useless?
I'm wrong here, maybe there is some use of 
`SimpleTemporaryObjectConstructionContext({ , })`, sorry to bother!



Comment at: include/clang/Analysis/ConstructionContext.h:377
+
+  explicit ElidedTemporaryObjectConstructionContext(
+  const CXXBindTemporaryExpr *BTE, const MaterializeTemporaryExpr *MTE,

MTC wrote:
> Same useless here?
Also I'm wrong here!


https://reviews.llvm.org/D47616



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

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D48027#1128301, @xazax.hun wrote:

> Having C++ support would be awesome!
>  Thanks for working on this!
>
> While I do agree matching is not trivial with qualified names, this problem 
> is already solved with AST matchers.
>
> I wonder if using matchers or reusing the `HasNameMatcher` class would make 
> this easier. That code path is already well tested and optimized.


Thank you for pointing out the direction, xazax.hun!

I will looking into the matchers and try to give a better solution.


Repository:
  rC Clang

https://reviews.llvm.org/D48027



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

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 150758.
MTC added a comment.

Remove useless header files for testing.


Repository:
  rC Clang

https://reviews.llvm.org/D48027

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  lib/StaticAnalyzer/Core/CallEvent.cpp


Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -256,11 +256,18 @@
 return false;
   if (!CD.IsLookupDone) {
 CD.IsLookupDone = true;
-CD.II = 
()->getStateManager().getContext().Idents.get(CD.FuncName);
+CD.II = ()->getStateManager().getContext().Idents.get(
+CD.getFunctionName());
   }
   const IdentifierInfo *II = getCalleeIdentifier();
   if (!II || II != CD.II)
 return false;
+
+  const auto *ND = dyn_cast_or_null(getDecl());
+  if (!ND)
+return false;
+  if (!CD.matchQualifiedName(ND->getQualifiedNameAsString()))
+return false;
   return (CD.RequiredArgs == CallDescription::NoArgRequirement ||
   CD.RequiredArgs == getNumArgs());
 }
Index: include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -79,6 +79,8 @@
 
   mutable IdentifierInfo *II = nullptr;
   mutable bool IsLookupDone = false;
+  // Represent the function name or method name, like "c_str" or
+  // "std::string::c_str".
   StringRef FuncName;
   unsigned RequiredArgs;
 
@@ -96,7 +98,25 @@
   : FuncName(FuncName), RequiredArgs(RequiredArgs) {}
 
   /// Get the name of the function that this object matches.
-  StringRef getFunctionName() const { return FuncName; }
+  StringRef getFunctionName() const {
+auto QualifierNamePair = FuncName.rsplit("::");
+return QualifierNamePair.second.empty() ? QualifierNamePair.first
+: QualifierNamePair.second;
+  }
+
+  bool matchQualifiedName(llvm::StringRef QualifiedName) const {
+llvm::SmallVector Names;
+// Split the function name with the separator "::".
+FuncName.split(Names, "::");
+
+// FIXME: Since there is no perfect way to get the qualified name without
+// template argument, we can only use a fuzzy matching approach.
+for (auto item : Names)
+  if (QualifiedName.find(item) == StringRef::npos)
+return false;
+
+return true;
+  }
 };
 
 template


Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -256,11 +256,18 @@
 return false;
   if (!CD.IsLookupDone) {
 CD.IsLookupDone = true;
-CD.II = ()->getStateManager().getContext().Idents.get(CD.FuncName);
+CD.II = ()->getStateManager().getContext().Idents.get(
+CD.getFunctionName());
   }
   const IdentifierInfo *II = getCalleeIdentifier();
   if (!II || II != CD.II)
 return false;
+
+  const auto *ND = dyn_cast_or_null(getDecl());
+  if (!ND)
+return false;
+  if (!CD.matchQualifiedName(ND->getQualifiedNameAsString()))
+return false;
   return (CD.RequiredArgs == CallDescription::NoArgRequirement ||
   CD.RequiredArgs == getNumArgs());
 }
Index: include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -79,6 +79,8 @@
 
   mutable IdentifierInfo *II = nullptr;
   mutable bool IsLookupDone = false;
+  // Represent the function name or method name, like "c_str" or
+  // "std::string::c_str".
   StringRef FuncName;
   unsigned RequiredArgs;
 
@@ -96,7 +98,25 @@
   : FuncName(FuncName), RequiredArgs(RequiredArgs) {}
 
   /// Get the name of the function that this object matches.
-  StringRef getFunctionName() const { return FuncName; }
+  StringRef getFunctionName() const {
+auto QualifierNamePair = FuncName.rsplit("::");
+return QualifierNamePair.second.empty() ? QualifierNamePair.first
+: QualifierNamePair.second;
+  }
+
+  bool matchQualifiedName(llvm::StringRef QualifiedName) const {
+llvm::SmallVector Names;
+// Split the function name with the separator "::".
+FuncName.split(Names, "::");
+
+// FIXME: Since there is no perfect way to get the qualified name without
+// template argument, we can only use a fuzzy matching approach.
+for (auto item : Names)
+  if (QualifiedName.find(item) == StringRef::npos)
+return false;
+
+return true;
+  }
 };
 
 template
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

The implementation is not complicated, the difficulty is that there is no good 
way to get the qualified name without template arguments. For 
'std::basic_string::c_str()', its qualified name may be 
`std::__1::basic_string, 
std::__1::allocator >::c_str`, it is almost impossible for users to 
provide such a name. So one possible implementation is to use `std`, 
`basic_string` and `c_str` to match in the `std::__1::basic_string, std::__1::allocator >::c_str` sequentially.


Repository:
  rC Clang

https://reviews.llvm.org/D48027



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

[PATCH] D48027: [analyzer] Improve `CallDescription` to handle c++ method.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: xazax.hun, NoQ, george.karpenkov.
Herald added subscribers: mikhail.ramalho, a.sidorin, rnkovacs, szepet.

`CallDecription` can only handle function for the time being. If we want to 
match c++ method, we can only use method name to match and can't improve the 
matching accuracy through the qualifiers.


Repository:
  rC Clang

https://reviews.llvm.org/D48027

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
  lib/StaticAnalyzer/Core/CallEvent.cpp


Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -60,6 +60,7 @@
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
+#include 
 
 #define DEBUG_TYPE "static-analyzer-call-event"
 
@@ -256,11 +257,18 @@
 return false;
   if (!CD.IsLookupDone) {
 CD.IsLookupDone = true;
-CD.II = 
()->getStateManager().getContext().Idents.get(CD.FuncName);
+CD.II = ()->getStateManager().getContext().Idents.get(
+CD.getFunctionName());
   }
   const IdentifierInfo *II = getCalleeIdentifier();
   if (!II || II != CD.II)
 return false;
+
+  const auto *ND = dyn_cast_or_null(getDecl());
+  if (!ND)
+return false;
+  if (!CD.matchQualifiedName(ND->getQualifiedNameAsString()))
+return false;
   return (CD.RequiredArgs == CallDescription::NoArgRequirement ||
   CD.RequiredArgs == getNumArgs());
 }
Index: include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 namespace clang {
 
@@ -79,6 +80,8 @@
 
   mutable IdentifierInfo *II = nullptr;
   mutable bool IsLookupDone = false;
+  // Represent the function name or method name, like "c_str" or
+  // "std::string::c_str".
   StringRef FuncName;
   unsigned RequiredArgs;
 
@@ -96,7 +99,25 @@
   : FuncName(FuncName), RequiredArgs(RequiredArgs) {}
 
   /// Get the name of the function that this object matches.
-  StringRef getFunctionName() const { return FuncName; }
+  StringRef getFunctionName() const {
+auto QualifierNamePair = FuncName.rsplit("::");
+return QualifierNamePair.second.empty() ? QualifierNamePair.first
+: QualifierNamePair.second;
+  }
+
+  bool matchQualifiedName(llvm::StringRef QualifiedName) const {
+llvm::SmallVector Names;
+// Split the function name with the separator "::".
+FuncName.split(Names, "::");
+
+// FIXME: Since there is no perfect way to get the qualified name without
+// template argument, we can only use a fuzzy matching approach.
+for (auto item : Names)
+  if (QualifiedName.find(item) == StringRef::npos)
+return false;
+
+return true;
+  }
 };
 
 template


Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -60,6 +60,7 @@
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
+#include 
 
 #define DEBUG_TYPE "static-analyzer-call-event"
 
@@ -256,11 +257,18 @@
 return false;
   if (!CD.IsLookupDone) {
 CD.IsLookupDone = true;
-CD.II = ()->getStateManager().getContext().Idents.get(CD.FuncName);
+CD.II = ()->getStateManager().getContext().Idents.get(
+CD.getFunctionName());
   }
   const IdentifierInfo *II = getCalleeIdentifier();
   if (!II || II != CD.II)
 return false;
+
+  const auto *ND = dyn_cast_or_null(getDecl());
+  if (!ND)
+return false;
+  if (!CD.matchQualifiedName(ND->getQualifiedNameAsString()))
+return false;
   return (CD.RequiredArgs == CallDescription::NoArgRequirement ||
   CD.RequiredArgs == getNumArgs());
 }
Index: include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -45,6 +45,7 @@
 #include 
 #include 
 #include 
+#include 
 
 namespace clang {
 
@@ -79,6 +80,8 @@
 
   mutable IdentifierInfo *II = nullptr;
   mutable bool IsLookupDone = false;
+  // Represent the function name or method name, like "c_str" or
+  // "std::string::c_str".
   StringRef FuncName;
   unsigned RequiredArgs;
 
@@ -96,7 +99,25 @@
   : FuncName(FuncName), RequiredArgs(RequiredArgs) {}
 
   /// Get the name of the function that this object matches.
-  StringRef getFunctionName() const { return FuncName; }
+  StringRef getFunctionName() const {
+auto QualifierNamePair = FuncName.rsplit("::");
+return QualifierNamePair.second.empty() ? QualifierNamePair.first
+  

[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

LGTM, @NoQ May have further feedback. Thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D47044



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

[PATCH] D47044: [analyzer] Ensure that we only visit a destructor for a reference if type information is available.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

I didn't test the code, but the code seems correct. Thanks!




Comment at: lib/StaticAnalyzer/Core/LoopWidening.cpp:88
+  MatchFinder Finder;
+  Finder.addMatcher(varDecl(hasType(referenceType())).bind("match"), new 
Callback(LCtx, MRMgr, ITraits));
+  Finder.matchAST(ASTCtx);

The code should fit within 80 columns of text.



Comment at: test/Analysis/loop-widening-invalid-type.cpp:1
+// RUN: %clang_cc1 -analyze 
-analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4 
-analyzer-config widen-loops=true -verify %s
+

I think it's better to add more expressive tests. Like:

```
struct A {
  int x;
  A(int x) : x(x) {}
};

void invalid_type_region_access() {
  const A  = A(10);
  for(int i = 0; i < 10; ++i) {}
  clang_analyzer_eval(a.x ==10); // expected-warning{{TRUE}}
}
```

I think should use more related names instead of 
`loop-widening-invalid-type.cpp`, like `loop-widening-reference-type`.



Comment at: test/Analysis/loop-widening-invalid-type.cpp:8
+
+void invalid_type_region_access() { // expected-no-diagnostics
+  const A  = B();

I don't know what the purpose of the test is, is the comment `no-crash` better?


Repository:
  rC Clang

https://reviews.llvm.org/D47044



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

[PATCH] D47658: [analyzer] Re-enable lifetime extension for temporaries with destructors and bring back static temporaries.

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:234
+  SVal V = UnknownVal();
+  if (MTE) {
+if (MTE->getStorageDuration() != SD_FullExpression) {

An unrelated question. I want to know, what considerations are you based on not 
continue to use short circuit style : )?


https://reviews.llvm.org/D47658



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

[PATCH] D47616: [CFG] [analyzer] Explain copy elision through construction contexts.

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: include/clang/Analysis/ConstructionContext.h:351
+
+  explicit SimpleTemporaryObjectConstructionContext(
+  const CXXBindTemporaryExpr *BTE, const MaterializeTemporaryExpr *MTE)

`explicit` useless?



Comment at: include/clang/Analysis/ConstructionContext.h:377
+
+  explicit ElidedTemporaryObjectConstructionContext(
+  const CXXBindTemporaryExpr *BTE, const MaterializeTemporaryExpr *MTE,

Same useless here?


https://reviews.llvm.org/D47616



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

[PATCH] D47451: [analyzer] Remove the redundant check about same state transition in `ArrayBoundCheckerV2.cpp`.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: NoQ, xazax.hun, george.karpenkov.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet.

Since the `addTransitionImpl()` has a check about same state transition, there 
is no need to check it in `ArrayBoundCheckerV2.cpp`.


Repository:
  rC Clang

https://reviews.llvm.org/D47451

Files:
  lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp


Index: lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
===
--- lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -125,7 +125,6 @@
   // have some flexibility in defining the base region, we can achieve
   // various levels of conservatism in our buffer overflow checking.
   ProgramStateRef state = checkerContext.getState();
-  ProgramStateRef originalState = state;
 
   SValBuilder  = checkerContext.getSValBuilder();
   const RegionRawOffsetV2  =
@@ -224,8 +223,7 @@
   }
   while (false);
 
-  if (state != originalState)
-checkerContext.addTransition(state);
+  checkerContext.addTransition(state);
 }
 
 void ArrayBoundCheckerV2::reportOOB(


Index: lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
===
--- lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
+++ lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
@@ -125,7 +125,6 @@
   // have some flexibility in defining the base region, we can achieve
   // various levels of conservatism in our buffer overflow checking.
   ProgramStateRef state = checkerContext.getState();
-  ProgramStateRef originalState = state;
 
   SValBuilder  = checkerContext.getSValBuilder();
   const RegionRawOffsetV2  =
@@ -224,8 +223,7 @@
   }
   while (false);
 
-  if (state != originalState)
-checkerContext.addTransition(state);
+  checkerContext.addTransition(state);
 }
 
 void ArrayBoundCheckerV2::reportOOB(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47417: [analyzer] Add missing state transition in IteratorChecker

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:399
+
+  C.addTransition(State);
 }

NoQ wrote:
> MTC wrote:
> > I have two questions may need @NoQ or @xazax.hun who is more familiar with 
> > the analyzer engine help to answer.
> > 
> >   - `State` may not change at all, do we need a check here like [[ 
> > https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp#L227
> >  | if (state != originalState) ]]
> >   - A more basic problem is that do we need `originalState = State` trick. 
> > It seems that `addTransitionImpl()` has a check about same state 
> > transition, see [[ 
> > https://github.com/llvm-mirror/clang/blob/master/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h#L339
> >  | addTransitionImp() ]].
> > 
> > Thanks in advance!
> > 
> > 
> > 
> > It seems that `addTransitionImpl()` has a check about same state 
> > transition, see `addTransitionImp()`.
> 
> Yep, you pretty much answered your question. The check in the checker code is 
> unnecessary.
Thanks, NoQ! It seems that `if (state != originalState)` in some checkers is 
misleading and may need to be cleaned up.


Repository:
  rC Clang

https://reviews.llvm.org/D47417



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

[PATCH] D47417: [analyzer] Add missing state transition in IteratorChecker

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp:399
+
+  C.addTransition(State);
 }

I have two questions may need @NoQ or @xazax.hun who is more familiar with the 
analyzer engine help to answer.

  - `State` may not change at all, do we need a check here like [[ 
https://github.com/llvm-mirror/clang/blob/master/lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp#L227
 | if (state != originalState) ]]
  - A more basic problem is that do we need `originalState = State` trick. It 
seems that `addTransitionImpl()` has a check about same state transition, see 
[[ 
https://github.com/llvm-mirror/clang/blob/master/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h#L339
 | addTransitionImp() ]].

Thanks in advance!





Repository:
  rC Clang

https://reviews.llvm.org/D47417



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

[PATCH] D47135: [analyzer][WIP] A checker for dangling string pointers in C++

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/DanglingStringPointerChecker.cpp:59
+  QualType RegType = TypedR->getValueType();
+  if (RegType.getAsString() != "std::string")
+return;

A little tip, there are other string types besides `std::string`, like 
`std::wstring`, which can be added in the future. See [[ 
http://en.cppreference.com/w/cpp/string/basic_string | basic_string ]].



Comment at: lib/StaticAnalyzer/Checkers/DanglingStringPointerChecker.cpp:71
+
+  if (dyn_cast(ICall)) {
+if (State->contains(TypedR)) {

`CXXDestructorCall::classof(const CallEvent *CA)` can also be used here. 



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:3002
+  // FIXME: This might not be the best allocation family for this purpose.
+  AllocationFamily Family = AF_CXXNew;
+  return State->set(Sym, RefState::getReleased(Family, Origin));

In addition to `std::string`, `std::vector::data()` in C++11 and 
`std::string_view` in C++17 may have similar problems. If these are all 
supported, a new `AllocationFamily` may be needed, like `AF_StdLibContainers` 
or something like that.



Comment at: lib/StaticAnalyzer/Checkers/RegionState.h:18
+
+namespace region_state {
+

I'm not sure whether `region_state` follows the naming conventions of LLVM 
coding standards. Can someone answer this question?


Repository:
  rC Clang

https://reviews.llvm.org/D47135



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

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 146816.
MTC added a comment.

- According to NoQ's suggestion, use `assumeZero()` instead of 
`isZeroConstant()` to determine whether the value is 0.
- Add test `memset26_upper_UCHAR_MAX()` and `memset27_symbol()`
- Since `void *memset( void *dest, int ch, size_t count );` will converts the 
value `ch` to `unsigned char`, we call `evalCast()` accordingly.


Repository:
  rC Clang

https://reviews.llvm.org/D44934

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/bstring.cpp
  test/Analysis/null-deref-ps-region.c
  test/Analysis/string.c

Index: test/Analysis/string.c
===
--- test/Analysis/string.c
+++ test/Analysis/string.c
@@ -1,7 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DSUPPRESS_OUT_OF_BOUND -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring.BufferOverlap,alpha.unix.cstring.NotNullTerminated,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
 
 //===--===
 // Declarations
@@ -1159,6 +1160,206 @@
   clang_analyzer_eval(str[1] == 'b'); // expected-warning{{UNKNOWN}}
 }
 
+//===--===
+// memset()
+//===--===
+
+void *memset(void *dest, int ch, size_t count);
+
+void *malloc(size_t size);
+void free(void *);
+
+void memset1_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '\0', 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+}
+
+void memset2_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '\0', strlen(str) + 1);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(str[2] == 0);  // expected-warning{{TRUE}}
+}
+
+void memset3_char_malloc_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  memset(str + 1, '\0', 8);
+  clang_analyzer_eval(str[1] == 0); // expected-warning{{UNKNOWN}}
+  free(str);
+}
+
+void memset4_char_malloc_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  //void *str = malloc(10 * sizeof(char));
+  memset(str, '\0', 10);
+  clang_analyzer_eval(str[1] == 0);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+  free(str);
+}
+
+#ifdef SUPPRESS_OUT_OF_BOUND
+void memset5_char_malloc_overflow_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  memset(str, '\0', 12);
+  clang_analyzer_eval(str[1] == 0); // expected-warning{{UNKNOWN}}
+  free(str);
+}
+#endif
+
+void memset6_char_array_nonnull() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '0', 2);
+  clang_analyzer_eval(str[0] == 'a');// expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{UNKNOWN}}
+}
+
+#ifdef SUPPRESS_OUT_OF_BOUND
+void memset8_char_array_nonnull() {
+  char str[5] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '0', 10);
+  clang_analyzer_eval(str[0] != '0'); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(strlen(str) >= 10); // expected-warning{{TRUE}}
+  clang_analyzer_eval(strlen(str) < 10);  // expected-warning{{FALSE}}
+}
+#endif
+
+struct 

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1037
+
+if (StateWholeReg && !StateNotWholeReg && CharVal.isZeroConstant()) {
+  // If the 'memset()' acts on the whole region of destination buffer and

NoQ wrote:
> I think we should use `StateNonNullChar` (that's computed below) instead of 
> `CharVal.isZeroConstant()` because the Environment doesn't provide a 
> guarantee that all symbols within it are collapsed to constants where 
> applicable.
Yea, thanks! 



Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:1055
+std::tie(StateNullChar, StateNonNullChar) =
+assumeZero(C, State, CharVal, Ctx.UnsignedCharTy);
+

NoQ wrote:
> I think this should use `IntTy` here. Because that's the type of the 
> `memset`'s argument, and that's what `assumeZero()` expects.
I confirmed again that `memset()` will convert the value to `unsigned char` 
first, see http://en.cppreference.com/w/c/string/byte/memset. 

In the next update, I will `evalCast(value, UnsignedCharTy, IntTy)` first, 
therefore, I will continue to use `UnsignedCharTy`.



Comment at: test/Analysis/string.c:1412
+  clang_analyzer_eval(strlen(str) >= 10); // expected-warning{{TRUE}}
+  // FIXME: This shoule be TRUE.
+  clang_analyzer_eval(str[1] == '0'); // expected-warning{{UNKNOWN}}

NoQ wrote:
> Typo: `should` :)
thanks, will do!


Repository:
  rC Clang

https://reviews.llvm.org/D44934



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

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

ping.


Repository:
  rC Clang

https://reviews.llvm.org/D44934



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

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 145361.
MTC added a comment.

- Since there is no perfect way to handle the default binding of non-zero 
character, remove the default binding of non-zero character. Use 
`bindDefaulrZero()` instead of `overwriteRegion()` to bind the zero character.
- Reuse `assume()` instead of `isZeroConstant()` to determine whether it is 
zero character. The purpose of this is to be able to set the string length 
**when dealing with non-zero symbol character**.


Repository:
  rC Clang

https://reviews.llvm.org/D44934

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  test/Analysis/bstring.cpp
  test/Analysis/null-deref-ps-region.c
  test/Analysis/string.c

Index: test/Analysis/string.c
===
--- test/Analysis/string.c
+++ test/Analysis/string.c
@@ -1,7 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DSUPPRESS_OUT_OF_BOUND -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring.BufferOverlap,alpha.unix.cstring.NotNullTerminated,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
 
 //===--===
 // Declarations
@@ -1159,6 +1160,186 @@
   clang_analyzer_eval(str[1] == 'b'); // expected-warning{{UNKNOWN}}
 }
 
+//===--===
+// memset()
+//===--===
+
+void *memset(void *dest, int ch, size_t count);
+
+void *malloc(size_t size);
+void free(void *);
+
+void memset1_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '\0', 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+}
+
+void memset2_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '\0', strlen(str) + 1);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(str[2] == 0);  // expected-warning{{TRUE}}
+}
+
+void memset3_char_malloc_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  memset(str + 1, '\0', 8);
+  clang_analyzer_eval(str[1] == 0); // expected-warning{{UNKNOWN}}
+  free(str);
+}
+
+void memset4_char_malloc_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  //void *str = malloc(10 * sizeof(char));
+  memset(str, '\0', 10);
+  clang_analyzer_eval(str[1] == 0);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+  free(str);
+}
+
+#ifdef SUPPRESS_OUT_OF_BOUND
+void memset5_char_malloc_overflow_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  memset(str, '\0', 12);
+  clang_analyzer_eval(str[1] == 0); // expected-warning{{UNKNOWN}}
+  free(str);
+}
+#endif
+
+void memset6_char_array_nonnull() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '0', 2);
+  clang_analyzer_eval(str[0] == 'a');// expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{UNKNOWN}}
+}
+
+#ifdef SUPPRESS_OUT_OF_BOUND
+void memset8_char_array_nonnull() {
+  char str[5] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '0', 10);
+  clang_analyzer_eval(str[0] != '0'); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(strlen(str) >= 10); // expected-warning{{TRUE}}
+  

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D44934#1088771, @NoQ wrote:

> Hmm, ok, it seems that i've just changed the API in 
> https://reviews.llvm.org/D46368, and i should have thought about this use 
> case. Well, at least i have some background understanding of these problems 
> now. Sorry for not keeping eye on this problem.
>
> In https://reviews.llvm.org/D44934#1051002, @NoQ wrote:
>
> > Why do you need separate code for null and non-null character? The 
> > function's semantics doesn't seem to care.
>
>
> I guess i can answer myself here:
>
>   int32_t x;
>   memset(, 1, sizeof(int32_t));
>   clang_analyzer_eval(x == 0x1010101); // should be TRUE
>
>
> I really doubt that we support this case.
>
> So, yeah, zero character is indeed special.


Thank you, Artem! I did not consider this common situation.  This patch does 
not really support this situation, in this patch the value of `x` will be 1, 
it's not correct!

> And since zero character is special, i guess we could use the new 
> `bindDefaultZero()` API, and perform a simple invalidation in the non-zero 
> character case.

Agree with you. The core problem here is that there is no perfect way to `bind 
default` for non-zero value, not the string length stuff. So **invalidate the 
destination buffer** but still **set string length** for non-zero character is 
OK. Right?

> @MTC, would this be enough for your use case? Or is it really important for 
> you to support the non-zero character case? Cause my example is fairly 
> artificial, and probably i'm worrying too much about it. If nobody really 
> codes that way, i'm fine with supporting the non-zero character case via a 
> simple default binding. In this case we should merge `bindDefaultZero()` with 
> your `overwriteRegion()` (i'd prefer your function name, but please keep the 
> empty class check).

Based on my limited programming experience, I think `memset` with non-zero 
character is common. However given that we can't handle non-zero character 
binding problems very well, we should now support only zero character. After 
all, IMHO,  correctness of semantic is also important  for the analyzer.

I will update this patch to only handle zero character case. In the future, as 
I am more familiar with the `RegionStore`, I will solve the problem of non-zero 
character binding.


Repository:
  rC Clang

https://reviews.llvm.org/D44934



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

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 145019.
MTC added a comment.

- fix typos
- code refactoring, add auxiliary method `memsetAux()`
- according to a.sidorin's suggestions, remove the useless state splitting.
- make `StoreManager::overwriteRegion()` pure virtual


Repository:
  rC Clang

https://reviews.llvm.org/D44934

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Core/ProgramState.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  test/Analysis/bstring.cpp
  test/Analysis/null-deref-ps-region.c
  test/Analysis/string.c

Index: test/Analysis/string.c
===
--- test/Analysis/string.c
+++ test/Analysis/string.c
@@ -1,7 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DSUPPRESS_OUT_OF_BOUND -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring.BufferOverlap,alpha.unix.cstring.NotNullTerminated,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
 
 //===--===
 // Declarations
@@ -1159,6 +1160,248 @@
   clang_analyzer_eval(str[1] == 'b'); // expected-warning{{UNKNOWN}}
 }
 
+//===--===
+// memset()
+//===--===
+
+void *memset( void *dest, int ch, size_t count );
+
+void* malloc(size_t size);
+void free(void*);
+
+void memset1_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '\0', 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+}
+
+void memset2_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '\0', strlen(str) + 1);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(str[2] == 0);  // expected-warning{{TRUE}}
+}
+
+void memset3_char_malloc_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  memset(str + 1, '\0', 8);
+  clang_analyzer_eval(str[1] == 0); // expected-warning{{UNKNOWN}}
+  free(str);
+}
+
+void memset4_char_malloc_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  //void *str = malloc(10 * sizeof(char));
+  memset(str, '\0', 10);
+  clang_analyzer_eval(str[1] == 0);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+  free(str);
+}
+
+#ifdef SUPPRESS_OUT_OF_BOUND
+void memset5_char_malloc_overflow_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  memset(str, '\0', 12);
+  clang_analyzer_eval(str[1] == 0); // expected-warning{{UNKNOWN}}
+  free(str);
+}
+#endif
+
+void memset6_char_array_nonnull() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '0', 2);
+  clang_analyzer_eval(str[0] == 'a');// expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{UNKNOWN}}
+}
+
+void memset7_char_array_nonnull() {
+  char str[5] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '0', 5);
+  clang_analyzer_eval(str[0] == '0');// expected-warning{{TRUE}}
+  clang_analyzer_eval(strlen(str) >= 5); // expected-warning{{TRUE}}
+}
+
+#ifdef SUPPRESS_OUT_OF_BOUND
+void 

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

Sorry for the long delay, I have just finished my holiday.

Thanks a lot for the review, I will fix the typo in next update.




Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2100
+std::tie(StateNullChar, StateNonNullChar) =
+assumeZero(C, State, CharVal, Ctx.UnsignedCharTy);
+

a.sidorin wrote:
> I think we can use the argument type here (which is int).
> One more question. Could we use `SVal::isZeroConstant()` here instead? Do we 
> need the path-sensitivity for the value or we can just check if the value is 
> a constant zero?
Yea, it's should be int.

`SVal::isZeroConstant()` is enough here, thanks!

One question that has nothing to do with the patch, I would like to know if 
there is a standard to determine whether there is a need for state splitting.




Comment at: lib/StaticAnalyzer/Checkers/CStringChecker.cpp:2127
+ /*IsSourceBuffer*/ false, Size);
+  }
+

a.sidorin wrote:
> Could you please factor this chunk to a function? It makes the method too big.
will do.



Comment at: lib/StaticAnalyzer/Core/ProgramState.cpp:148
+  ProgramStateRef NewState = makeWithStore(NewStore);
+  return Mgr.getOwningEngine()
+ ? Mgr.getOwningEngine()->processRegionChange(NewState, R, LCtx)

a.sidorin wrote:
> Do clients of `overwriteRegion` really need to call checkers?
It is possible that my understanding of the engine is not enough, I think we 
need to call `processRangeChange()` for memory change. `memset()` will 
overwrite the memory region, so I think there is a need to call this API.

What do you think?



Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1718
 
-llvm_unreachable("Unknown default value");
+return val;
   }

a.sidorin wrote:
> Could you please explain what is the reason of this change? Do we get new 
> value kinds?
`memset()` can bind anything to the region with `default binding`, if there is 
no such change, it will trigger `llvm_unreachable()`. 

I don't think much about it, just put `return val;` here. Any suggestions?



Comment at: lib/StaticAnalyzer/Core/Store.cpp:72
 
+StoreRef StoreManager::OverwriteRegion(Store store, const MemRegion *R, SVal 
V) {
+  return StoreRef(store, *this); 

a.sidorin wrote:
> Could we make this method pure virtual?
To be honest, I implemented this function according to `BindDefault()`. And I 
also think pure virtual is better, thanks.


Repository:
  rC Clang

https://reviews.llvm.org/D44934



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

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

ping^2


Repository:
  rC Clang

https://reviews.llvm.org/D44934



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

[PATCH] D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 143908.
MTC marked an inline comment as done.
MTC added a comment.

Since `BugReport::addVisitor()` has checks for the null `Visitor`, remove the 
checks before `BugReport->addVisitor()`.


Repository:
  rC Clang

https://reviews.llvm.org/D46007

Files:
  lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
  lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  test/Analysis/taint-diagnostic-visitor.c

Index: test/Analysis/taint-diagnostic-visitor.c
===
--- test/Analysis/taint-diagnostic-visitor.c
+++ test/Analysis/taint-diagnostic-visitor.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core -analyzer-output=text -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 -analyzer-output=text -verify %s
 
 // This file is for testing enhanced diagnostics produced by the GenericTaintChecker
 
@@ -11,3 +11,26 @@
   scanf("%s", buf); // expected-note {{Taint originated here}}
   system(buf); // expected-warning {{Untrusted data is passed to a system call}} // expected-note {{Untrusted data is passed to a system call (CERT/STR02-C. Sanitize data passed to complex subsystems)}}
 }
+
+int taintDiagnosticOutOfBound() {
+  int index;
+  int Array[] = {1, 2, 3, 4, 5};
+  scanf("%d", ); // expected-note {{Taint originated here}}
+  return Array[index]; // expected-warning {{Out of bound memory access (index is tainted)}}
+   // expected-note@-1 {{Out of bound memory access (index is tainted)}}
+}
+
+int taintDiagnosticDivZero(int operand) {
+  scanf("%d", ); // expected-note {{Value assigned to 'operand'}}
+ // expected-note@-1 {{Taint originated here}}
+  return 10 / operand; // expected-warning {{Division by a tainted value, possibly zero}}
+   // expected-note@-1 {{Division by a tainted value, possibly zero}}
+}
+
+void taintDiagnosticVLA() {
+  int x;
+  scanf("%d", ); // expected-note {{Value assigned to 'x'}}
+   // expected-note@-1 {{Taint originated here}}
+  int vla[x]; // expected-warning {{Declared variable-length array (VLA) has tainted size}}
+  // expected-note@-1 {{Declared variable-length array (VLA) has tainted size}}
+}
Index: lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
@@ -32,19 +32,18 @@
   mutable std::unique_ptr BT;
   enum VLASize_Kind { VLA_Garbage, VLA_Zero, VLA_Tainted, VLA_Negative };
 
-  void reportBug(VLASize_Kind Kind,
- const Expr *SizeE,
- ProgramStateRef State,
- CheckerContext ) const;
+  void reportBug(VLASize_Kind Kind, const Expr *SizeE, ProgramStateRef State,
+ CheckerContext ,
+ std::unique_ptr Visitor = nullptr) const;
+
 public:
   void checkPreStmt(const DeclStmt *DS, CheckerContext ) const;
 };
 } // end anonymous namespace
 
-void VLASizeChecker::reportBug(VLASize_Kind Kind,
-   const Expr *SizeE,
-   ProgramStateRef State,
-   CheckerContext ) const {
+void VLASizeChecker::reportBug(
+VLASize_Kind Kind, const Expr *SizeE, ProgramStateRef State,
+CheckerContext , std::unique_ptr Visitor) const {
   // Generate an error node.
   ExplodedNode *N = C.generateErrorNode(State);
   if (!N)
@@ -73,6 +72,7 @@
   }
 
   auto report = llvm::make_unique(*BT, os.str(), N);
+  report->addVisitor(std::move(Visitor));
   report->addRange(SizeE->getSourceRange());
   bugreporter::trackNullOrUndefValue(N, SizeE, *report);
   C.emitReport(std::move(report));
@@ -108,7 +108,8 @@
 
   // Check if the size is tainted.
   if (state->isTainted(sizeV)) {
-reportBug(VLA_Tainted, SE, nullptr, C);
+reportBug(VLA_Tainted, SE, nullptr, C,
+  llvm::make_unique(sizeV));
 return;
   }
 
Index: lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
@@ -24,22 +24,23 @@
 namespace {
 class DivZeroChecker : public Checker< check::PreStmt > {
   mutable std::unique_ptr BT;
-  void reportBug(const char *Msg,
- ProgramStateRef StateZero,
- CheckerContext ) const ;
+  void reportBug(const char *Msg, ProgramStateRef StateZero, CheckerContext ,
+ std::unique_ptr Visitor = nullptr) const;
+
 public:
   void checkPreStmt(const BinaryOperator *B, CheckerContext ) const;
 };
 } // end anonymous namespace
 
-void DivZeroChecker::reportBug(const char *Msg,
-   ProgramStateRef StateZero,
- 

[PATCH] D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize.

[Henry Wong via Phabricator via cfe-commits]

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



Comment at: lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp:75
   auto report = llvm::make_unique(*BT, os.str(), N);
+  report->addVisitor(std::move(Visitor));
   report->addRange(SizeE->getSourceRange());

a.sidorin wrote:
> In this patch, sometimes we check the visitor to be non-null, sometimes not.  
> As I can see, `BugReport::addVisitor()` works well with `nullptr` arguments 
> (it checks arguments) so I think we can omit the checks.
Thanks for your reminder, a.sidorin! 

My mistakes led to some checkers doing the check and some did not check!  But 
as you said, there is no need to check the nullptr.

I will update the patch.


Repository:
  rC Clang

https://reviews.llvm.org/D46007



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

[PATCH] D46007: [analyzer] Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero and VLASize.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: NoQ, george.karpenkov, xazax.hun, a.sidorin.
Herald added subscribers: cfe-commits, rnkovacs, szepet.

Add `TaintBugVisitor` to the ArrayBoundV2, DivideZero, VLASize to be able to 
indicate where the taint information originated from.


Repository:
  rC Clang

https://reviews.llvm.org/D46007

Files:
  lib/StaticAnalyzer/Checkers/ArrayBoundCheckerV2.cpp
  lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
  lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
  test/Analysis/taint-diagnostic-visitor.c

Index: test/Analysis/taint-diagnostic-visitor.c
===
--- test/Analysis/taint-diagnostic-visitor.c
+++ test/Analysis/taint-diagnostic-visitor.c
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core -analyzer-output=text -verify %s
+// RUN: %clang_cc1 -analyze -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 -analyzer-output=text -verify %s
 
 // This file is for testing enhanced diagnostics produced by the GenericTaintChecker
 
@@ -11,3 +11,26 @@
   scanf("%s", buf); // expected-note {{Taint originated here}}
   system(buf); // expected-warning {{Untrusted data is passed to a system call}} // expected-note {{Untrusted data is passed to a system call (CERT/STR02-C. Sanitize data passed to complex subsystems)}}
 }
+
+int taintDiagnosticOutOfBound() {
+  int index;
+  int Array[] = {1, 2, 3, 4, 5};
+  scanf("%d", ); // expected-note {{Taint originated here}}
+  return Array[index]; // expected-warning {{Out of bound memory access (index is tainted)}}
+   // expected-note@-1 {{Out of bound memory access (index is tainted)}}
+}
+
+int taintDiagnosticDivZero(int operand) {
+  scanf("%d", ); // expected-note {{Value assigned to 'operand'}}
+ // expected-note@-1 {{Taint originated here}}
+  return 10 / operand; // expected-warning {{Division by a tainted value, possibly zero}}
+   // expected-note@-1 {{Division by a tainted value, possibly zero}}
+}
+
+void taintDiagnosticVLA() {
+  int x;
+  scanf("%d", ); // expected-note {{Value assigned to 'x'}}
+   // expected-note@-1 {{Taint originated here}}
+  int vla[x]; // expected-warning {{Declared variable-length array (VLA) has tainted size}}
+  // expected-note@-1 {{Declared variable-length array (VLA) has tainted size}}
+}
Index: lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
+++ lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
@@ -32,19 +32,18 @@
   mutable std::unique_ptr BT;
   enum VLASize_Kind { VLA_Garbage, VLA_Zero, VLA_Tainted, VLA_Negative };
 
-  void reportBug(VLASize_Kind Kind,
- const Expr *SizeE,
- ProgramStateRef State,
- CheckerContext ) const;
+  void reportBug(VLASize_Kind Kind, const Expr *SizeE, ProgramStateRef State,
+ CheckerContext ,
+ std::unique_ptr Visitor = nullptr) const;
+
 public:
   void checkPreStmt(const DeclStmt *DS, CheckerContext ) const;
 };
 } // end anonymous namespace
 
-void VLASizeChecker::reportBug(VLASize_Kind Kind,
-   const Expr *SizeE,
-   ProgramStateRef State,
-   CheckerContext ) const {
+void VLASizeChecker::reportBug(
+VLASize_Kind Kind, const Expr *SizeE, ProgramStateRef State,
+CheckerContext , std::unique_ptr Visitor) const {
   // Generate an error node.
   ExplodedNode *N = C.generateErrorNode(State);
   if (!N)
@@ -73,6 +72,7 @@
   }
 
   auto report = llvm::make_unique(*BT, os.str(), N);
+  report->addVisitor(std::move(Visitor));
   report->addRange(SizeE->getSourceRange());
   bugreporter::trackNullOrUndefValue(N, SizeE, *report);
   C.emitReport(std::move(report));
@@ -108,7 +108,8 @@
 
   // Check if the size is tainted.
   if (state->isTainted(sizeV)) {
-reportBug(VLA_Tainted, SE, nullptr, C);
+reportBug(VLA_Tainted, SE, nullptr, C,
+  llvm::make_unique(sizeV));
 return;
   }
 
Index: lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
+++ lib/StaticAnalyzer/Checkers/DivZeroChecker.cpp
@@ -24,22 +24,25 @@
 namespace {
 class DivZeroChecker : public Checker< check::PreStmt > {
   mutable std::unique_ptr BT;
-  void reportBug(const char *Msg,
- ProgramStateRef StateZero,
- CheckerContext ) const ;
+  void reportBug(const char *Msg, ProgramStateRef StateZero, CheckerContext ,
+ std::unique_ptr Visitor = nullptr) const;
+
 public:
   void checkPreStmt(const BinaryOperator *B, CheckerContext ) const;
 };
 } // end anonymous namespace
 
-void DivZeroChecker::reportBug(const char *Msg,
-   

[PATCH] D45682: [analyzer] Move `TaintBugVisitor` from `GenericTaintChecker.cpp` to `BugReporterVisitors.h`.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D45682#1074407, @george.karpenkov wrote:

> I'm new to the taint visitor, but I am quite confused by your change 
> description.
>
> > and many checkers rely on it
>
> How can other checkers rely on it if it's private to the taint checker?


Thanks for your review, george! `TaintBugVisitor` is an utility to add extra 
information to illustrate where the taint information originated from. There 
are several checkers use taint information, e.g. `ArrayBoundCheckerV2.cpp`, in 
some cases it will report a warning, like `warning: Out of bound memory access 
(index is tainted)`. If `TaintBugVisitor` moves to `BugReporterVisitors.h`, 
`ArrayBoundCheckerV2` can add extra notes like `Taint originated here` to the 
report by adding `TaintBugVisitor`.

> Also, it's probably to explicitly include BugReporterVisitors.h in the 
> checker file then.

If these checkers want to add `Taint originated here` using `TaintBugVisitor`, 
it is necessary to explicitly include `BugReporterVisitors.h` in following 
patch.


Repository:
  rC Clang

https://reviews.llvm.org/D45682



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

[PATCH] D45774: [analyzer] cover more cases where a Loc can be bound to constants

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

Test files for `initialization` missing? : )


Repository:
  rC Clang

https://reviews.llvm.org/D45774



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

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D45532#1069683, @whisperity wrote:

> There is something that came up in my mind:
>
> Consider a construct like this:
>
>   class A
>   {
> A()
> {
>   memset(X, 0, 10 * sizeof(int));
> }
>  
> int X[10];
>   };
>
>
> I think it's worthy for a test case if this `X` is considered unitialised at 
> the end of the constructor or not. (And if not, a ticket, or a fix for SA in 
> a subpatch.)


Just for a reminding, there is a very primitive patch to model `memset()`, see 
D44934 . And if we think that `memset` is a 
way of initialization, we may need to distinguish between POD objects and 
non-POD Objects.


https://reviews.llvm.org/D45532



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

[PATCH] D45682: [analyzer] Move `TaintBugVisitor` from `GenericTaintChecker.cpp` to `BugReporterVisitors.h`.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: NoQ, george.karpenkov, xazax.hun.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet.

`TaintBugVisitor` is a universal visitor, and many checkers rely on it, such as 
`ArrayBoundCheckerV2.cpp`, `DivZeroChecker.cpp` and `VLASizeChecker.cpp`. 
Moving `TaintBugVisitor` to `BugReporterVisitors.h` enables other checker can 
also track where `tainted` value came from.


Repository:
  rC Clang

https://reviews.llvm.org/D45682

Files:
  include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
  lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  lib/StaticAnalyzer/Core/BugReporterVisitors.cpp

Index: lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
===
--- lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
+++ lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
@@ -2333,3 +2333,24 @@
 
   return std::move(Piece);
 }
+
+std::shared_ptr
+TaintBugVisitor::VisitNode(const ExplodedNode *N, const ExplodedNode *PrevN,
+   BugReporterContext , BugReport ) {
+
+  // Find the ExplodedNode where the taint was first introduced
+  if (!N->getState()->isTainted(V) || PrevN->getState()->isTainted(V))
+return nullptr;
+
+  const Stmt *S = PathDiagnosticLocation::getStmt(N);
+  if (!S)
+return nullptr;
+
+  const LocationContext *NCtx = N->getLocationContext();
+  PathDiagnosticLocation L =
+  PathDiagnosticLocation::createBegin(S, BRC.getSourceManager(), NCtx);
+  if (!L.isValid() || !L.asLocation().isValid())
+return nullptr;
+
+  return std::make_shared(L, "Taint originated here");
+}
Index: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -100,23 +100,6 @@
   bool generateReportIfTainted(const Expr *E, const char Msg[],
CheckerContext ) const;
 
-  /// The bug visitor prints a diagnostic message at the location where a given
-  /// variable was tainted.
-  class TaintBugVisitor
-  : public BugReporterVisitorImpl {
-  private:
-const SVal V;
-
-  public:
-TaintBugVisitor(const SVal V) : V(V) {}
-void Profile(llvm::FoldingSetNodeID ) const override { ID.Add(V); }
-
-std::shared_ptr VisitNode(const ExplodedNode *N,
-   const ExplodedNode *PrevN,
-   BugReporterContext ,
-   BugReport ) override;
-  };
-
   typedef SmallVector ArgVector;
 
   /// \brief A struct used to specify taint propagation rules for a function.
@@ -214,28 +197,6 @@
 /// points to data, which should be tainted on return.
 REGISTER_SET_WITH_PROGRAMSTATE(TaintArgsOnPostVisit, unsigned)
 
-std::shared_ptr
-GenericTaintChecker::TaintBugVisitor::VisitNode(const ExplodedNode *N,
-const ExplodedNode *PrevN, BugReporterContext , BugReport ) {
-
-  // Find the ExplodedNode where the taint was first introduced
-  if (!N->getState()->isTainted(V) || PrevN->getState()->isTainted(V))
-return nullptr;
-
-  const Stmt *S = PathDiagnosticLocation::getStmt(N);
-  if (!S)
-return nullptr;
-
-  const LocationContext *NCtx = N->getLocationContext();
-  PathDiagnosticLocation L =
-  PathDiagnosticLocation::createBegin(S, BRC.getSourceManager(), NCtx);
-  if (!L.isValid() || !L.asLocation().isValid())
-return nullptr;
-
-  return std::make_shared(
-  L, "Taint originated here");
-}
-
 GenericTaintChecker::TaintPropagationRule
 GenericTaintChecker::TaintPropagationRule::getTaintPropagationRule(
  const FunctionDecl *FDecl,
Index: include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
===
--- include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
+++ include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h
@@ -343,6 +343,22 @@
  BugReport ) override;
 };
 
+/// The bug visitor prints a diagnostic message at the location where a given
+/// variable was tainted.
+class TaintBugVisitor final : public BugReporterVisitorImpl {
+private:
+  const SVal V;
+
+public:
+  TaintBugVisitor(const SVal V) : V(V) {}
+  void Profile(llvm::FoldingSetNodeID ) const override { ID.Add(V); }
+
+  std::shared_ptr VisitNode(const ExplodedNode *N,
+ const ExplodedNode *PrevN,
+ BugReporterContext ,
+ BugReport ) override;
+};
+
 namespace bugreporter {
 
 /// Attempts to add visitors to trace a null or undefined value back to its
___

[PATCH] D45491: [analyzer] Do not invalidate the `this` pointer.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D45491#1067852, @NoQ wrote:

> Yeah, i think this makes sense, thanks! It feels a bit weird that we have to 
> add it as an exception - i wonder if there are other exceptions that we need 
> to make. Widening over the stack memory space should be a whitelist, not a 
> blacklist, because we can easily enumerate all stack variables and see which 
> of them can be modified at all from the loop. But until we have that, this 
> looks like a reasonable workaround.


You are right , it's really weird. And the better solution, D36690 
, seems to be blocked.


Repository:
  rC Clang

https://reviews.llvm.org/D45491



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

[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:10
+//
+//  This file defines a checker that checks cunstructors for possibly
+//  uninitialized fields.

typo :)  `cunstructors` -> `constructors`



Comment at: lib/StaticAnalyzer/Checkers/CtorUninitializedMemberChecker.cpp:54
+  StringRef getAsString(SmallString<200> ) const;
+  friend class FieldChainInfoComparator;
+};

When I apply this patch, hit a compiler warning. Maybe `friend struct` is 
better.
```
warning: 'FieldChainInfoComparator' defined as a struct here but previously 
declared as a class [-Wmismatched-tags]
```


https://reviews.llvm.org/D45532



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

[PATCH] D45491: [analyzer] Do not invalidate the `this` pointer.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 142026.
MTC marked 2 inline comments as done.
MTC added a comment.

- Move the `CXXThisRegion`'s check to `LoopWidening.cpp`
- Use `isa(R)` instead of `CXXThisRegion::classof(R)`.


Repository:
  rC Clang

https://reviews.llvm.org/D45491

Files:
  lib/StaticAnalyzer/Core/LoopWidening.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  test/Analysis/this-pointer.cpp

Index: test/Analysis/this-pointer.cpp
===
--- /dev/null
+++ test/Analysis/this-pointer.cpp
@@ -0,0 +1,88 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config widen-loops=true -analyzer-disable-retry-exhausted -verify %s
+
+void clang_analyzer_eval(bool);
+void clang_analyzer_dump(int);
+
+// 'this' pointer is not an lvalue, we should not invalidate it.
+namespace this_pointer_after_loop_widen {
+class A {
+public:
+  A() {
+int count = 10;
+do {
+} while (count--);
+  }
+};
+
+void goo(A a);
+void test_temporary_object() {
+  goo(A()); // no-crash
+}
+
+struct B {
+  int mem;
+  B() : mem(0) {
+for (int i = 0; i < 10; ++i) {
+}
+mem = 0;
+  }
+};
+
+void test_ctor() {
+  B b;
+  clang_analyzer_eval(b.mem == 0); // expected-warning{{TRUE}}
+}
+
+struct C {
+  int mem;
+  C() : mem(0) {}
+  void set() {
+for (int i = 0; i < 10; ++i) {
+}
+mem = 10;
+  }
+};
+
+void test_method() {
+  C c;
+  clang_analyzer_eval(c.mem == 0); // expected-warning{{TRUE}}
+  c.set();
+  clang_analyzer_eval(c.mem == 10); // expected-warning{{TRUE}}
+}
+
+struct D {
+  int mem;
+  D() : mem(0) {}
+  void set() {
+for (int i = 0; i < 10; ++i) {
+}
+mem = 10;
+  }
+};
+
+void test_new() {
+  D *d = new D;
+  clang_analyzer_eval(d->mem == 0); // expected-warning{{TRUE}}
+  d->set();
+  clang_analyzer_eval(d->mem == 10); // expected-warning{{TRUE}}
+}
+
+struct E {
+  int mem;
+  E() : mem(0) {}
+  void set() {
+for (int i = 0; i < 10; ++i) {
+}
+setAux();
+  }
+  void setAux() {
+this->mem = 10;
+  }
+};
+
+void test_chained_method_call() {
+  E e;
+  e.set();
+  clang_analyzer_eval(e.mem == 10); // expected-warning{{TRUE}}
+}
+} // namespace this_pointer_after_loop_widen
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -2050,6 +2050,9 @@
 R = GetElementZeroRegion(SR, T);
   }
 
+  assert((!isa(R) || !B.lookup(R)) &&
+ "'this' pointer is not an l-value and is not assignable");
+
   // Clear out bindings that may overlap with this binding.
   RegionBindingsRef NewB = removeSubRegionBindings(B, cast(R));
   return NewB.addBinding(BindingKey::Make(R, BindingKey::Direct), V);
Index: lib/StaticAnalyzer/Core/LoopWidening.cpp
===
--- lib/StaticAnalyzer/Core/LoopWidening.cpp
+++ lib/StaticAnalyzer/Core/LoopWidening.cpp
@@ -59,6 +59,18 @@
 ITraits.setTrait(Region,
  RegionAndSymbolInvalidationTraits::TK_EntireMemSpace);
   }
+
+  // 'this' pointer is not an lvalue, we should not invalidate it. If the loop
+  // is located in a method, constructor or destructor, the value of 'this'
+  // pointer shoule remain unchanged.
+  if (const CXXMethodDecl *CXXMD = dyn_cast(STC->getDecl())) {
+const CXXThisRegion *ThisR = MRMgr.getCXXThisRegion(
+CXXMD->getThisType(STC->getAnalysisDeclContext()->getASTContext()),
+STC);
+ITraits.setTrait(ThisR,
+ RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+  }
+
   return PrevState->invalidateRegions(Regions, getLoopCondition(LoopStmt),
   BlockCount, LCtx, true, nullptr, nullptr,
   );
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45491: [analyzer] Do not invalidate the `this` pointer.

[Henry Wong via Phabricator via cfe-commits]

MTC marked 2 inline comments as done.
MTC added inline comments.



Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1024-1027
+  // 'this' pointer is not an lvalue, we should not invalidate it.
+  if (CXXThisRegion::classof(baseR))
+return;
+

NoQ wrote:
> I don't think this run-time check belongs here. The fix should be isolated in 
> loop widening because everything else is already known to work correctly. The 
> invalidation worker is not to be blamed for other entities throwing incorrect 
> stuff into it.
Make sense, will do.



Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:2057-2060
+  assert((!CXXThisRegion::classof(R) ||
+  CXXThisRegion::classof(R) && !B.lookup(R)) &&
+ "'this' pointer is not an l-value and is not assignable");
+

NoQ wrote:
> This assertion is great to have.
> 
> Please use `!isa(R)` instead of `CXXThisRegion::classof(R)`. 
> The left argument of `&&` is always true, so you can omit it.
Excellent advice, thank you! Will do.


Repository:
  rC Clang

https://reviews.llvm.org/D45491



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

[PATCH] D45491: [analyzer] Do not invalidate the `this` pointer.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D45491#1063364, @george.karpenkov wrote:

> @MTC what happens for
>
>   this.j = 0;
>   for (int i=0; i<100; i++)
>  this.j++;
>
>
> ?


@george.karpenkov  `this`'s value will remain unchanged, `j` will be 
invalidated.

   1   void clang_analyzer_printState();
   2   struct A {
   3   int j;
   4   void foo() {
   5   this->j = 0;
   6   clang_analyzer_printState();
   7   for (int i = 0; i < 100; ++i)
   8   this->j++;
   9   clang_analyzer_printState();
  10 }
  11   };
  12
  13   void func() {
  14   A a;
  15   a.foo();
  16   }

For the above code, given the command `clang -cc1 -analyze 
-analyzer-checker=core,debug.ExprInspection -analyzer-config widen-loops=true 
test.cpp`. The output about `Store` is as follows.

  Store (direct and default bindings), 0x7fb7d008c068 :
   (a,0,direct) : 0 S32b
  
   (this,0,direct) : 
  
  
  Store (direct and default bindings), 0x7fb7d080a618 :
   (a,0,default) : conj_$1{int}
  
   (this,0,direct) : 


Repository:
  rC Clang

https://reviews.llvm.org/D45491



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

[PATCH] D45491: [analyzer] Do not invalidate the `this` pointer.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: NoQ, george.karpenkov, a.sidorin, seaneveson, szepet.
Herald added subscribers: cfe-commits, rnkovacs, xazax.hun.
MTC edited the summary of this revision.

`this` pointer is not an l-value, although we have modeled `CXXThisRegion` for 
`this` pointer, we can only bind it once, which is when we start to inline 
method. And this patch fixes https://bugs.llvm.org/show_bug.cgi?id=35506.

In addition, I didn't find any other cases other than loop-widen that could 
invalidate `this` pointer.


Repository:
  rC Clang

https://reviews.llvm.org/D45491

Files:
  lib/StaticAnalyzer/Core/RegionStore.cpp
  test/Analysis/this-pointer.cpp

Index: test/Analysis/this-pointer.cpp
===
--- /dev/null
+++ test/Analysis/this-pointer.cpp
@@ -0,0 +1,87 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -analyzer-config widen-loops=true -analyzer-disable-retry-exhausted -verify %s
+
+void clang_analyzer_eval(bool);
+
+// 'this' pointer is not an lvalue, we should not invalidate it.
+namespace this_pointer_after_loop_widen {
+class A {
+public:
+  A() {
+int count = 10;
+do {
+} while (count--);
+  }
+};
+
+void goo(A a);
+void func1() {
+  goo(A()); // no-crash
+}
+
+struct B {
+  int mem;
+  B() : mem(0) {
+for (int i = 0; i < 10; ++i) {
+}
+mem = 0;
+  }
+};
+
+void func2() {
+  B b;
+  clang_analyzer_eval(b.mem == 0); // expected-warning{{TRUE}}
+}
+
+struct C {
+  int mem;
+  C() : mem(0) {}
+  void set() {
+for (int i = 0; i < 10; ++i) {
+}
+mem = 10;
+  }
+};
+
+void func3() {
+  C c;
+  clang_analyzer_eval(c.mem == 0); // expected-warning{{TRUE}}
+  c.set();
+  clang_analyzer_eval(c.mem == 10); // expected-warning{{TRUE}}
+}
+
+struct D {
+  int mem;
+  D() : mem(0) {}
+  void set() {
+for (int i = 0; i < 10; ++i) {
+}
+mem = 10;
+  }
+};
+
+void func4() {
+  D *d = new D;
+  clang_analyzer_eval(d->mem == 0); // expected-warning{{TRUE}}
+  d->set();
+  clang_analyzer_eval(d->mem == 10); // expected-warning{{TRUE}}
+}
+
+struct E {
+  int mem;
+  E() : mem(0) {}
+  void set() {
+for (int i = 0; i < 10; ++i) {
+}
+setAux();
+  }
+  void setAux() {
+this->mem = 10;
+  }
+};
+
+void func6() {
+  E e;
+  e.set();
+  clang_analyzer_eval(e.mem == 10); // expected-warning{{TRUE}}
+}
+} // namespace this_pointer_after_loop_widen
Index: lib/StaticAnalyzer/Core/RegionStore.cpp
===
--- lib/StaticAnalyzer/Core/RegionStore.cpp
+++ lib/StaticAnalyzer/Core/RegionStore.cpp
@@ -1021,6 +1021,10 @@
 void invalidateRegionsWorker::VisitCluster(const MemRegion *baseR,
const ClusterBindings *C) {
 
+  // 'this' pointer is not an lvalue, we should not invalidate it.
+  if (CXXThisRegion::classof(baseR))
+return;
+
   bool PreserveRegionsContents =
   ITraits.hasTrait(baseR,
RegionAndSymbolInvalidationTraits::TK_PreserveContents);
@@ -2050,8 +2054,12 @@
 R = GetElementZeroRegion(SR, T);
   }
 
+  assert((!CXXThisRegion::classof(R) ||
+  CXXThisRegion::classof(R) && !B.lookup(R)) &&
+ "'this' pointer is not an l-value and is not assignable");
+
   // Clear out bindings that may overlap with this binding.
-  RegionBindingsRef NewB = removeSubRegionBindings(B, cast(R));
+  RegionBindingsRef NewB = removeSubRegionBindings(B, cast(R));  
   return NewB.addBinding(BindingKey::Make(R, BindingKey::Direct), V);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

Kindly ping!


Repository:
  rC Clang

https://reviews.llvm.org/D44934



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

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 140629.
MTC added a comment.

> Thank you for your reminding, I overlooked this point. However for 
> non-concrete character, the symbol value, if we just invalidate the region, 
> the constraint information of the non-concrete character will be lost. Do we 
> need to consider this?

Sorry for my hasty question, analyzer does not support to bind a symbol with a 
default binding.

The update of this diff is as follows.

- If the char value is not concrete, just invalidate the region.
- Add a test about symbolic char value.
- A little code refactoring.


Repository:
  rC Clang

https://reviews.llvm.org/D44934

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Core/ProgramState.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Core/Store.cpp
  test/Analysis/bstring.cpp
  test/Analysis/null-deref-ps-region.c
  test/Analysis/string.c

Index: test/Analysis/string.c
===
--- test/Analysis/string.c
+++ test/Analysis/string.c
@@ -1,7 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DSUPPRESS_OUT_OF_BOUND -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring.BufferOverlap,alpha.unix.cstring.NotNullTerminated,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
 
 //===--===
 // Declarations
@@ -1159,6 +1160,248 @@
   clang_analyzer_eval(str[1] == 'b'); // expected-warning{{UNKNOWN}}
 }
 
+//===--===
+// memset()
+//===--===
+
+void *memset( void *dest, int ch, size_t count );
+
+void* malloc(size_t size);
+void free(void*);
+
+void memset1_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '\0', 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+}
+
+void memset2_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '\0', strlen(str) + 1);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(str[2] == 0);  // expected-warning{{TRUE}}
+}
+
+void memset3_char_malloc_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  memset(str + 1, '\0', 8);
+  clang_analyzer_eval(str[1] == 0); // expected-warning{{UNKNOWN}}
+  free(str);
+}
+
+void memset4_char_malloc_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  //void *str = malloc(10 * sizeof(char));
+  memset(str, '\0', 10);
+  clang_analyzer_eval(str[1] == 0);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+  free(str);
+}
+
+#ifdef SUPPRESS_OUT_OF_BOUND
+void memset5_char_malloc_overflow_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  memset(str, '\0', 12);
+  clang_analyzer_eval(str[1] == 0); // expected-warning{{UNKNOWN}}
+  free(str);
+}
+#endif
+
+void memset6_char_array_nonnull() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '0', 2);
+  clang_analyzer_eval(str[0] == 'a');// expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(strlen(str) == 4); // 

[PATCH] D45086: [analyzer] Unroll the loop when it has a unsigned counter.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 140524.
MTC added a comment.

Fix typo, `unsinged` -> `unsigned`


Repository:
  rC Clang

https://reviews.llvm.org/D45086

Files:
  lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  test/Analysis/loop-unrolling.cpp


Index: test/Analysis/loop-unrolling.cpp
===
--- test/Analysis/loop-unrolling.cpp
+++ test/Analysis/loop-unrolling.cpp
@@ -36,6 +36,29 @@
   return 0;
 }
 
+int simple_unroll3_unsigned() {
+  int a[9];
+  int k = 42;
+  for (unsigned i = 0; i < 9; i++) {
+clang_analyzer_numTimesReached(); // expected-warning {{9}}
+a[i] = 42;
+  }
+  int b = 22 / (k - 42); // expected-warning {{Division by zero}}
+  return 0;
+}
+
+int simple_unroll4_unsigned() {
+  int a[9];
+  int k = 42;
+  unsigned i;
+  for (i = (0); i < 9; i++) {
+clang_analyzer_numTimesReached(); // expected-warning {{9}}
+a[i] = 42;
+  }
+  int b = 22 / (k - 42); // expected-warning {{Division by zero}}
+  return 0;
+}
+
 int simple_no_unroll1() {
   int a[9];
   int k = 42;
Index: lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -141,13 +141,15 @@
   return forStmt(
  hasCondition(simpleCondition("initVarName")),
  // Initialization should match the form: 'int i = 6' or 'i = 42'.
- hasLoopInit(anyOf(
- declStmt(hasSingleDecl(varDecl(
- allOf(hasInitializer(integerLiteral().bind("initNum")),
-   equalsBoundNode("initVarName"),
- binaryOperator(hasLHS(declRefExpr(to(
-varDecl(equalsBoundNode("initVarName"),
-hasRHS(integerLiteral().bind("initNum"),
+ hasLoopInit(
+ anyOf(declStmt(hasSingleDecl(
+   varDecl(allOf(hasInitializer(ignoringParenImpCasts(
+ 
integerLiteral().bind("initNum"))),
+ equalsBoundNode("initVarName"),
+   binaryOperator(hasLHS(declRefExpr(to(varDecl(
+  equalsBoundNode("initVarName"),
+  hasRHS(ignoringParenImpCasts(
+  
integerLiteral().bind("initNum")),
  // Incrementation should be a simple increment or decrement
  // operator call.
  hasIncrement(unaryOperator(


Index: test/Analysis/loop-unrolling.cpp
===
--- test/Analysis/loop-unrolling.cpp
+++ test/Analysis/loop-unrolling.cpp
@@ -36,6 +36,29 @@
   return 0;
 }
 
+int simple_unroll3_unsigned() {
+  int a[9];
+  int k = 42;
+  for (unsigned i = 0; i < 9; i++) {
+clang_analyzer_numTimesReached(); // expected-warning {{9}}
+a[i] = 42;
+  }
+  int b = 22 / (k - 42); // expected-warning {{Division by zero}}
+  return 0;
+}
+
+int simple_unroll4_unsigned() {
+  int a[9];
+  int k = 42;
+  unsigned i;
+  for (i = (0); i < 9; i++) {
+clang_analyzer_numTimesReached(); // expected-warning {{9}}
+a[i] = 42;
+  }
+  int b = 22 / (k - 42); // expected-warning {{Division by zero}}
+  return 0;
+}
+
 int simple_no_unroll1() {
   int a[9];
   int k = 42;
Index: lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -141,13 +141,15 @@
   return forStmt(
  hasCondition(simpleCondition("initVarName")),
  // Initialization should match the form: 'int i = 6' or 'i = 42'.
- hasLoopInit(anyOf(
- declStmt(hasSingleDecl(varDecl(
- allOf(hasInitializer(integerLiteral().bind("initNum")),
-   equalsBoundNode("initVarName"),
- binaryOperator(hasLHS(declRefExpr(to(
-varDecl(equalsBoundNode("initVarName"),
-hasRHS(integerLiteral().bind("initNum"),
+ hasLoopInit(
+ anyOf(declStmt(hasSingleDecl(
+   varDecl(allOf(hasInitializer(ignoringParenImpCasts(
+ integerLiteral().bind("initNum"))),
+ equalsBoundNode("initVarName"),
+   binaryOperator(hasLHS(declRefExpr(to(varDecl(
+  equalsBoundNode("initVarName"),
+  hasRHS(ignoringParenImpCasts(
+  integerLiteral().bind("initNum")),
  // Incrementation should be a simple increment or 

[PATCH] D45086: [analyzer] Unroll the loop when it has a unsigned counter.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: szepet, a.sidorin, NoQ.
Herald added subscribers: cfe-commits, rnkovacs, xazax.hun.
Herald added a reviewer: george.karpenkov.
MTC edited the summary of this revision.

The original implementation in the `LoopUnrolling.cpp` didn't consider the case 
where the counter is unsigned. This case is only handled in 
`simpleCondition()`, but this is not enough, we also need to deal with the 
unsinged counter with the counter initialization.

Since `IntegerLiteral` is `signed`, there is a `ImplicitCastExpr` 
in `unsigned counter = IntergerLiteral`. This patch add the 
`ignoringParenImpCasts()` in the `IntegerLiteral` matcher.


Repository:
  rC Clang

https://reviews.llvm.org/D45086

Files:
  lib/StaticAnalyzer/Core/LoopUnrolling.cpp
  test/Analysis/loop-unrolling.cpp


Index: test/Analysis/loop-unrolling.cpp
===
--- test/Analysis/loop-unrolling.cpp
+++ test/Analysis/loop-unrolling.cpp
@@ -36,6 +36,29 @@
   return 0;
 }
 
+int simple_unroll3_unsinged() {
+  int a[9];
+  int k = 42;
+  for (unsigned i = 0; i < 9; i++) {
+clang_analyzer_numTimesReached(); // expected-warning {{9}}
+a[i] = 42;
+  }
+  int b = 22 / (k - 42); // expected-warning {{Division by zero}}
+  return 0;
+}
+
+int simple_unroll4_unsinged() {
+  int a[9];
+  int k = 42;
+  unsigned i;
+  for (i = (0); i < 9; i++) {
+clang_analyzer_numTimesReached(); // expected-warning {{9}}
+a[i] = 42;
+  }
+  int b = 22 / (k - 42); // expected-warning {{Division by zero}}
+  return 0;
+}
+
 int simple_no_unroll1() {
   int a[9];
   int k = 42;
Index: lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -141,13 +141,15 @@
   return forStmt(
  hasCondition(simpleCondition("initVarName")),
  // Initialization should match the form: 'int i = 6' or 'i = 42'.
- hasLoopInit(anyOf(
- declStmt(hasSingleDecl(varDecl(
- allOf(hasInitializer(integerLiteral().bind("initNum")),
-   equalsBoundNode("initVarName"),
- binaryOperator(hasLHS(declRefExpr(to(
-varDecl(equalsBoundNode("initVarName"),
-hasRHS(integerLiteral().bind("initNum"),
+ hasLoopInit(
+ anyOf(declStmt(hasSingleDecl(
+   varDecl(allOf(hasInitializer(ignoringParenImpCasts(
+ 
integerLiteral().bind("initNum"))),
+ equalsBoundNode("initVarName"),
+   binaryOperator(hasLHS(declRefExpr(to(varDecl(
+  equalsBoundNode("initVarName"),
+  hasRHS(ignoringParenImpCasts(
+  
integerLiteral().bind("initNum")),
  // Incrementation should be a simple increment or decrement
  // operator call.
  hasIncrement(unaryOperator(


Index: test/Analysis/loop-unrolling.cpp
===
--- test/Analysis/loop-unrolling.cpp
+++ test/Analysis/loop-unrolling.cpp
@@ -36,6 +36,29 @@
   return 0;
 }
 
+int simple_unroll3_unsinged() {
+  int a[9];
+  int k = 42;
+  for (unsigned i = 0; i < 9; i++) {
+clang_analyzer_numTimesReached(); // expected-warning {{9}}
+a[i] = 42;
+  }
+  int b = 22 / (k - 42); // expected-warning {{Division by zero}}
+  return 0;
+}
+
+int simple_unroll4_unsinged() {
+  int a[9];
+  int k = 42;
+  unsigned i;
+  for (i = (0); i < 9; i++) {
+clang_analyzer_numTimesReached(); // expected-warning {{9}}
+a[i] = 42;
+  }
+  int b = 22 / (k - 42); // expected-warning {{Division by zero}}
+  return 0;
+}
+
 int simple_no_unroll1() {
   int a[9];
   int k = 42;
Index: lib/StaticAnalyzer/Core/LoopUnrolling.cpp
===
--- lib/StaticAnalyzer/Core/LoopUnrolling.cpp
+++ lib/StaticAnalyzer/Core/LoopUnrolling.cpp
@@ -141,13 +141,15 @@
   return forStmt(
  hasCondition(simpleCondition("initVarName")),
  // Initialization should match the form: 'int i = 6' or 'i = 42'.
- hasLoopInit(anyOf(
- declStmt(hasSingleDecl(varDecl(
- allOf(hasInitializer(integerLiteral().bind("initNum")),
-   equalsBoundNode("initVarName"),
- binaryOperator(hasLHS(declRefExpr(to(
-varDecl(equalsBoundNode("initVarName"),
-hasRHS(integerLiteral().bind("initNum"),
+ hasLoopInit(
+ anyOf(declStmt(hasSingleDecl(
+   

[PATCH] D45081: [analyzer] Remove the unused method declaration in `ValistChecker.cpp`.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added a reviewer: xazax.hun.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet.
Herald added a reviewer: george.karpenkov.

`getVariableNameFromRegion()` seems useless.


Repository:
  rC Clang

https://reviews.llvm.org/D45081

Files:
  lib/StaticAnalyzer/Checkers/ValistChecker.cpp


Index: lib/StaticAnalyzer/Checkers/ValistChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -56,7 +56,6 @@
 private:
   const MemRegion *getVAListAsRegion(SVal SV, const Expr *VAExpr,
  bool , CheckerContext ) 
const;
-  StringRef getVariableNameFromRegion(const MemRegion *Reg) const;
   const ExplodedNode *getStartCallSite(const ExplodedNode *N,
const MemRegion *Reg) const;
 


Index: lib/StaticAnalyzer/Checkers/ValistChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/ValistChecker.cpp
+++ lib/StaticAnalyzer/Checkers/ValistChecker.cpp
@@ -56,7 +56,6 @@
 private:
   const MemRegion *getVAListAsRegion(SVal SV, const Expr *VAExpr,
  bool , CheckerContext ) const;
-  StringRef getVariableNameFromRegion(const MemRegion *Reg) const;
   const ExplodedNode *getStartCallSite(const ExplodedNode *N,
const MemRegion *Reg) const;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 140405.
MTC added a comment.

According to @NoQ's suggestion, remove the duplicated code.


Repository:
  rC Clang

https://reviews.llvm.org/D44934

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Core/ProgramState.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Core/Store.cpp
  test/Analysis/bstring.cpp
  test/Analysis/null-deref-ps-region.c
  test/Analysis/string.c

Index: test/Analysis/string.c
===
--- test/Analysis/string.c
+++ test/Analysis/string.c
@@ -1,7 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DSUPPRESS_OUT_OF_BOUND -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring.BufferOverlap,alpha.unix.cstring.NotNullTerminated,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
 
 //===--===
 // Declarations
@@ -1159,6 +1160,248 @@
   clang_analyzer_eval(str[1] == 'b'); // expected-warning{{UNKNOWN}}
 }
 
+//===--===
+// memset()
+//===--===
+
+void *memset( void *dest, int ch, size_t count );
+
+void* malloc(size_t size);
+void free(void*);
+
+void memset1_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '\0', 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+}
+
+void memset2_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '\0', strlen(str) + 1);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(str[2] == 0);  // expected-warning{{TRUE}}
+}
+
+void memset3_char_malloc_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  memset(str + 1, '\0', 8);
+  clang_analyzer_eval(str[1] == 0); // expected-warning{{UNKNOWN}}
+  free(str);
+}
+
+void memset4_char_malloc_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  //void *str = malloc(10 * sizeof(char));
+  memset(str, '\0', 10);
+  clang_analyzer_eval(str[1] == 0);  // expected-warning{{TRUE}}
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+  free(str);
+}
+
+#ifdef SUPPRESS_OUT_OF_BOUND
+void memset5_char_malloc_overflow_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  memset(str, '\0', 12);
+  clang_analyzer_eval(str[1] == 0); // expected-warning{{UNKNOWN}}
+  free(str);
+}
+#endif
+
+void memset6_char_array_nonnull() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '0', 2);
+  clang_analyzer_eval(str[0] == 'a');// expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{UNKNOWN}}
+}
+
+void memset7_char_array_nonnull() {
+  char str[5] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '0', 5);
+  clang_analyzer_eval(str[0] == '0');// expected-warning{{TRUE}}
+  clang_analyzer_eval(strlen(str) >= 5); // expected-warning{{TRUE}}
+}
+
+#ifdef SUPPRESS_OUT_OF_BOUND
+void memset8_char_array_nonnull() {
+	char str[5] = "abcd";
+	clang_analyzer_eval(strlen(str) == 4); // 

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

Thanks for your review, NoQ!

> Why do you need separate code for null and non-null character? The function's 
> semantics doesn't seem to care.

Thank you for your advice, I will remove the duplicate code！

> I'd rather consider the case of non-concrete character separately. Because 
> wiping a region with a symbol is not something we currently support; a 
> symbolic default binding over a region means a different thing and it'd be 
> equivalent to invalidation, so for non-concrete character we don't have a 
> better choice than to invalidate. For concrete non-zero character, on the 
> contrary, a default binding would work just fine.

Thank you for your reminding, I overlooked this point. However for non-concrete 
character, the symbol value, if we just invalidate the region, the constraint 
information of the non-concrete character will be lost. Do we need to consider 
this?

> Could you explain why didn't a straightforward `bindLoc` over a base region 
> wasn't doing the thing you wanted? I.e., why is new Store API function 
> necessary?

I am also very resistant to adding new APIs. One is that I am not very familiar 
with RegionStore. One is that adding a new API is always the last option.

- The semantics of `memset()` need default binding, and only `bindDefault()` 
can guarantee that. However `bindDefault()` is just used to initialize the 
region and can only be called once on the same region.
- Only when the region is `TypedValueRegion` and the value type is `ArrayType` 
or `StructType`, `bindLoc()` can add a default binding. So if we want to 
continue using `default binding`, `bindLoc()` is not the correct choice.

And if I use `bindLoc()` instead of `overwriteRegion()`, there will be some 
crashes occured because `bindLoc()` broke some assumptions, e.g. `bindArray()` 
believes that the value for binding should be `CompoundVal`, `LazyCompoundVal` 
or `UnknownVal`.


Repository:
  rC Clang

https://reviews.llvm.org/D44934



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

[PATCH] D44934: [analyzer] Improve the modeling of `memset()`.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: dcoughlin, NoQ, xazax.hun, a.sidorin.
Herald added subscribers: cfe-commits, rnkovacs, szepet.
Herald added a reviewer: george.karpenkov.

This patch originates from https://reviews.llvm.org/D31868. There are two key 
points in this 
patch:

- Add `OverwriteRegion()`, this method used to model `memset()` or something 
like that.
- Improve the modeling of `memset`.

For `OverwriteRegion()`, is basically invalidate region and bind default. But I 
think this 
method requires more in-depth thinking and more extensive testing.

For `evalMemset()`, this patch only considers the case where the buffer's 
offset is zero. And
if the whole region is `memset`ed, bind a default value. According to the value 
for 
overwriting, decide how to update the string length.

For `void *memset(void *dest, int ch, size_t count)`:

1). offset is 0, `ch` is `'\0'` and `count` < dest-buffer's length. 
Invalidate the buffer and set the string length to 0.

2). offset is 0, `ch` is `'\0'` and `count` == dest-buffer's length.
Bind `\0` to the buffer with default binding and set the string length to 0.

3). offset is 0, `ch` is not `'\0'` and `count` < dest-buffer's length.
Invalidate the buffer and set the string length >= `count`.

4). offset is 0, `ch` is not `'\0'` and `count` == dest-buffer's length.
Bind `ch` to the buffer and set the string length >= `count`.

I have tested this patch on `sqlite`, but there's no difference int the 
warnings.

Thanks in advance for the review!


Repository:
  rC Clang

https://reviews.llvm.org/D44934

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  include/clang/StaticAnalyzer/Core/PathSensitive/Store.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Core/ProgramState.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Core/Store.cpp
  test/Analysis/bstring.cpp
  test/Analysis/null-deref-ps-region.c
  test/Analysis/string.c

Index: test/Analysis/string.c
===
--- test/Analysis/string.c
+++ test/Analysis/string.c
@@ -1,7 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
-// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DVARIANT -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DUSE_BUILTINS -DVARIANT -analyzer-checker=alpha.security.taint,core,unix.cstring,unix.Malloc,alpha.unix.cstring,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
+// RUN: %clang_analyze_cc1 -DSUPPRESS_OUT_OF_BOUND -analyzer-checker=core,unix.cstring,unix.Malloc,alpha.unix.cstring.BufferOverlap,alpha.unix.cstring.NotNullTerminated,debug.ExprInspection -analyzer-store=region -Wno-null-dereference -verify %s
 
 //===--===
 // Declarations
@@ -1159,6 +1160,248 @@
   clang_analyzer_eval(str[1] == 'b'); // expected-warning{{UNKNOWN}}
 }
 
+//===--===
+// memset()
+//===--===
+
+void *memset( void *dest, int ch, size_t count );
+
+void* malloc(size_t size);
+void free(void*);
+
+void memset1_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '\0', 2);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+}
+
+void memset2_char_array_null() {
+  char str[] = "abcd";
+  clang_analyzer_eval(strlen(str) == 4); // expected-warning{{TRUE}}
+  memset(str, '\0', strlen(str) + 1);
+  clang_analyzer_eval(strlen(str) == 0); // expected-warning{{TRUE}}
+  clang_analyzer_eval(str[2] == 0);  // expected-warning{{TRUE}}
+}
+
+void memset3_char_malloc_null() {
+  char *str = (char *)malloc(10 * sizeof(char));
+  memset(str + 1, '\0', 8);
+  

[PATCH] D34260: [StaticAnalyzer] Completely unrolling specific loops with known bound option

[Henry Wong via Phabricator via cfe-commits]

MTC added inline comments.
Herald added subscribers: dkrupp, rnkovacs.
Herald added a reviewer: george.karpenkov.



Comment at: lib/StaticAnalyzer/Core/LoopUnrolling.cpp:107
+  equalsBoundNode("initVarName"),
+  hasRHS(integerLiteral(),
+ // Incrementation should be a simple increment or decrement

Hi Peter, sorry to bother you! 

I have some confusion here. Whether `hasRHS(integerLiteral())` needs to added 
with `ignoringParenImpCasts`? 

The given code below does not unroll the loop because `i = 0;` has a 
`ImplicitCastExpr` for `0`. But if I change `unsigned i = 0` to `int i = 0`, 
loop unrolling will happen. So I don't know if this is intentional. 
```
int func() {
  int sum = 0;
  unsigned i;
  for (i = 0; i < 10; ++i) {
sum++;
  }
  return sum;
}
```

Hope you can help me, thank you!


https://reviews.llvm.org/D34260



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

[PATCH] D44606: [analyzer] Fix the crash in `IteratorChecker.cpp` when `SymbolConjured` has a null Stmt.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 139075.
MTC added a comment.

Add the comments as suggested by @szepet .


Repository:
  rC Clang

https://reviews.llvm.org/D44606

Files:
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  test/Analysis/loop-widening.c


Index: test/Analysis/loop-widening.c
===
--- test/Analysis/loop-widening.c
+++ test/Analysis/loop-widening.c
@@ -1,4 +1,5 @@
 // RUN: %clang_analyze_cc1 
-analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4 
-analyzer-config widen-loops=true -verify %s
+// RUN: %clang_analyze_cc1 -DTEST_NULL_TERM 
-analyzer-checker=core,unix.Malloc,debug.ExprInspection,alpha.cplusplus.IteratorRange
 -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
 
 void clang_analyzer_eval(int);
 void clang_analyzer_warnIfReached();
@@ -188,3 +189,16 @@
   }
   clang_analyzer_eval(i >= 2); // expected-warning {{TRUE}}
 }
+
+#ifdef TEST_NULL_TERM
+void null_terminator_loop_widen(int *a) {
+  int c;
+  // Loop widening will call 'invalidateRegions()' and 'invalidateRegions()'
+  // will construct the SymbolConjured with null Stmt because of the null
+  // terminator statement. Accessing the null Stmt will cause a crash.
+  for (;;) {
+c = *a; // no-crash
+a++;
+  }
+}
+#endif
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -604,7 +604,7 @@
   if (const auto *BSE = dyn_cast(SE)) {
 return BSE->getOpcode();
   } else if (const auto *SC = dyn_cast(SE)) {
-const auto *COE = dyn_cast(SC->getStmt());
+const auto *COE = dyn_cast_or_null(SC->getStmt());
 if (!COE)
   return BO_Comma; // Extremal value, neither EQ nor NE
 if (COE->getOperator() == OO_EqualEqual) {


Index: test/Analysis/loop-widening.c
===
--- test/Analysis/loop-widening.c
+++ test/Analysis/loop-widening.c
@@ -1,4 +1,5 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
+// RUN: %clang_analyze_cc1 -DTEST_NULL_TERM -analyzer-checker=core,unix.Malloc,debug.ExprInspection,alpha.cplusplus.IteratorRange -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
 
 void clang_analyzer_eval(int);
 void clang_analyzer_warnIfReached();
@@ -188,3 +189,16 @@
   }
   clang_analyzer_eval(i >= 2); // expected-warning {{TRUE}}
 }
+
+#ifdef TEST_NULL_TERM
+void null_terminator_loop_widen(int *a) {
+  int c;
+  // Loop widening will call 'invalidateRegions()' and 'invalidateRegions()'
+  // will construct the SymbolConjured with null Stmt because of the null
+  // terminator statement. Accessing the null Stmt will cause a crash.
+  for (;;) {
+c = *a; // no-crash
+a++;
+  }
+}
+#endif
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -604,7 +604,7 @@
   if (const auto *BSE = dyn_cast(SE)) {
 return BSE->getOpcode();
   } else if (const auto *SC = dyn_cast(SE)) {
-const auto *COE = dyn_cast(SC->getStmt());
+const auto *COE = dyn_cast_or_null(SC->getStmt());
 if (!COE)
   return BO_Comma; // Extremal value, neither EQ nor NE
 if (COE->getOperator() == OO_EqualEqual) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44557: [analyzer] CStringChecker.cpp - Code refactoring on bug report.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D44557#1042357, @NoQ wrote:

> Sorry, one moment, i'm seeing a few regressions after the previous 
> refactoring but i didn't look at them closely yet to provide a reproducer. 
> I'll get back to this.


Thank you for the code review, @NoQ !

The regression is maybe caused by https://reviews.llvm.org/D44075, my patch! 
`CheckBufferAccess()` may not guarantee `checkNonNull()` will be called. If 
that's true, please revert https://reviews.llvm.org/rC326782 and I'll fix it.


Repository:
  rC Clang

https://reviews.llvm.org/D44557



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

[PATCH] D44606: [analyzer] Fix the crash in `IteratorChecker.cpp` when `SymbolConjured` has a null Stmt.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

> Just in case: we indeed do not guarantee that `SymbolConjured` corresponds to 
> a statement; it is, however, not intended, but rather a bug.

Thank you for your explanation and the reasonable example, NoQ.


Repository:
  rC Clang

https://reviews.llvm.org/D44606



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

[PATCH] D44606: [analyzer] Fix the crash in `IteratorChecker.cpp` when `SymbolConjured` has a null Stmt.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

> One small nit for future debugging people: Could you insert a comment line in 
> the test case where you explain what is this all about? E.g what you just 
> have written in the description: "invalidateRegions() will construct the 
> SymbolConjured with null Stmt" or something like this.

Thank you for pointing this out, and I'll add the comment.


Repository:
  rC Clang

https://reviews.llvm.org/D44606



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

[PATCH] D44606: [analyzer] Fix the crash in `IteratorChecker.cpp` when `SymbolConjured` has a null Stmt.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: NoQ, baloghadamsoftware, szepet, dcoughlin.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, xazax.hun.
Herald added a reviewer: george.karpenkov.

When the loop has a null terminator statement and sets `widen-loops=true`, 
`invalidateRegions()` will constructs the
`SymbolConjured` with null `Stmt`. And this will lead to a crash in 
`IteratorChecker.cpp`.

Given the code below:

  void null_terminator_loop_widen(int *a) {
int c;
for (;;) {
  c = *a;
  a++;
}
  }

I haven't found any problems with `SymbolConjured` containing null `Stmt` for 
the time being. So I just use 
`dyn_cast_or_null<>` instead of `dyn_cast<>` in `IteratorChecker.cpp`, and 
didn't delve into the widen loop part.


Repository:
  rC Clang

https://reviews.llvm.org/D44606

Files:
  lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
  test/Analysis/loop-widening.c


Index: test/Analysis/loop-widening.c
===
--- test/Analysis/loop-widening.c
+++ test/Analysis/loop-widening.c
@@ -1,4 +1,5 @@
 // RUN: %clang_analyze_cc1 
-analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4 
-analyzer-config widen-loops=true -verify %s
+// RUN: %clang_analyze_cc1 -DTEST_NULL_TERM 
-analyzer-checker=core,unix.Malloc,debug.ExprInspection,alpha.cplusplus.IteratorRange
 -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
 
 void clang_analyzer_eval(int);
 void clang_analyzer_warnIfReached();
@@ -188,3 +189,13 @@
   }
   clang_analyzer_eval(i >= 2); // expected-warning {{TRUE}}
 }
+
+#ifdef TEST_NULL_TERM
+void null_terminator_loop_widen(int *a) {
+  int c;
+  for (;;) {
+c = *a; // no-crash
+a++;
+  }
+}
+#endif
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -604,7 +604,7 @@
   if (const auto *BSE = dyn_cast(SE)) {
 return BSE->getOpcode();
   } else if (const auto *SC = dyn_cast(SE)) {
-const auto *COE = dyn_cast(SC->getStmt());
+const auto *COE = dyn_cast_or_null(SC->getStmt());
 if (!COE)
   return BO_Comma; // Extremal value, neither EQ nor NE
 if (COE->getOperator() == OO_EqualEqual) {


Index: test/Analysis/loop-widening.c
===
--- test/Analysis/loop-widening.c
+++ test/Analysis/loop-widening.c
@@ -1,4 +1,5 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
+// RUN: %clang_analyze_cc1 -DTEST_NULL_TERM -analyzer-checker=core,unix.Malloc,debug.ExprInspection,alpha.cplusplus.IteratorRange -analyzer-max-loop 4 -analyzer-config widen-loops=true -verify %s
 
 void clang_analyzer_eval(int);
 void clang_analyzer_warnIfReached();
@@ -188,3 +189,13 @@
   }
   clang_analyzer_eval(i >= 2); // expected-warning {{TRUE}}
 }
+
+#ifdef TEST_NULL_TERM
+void null_terminator_loop_widen(int *a) {
+  int c;
+  for (;;) {
+c = *a; // no-crash
+a++;
+  }
+}
+#endif
Index: lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
+++ lib/StaticAnalyzer/Checkers/IteratorChecker.cpp
@@ -604,7 +604,7 @@
   if (const auto *BSE = dyn_cast(SE)) {
 return BSE->getOpcode();
   } else if (const auto *SC = dyn_cast(SE)) {
-const auto *COE = dyn_cast(SC->getStmt());
+const auto *COE = dyn_cast_or_null(SC->getStmt());
 if (!COE)
   return BO_Comma; // Extremal value, neither EQ nor NE
 if (COE->getOperator() == OO_EqualEqual) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44557: [analyzer] CStringChecker.cpp - Code refactoring on bug report.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: NoQ, george.karpenkov, xazax.hun.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet.

When improving the modeling `evalMemset()`, some scenes need to emit report of 
`NotNullTerm`. In this case, 
there are three places in `CStringChecker.cpp` that have the same code about 
`NotNullTerm` report. In 
addition, `CStringChecker.cpp` already has `emitOverlapBug()`, so I think it 
needs code refactoring.

Thanks in advance for the code review!


Repository:
  rC Clang

https://reviews.llvm.org/D44557

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp

Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -194,6 +194,14 @@
   const Stmt *First,
   const Stmt *Second) const;
 
+  void emitNullArgBug(CheckerContext , ProgramStateRef State, const Stmt *S,
+  StringRef WarningMsg) const;
+  void emitOutOfBoundsBug(CheckerContext , ProgramStateRef State,
+  const Stmt *S, StringRef WarningMsg) const;
+  void emitNotCStringBug(CheckerContext , ProgramStateRef State,
+ const Stmt *S, StringRef WarningMsg) const;
+  void emitAdditionOverflowBug(CheckerContext , ProgramStateRef State) const;
+
   ProgramStateRef checkAdditionOverflow(CheckerContext ,
 ProgramStateRef state,
 NonLoc left,
@@ -239,30 +247,14 @@
   std::tie(stateNull, stateNonNull) = assumeZero(C, state, l, S->getType());
 
   if (stateNull && !stateNonNull) {
-if (!Filter.CheckCStringNullArg)
-  return nullptr;
-
-ExplodedNode *N = C.generateErrorNode(stateNull);
-if (!N)
-  return nullptr;
-
-if (!BT_Null)
-  BT_Null.reset(new BuiltinBug(
-  Filter.CheckNameCStringNullArg, categories::UnixAPI,
-  "Null pointer argument in call to byte string function"));
-
-SmallString<80> buf;
-llvm::raw_svector_ostream os(buf);
-assert(CurrentFunctionDescription);
-os << "Null pointer argument in call to " << CurrentFunctionDescription;
-
-// Generate a report for this bug.
-BuiltinBug *BT = static_cast(BT_Null.get());
-auto report = llvm::make_unique(*BT, os.str(), N);
+if (Filter.CheckCStringNullArg) {
+  SmallString<80> buf;
+  llvm::raw_svector_ostream os(buf);
+  assert(CurrentFunctionDescription);
+  os << "Null pointer argument in call to " << CurrentFunctionDescription;
 
-report->addRange(S->getSourceRange());
-bugreporter::trackNullOrUndefValue(N, S, *report);
-C.emitReport(std::move(report));
+  emitNullArgBug(C, stateNull, S, os.str());
+}
 return nullptr;
   }
 
@@ -305,31 +297,14 @@
   ProgramStateRef StInBound = state->assumeInBound(Idx, Size, true);
   ProgramStateRef StOutBound = state->assumeInBound(Idx, Size, false);
   if (StOutBound && !StInBound) {
-ExplodedNode *N = C.generateErrorNode(StOutBound);
-if (!N)
-  return nullptr;
-
-CheckName Name;
 // These checks are either enabled by the CString out-of-bounds checker
 // explicitly or the "basic" CStringNullArg checker support that Malloc
 // checker enables.
 assert(Filter.CheckCStringOutOfBounds || Filter.CheckCStringNullArg);
-if (Filter.CheckCStringOutOfBounds)
-  Name = Filter.CheckNameCStringOutOfBounds;
-else
-  Name = Filter.CheckNameCStringNullArg;
 
-if (!BT_Bounds) {
-  BT_Bounds.reset(new BuiltinBug(
-  Name, "Out-of-bound array access",
-  "Byte string function accesses out-of-bound array element"));
-}
-BuiltinBug *BT = static_cast(BT_Bounds.get());
-
-// Generate a report for this bug.
-std::unique_ptr report;
+// Emit a bug report.
 if (warningMsg) {
-  report = llvm::make_unique(*BT, warningMsg, N);
+  emitOutOfBoundsBug(C, StOutBound, S, warningMsg);
 } else {
   assert(CurrentFunctionDescription);
   assert(CurrentFunctionDescription[0] != '\0');
@@ -339,15 +314,8 @@
   os << toUppercase(CurrentFunctionDescription[0])
  << [1]
  << " accesses out-of-bound array element";
-  report = llvm::make_unique(*BT, os.str(), N);
+  emitOutOfBoundsBug(C, StOutBound, S, os.str());
 }
-
-// FIXME: It would be nice to eventually make this diagnostic more clear,
-// e.g., by referencing the original declaration or by saying *why* this
-// reference is outside the range.
-
-report->addRange(S->getSourceRange());
-C.emitReport(std::move(report));
 return nullptr;
   }
 
@@ -565,6 +533,79 @@
   C.emitReport(std::move(report));
 }
 
+void CStringChecker::emitNullArgBug(CheckerContext , ProgramStateRef State,
+const Stmt *S, 

[PATCH] D43741: [Analyzer] More accurate modeling about the increment operator of the operand with type bool.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 137154.
MTC added a comment.

Remove the default configuration `-analyzer-store=region` in the test file.


Repository:
  rC Clang

https://reviews.llvm.org/D43741

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  lib/StaticAnalyzer/Core/ExprEngineC.cpp
  test/Analysis/_Bool-increment-decrement.c
  test/Analysis/bool-increment.cpp

Index: test/Analysis/bool-increment.cpp
===
--- /dev/null
+++ test/Analysis/bool-increment.cpp
@@ -0,0 +1,84 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify -std=c++98 -Wno-deprecated %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify -std=c++11 -Wno-deprecated %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify -std=c++14 -Wno-deprecated %s
+
+extern void clang_analyzer_eval(bool);
+
+void test_bool_value() {
+  {
+bool b = true;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = -10;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+b++;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 0;
+b++;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+}
+
+void test_bool_increment() {
+  {
+bool b = true;
+b++;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+b++;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = true;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 0;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+++b;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = -10;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+}
Index: test/Analysis/_Bool-increment-decrement.c
===
--- /dev/null
+++ test/Analysis/_Bool-increment-decrement.c
@@ -0,0 +1,140 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify -std=c99 -Dbool=_Bool -Dtrue=1 -Dfalse=0 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -verify -std=c11 -Dbool=_Bool -Dtrue=1 -Dfalse=0 %s
+extern void clang_analyzer_eval(bool);
+
+void test__Bool_value() {
+  {
+bool b = true;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = -10;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+b++;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 0;
+b++;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+}
+
+void test__Bool_increment() {
+  {
+bool b = true;
+b++;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+b++;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = true;
+++b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+++b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 0;
+++b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+++b;
+++b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = -10;
+++b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = -1;
+++b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+}
+
+void test__Bool_decrement() {
+  {
+bool b = true;
+b--;
+clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+b--;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = true;
+--b;
+clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+--b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 0;
+--b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+--b;
+clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+--b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = -10;
+--b;
+

[PATCH] D44075: [analyzer] CStringChecker.cpp: Remove the duplicated check about null dereference on dest-buffer or src-buffer.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: dcoughlin, NoQ, xazax.hun, cfe-commits.
Herald added subscribers: a.sidorin, rnkovacs, szepet.
Herald added a reviewer: george.karpenkov.

`CheckBufferAccess()` calls `CheckNonNull()`, so there are some calls to 
`CheckNonNull()` that are useless.


Repository:
  rC Clang

https://reviews.llvm.org/D44075

Files:
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp


Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -1033,21 +1033,6 @@
   if (stateNonZeroSize) {
 state = stateNonZeroSize;
 
-// Ensure the destination is not null. If it is NULL there will be a
-// NULL pointer dereference.
-state = checkNonNull(C, state, Dest, destVal);
-if (!state)
-  return;
-
-// Get the value of the Src.
-SVal srcVal = state->getSVal(Source, LCtx);
-
-// Ensure the source is not null. If it is NULL there will be a
-// NULL pointer dereference.
-state = checkNonNull(C, state, Source, srcVal);
-if (!state)
-  return;
-
 // Ensure the accesses are valid and that the buffers do not overlap.
 const char * const writeWarning =
   "Memory copy function overflows destination buffer";
@@ -2033,12 +2018,6 @@
 return;
   }
 
-  // Ensure the memory area is not null.
-  // If it is NULL there will be a NULL pointer dereference.
-  State = checkNonNull(C, StateNonZeroSize, Mem, MemVal);
-  if (!State)
-return;
-
   State = CheckBufferAccess(C, State, Size, Mem);
   if (!State)
 return;


Index: lib/StaticAnalyzer/Checkers/CStringChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/CStringChecker.cpp
+++ lib/StaticAnalyzer/Checkers/CStringChecker.cpp
@@ -1033,21 +1033,6 @@
   if (stateNonZeroSize) {
 state = stateNonZeroSize;
 
-// Ensure the destination is not null. If it is NULL there will be a
-// NULL pointer dereference.
-state = checkNonNull(C, state, Dest, destVal);
-if (!state)
-  return;
-
-// Get the value of the Src.
-SVal srcVal = state->getSVal(Source, LCtx);
-
-// Ensure the source is not null. If it is NULL there will be a
-// NULL pointer dereference.
-state = checkNonNull(C, state, Source, srcVal);
-if (!state)
-  return;
-
 // Ensure the accesses are valid and that the buffers do not overlap.
 const char * const writeWarning =
   "Memory copy function overflows destination buffer";
@@ -2033,12 +2018,6 @@
 return;
   }
 
-  // Ensure the memory area is not null.
-  // If it is NULL there will be a NULL pointer dereference.
-  State = checkNonNull(C, StateNonZeroSize, Mem, MemVal);
-  if (!State)
-return;
-
   State = CheckBufferAccess(C, State, Size, Mem);
   if (!State)
 return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39159: [analyzer] Improves the logic of GenericTaintChecker identifying stdin.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

@NoQ, Very sorry, I've forgotten about this patch, it has now been updated.


Repository:
  rC Clang

https://reviews.llvm.org/D39159



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

[PATCH] D39159: [analyzer] Improves the logic of GenericTaintChecker identifying stdin.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 136934.
MTC set the repository for this revision to rC Clang.
MTC added a comment.
Herald added subscribers: cfe-commits, a.sidorin.
Herald added a reviewer: george.karpenkov.

Update the `taint-generic.c` to test both `stdin` declaration variants.


Repository:
  rC Clang

https://reviews.llvm.org/D39159

Files:
  lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
  test/Analysis/taint-generic.c


Index: test/Analysis/taint-generic.c
===
--- test/Analysis/taint-generic.c
+++ test/Analysis/taint-generic.c
@@ -1,10 +1,16 @@
 // RUN: %clang_analyze_cc1  
-analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 
-Wno-format-security -verify %s
+// RUN: %clang_analyze_cc1  -DFILE_IS_STRUCT 
-analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 
-Wno-format-security -verify %s
 
 int scanf(const char *restrict format, ...);
 int getchar(void);
 
 typedef struct _FILE FILE;
+#ifdef FILE_IS_STRUCT
+extern struct _FILE *stdin;
+#else
 extern FILE *stdin;
+#endif
+
 int fscanf(FILE *restrict stream, const char *restrict format, ...);
 int sprintf(char *str, const char *format, ...);
 void setproctitle(const char *fmt, ...);
Index: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -646,7 +646,8 @@
 if ((D->getName().find("stdin") != StringRef::npos) && D->isExternC())
 if (const PointerType * PtrTy =
   dyn_cast(D->getType().getTypePtr()))
-  if (PtrTy->getPointeeType() == C.getASTContext().getFILEType())
+  if (PtrTy->getPointeeType().getCanonicalType() ==
+  C.getASTContext().getFILEType().getCanonicalType())
 return true;
   }
   return false;


Index: test/Analysis/taint-generic.c
===
--- test/Analysis/taint-generic.c
+++ test/Analysis/taint-generic.c
@@ -1,10 +1,16 @@
 // RUN: %clang_analyze_cc1  -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 -Wno-format-security -verify %s
+// RUN: %clang_analyze_cc1  -DFILE_IS_STRUCT -analyzer-checker=alpha.security.taint,core,alpha.security.ArrayBoundV2 -Wno-format-security -verify %s
 
 int scanf(const char *restrict format, ...);
 int getchar(void);
 
 typedef struct _FILE FILE;
+#ifdef FILE_IS_STRUCT
+extern struct _FILE *stdin;
+#else
 extern FILE *stdin;
+#endif
+
 int fscanf(FILE *restrict stream, const char *restrict format, ...);
 int sprintf(char *str, const char *format, ...);
 void setproctitle(const char *fmt, ...);
Index: lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
+++ lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
@@ -646,7 +646,8 @@
 if ((D->getName().find("stdin") != StringRef::npos) && D->isExternC())
 if (const PointerType * PtrTy =
   dyn_cast(D->getType().getTypePtr()))
-  if (PtrTy->getPointeeType() == C.getASTContext().getFILEType())
+  if (PtrTy->getPointeeType().getCanonicalType() ==
+  C.getASTContext().getFILEType().getCanonicalType())
 return true;
   }
   return false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43741: [Analyzer] More accurate modeling about the increment operator of the operand with type bool.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

Thank you for your review, @NoQ!

- `isBooleanType()` is used to check `_Bool` in C99/C11 and `bool` in C++. For 
`_Bool` , there is the same overflow problem.
- In C++98/C++11/C++14, for `++bool` and `bool+`,  both sets true directly.
- In C++, `--bool` and `bool--` is illeagal.
- In C99/C11 standard, there is not much information about `_Bool++` and 
`_Bool--`.

From the implementation of the compiler, `_Bool++` and `_Bool--` are divided 
into the following situations.

- _Bool b = 0; b++; // b -> 1
- _Bool b = 1; b++; // b -> 1
- _Bool b = 0; b--; // b -> 1
- _Bool b = 1; b--; // b -> 0

So it's reasonable to set to true if the operand of the increment operator is 
of type _Bool, just my opinion.

I not familiar with Objective-C++, can you provide a appropriate test about 
Objective-C++ for me, thank you!

And I'm not a native speaker of English, the grammar of the comments may not be 
correct. If so, please correct me, thanks!


Repository:
  rC Clang

https://reviews.llvm.org/D43741



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

[PATCH] D43741: [Analyzer] More accurate modeling about the increment operator of the operand with type bool.

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 136901.
MTC added a comment.

- If the operand of the ++ operator is of type `_Bool`, also set to true.
- Add test file `_Bool-increment-decement.c`.




Repository:
  rC Clang

https://reviews.llvm.org/D43741

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  lib/StaticAnalyzer/Core/ExprEngineC.cpp
  test/Analysis/_Bool-increment-decrement.c
  test/Analysis/bool-increment.cpp

Index: test/Analysis/bool-increment.cpp
===
--- /dev/null
+++ test/Analysis/bool-increment.cpp
@@ -0,0 +1,84 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -analyzer-store=region -verify -std=c++98 -Wno-deprecated %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -analyzer-store=region -verify -std=c++11 -Wno-deprecated %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -analyzer-store=region -verify -std=c++14 -Wno-deprecated %s
+
+extern void clang_analyzer_eval(bool);
+
+void test_bool_value() {
+  {
+bool b = true;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = -10;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+b++;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 0;
+b++;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+}
+
+void test_bool_increment() {
+  {
+bool b = true;
+b++;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+b++;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = true;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 0;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+++b;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = -10;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+}
Index: test/Analysis/_Bool-increment-decrement.c
===
--- /dev/null
+++ test/Analysis/_Bool-increment-decrement.c
@@ -0,0 +1,140 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -analyzer-store=region -verify -std=c99 -Dbool=_Bool -Dtrue=1 -Dfalse=0 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -analyzer-store=region -verify -std=c11 -Dbool=_Bool -Dtrue=1 -Dfalse=0 %s
+extern void clang_analyzer_eval(bool);
+
+void test__Bool_value() {
+  {
+bool b = true;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = -10;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+b++;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 0;
+b++;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+}
+
+void test__Bool_increment() {
+  {
+bool b = true;
+b++;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+b++;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = true;
+++b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+++b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 0;
+++b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+++b;
+++b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = -10;
+++b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = -1;
+++b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+}
+
+void test__Bool_decrement() {
+  {
+bool b = true;
+b--;
+clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+b--;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = true;
+--b;
+clang_analyzer_eval(b == 0); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+--b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 0;
+--b;
+clang_analyzer_eval(b == 1); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+--b;
+clang_analyzer_eval(b == 0); 

[PATCH] D43741: [Analyzer] More accurate modeling about the increment operator of the operand with type bool.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D43741#1024949, @alexshap wrote:

> i see, but just in case - what about the decrement operator ?


@alexshap, If I'm not wrong, decrement operation is not allowed on bool type in 
C++98 and C++14.

> 5.2.6 Increment and decrement [expr.post.incr]
>  The operand of postfix -- is decremented analogously to the postfix ++ 
> operator, except that the operand
>  shall not be of type bool. [ Note: For prefix increment and decrement, see 
> 5.3.2. — end note ]




Repository:
  rC Clang

https://reviews.llvm.org/D43741



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

[PATCH] D43741: [Analyzer] More accurate modeling about the increment operator of the operand with type bool.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: alexshap, NoQ, dcoughlin.
Herald added subscribers: cfe-commits, a.sidorin, szepet, xazax.hun.
Herald added a reviewer: george.karpenkov.

There is a problem with analyzer that a wrong value is given when modeling the 
increment operator of the operand with type bool. After `rL307604` is applied, 
a unsigned overflow may occur.

Example:

  void func() {
bool b = true;
// unsigned overflow occur, 2 -> 0 U1b
b++;
  }

The use of an operand of type bool with the ++ operators is deprecated but 
valid untill C++17. And if the operand of the increment operator is of type 
bool, it is set to true.

This patch includes two parts:

- If the operand of the increment operator is of type bool, set to true.
- Modify `BasicValueFactory::getTruthValue()`, use `getIntWidth()` instead 
`getTypeSize()` and use `unsigned` instead `signed`.


Repository:
  rC Clang

https://reviews.llvm.org/D43741

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
  lib/StaticAnalyzer/Core/ExprEngineC.cpp
  test/Analysis/bool.cpp

Index: test/Analysis/bool.cpp
===
--- /dev/null
+++ test/Analysis/bool.cpp
@@ -0,0 +1,84 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -analyzer-store=region -verify -std=c++11 -Wno-deprecated %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.ExprInspection -analyzer-store=region -verify -std=c++14 -Wno-deprecated %s
+
+extern void clang_analyzer_eval(bool);
+extern void clang_analyzer_dump(bool);
+
+void test_bool_increment() {
+  {
+bool b = true;
+b++;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+b++;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = true;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = false;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 0;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = 10;
+++b;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+
+  {
+bool b = -10;
+++b;
+clang_analyzer_eval(b); // expected-warning{{TRUE}}
+  }
+}
+
+void test_bool_value() {
+  {
+bool b = true;
+clang_analyzer_dump(b); // expected-warning{{1 U1b}}
+  }
+
+  {
+bool b = false;
+clang_analyzer_dump(b); // expected-warning{{0 U1b}}
+  }
+
+  {
+bool b = -10;
+clang_analyzer_dump(b); // expected-warning{{1 U1b}}
+  }
+
+  {
+bool b = 10;
+clang_analyzer_dump(b); // expected-warning{{1 U1b}}
+  }
+
+  {
+bool b = 10;
+b++;
+clang_analyzer_dump(b); // expected-warning{{1 U1b}}
+  }
+
+  {
+bool b = 0;
+b++;
+clang_analyzer_dump(b); // expected-warning{{1 U1b}}
+  }
+}
Index: lib/StaticAnalyzer/Core/ExprEngineC.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngineC.cpp
+++ lib/StaticAnalyzer/Core/ExprEngineC.cpp
@@ -1066,15 +1066,23 @@
 // constant value. If the UnaryOperator has location type, create the
 // constant with int type and pointer width.
 SVal RHS;
+SVal Result;
 
 if (U->getType()->isAnyPointerType())
   RHS = svalBuilder.makeArrayIndex(1);
 else if (U->getType()->isIntegralOrEnumerationType())
   RHS = svalBuilder.makeIntVal(1, U->getType());
 else
   RHS = UnknownVal();
 
-SVal Result = evalBinOp(state, Op, V2, RHS, U->getType());
+// The use of an operand of type bool with the ++ operators is deprecated
+// but valid untill C++17. And if the operand of the increment operator is
+// of type bool, it is set to true untill C++17.
+if (U->getType()->isBooleanType() && U->isIncrementOp() &&
+AMgr.getLangOpts().CPlusPlus)
+  Result = svalBuilder.makeTruthVal(true, U->getType());
+else
+  Result = evalBinOp(state, Op, V2, RHS, U->getType());
 
 // Conjure a new symbol if necessary to recover precision.
 if (Result.isUnknown()){
@@ -1096,7 +1104,6 @@
   Constraint = svalBuilder.evalEQ(state, SymVal,
svalBuilder.makeZeroVal(U->getType()));
 
-
   state = state->assume(Constraint, false);
   assert(state);
 }
Index: include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
===
--- include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
+++ include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h
@@ -194,7 +194,7 @@
   }
 
   inline const llvm::APSInt& getTruthValue(bool b, QualType T) {
-return getValue(b ? 1 : 0, Ctx.getTypeSize(T), false);
+return getValue(b ? 1 : 0, Ctx.getIntWidth(T), true);
   }
 
   inline const llvm::APSInt& getTruthValue(bool b) {

[PATCH] D42300: [Analyzer] Add PreStmt and PostStmt callbacks for OffsetOfExpr

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

@NoQ Sorry to bother you again. It seems that this patch is useless to analyzer 
temporarily, if you think so, I will abandon it : ).


Repository:
  rC Clang

https://reviews.llvm.org/D42300



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

[PATCH] D42300: [Analyzer] Add PreStmt and PostStmt callbacks for OffsetOfExpr

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 133421.
MTC added a comment.
Herald added a reviewer: george.karpenkov.

rebase


Repository:
  rC Clang

https://reviews.llvm.org/D42300

Files:
  lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  test/Analysis/Inputs/system-header-simulator.h
  test/Analysis/offsetofexpr-callback.c


Index: test/Analysis/offsetofexpr-callback.c
===
--- /dev/null
+++ test/Analysis/offsetofexpr-callback.c
@@ -0,0 +1,13 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder 
-analyzer-config 
debug.AnalysisOrder:PreStmtOffsetOfExpr=true,debug.AnalysisOrder:PostStmtOffsetOfExpr=true
 %s 2>&1 | FileCheck %s
+#include "Inputs/system-header-simulator.h"
+
+struct S {
+  char c;
+};
+
+void test() {
+  offsetof(struct S, c); 
+}
+
+// CHECK: PreStmt
+// CHECK-NEXT: PostStmt
\ No newline at end of file
Index: test/Analysis/Inputs/system-header-simulator.h
===
--- test/Analysis/Inputs/system-header-simulator.h
+++ test/Analysis/Inputs/system-header-simulator.h
@@ -110,4 +110,6 @@
 #ifndef NULL
 #define __DARWIN_NULL 0
 #define NULL __DARWIN_NULL
-#endif
\ No newline at end of file
+#endif
+
+#define offsetof(t, d) __builtin_offsetof(t, d)
\ No newline at end of file
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1488,12 +1488,19 @@
   Bldr.addNodes(Dst);
   break;
 
-case Stmt::OffsetOfExprClass:
+case Stmt::OffsetOfExprClass: {
   Bldr.takeNodes(Pred);
-  VisitOffsetOfExpr(cast(S), Pred, Dst);
+  ExplodedNodeSet PreVisit;
+  getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
+
+  ExplodedNodeSet PostVisit;
+  for (ExplodedNode *Node : PreVisit)
+VisitOffsetOfExpr(cast(S), Node, PostVisit);
+
+  getCheckerManager().runCheckersForPostStmt(Dst, PostVisit, S, *this);
   Bldr.addNodes(Dst);
   break;
-
+}
 case Stmt::UnaryExprOrTypeTraitExprClass:
   Bldr.takeNodes(Pred);
   VisitUnaryExprOrTypeTraitExpr(cast(S),
Index: lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
+++ lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
@@ -33,6 +33,8 @@
  check::PostStmt,
  check::PreStmt,
  check::PostStmt,
+ check::PreStmt,
+ check::PostStmt,
  check::PreCall,
  check::PostCall,
  check::NewAllocator,
@@ -91,6 +93,16 @@
   llvm::errs() << "PostStmt\n";
   }
 
+  void checkPreStmt(const OffsetOfExpr *OOE, CheckerContext ) const {
+if (isCallbackEnabled(C, "PreStmtOffsetOfExpr"))
+  llvm::errs() << "PreStmt\n";
+  }
+
+  void checkPostStmt(const OffsetOfExpr *OOE, CheckerContext ) const {
+if (isCallbackEnabled(C, "PostStmtOffsetOfExpr"))
+  llvm::errs() << "PostStmt\n";
+  }
+
   void checkPreCall(const CallEvent , CheckerContext ) const {
 if (isCallbackEnabled(C, "PreCall")) {
   llvm::errs() << "PreCall";


Index: test/Analysis/offsetofexpr-callback.c
===
--- /dev/null
+++ test/Analysis/offsetofexpr-callback.c
@@ -0,0 +1,13 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:PreStmtOffsetOfExpr=true,debug.AnalysisOrder:PostStmtOffsetOfExpr=true %s 2>&1 | FileCheck %s
+#include "Inputs/system-header-simulator.h"
+
+struct S {
+  char c;
+};
+
+void test() {
+  offsetof(struct S, c); 
+}
+
+// CHECK: PreStmt
+// CHECK-NEXT: PostStmt
\ No newline at end of file
Index: test/Analysis/Inputs/system-header-simulator.h
===
--- test/Analysis/Inputs/system-header-simulator.h
+++ test/Analysis/Inputs/system-header-simulator.h
@@ -110,4 +110,6 @@
 #ifndef NULL
 #define __DARWIN_NULL 0
 #define NULL __DARWIN_NULL
-#endif
\ No newline at end of file
+#endif
+
+#define offsetof(t, d) __builtin_offsetof(t, d)
\ No newline at end of file
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1488,12 +1488,19 @@
   Bldr.addNodes(Dst);
   break;
 
-case Stmt::OffsetOfExprClass:
+case Stmt::OffsetOfExprClass: {
   Bldr.takeNodes(Pred);
-  VisitOffsetOfExpr(cast(S), Pred, Dst);
+  ExplodedNodeSet PreVisit;
+  getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
+
+  ExplodedNodeSet PostVisit;
+  for 

[PATCH] D43074: [Analyzer] Fix a typo about `categories::MemoryError` in `MallocChecker.cpp`

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: NoQ, xazax.hun.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet.
Herald added a reviewer: george.karpenkov.

It should be an omission when committing https://reviews.llvm.org/rL302016.


Repository:
  rC Clang

https://reviews.llvm.org/D43074

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp


Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -2075,8 +2075,8 @@
 
   if (ExplodedNode *N = C.generateErrorNode()) {
 if (!BT_BadFree[*CheckKind])
-  BT_BadFree[*CheckKind].reset(
-  new BugType(CheckNames[*CheckKind], "Bad free", "Memory Error"));
+  BT_BadFree[*CheckKind].reset(new BugType(
+  CheckNames[*CheckKind], "Bad free", categories::MemoryError));
 
 SmallString<100> Buf;
 llvm::raw_svector_ostream Os(Buf);


Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -2075,8 +2075,8 @@
 
   if (ExplodedNode *N = C.generateErrorNode()) {
 if (!BT_BadFree[*CheckKind])
-  BT_BadFree[*CheckKind].reset(
-  new BugType(CheckNames[*CheckKind], "Bad free", "Memory Error"));
+  BT_BadFree[*CheckKind].reset(new BugType(
+  CheckNames[*CheckKind], "Bad free", categories::MemoryError));
 
 SmallString<100> Buf;
 llvm::raw_svector_ostream Os(Buf);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42785: [Analyzer] Fix a typo in `ExprEngine::VisitMemberExpr`

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D42785#995211, @NoQ wrote:

> Ew. So it means that checker transitions are currently discarded. Great 
> catch. I guess we don't use this functionality yet, so we can't test it, but 
> the fix should definitely go in.


You are right, that's why I don't know how to add test for this change.

> Then, again, i'm suspecting that you may want to use `checkLocation` or 
> `checkBind` instead of `checkPreStmt` - i doubt that there's 
> anything special about member expressions that make them different from other 
> sorts of bindings. And if you're tracking how a symbol is assigned to a field 
> of the structure - well, you shouldn't, this info is already in the program 
> state and you can retrieve it any time with `getSVal(Region)`.

Thanks for your explanation, NoQ. I discovered the problem by chance, and I'll 
take your advice when it comes to dealing with //`MemberExpr`//.

And I have no commit access. If you plan to commit this change, I hope you can 
help me commit it, thank you in advance!


Repository:
  rC Clang

https://reviews.llvm.org/D42785



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

[PATCH] D42785: [Analyzer] Fix a typo in `ExprEngine::VisitMemberExpr`

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: NoQ, dcoughlin.
Herald added subscribers: cfe-commits, a.sidorin, szepet, xazax.hun.
Herald added a reviewer: george.karpenkov.
MTC edited the summary of this revision.

`VisitCommonDeclRefExpr()` should accept the ExplodedNode in `CheckedSet` as an 
argument, rather than `Pred`.


Repository:
  rC Clang

https://reviews.llvm.org/D42785

Files:
  lib/StaticAnalyzer/Core/ExprEngine.cpp


Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -2251,16 +2251,15 @@
   ExplodedNodeSet CheckedSet;
   getCheckerManager().runCheckersForPreStmt(CheckedSet, Pred, M, *this);
 
-  ExplodedNodeSet EvalSet;
-  ValueDecl *Member = M->getMemberDecl();
+  ExplodedNodeSet EvalSet;  
+  ValueDecl *Member = M->getMemberDecl();  
 
   // Handle static member variables and enum constants accessed via
   // member syntax.
-  if (isa(Member) || isa(Member)) {
-ExplodedNodeSet Dst;
+  if (isa(Member) || isa(Member)) {
 for (ExplodedNodeSet::iterator I = CheckedSet.begin(), E = 
CheckedSet.end();
  I != E; ++I) {
-  VisitCommonDeclRefExpr(M, Member, Pred, EvalSet);
+  VisitCommonDeclRefExpr(M, Member, *I, EvalSet);
 }
   } else {
 StmtNodeBuilder Bldr(CheckedSet, EvalSet, *currBldrCtx);


Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -2251,16 +2251,15 @@
   ExplodedNodeSet CheckedSet;
   getCheckerManager().runCheckersForPreStmt(CheckedSet, Pred, M, *this);
 
-  ExplodedNodeSet EvalSet;
-  ValueDecl *Member = M->getMemberDecl();
+  ExplodedNodeSet EvalSet;  
+  ValueDecl *Member = M->getMemberDecl();  
 
   // Handle static member variables and enum constants accessed via
   // member syntax.
-  if (isa(Member) || isa(Member)) {
-ExplodedNodeSet Dst;
+  if (isa(Member) || isa(Member)) {
 for (ExplodedNodeSet::iterator I = CheckedSet.begin(), E = CheckedSet.end();
  I != E; ++I) {
-  VisitCommonDeclRefExpr(M, Member, Pred, EvalSet);
+  VisitCommonDeclRefExpr(M, Member, *I, EvalSet);
 }
   } else {
 StmtNodeBuilder Bldr(CheckedSet, EvalSet, *currBldrCtx);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42300: [Analyzer] Add PreStmt and PostStmt callbacks for OffsetOfExpr

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D42300#982187, @NoQ wrote:

> My intuition suggests that this checker shouldn't be path-sensitive; our 
> path-sensitive analysis does very little to help you with this particular 
> checker, and you might end up with a much easier and more reliable checker if 
> you turn it into a simple AST visitor or an AST matcher. Just a heads up.


This is a very useful suggestion, many thanks, Noq! Path-sensitive is really a 
bit too heavy for this checker.


Repository:
  rC Clang

https://reviews.llvm.org/D42300



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

[PATCH] D42300: [Analyzer] Add PreStmt and PostStmt callbacks for OffsetOfExpr

[Henry Wong via Phabricator via cfe-commits]

MTC updated this revision to Diff 130753.
MTC added a comment.

- Use C++11 range-based for loop to traverse ExplodedNodeSet.
- Define the macro `offsetof` in `system-header-simulator.h`.


Repository:
  rC Clang

https://reviews.llvm.org/D42300

Files:
  lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  test/Analysis/Inputs/system-header-simulator.h
  test/Analysis/offsetofexpr-callback.c


Index: test/Analysis/offsetofexpr-callback.c
===
--- /dev/null
+++ test/Analysis/offsetofexpr-callback.c
@@ -0,0 +1,13 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder 
-analyzer-config 
debug.AnalysisOrder:PreStmtOffsetOfExpr=true,debug.AnalysisOrder:PostStmtOffsetOfExpr=true
 %s 2>&1 | FileCheck %s
+#include "Inputs/system-header-simulator.h"
+
+struct S {
+char c;
+};
+
+void test() {
+  offsetof(struct S, c);
+}
+
+// CHECK: PreStmt
+// CHECK-NEXT: PostStmt
Index: test/Analysis/Inputs/system-header-simulator.h
===
--- test/Analysis/Inputs/system-header-simulator.h
+++ test/Analysis/Inputs/system-header-simulator.h
@@ -109,4 +109,6 @@
 #ifndef NULL
 #define __DARWIN_NULL 0
 #define NULL __DARWIN_NULL
-#endif
\ No newline at end of file
+#endif
+
+#define offsetof(t, d) __builtin_offsetof(t, d)
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1488,12 +1488,19 @@
   Bldr.addNodes(Dst);
   break;
 
-case Stmt::OffsetOfExprClass:
+case Stmt::OffsetOfExprClass: {
   Bldr.takeNodes(Pred);
-  VisitOffsetOfExpr(cast(S), Pred, Dst);
+  ExplodedNodeSet PreVisit;
+  getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
+
+  ExplodedNodeSet PostVisit;
+  for (ExplodedNode *Node : PreVisit)
+VisitOffsetOfExpr(cast(S), Node, PostVisit);
+
+  getCheckerManager().runCheckersForPostStmt(Dst, PostVisit, S, *this);
   Bldr.addNodes(Dst);
   break;
-
+}
 case Stmt::UnaryExprOrTypeTraitExprClass:
   Bldr.takeNodes(Pred);
   VisitUnaryExprOrTypeTraitExpr(cast(S),
Index: lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
+++ lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
@@ -33,6 +33,8 @@
  check::PostStmt,
  check::PreStmt,
  check::PostStmt,
+ check::PreStmt,
+ check::PostStmt,
  check::PreCall,
  check::PostCall,
  check::NewAllocator,
@@ -89,6 +91,16 @@
   llvm::errs() << "PostStmt\n";
   }
 
+  void checkPreStmt(const OffsetOfExpr *OOE, CheckerContext ) const {
+if (isCallbackEnabled(C, "PreStmtOffsetOfExpr"))
+  llvm::errs() << "PreStmt\n";
+  }
+
+  void checkPostStmt(const OffsetOfExpr *OOE, CheckerContext ) const {
+if (isCallbackEnabled(C, "PostStmtOffsetOfExpr"))
+  llvm::errs() << "PostStmt\n";
+  }
+
   void checkPreCall(const CallEvent , CheckerContext ) const {
 if (isCallbackEnabled(C, "PreCall")) {
   llvm::errs() << "PreCall";


Index: test/Analysis/offsetofexpr-callback.c
===
--- /dev/null
+++ test/Analysis/offsetofexpr-callback.c
@@ -0,0 +1,13 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:PreStmtOffsetOfExpr=true,debug.AnalysisOrder:PostStmtOffsetOfExpr=true %s 2>&1 | FileCheck %s
+#include "Inputs/system-header-simulator.h"
+
+struct S {
+char c;
+};
+
+void test() {
+  offsetof(struct S, c);
+}
+
+// CHECK: PreStmt
+// CHECK-NEXT: PostStmt
Index: test/Analysis/Inputs/system-header-simulator.h
===
--- test/Analysis/Inputs/system-header-simulator.h
+++ test/Analysis/Inputs/system-header-simulator.h
@@ -109,4 +109,6 @@
 #ifndef NULL
 #define __DARWIN_NULL 0
 #define NULL __DARWIN_NULL
-#endif
\ No newline at end of file
+#endif
+
+#define offsetof(t, d) __builtin_offsetof(t, d)
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1488,12 +1488,19 @@
   Bldr.addNodes(Dst);
   break;
 
-case Stmt::OffsetOfExprClass:
+case Stmt::OffsetOfExprClass: {
   Bldr.takeNodes(Pred);
-  VisitOffsetOfExpr(cast(S), Pred, Dst);
+  ExplodedNodeSet PreVisit;
+  getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
+
+  ExplodedNodeSet PostVisit;
+  for (ExplodedNode *Node : PreVisit)
+

[PATCH] D42300: [Analyzer] Add PreStmt and PostStmt callbacks for OffsetOfExpr

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: NoQ, a.sidorin, dcoughlin.
Herald added subscribers: cfe-commits, szepet, xazax.hun.

PreStmt and PostStmt callbacks for OffsetOfExpr are necessary to implement 
`Cert ARR39-C: Do not add or subtract a scaled integer to a pointer`. And 
should I define the `offsetof` macro in 
`clang/test/Analysis/Inputs/system-header-simulator.h`? Or, like 
`clang/test/Analysis/malloc-sizeof.c`, use `#include `?


Repository:
  rC Clang

https://reviews.llvm.org/D42300

Files:
  lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  test/Analysis/offsetofexpr-callback.c


Index: test/Analysis/offsetofexpr-callback.c
===
--- /dev/null
+++ test/Analysis/offsetofexpr-callback.c
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder 
-analyzer-config 
debug.AnalysisOrder:PreStmtOffsetOfExpr=true,debug.AnalysisOrder:PostStmtOffsetOfExpr=true
 %s 2>&1 | FileCheck %s
+
+#include 
+
+struct S {
+char c;
+double d;
+};
+
+void test() {
+  offsetof(struct S, c);
+}
+
+// CHECK: PreStmt
+// CHECK-NEXT: PostStmt
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1488,12 +1488,21 @@
   Bldr.addNodes(Dst);
   break;
 
-case Stmt::OffsetOfExprClass:
+case Stmt::OffsetOfExprClass: {
   Bldr.takeNodes(Pred);
-  VisitOffsetOfExpr(cast(S), Pred, Dst);
+  ExplodedNodeSet PreVisit;
+  getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
+
+  ExplodedNodeSet PostVisit;
+  for (ExplodedNodeSet::iterator i = PreVisit.begin(), e = PreVisit.end();
+   i != e; ++i) {
+VisitOffsetOfExpr(cast(S), *i, PostVisit);
+  }
+
+  getCheckerManager().runCheckersForPostStmt(Dst, PostVisit, S, *this);
   Bldr.addNodes(Dst);
   break;
-
+}
 case Stmt::UnaryExprOrTypeTraitExprClass:
   Bldr.takeNodes(Pred);
   VisitUnaryExprOrTypeTraitExpr(cast(S),
Index: lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
+++ lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
@@ -33,6 +33,8 @@
  check::PostStmt,
  check::PreStmt,
  check::PostStmt,
+ check::PreStmt,
+ check::PostStmt,
  check::PreCall,
  check::PostCall,
  check::NewAllocator,
@@ -89,6 +91,16 @@
   llvm::errs() << "PostStmt\n";
   }
 
+  void checkPreStmt(const OffsetOfExpr *OOE, CheckerContext ) const {
+if (isCallbackEnabled(C, "PreStmtOffsetOfExpr"))
+  llvm::errs() << "PreStmt\n";
+  }
+
+  void checkPostStmt(const OffsetOfExpr *OOE, CheckerContext ) const {
+if (isCallbackEnabled(C, "PostStmtOffsetOfExpr"))
+  llvm::errs() << "PostStmt\n";
+  }
+
   void checkPreCall(const CallEvent , CheckerContext ) const {
 if (isCallbackEnabled(C, "PreCall")) {
   llvm::errs() << "PreCall";


Index: test/Analysis/offsetofexpr-callback.c
===
--- /dev/null
+++ test/Analysis/offsetofexpr-callback.c
@@ -0,0 +1,15 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=debug.AnalysisOrder -analyzer-config debug.AnalysisOrder:PreStmtOffsetOfExpr=true,debug.AnalysisOrder:PostStmtOffsetOfExpr=true %s 2>&1 | FileCheck %s
+
+#include 
+
+struct S {
+char c;
+double d;
+};
+
+void test() {
+  offsetof(struct S, c);
+}
+
+// CHECK: PreStmt
+// CHECK-NEXT: PostStmt
Index: lib/StaticAnalyzer/Core/ExprEngine.cpp
===
--- lib/StaticAnalyzer/Core/ExprEngine.cpp
+++ lib/StaticAnalyzer/Core/ExprEngine.cpp
@@ -1488,12 +1488,21 @@
   Bldr.addNodes(Dst);
   break;
 
-case Stmt::OffsetOfExprClass:
+case Stmt::OffsetOfExprClass: {
   Bldr.takeNodes(Pred);
-  VisitOffsetOfExpr(cast(S), Pred, Dst);
+  ExplodedNodeSet PreVisit;
+  getCheckerManager().runCheckersForPreStmt(PreVisit, Pred, S, *this);
+
+  ExplodedNodeSet PostVisit;
+  for (ExplodedNodeSet::iterator i = PreVisit.begin(), e = PreVisit.end();
+   i != e; ++i) {
+VisitOffsetOfExpr(cast(S), *i, PostVisit);
+  }
+
+  getCheckerManager().runCheckersForPostStmt(Dst, PostVisit, S, *this);
   Bldr.addNodes(Dst);
   break;
-
+}
 case Stmt::UnaryExprOrTypeTraitExprClass:
   Bldr.takeNodes(Pred);
   VisitUnaryExprOrTypeTraitExpr(cast(S),
Index: lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/AnalysisOrderChecker.cpp
+++ 

[PATCH] D37189: Fix an assertion failure that occured when custom 'operator new[]' return non-ElementRegion and 'c++-allocator-inlining' sets true.

[Henry Wong via Phabricator via cfe-commits]

MTC added a comment.

In https://reviews.llvm.org/D37189#979795, @NoQ wrote:

> Oh well, i guess i covered this in my recent patches anyway (esp. 
> r322787/https://reviews.llvm.org/D41406). Sorry, i just fixed everything 
> differently and it became unclear how to integrate your patch into the whole 
> thing.


Hi NoQ, that's all right, I'm totally fine with it :). And I don’t know how to 
abandon this patch, if you have the authority, please help me to abandon it, 
thank you!


Repository:
  rC Clang

https://reviews.llvm.org/D37189



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

[PATCH] D42106: [analyzer] Remove the useless method declararion 'BugReporter::RemoveUnneededCalls()'.

[Henry Wong via Phabricator via cfe-commits]

MTC created this revision.
MTC added reviewers: NoQ, dcoughlin, xazax.hun.
Herald added subscribers: cfe-commits, a.sidorin, rnkovacs, szepet.
MTC retitled this revision from "[analyzer] Remove the useless method 
declararion 'BugReporter::RemoveUnneedCalls()'." to "[analyzer] Remove the 
useless method declararion 'BugReporter::RemoveUnneededCalls()'.".
MTC edited the summary of this revision.

Since a static function `static bool removeUnneededCalls(...)` is defined in 
BugReporter.cpp, `BugReporter::RemoveUnneededCalls()` is no longer needed.


Repository:
  rC Clang

https://reviews.llvm.org/D42106

Files:
  include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h


Index: include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===
--- include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -470,8 +470,6 @@
 return true;
   }
 
-  bool RemoveUnneededCalls(PathPieces , BugReport *R);
-
   void Register(BugType *BT);
 
   /// \brief Add the given report to the set of reports tracked by BugReporter.


Index: include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
===
--- include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
+++ include/clang/StaticAnalyzer/Core/BugReporter/BugReporter.h
@@ -470,8 +470,6 @@
 return true;
   }
 
-  bool RemoveUnneededCalls(PathPieces , BugReport *R);
-
   void Register(BugType *BT);
 
   /// \brief Add the given report to the set of reports tracked by BugReporter.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits