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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Welp, windows builtbots broke again. I'll try to see whats wrong with undef 
sanitizer, but fear this will end up in a revert.

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/19769/steps/test-check-all/logs/stdio


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54823



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

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

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL372414: Reland [analyzer][MallocChecker][NFC] Document 
and reorganize some functions (authored by Szelethus, committed by ).
Herald added a project: LLVM.

Changed prior to commit:
  https://reviews.llvm.org/D54823?vs=220873=221073#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54823

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -6,8 +6,41 @@
 //
 //===--===//
 //
-// This file defines malloc/free checker, which checks for potential memory
-// leaks, double free, and use-after-free problems.
+// This file defines a variety of memory management related checkers, such as
+// leak, double free, and use-after-free.
+//
+// The following checkers are defined here:
+//
+//   * MallocChecker
+//   Despite its name, it models all sorts of memory allocations and
+//   de- or reallocation, including but not limited to malloc, free,
+//   relloc, new, delete. It also reports on a variety of memory misuse
+//   errors.
+//   Many other checkers interact very closely with this checker, in fact,
+//   most are merely options to this one. Other checkers may register
+//   MallocChecker, but do not enable MallocChecker's reports (more details
+//   to follow around its field, ChecksEnabled).
+//   It also has a boolean "Optimistic" checker option, which if set to true
+//   will cause the checker to model user defined memory management related
+//   functions annotated via the attribute ownership_takes, ownership_holds
+//   and ownership_returns.
+//
+//   * NewDeleteChecker
+//   Enables the modeling of new, new[], delete, delete[] in MallocChecker,
+//   and checks for related double-free and use-after-free errors.
+//
+//   * NewDeleteLeaksChecker
+//   Checks for leaks related to new, new[], delete, delete[].
+//   Depends on NewDeleteChecker.
+//
+//   * MismatchedDeallocatorChecker
+//   Enables checking whether memory is deallocated with the correspending
+//   allocation function in MallocChecker, such as malloc() allocated
+//   regions are only freed by free(), new by delete, new[] by delete[].
+//
+//  InnerPointerChecker interacts very closely with MallocChecker, but unlike
+//  the above checkers, it has it's own file, hence the many InnerPointerChecker
+//  related headers and non-static functions.
 //
 //===--===//
 
@@ -37,6 +70,10 @@
 using namespace clang;
 using namespace ento;
 
+//===--===//
+// The types of allocation we're modeling.
+//===--===//
+
 namespace {
 
 // Used to check correspondence between allocators and deallocators.
@@ -50,57 +87,88 @@
   AF_InnerBuffer
 };
 
+struct MemFunctionInfoTy;
+
+} // end of anonymous namespace
+
+/// Determine family of a deallocation expression.
+static AllocationFamily
+getAllocationFamily(const MemFunctionInfoTy , CheckerContext ,
+const Stmt *S);
+
+/// Print names of allocators and deallocators.
+///
+/// \returns true on success.
+static bool printAllocDeallocName(raw_ostream , CheckerContext ,
+  const Expr *E);
+
+/// Print expected name of an allocator based on the deallocator's
+/// family derived from the DeallocExpr.
+static void printExpectedAllocName(raw_ostream ,
+   const MemFunctionInfoTy ,
+   CheckerContext , const Expr *E);
+
+/// Print expected name of a deallocator based on the allocator's
+/// family.
+static void printExpectedDeallocName(raw_ostream , AllocationFamily Family);
+
+//===--===//
+// The state of a symbol, in terms of memory management.
+//===--===//
+
+namespace {
+
 class RefState {
-  enum Kind { // Reference to allocated memory.
-  Allocated,
-  // Reference to zero-allocated memory.
-  AllocatedOfSizeZero,
-  // Reference to released/freed memory.
-  Released,
-  // The responsibility for freeing resources has transferred from
-  // this reference. A relinquished symbol should not be freed.
-  Relinquished,
-  // We are no longer guaranteed to have observed all manipulations
-   

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In retrospect, I would have made this patch a little more fragmented (its 
almost a year old, does it beat the revival of the symbolreaper patch?), but it 
would be a painful chore to separate at this point, if you dont mind. I have a 
couple more cooking in the cauldron that are more digestible, so, lesson 
learned :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54823



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

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

[Kristóf Umann via Phabricator via cfe-commits]

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

In D54823#1675604 , @NoQ wrote:

> Is it just me or phabricator somehow refuses to display the changes since the 
> last diff here? That's probably the commit diff is involved somehow. So i'm 
> confused what exactly has changed >.<


File name change, I'm using the monorepo now.




Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:3137
+  if (RS->isAllocated() || RS->isAllocatedOfSizeZero())
+if (!IsConstPointerEscape || checkIfNewOrNewArrayFamily(RS))
+  State = State->set(sym, RefState::getEscaped(RS));

This was the change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54823



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

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Is it just me or phabricator somehow refuses to display the changes since the 
last diff here? That's probably the commit diff is involved somehow. So i'm 
confused what exactly has changed >.<


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54823



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

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D54823#1675352 , @Szelethus wrote:

> Rebase, fix (suspected) error that caused buildbot errors. Hold off commiting 
> in favor checking whether putting `CallDescriptionMap` in would be too 
> invasive, but really, can't be worse then it already is.


Nope. It implies far too heavy changes. I'll land this as is, but will wait a 
bit as its been a while since the acceptance.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54823



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

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 220873.
Szelethus added a comment.

Rebase, fix (suspected) error that caused buildbot errors. Hold off commiting 
in favor checking whether putting `CallDescriptionMap` in would be too 
invasive, but really, can't be worse then it already is.


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

https://reviews.llvm.org/D54823

Files:
  clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Index: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -6,8 +6,41 @@
 //
 //===--===//
 //
-// This file defines malloc/free checker, which checks for potential memory
-// leaks, double free, and use-after-free problems.
+// This file defines a variety of memory management related checkers, such as
+// leak, double free, and use-after-free.
+//
+// The following checkers are defined here:
+//
+//   * MallocChecker
+//   Despite its name, it models all sorts of memory allocations and
+//   de- or reallocation, including but not limited to malloc, free,
+//   relloc, new, delete. It also reports on a variety of memory misuse
+//   errors.
+//   Many other checkers interact very closely with this checker, in fact,
+//   most are merely options to this one. Other checkers may register
+//   MallocChecker, but do not enable MallocChecker's reports (more details
+//   to follow around its field, ChecksEnabled).
+//   It also has a boolean "Optimistic" checker option, which if set to true
+//   will cause the checker to model user defined memory management related
+//   functions annotated via the attribute ownership_takes, ownership_holds
+//   and ownership_returns.
+//
+//   * NewDeleteChecker
+//   Enables the modeling of new, new[], delete, delete[] in MallocChecker,
+//   and checks for related double-free and use-after-free errors.
+//
+//   * NewDeleteLeaksChecker
+//   Checks for leaks related to new, new[], delete, delete[].
+//   Depends on NewDeleteChecker.
+//
+//   * MismatchedDeallocatorChecker
+//   Enables checking whether memory is deallocated with the correspending
+//   allocation function in MallocChecker, such as malloc() allocated
+//   regions are only freed by free(), new by delete, new[] by delete[].
+//
+//  InnerPointerChecker interacts very closely with MallocChecker, but unlike
+//  the above checkers, it has it's own file, hence the many InnerPointerChecker
+//  related headers and non-static functions.
 //
 //===--===//
 
@@ -37,6 +70,10 @@
 using namespace clang;
 using namespace ento;
 
+//===--===//
+// The types of allocation we're modeling.
+//===--===//
+
 namespace {
 
 // Used to check correspondence between allocators and deallocators.
@@ -50,57 +87,88 @@
   AF_InnerBuffer
 };
 
+struct MemFunctionInfoTy;
+
+} // end of anonymous namespace
+
+/// Determine family of a deallocation expression.
+static AllocationFamily
+getAllocationFamily(const MemFunctionInfoTy , CheckerContext ,
+const Stmt *S);
+
+/// Print names of allocators and deallocators.
+///
+/// \returns true on success.
+static bool printAllocDeallocName(raw_ostream , CheckerContext ,
+  const Expr *E);
+
+/// Print expected name of an allocator based on the deallocator's
+/// family derived from the DeallocExpr.
+static void printExpectedAllocName(raw_ostream ,
+   const MemFunctionInfoTy ,
+   CheckerContext , const Expr *E);
+
+/// Print expected name of a deallocator based on the allocator's
+/// family.
+static void printExpectedDeallocName(raw_ostream , AllocationFamily Family);
+
+//===--===//
+// The state of a symbol, in terms of memory management.
+//===--===//
+
+namespace {
+
 class RefState {
-  enum Kind { // Reference to allocated memory.
-  Allocated,
-  // Reference to zero-allocated memory.
-  AllocatedOfSizeZero,
-  // Reference to released/freed memory.
-  Released,
-  // The responsibility for freeing resources has transferred from
-  // this reference. A relinquished symbol should not be freed.
-  Relinquished,
-  // We are no longer guaranteed to have observed all manipulations
-  // of this pointer/memory. For example, it could have been
-  // 

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added inline comments.
Herald added a subscriber: Charusso.
Herald added a project: LLVM.



Comment at: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:3090
   return checkPointerEscapeAux(State, Escaped, Call, Kind,
-   );
+   /*IsConstPointerEscape*/ true);
 }

Praise clangd! This is why the buildbots broke :) `checkIfNewOrNewArrayFamily` 
doesn't return `false` in all cases.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54823



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

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

Lit test failures are gone for Windows after reverting this. Will probably deal 
with this revision after 8.0.0.


Repository:
  rL LLVM

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

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] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

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

Reverted in rC349344 .


Repository:
  rL LLVM

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

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] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL349281: [analyzer][MallocChecker][NFC] Document and 
reorganize some functions (authored by Szelethus, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D54823?vs=177057=178371#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D54823

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

Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -7,8 +7,41 @@
 //
 //===--===//
 //
-// This file defines malloc/free checker, which checks for potential memory
-// leaks, double free, and use-after-free problems.
+// This file defines a variety of memory management related checkers, such as
+// leak, double free, and use-after-free.
+//
+// The following checkers are defined here:
+//
+//   * MallocChecker
+//   Despite its name, it models all sorts of memory allocations and
+//   de- or reallocation, including but not limited to malloc, free,
+//   relloc, new, delete. It also reports on a variety of memory misuse
+//   errors.
+//   Many other checkers interact very closely with this checker, in fact,
+//   most are merely options to this one. Other checkers may register
+//   MallocChecker, but do not enable MallocChecker's reports (more details
+//   to follow around its field, ChecksEnabled).
+//   It also has a boolean "Optimistic" checker option, which if set to true
+//   will cause the checker to model user defined memory management related
+//   functions annotated via the attribute ownership_takes, ownership_holds
+//   and ownership_returns.
+//
+//   * NewDeleteChecker
+//   Enables the modeling of new, new[], delete, delete[] in MallocChecker,
+//   and checks for related double-free and use-after-free errors.
+//
+//   * NewDeleteLeaksChecker
+//   Checks for leaks related to new, new[], delete, delete[].
+//   Depends on NewDeleteChecker.
+//
+//   * MismatchedDeallocatorChecker
+//   Enables checking whether memory is deallocated with the correspending
+//   allocation function in MallocChecker, such as malloc() allocated
+//   regions are only freed by free(), new by delete, new[] by delete[].
+//
+//  InnerPointerChecker interacts very closely with MallocChecker, but unlike
+//  the above checkers, it has it's own file, hence the many InnerPointerChecker
+//  related headers and non-static functions.
 //
 //===--===//
 
@@ -37,6 +70,10 @@
 using namespace clang;
 using namespace ento;
 
+//===--===//
+// The types of allocation we're modeling.
+//===--===//
+
 namespace {
 
 // Used to check correspondence between allocators and deallocators.
@@ -50,28 +87,59 @@
   AF_InnerBuffer
 };
 
+struct MemFunctionInfoTy;
+
+} // end of anonymous namespace
+
+/// Determine family of a deallocation expression.
+static AllocationFamily getAllocationFamily(
+const MemFunctionInfoTy , CheckerContext , const Stmt *S);
+
+/// Print names of allocators and deallocators.
+///
+/// \returns true on success.
+static bool printAllocDeallocName(raw_ostream , CheckerContext ,
+  const Expr *E);
+
+/// Print expected name of an allocator based on the deallocator's
+/// family derived from the DeallocExpr.
+static void printExpectedAllocName(raw_ostream ,
+   const MemFunctionInfoTy  ,
+   CheckerContext , const Expr *E);
+
+/// Print expected name of a deallocator based on the allocator's
+/// family.
+static void printExpectedDeallocName(raw_ostream , AllocationFamily Family);
+
+//===--===//
+// The state of a symbol, in terms of memory management.
+//===--===//
+
+namespace {
+
 class RefState {
-  enum Kind { // Reference to allocated memory.
-  Allocated,
-  // Reference to zero-allocated memory.
-  AllocatedOfSizeZero,
-  // Reference to released/freed memory.
-  Released,
-  // The responsibility for freeing resources has transferred from
-  // this reference. A relinquished symbol should not be freed.
-  Relinquished,
-  // We 

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 177057.
Szelethus added a comment.

Woops, forgot to fix the doc of deallocation functions.


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

https://reviews.llvm.org/D54823

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/malloc-annotations.c
  test/Analysis/malloc.c

Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=debug.ExprInspection
 
 #include "Inputs/system-header-simulator.h"
 
Index: test/Analysis/malloc-annotations.c
===
--- test/Analysis/malloc-annotations.c
+++ test/Analysis/malloc-annotations.c
@@ -1,4 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc -analyzer-store=region -verify -analyzer-config unix.Malloc:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize,unix.Malloc \
+// RUN:   -analyzer-config unix.Malloc:Optimistic=true
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -7,8 +7,41 @@
 //
 //===--===//
 //
-// This file defines malloc/free checker, which checks for potential memory
-// leaks, double free, and use-after-free problems.
+// This file defines a variety of memory management related checkers, such as
+// leak, double free, and use-after-free.
+//
+// The following checkers are defined here:
+//
+//   * MallocChecker
+//   Despite its name, it models all sorts of memory allocations and
+//   de- or reallocation, including but not limited to malloc, free,
+//   relloc, new, delete. It also reports on a variety of memory misuse
+//   errors.
+//   Many other checkers interact very closely with this checker, in fact,
+//   most are merely options to this one. Other checkers may register
+//   MallocChecker, but do not enable MallocChecker's reports (more details
+//   to follow around its field, ChecksEnabled).
+//   It also has a boolean "Optimistic" checker option, which if set to true
+//   will cause the checker to model user defined memory management related
+//   functions annotated via the attribute ownership_takes, ownership_holds
+//   and ownership_returns.
+//
+//   * NewDeleteChecker
+//   Enables the modeling of new, new[], delete, delete[] in MallocChecker,
+//   and checks for related double-free and use-after-free errors.
+//
+//   * NewDeleteLeaksChecker
+//   Checks for leaks related to new, new[], delete, delete[].
+//   Depends on NewDeleteChecker.
+//
+//   * MismatchedDeallocatorChecker
+//   Enables checking whether memory is deallocated with the correspending
+//   allocation function in MallocChecker, such as malloc() allocated
+//   regions are only freed by free(), new by delete, new[] by delete[].
+//
+//  InnerPointerChecker interacts very closely with MallocChecker, but unlike
+//  the above checkers, it has it's own file, hence the many InnerPointerChecker
+//  related headers and non-static functions.
 //
 //===--===//
 
@@ -37,6 +70,10 @@
 using namespace clang;
 using namespace ento;
 
+//===--===//
+// The types of allocation we're modeling.
+//===--===//
+
 namespace {
 
 // Used to check correspondence between allocators and deallocators.
@@ -50,28 +87,59 @@
   AF_InnerBuffer
 };
 
+struct MemFunctionInfoTy;
+
+} // end of anonymous namespace
+
+/// Determine family of a deallocation expression.
+static AllocationFamily getAllocationFamily(
+const MemFunctionInfoTy , CheckerContext , const Stmt *S);
+
+/// Print names of allocators and deallocators.
+///
+/// \returns true on success.
+static bool printAllocDeallocName(raw_ostream , CheckerContext ,
+  const Expr 

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

[Kristóf Umann via Phabricator via cfe-commits]

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



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:
> NoQ wrote:
> > Szelethus wrote:
> > > MTC wrote:
> > > > 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.
> > > Interestingly, `MemFunctionInfo` is not always initialized, so a 
> > > performance argument can be made here. I specifically checked whether 
> > > there's any point in doing this hackery, and the answer is... well, some. 
> > > I'll probably touch on these in a followup patch.
> > Lazy initialization of identifiers is kinda perceived as a fairly worthless 
> > optimization. Hence `CallDescription`.
> > 
> > Also it cannot be done during registration because the AST is not 
> > constructed yet at this point. Well, probably it can be done anyway because 
> > the empty identifier table is already there in the `ASTContext` and we can 
> > always eagerly add a few items into it, but it still sounds scary.
> I would personally prefer to risk initializing these during registration, but 
> if we're this many comments into this discussion, then I believe it deserves 
> a separate patch.
> 
> I'll leave a `TODO:` in the code.
Or just go with `CallDescription`.


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

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] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



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

NoQ wrote:
> Szelethus wrote:
> > MTC wrote:
> > > 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.
> > Interestingly, `MemFunctionInfo` is not always initialized, so a 
> > performance argument can be made here. I specifically checked whether 
> > there's any point in doing this hackery, and the answer is... well, some. 
> > I'll probably touch on these in a followup patch.
> Lazy initialization of identifiers is kinda perceived as a fairly worthless 
> optimization. Hence `CallDescription`.
> 
> Also it cannot be done during registration because the AST is not constructed 
> yet at this point. Well, probably it can be done anyway because the empty 
> identifier table is already there in the `ASTContext` and we can always 
> eagerly add a few items into it, but it still sounds scary.
I would personally prefer to risk initializing these during registration, but 
if we're this many comments into this discussion, then I believe it deserves a 
separate patch.

I'll leave a `TODO:` in the code.



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:
> > 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, so I think it is 
> > appropriate to extract the summary information when he submitted this 
> > patch, see [[ 
> > https://github.com/llvm-mirror/clang/commit/2e61d2893934f7b91ca9e2f889a4e4cc9939b05c#diff-ff9160d4628cb9b6186559837c2c8668
> >  | [Analyzer] fix for PR19102 ]]. What do you think?
> Wow, nice detective work there :D Sounds good to me.
Umm, yea, I really couldn't come up with a better name. Hopefully the comments 
both here and at the call site help on this.


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

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] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus updated this revision to Diff 177002.
Szelethus marked 32 inline comments as done.
Szelethus added a reviewer: MTC.
Szelethus added a comment.

Addressing inline comments.


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

https://reviews.llvm.org/D54823

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  test/Analysis/malloc-annotations.c
  test/Analysis/malloc.c

Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=debug.ExprInspection
 
 #include "Inputs/system-header-simulator.h"
 
Index: test/Analysis/malloc-annotations.c
===
--- test/Analysis/malloc-annotations.c
+++ test/Analysis/malloc-annotations.c
@@ -1,4 +1,8 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc -analyzer-store=region -verify -analyzer-config unix.Malloc:Optimistic=true %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize,unix.Malloc \
+// RUN:   -analyzer-config unix.Malloc:Optimistic=true
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t);
 void free(void *);
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -7,8 +7,41 @@
 //
 //===--===//
 //
-// This file defines malloc/free checker, which checks for potential memory
-// leaks, double free, and use-after-free problems.
+// This file defines a variety of memory management related checkers, such as
+// leak, double free, and use-after-free.
+//
+// The following checkers are defined here:
+//
+//   * MallocChecker
+//   Despite its name, it models all sorts of memory allocations and
+//   de- or reallocation, including but not limited to malloc, free,
+//   relloc, new, delete. It also reports on a variety of memory misuse
+//   errors.
+//   Many other checkers interact very closely with this checker, in fact,
+//   most are merely options to this one. Other checkers may register
+//   MallocChecker, but do not enable MallocChecker's reports (more details
+//   to follow around its field, ChecksEnabled).
+//   It also has a boolean "Optimistic" checker option, which if set to true
+//   will cause the checker to model user defined memory management related
+//   functions annotated via the attribute ownership_takes, ownership_holds
+//   and ownership_returns.
+//
+//   * NewDeleteChecker
+//   Enables the modeling of new, new[], delete, delete[] in MallocChecker,
+//   and checks for related double-free and use-after-free errors.
+//
+//   * NewDeleteLeaksChecker
+//   Checks for leaks related to new, new[], delete, delete[].
+//   Depends on NewDeleteChecker.
+//
+//   * MismatchedDeallocatorChecker
+//   Enables checking whether memory is deallocated with the correspending
+//   allocation function in MallocChecker, such as malloc() allocated
+//   regions are only freed by free(), new by delete, new[] by delete[].
+//
+//  InnerPointerChecker interacts very closely with MallocChecker, but unlike
+//  the above checkers, it has it's own file, hence the many InnerPointerChecker
+//  related headers and non-static functions.
 //
 //===--===//
 
@@ -37,6 +70,10 @@
 using namespace clang;
 using namespace ento;
 
+//===--===//
+// The types of allocation we're modeling.
+//===--===//
+
 namespace {
 
 // Used to check correspondence between allocators and deallocators.
@@ -50,28 +87,59 @@
   AF_InnerBuffer
 };
 
+struct MemFunctionInfoTy;
+
+} // end of anonymous namespace
+
+/// Determine family of a deallocation expression.
+static AllocationFamily getAllocationFamily(
+const MemFunctionInfoTy , CheckerContext , const Stmt *S);
+
+/// Print names of allocators and deallocators.
+///
+/// \returns true on success.
+static bool printAllocDeallocName(raw_ostream , CheckerContext 

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

[Artem Dergachev via Phabricator via cfe-commits]

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

Yay. Wow. Cool. Thanks.

I heard that previously there was an explicit desire to avoid doxygen comments 
in checkers because they're quite useless because the checker is usually all in 
one file and doesn't have any clients that are interested in calling its 
methods directly.

But now that our checkers are split into files and start interacting with each 
other, this argument no longer holds.

So, well, thanks a lot.




Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:16
+//   * MallocChecker
+//   Despite it's name, it models all sorts of memory allocations and
+//   de- or reallocation, including but not limited to malloc, free,

~~it's~~ its



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:23
+//   MallocChecker, but do not enable MallocChecker's reports (more details
+//   to follow around it's field, ChecksEnabled).
+//   It also has a boolean "Optimistic" checker option, which if set to 
true

~~it's~~ its



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:283-300
+  mutable IdentifierInfo *II_alloca = nullptr, *II_win_alloca = nullptr,
+ *II_malloc = nullptr, *II_free = nullptr,
+ *II_realloc = nullptr, *II_calloc = nullptr,
+ *II_valloc = nullptr, *II_reallocf = nullptr,
+ *II_strndup = nullptr, *II_strdup = nullptr,
+ *II_win_strdup = nullptr, *II_kmalloc = nullptr,
+ *II_if_nameindex = nullptr,

Not now of course, but see also D45458. It'd be great to have call descriptions 
here, so that it looked like
```lang=c++
CallDescription alloca("alloca"),
win_alloca("win_alloca"),
malloc("malloc"),
// ...
```
..and no initialization necessary later.

(we can even wrap them into a macro so that we didn't have to write names 
twice).



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

MTC wrote:
> `CE` typo?
Looks fine. It's the name of the parameter in the doxygen syntax.



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.

MTC wrote:
> `before realloction` typo?
Seems fine.



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:
> > 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.
> Interestingly, `MemFunctionInfo` is not always initialized, so a performance 
> argument can be made here. I specifically checked whether there's any point 
> in doing this hackery, and the answer is... well, some. I'll probably touch 
> on these in a followup patch.
Lazy initialization of identifiers is kinda perceived as a fairly worthless 
optimization. Hence `CallDescription`.

Also it cannot be done during registration because the AST is not constructed 
yet at this point. Well, probably it can be done anyway because the empty 
identifier table is already there in the `ASTContext` and we can always eagerly 
add a few items into it, but it still sounds scary.


Repository:
  rC Clang

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

https://reviews.llvm.org/D54823



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus marked 3 inline comments as done.
Szelethus added inline comments.



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

MTC wrote:
> 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.
Interestingly, `MemFunctionInfo` is not always initialized, so a performance 
argument can be made here. I specifically checked whether there's any point in 
doing this hackery, and the answer is... well, some. I'll probably touch on 
these in a followup patch.



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

MTC wrote:
> Szelethus wrote:
> > MTC wrote:
> > > `const` qualifier missing?
> > This method was made `static`.
> You are right, sorry for my oversight!
Actually, I forgot about it 10 times when I re-read the patch, no shame in that 
:)



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) {
 

MTC wrote:
> 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, so I think it is 
> appropriate to extract the summary information when he submitted this patch, 
> see [[ 
> https://github.com/llvm-mirror/clang/commit/2e61d2893934f7b91ca9e2f889a4e4cc9939b05c#diff-ff9160d4628cb9b6186559837c2c8668
>  | [Analyzer] fix for PR19102 ]]. What do you think?
Wow, nice detective work there :D Sounds good to me.


Repository:
  rC Clang

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

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

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added inline comments.



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

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 


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] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

And thank you for helping me along the way! :D




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

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.



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

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?



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

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.



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

MTC wrote:
> `const` qualifier missing?
This method was made `static`.



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) {
 

[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] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus added a comment.

I also have some very neat ideas how the split up should go, but I'd like to 
mature the idea in my head for just a bit longer.




Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:24-27
+//   It also has a boolean "Optimistic" checker option, which if set to 
true
+//   will cause the checker to model user defined memory management related
+//   functions annotated via the attribute ownership_takes, ownership_holds
+//   and ownership_returns.

Source: https://reviews.llvm.org/D7905



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:242-245
+/// This is important because realloc may fail, and that needs special 
modeling.
+/// Whether reallocation failed or not will not be known until later, so we'll
+/// store whether upon failure 'fromPtr' will be freed, or needs to be freed
+/// later, etc.

Source: http://lists.llvm.org/pipermail/cfe-dev/2018-November/060216.html



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:435-443
+  /// User-defined function may have the ownership_returns attribute, which
+  /// annotates that the function returns with an object that was allocated on
+  /// the heap, and passes the ownertship to the callee.
+  ///
+  ///   void __attribute((ownership_returns(malloc, 1))) *my_malloc(size_t);
+  ///
+  /// It has two parameters:

Source:
http://lists.llvm.org/pipermail/cfe-dev/2010-June/009600.html
https://github.com/llvm-mirror/clang/commit/dd0e490c24aeade2c59ca4cae171199f6af9f02e



Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1359-1363
+  /// Non-trivial constructors have a chance to escape 'this', but marking all
+  /// invocations of trivial constructors as escaped would cause too great of
+  /// reduction of true positives, so let's just do that for constructor that
+  /// have an argument of a pointer-to-record type.
+  if (!PM.isConsumedExpr(NE) && hasNonTrivialConstructorCall(NE))

Source:
https://bugs.llvm.org/show_bug.cgi?id=19102
https://reviews.llvm.org/D4025


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] D54823: [analyzer][MallocChecker][NFC] Document and reorganize some functions

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, george.karpenkov, xazax.hun, rnkovacs.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, whisperity.

I'm in the process of splitting this checker into multiple other ones, and 
while I was there, I provided documentation to most functions, and made some 
static, and in some cases moved them out of class.

This patch merely reorganizes some things, and features no functional change.

In detail:

- Provided documentation, or moved existing documentation in more obvious 
places.
- Added dividers. (the `//===--===//` thing).
- Moved `getAllocationFamily`, `printAllocDeallocName`, 
`printExpectedAllocName` and `printExpectedDeallocName` in the global namespace 
on top of the file where `AllocationFamily` is declared, as they are very 
strongly related.
- Realloc modeling was very poor in terms of variable and structure naming, as 
well as documentation, so I renamed some of them and added much needed docs.
- Moved function `IdentifierInfo`s to a separate struct, and moved 
`isMemFunction`, `isCMemFunction` adn `isStandardNewDelete` inside it. This 
makes the patch affect quite a lot of lines, should I extract it to a separate 
one?
- Moved `MallocBugVisitor` out of `MallocChecker`.
- Preferred switches to long else-if branches in some places.
- Neatly organized some `RUN:` lines.


Repository:
  rC Clang

https://reviews.llvm.org/D54823

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

Index: test/Analysis/malloc.c
===
--- test/Analysis/malloc.c
+++ test/Analysis/malloc.c
@@ -1,4 +1,9 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.deadcode.UnreachableCode,alpha.core.CastSize,unix.Malloc,debug.ExprInspection -analyzer-store=region -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-store=region -verify %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.deadcode.UnreachableCode \
+// RUN:   -analyzer-checker=alpha.core.CastSize \
+// RUN:   -analyzer-checker=unix.Malloc \
+// RUN:   -analyzer-checker=debug.ExprInspection
 
 #include "Inputs/system-header-simulator.h"
 
Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -7,8 +7,41 @@
 //
 //===--===//
 //
-// This file defines malloc/free checker, which checks for potential memory
-// leaks, double free, and use-after-free problems.
+// This file defines a variety of memory management related checkers, such as
+// leak, double free, and use-after-free.
+//
+// The following checkers are defined here:
+//
+//   * MallocChecker
+//   Despite it's name, it models all sorts of memory allocations and
+//   de- or reallocation, including but not limited to malloc, free,
+//   relloc, new, delete. It also reports on a variety of memory misuse
+//   errors.
+//   Many other checkers interact very closely with this checker, in fact,
+//   most are merely options to this one. Other checkers may register
+//   MallocChecker, but do not enable MallocChecker's reports (more details
+//   to follow around it's field, ChecksEnabled).
+//   It also has a boolean "Optimistic" checker option, which if set to true
+//   will cause the checker to model user defined memory management related
+//   functions annotated via the attribute ownership_takes, ownership_holds
+//   and ownership_returns.
+//
+//   * NewDeleteChecker
+//   Enables the modeling of new, new[], delete, delete[] in MallocChecker,
+//   and checks for related double-free and use-after-free errors.
+//
+//   * NewDeleteLeaksChecker
+//   Checks for leaks related to new, new[], delete, delete[].
+//   Depends on NewDeleteChecker.
+//
+//   * MismatchedDeallocatorChecker
+//   Enables checking whether memory is deallocated with the correspending
+//   allocation function in MallocChecker, such as malloc() allocated
+//   regions are only freed by free(), new by delete, new[] by delete[].
+//
+//  InnerPointerChecker interacts very closely with MallocChecker, but unlike
+//  the above checkers, it has it's own file, hence the many InnerPointerChecker
+//  related headers and non-static functions.
 //
 //===--===//
 
@@ -37,6 +70,10 @@
 using namespace clang;
 using namespace ento;
 
+//===--===//
+// The types of allocation we're modeling.
+//===--===//
+
 namespace {
 
 // Used to check correspondence between allocators and