[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

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

Szelethus added a comment.

In D68165#1950451 , @ldionne wrote:

> Always with me? Maybe you should double-check that you don't have something 
> weird in your git config, aliases or other? I don't know *why* it would be me 
> in particular, but the fact that it is consistently the same person is kind 
> of suspicious.


Will do. Sorry for the trouble!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In D68165#1950357 , @Szelethus wrote:

> Ugh, I squashed my local commits and pushed, it seems like for some reason it 
> made you the author... it has happened to me before (always with you, for 
> some reason), but was able to notice and correct it.


Always with me? Maybe you should double-check that you don't have something 
weird in your git config, aliases or other? I don't know *why* it would be me 
in particular, but the fact that it is consistently the same person is kind of 
suspicious.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

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

Szelethus added a comment.

In D68165#1949954 , @ldionne wrote:

> > Closed by commit rG703a1b8caf09 
> > : 
> > [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy 
> > CallDescriptionMap (authored by ldionne, committed by Szelethus). · Explain 
> > Why
>
> This is really strange -- it looks like 703a1b8caf09 
>  was 
> attributed to me in git, but I have nothing to do with this change. How did 
> you commit the patch? Is that some Phabricator bug?


Ugh, I squashed my local commits and pushed, it seems like for some reason it 
made you the author... it has happened to me before (always with you, for some 
reason), but was able to notice and correct it. There is little we can do about 
this, right?

In D68165#1950212 , @NoQ wrote:

> Not a phabricator bug. Maybe @Szelethus's local git somehow got in a weird 
> state(?)


Its been a source of endless misery :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

**`$ git show --pretty=full 703a1b8caf09`**

  commit 703a1b8caf09a5262a45c2179b8131922f71cf25
  Author: Louis Dionne 
  Commit: Kirstóf Umann 
  
  [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy 
CallDescriptionMap
  
  ...

Not a phabricator bug. Maybe @Szelethus's local git somehow got in a weird 
state(?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

> Closed by commit rG703a1b8caf09 
> : 
> [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy 
> CallDescriptionMap (authored by ldionne, committed by Szelethus). · Explain 
> Why

This is really strange -- it looks like 703a1b8caf09 
 was 
attributed to me in git, but I have nothing to do with this change. How did you 
commit the patch? Is that some Phabricator bug?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

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

This revision was automatically updated to reflect the committed changes.
Szelethus marked 5 inline comments as done.
Closed by commit rG703a1b8caf09: [analyzer][MallocChecker][NFC] Split 
checkPostCall up, deploy CallDescriptionMap (authored by ldionne, committed by 
Szelethus).
Herald added a subscriber: ASDenysPetrov.

Changed prior to commit:
  https://reviews.llvm.org/D68165?vs=247512=253593#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165

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
@@ -62,16 +62,19 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/ErrorHandling.h"
 #include 
+#include 
 #include 
 
 using namespace clang;
 using namespace ento;
+using namespace std::placeholders;
 
 //===--===//
 // The types of allocation we're modeling. This is used to check whether a
@@ -93,8 +96,6 @@
   AF_InnerBuffer
 };
 
-struct MemFunctionInfoTy;
-
 } // end of anonymous namespace
 
 /// Print names of allocators and deallocators.
@@ -263,47 +264,6 @@
 
 REGISTER_MAP_WITH_PROGRAMSTATE(ReallocPairs, SymbolRef, ReallocPair)
 
-//===--===//
-// Kinds of memory operations, information about resource managing functions.
-//===--===//
-
-namespace {
-
-struct MemFunctionInfoTy {
-  /// The value of the MallocChecker:Optimistic is stored in this variable.
-  ///
-  /// In pessimistic mode, the checker assumes that it does not know which
-  /// functions might free the memory.
-  /// In optimistic mode, the checker assumes that all user-defined functions
-  /// which might free a pointer are annotated.
-  DefaultBool ShouldIncludeOwnershipAnnotatedFunctions;
-
-  CallDescription CD_alloca{{"alloca"}, 1}, CD_win_alloca{{"_alloca"}, 1},
-  CD_malloc{{"malloc"}, 1}, CD_BSD_malloc{{"malloc"}, 3},
-  CD_free{{"free"}, 1}, CD_realloc{{"realloc"}, 2},
-  CD_calloc{{"calloc"}, 2}, CD_valloc{{"valloc"}, 1},
-  CD_reallocf{{"reallocf"}, 2}, CD_strndup{{"strndup"}, 2},
-  CD_strdup{{"strdup"}, 1}, CD_win_strdup{{"_strdup"}, 1},
-  CD_kmalloc{{"kmalloc"}, 2}, CD_if_nameindex{{"if_nameindex"}, 1},
-  CD_if_freenameindex{{"if_freenameindex"}, 1}, CD_wcsdup{{"wcsdup"}, 1},
-  CD_win_wcsdup{{"_wcsdup"}, 1}, CD_kfree{{"kfree"}, 1},
-  CD_g_malloc{{"g_malloc"}, 1}, CD_g_malloc0{{"g_malloc0"}, 1},
-  CD_g_realloc{{"g_realloc"}, 2}, CD_g_try_malloc{{"g_try_malloc"}, 1},
-  CD_g_try_malloc0{{"g_try_malloc0"}, 1},
-  CD_g_try_realloc{{"g_try_realloc"}, 2}, CD_g_free{{"g_free"}, 1},
-  CD_g_memdup{{"g_memdup"}, 2}, CD_g_malloc_n{{"g_malloc_n"}, 2},
-  CD_g_malloc0_n{{"g_malloc0_n"}, 2}, CD_g_realloc_n{{"g_realloc_n"}, 3},
-  CD_g_try_malloc_n{{"g_try_malloc_n"}, 2},
-  CD_g_try_malloc0_n{{"g_try_malloc0_n"}, 2},
-  CD_g_try_realloc_n{{"g_try_realloc_n"}, 3};
-
-  bool isMemFunction(const CallEvent ) const;
-  bool isCMemFunction(const CallEvent ) const;
-  bool isCMemFreeFunction(const CallEvent ) const;
-  bool isCMemAllocFunction(const CallEvent ) const;
-};
-} // end of anonymous namespace
-
 /// Tells if the callee is one of the builtin new/delete operators, including
 /// placement operators and other standard overloads.
 static bool isStandardNewDelete(const FunctionDecl *FD);
@@ -327,7 +287,11 @@
  check::PreStmt, check::PostStmt,
  check::PostObjCMessage, check::Location, eval::Assume> {
 public:
-  MemFunctionInfoTy MemFunctionInfo;
+  /// In pessimistic mode, the checker assumes that it does not know which
+  /// functions might free the memory.
+  /// In optimistic mode, the checker assumes that all user-defined functions
+  /// which might free a pointer are annotated.
+  DefaultBool ShouldIncludeOwnershipAnnotatedFunctions;
 
   /// Many checkers are essentially built into this one, so enabling them will
   /// make MallocChecker perform additional modeling and reporting.
@@ -387,6 +351,81 @@
   mutable std::unique_ptr BT_OffsetFree[CK_NumCheckKinds];
   mutable std::unique_ptr BT_UseZerroAllocated[CK_NumCheckKinds];
 
+#define CHECK_FN(NAME)  

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Looks wonderful, thanks!




Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:355
+  template 
+  void checkRealloc(CheckerContext , const CallExpr *CE,
+ProgramStateRef State) const;

Charusso wrote:
> balazske wrote:
> > The `CHECK_FN` could be used even here?
> > ```
> > template 
> > CHECK_FN(checkRealloc)
> > ```
> I think about the opposite, passing around arguments by templates is not 
> cool. They meant to be arguments.
> 
> This type of callback simply could be a third `CallDescriptionMap` for future 
> profeness.
> passing around arguments by templates is not cool. They meant to be arguments.

+1! It's accidental that these values are currently known at compile time.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1184
+checkCXXNewOrCXXDelete(C, CE, State);
+
+  checkOwnershipAttr(C, CE, State);

balazske wrote:
> If these cases are exclusive the code looks better when `else` statements are 
> added?
Yup, or even better, `return`s, because this code is very fragile because it'll 
do horrible things if we accidentally invoke two callbacks at once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

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

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

In D68165#1902702 , @Charusso wrote:

> I wish for a third map, something like `ReallocationMap`. Other than that it 
> is a great direction, I love it. Thanks!


Hah, that is a neat idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Csaba Dabis via Phabricator via cfe-commits]

Charusso added a comment.

I wish for a third map, something like `ReallocationMap`. Other than that it is 
a great direction, I love it. Thanks!




Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:355
+  template 
+  void checkRealloc(CheckerContext , const CallExpr *CE,
+ProgramStateRef State) const;

balazske wrote:
> The `CHECK_FN` could be used even here?
> ```
> template 
> CHECK_FN(checkRealloc)
> ```
I think about the opposite, passing around arguments by templates is not cool. 
They meant to be arguments.

This type of callback simply could be a third `CallDescriptionMap` for future 
profeness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Balázs Kéri via Phabricator via cfe-commits]

balazske added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:355
+  template 
+  void checkRealloc(CheckerContext , const CallExpr *CE,
+ProgramStateRef State) const;

The `CHECK_FN` could be used even here?
```
template 
CHECK_FN(checkRealloc)
```



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1184
+checkCXXNewOrCXXDelete(C, CE, State);
+
+  checkOwnershipAttr(C, CE, State);

If these cases are exclusive the code looks better when `else` statements are 
added?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

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

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



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:378-379
+
+  using CheckFn = void (MallocChecker::*)(CheckerContext , const CallExpr 
*CE,
+  ProgramStateRef State) const;
+

NoQ wrote:
> baloghadamsoftware wrote:
> > Szelethus wrote:
> > > Szelethus wrote:
> > > > NoQ wrote:
> > > > > Whenever i see a (`CE`, `State`) pair, it screams `CallEvent` to me. 
> > > > > That said, i'm worried that `State` in these callbacks isn't 
> > > > > necessarily equal to `C.getState()` (the latter, by the way, is 
> > > > > always equal to the `CallEvent`'s `.getState()` - that's a relief, 
> > > > > right?), so if you'll ever be in the mood to check that, that'd be 
> > > > > great :)
> > > > It should be always equal to it. I'll change it.
> > > Hmmm, I tried making this take a (`CallEvent`, `CheckerContext`), but it 
> > > bloated the code for little gain, since every handler function would 
> > > start with the retrieval of the state and the call expression. That said, 
> > > I could cascade these down to `FreeMemAux`, `MallocMemAux`, etc, to also 
> > > take this pair instead, but I'll be honest, I don't see point until 
> > > something actually breaks.
> > This is the standard way in the checkers: almost every handler function 
> > starts with the retrieval of the state from the `CheckerContext`. The only 
> > reason for an extra `State` parameter is that sometimes we create more 
> > states in the lower level functions but only add the final one to the 
> > `CheckerContext` as a new transition. Does something like this happen here?
> I agree with @baloghadamsoftware here. If you pass `State` explicitly, it 
> looks like an indication for the reader that this `State` may be different 
> from `C.getState()`. If in practice they're the same, i'd rather do 
> `C.getState()` in every method than confuse the reader.
> 
> Also do you remember what makes us query `CallExpr` so often? Given that 
> `CallEvent` includes `CallExpr`, we should be able to expose everything we 
> need as `CallEvent` methods. Say, we should be able to replace 
> `MallocMemAux(C, CE, CE->getArg(0), ...)` with `MallocMemAux(C, Call, 
> Call.getArgExpr(0), ...)` and only do `Call.getOriginExpr()` once when we 
> report a bug.
I'll upload followups where I'm addressing these issues -- to keep things 
simple, I decided against increasing this patch's scope. I won't commit until 
those are also accepted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

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

Szelethus updated this revision to Diff 247512.
Szelethus added a reviewer: balazske.
Szelethus set the repository for this revision to rG LLVM Github Monorepo.
Szelethus added a comment.

Address inlines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165

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
@@ -61,6 +61,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/DynamicSize.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
@@ -92,8 +93,6 @@
   AF_InnerBuffer
 };
 
-struct MemFunctionInfoTy;
-
 } // end of anonymous namespace
 
 /// Print names of allocators and deallocators.
@@ -262,47 +261,6 @@
 
 REGISTER_MAP_WITH_PROGRAMSTATE(ReallocPairs, SymbolRef, ReallocPair)
 
-//===--===//
-// Kinds of memory operations, information about resource managing functions.
-//===--===//
-
-namespace {
-
-struct MemFunctionInfoTy {
-  /// The value of the MallocChecker:Optimistic is stored in this variable.
-  ///
-  /// In pessimistic mode, the checker assumes that it does not know which
-  /// functions might free the memory.
-  /// In optimistic mode, the checker assumes that all user-defined functions
-  /// which might free a pointer are annotated.
-  DefaultBool ShouldIncludeOwnershipAnnotatedFunctions;
-
-  CallDescription CD_alloca{{"alloca"}, 1}, CD_win_alloca{{"_alloca"}, 1},
-  CD_malloc{{"malloc"}, 1}, CD_BSD_malloc{{"malloc"}, 3},
-  CD_free{{"free"}, 1}, CD_realloc{{"realloc"}, 2},
-  CD_calloc{{"calloc"}, 2}, CD_valloc{{"valloc"}, 1},
-  CD_reallocf{{"reallocf"}, 2}, CD_strndup{{"strndup"}, 2},
-  CD_strdup{{"strdup"}, 1}, CD_win_strdup{{"_strdup"}, 1},
-  CD_kmalloc{{"kmalloc"}, 2}, CD_if_nameindex{{"if_nameindex"}, 1},
-  CD_if_freenameindex{{"if_freenameindex"}, 1}, CD_wcsdup{{"wcsdup"}, 1},
-  CD_win_wcsdup{{"_wcsdup"}, 1}, CD_kfree{{"kfree"}, 2},
-  CD_g_malloc{{"g_malloc"}, 1}, CD_g_malloc0{{"g_malloc0"}, 1},
-  CD_g_realloc{{"g_realloc"}, 2}, CD_g_try_malloc{{"g_try_malloc"}, 1},
-  CD_g_try_malloc0{{"g_try_malloc0"}, 1},
-  CD_g_try_realloc{{"g_try_realloc"}, 2}, CD_g_free{{"g_free"}, 1},
-  CD_g_memdup{{"g_memdup"}, 2}, CD_g_malloc_n{{"g_malloc_n"}, 2},
-  CD_g_malloc0_n{{"g_malloc0_n"}, 2}, CD_g_realloc_n{{"g_realloc_n"}, 3},
-  CD_g_try_malloc_n{{"g_try_malloc_n"}, 2},
-  CD_g_try_malloc0_n{{"g_try_malloc0_n"}, 2},
-  CD_g_try_realloc_n{{"g_try_realloc_n"}, 3};
-
-  bool isMemFunction(const CallEvent ) const;
-  bool isCMemFunction(const CallEvent ) const;
-  bool isCMemFreeFunction(const CallEvent ) const;
-  bool isCMemAllocFunction(const CallEvent ) const;
-};
-} // end of anonymous namespace
-
 /// Tells if the callee is one of the builtin new/delete operators, including
 /// placement operators and other standard overloads.
 static bool isStandardNewDelete(const FunctionDecl *FD);
@@ -326,7 +284,11 @@
  check::PreStmt, check::PostStmt,
  check::PostObjCMessage, check::Location, eval::Assume> {
 public:
-  MemFunctionInfoTy MemFunctionInfo;
+  /// In pessimistic mode, the checker assumes that it does not know which
+  /// functions might free the memory.
+  /// In optimistic mode, the checker assumes that all user-defined functions
+  /// which might free a pointer are annotated.
+  DefaultBool ShouldIncludeOwnershipAnnotatedFunctions;
 
   /// Many checkers are essentially built into this one, so enabling them will
   /// make MallocChecker perform additional modeling and reporting.
@@ -386,6 +348,74 @@
   mutable std::unique_ptr BT_OffsetFree[CK_NumCheckKinds];
   mutable std::unique_ptr BT_UseZerroAllocated[CK_NumCheckKinds];
 
+#define CHECK_FN(NAME) \
+  void NAME(CheckerContext , const CallExpr *CE, ProgramStateRef State) const;
+
+  template 
+  void checkRealloc(CheckerContext , const CallExpr *CE,
+ProgramStateRef State) const;
+
+  CHECK_FN(checkBasicAlloc)
+  CHECK_FN(checkKernelMalloc)
+  CHECK_FN(checkCalloc)
+  CHECK_FN(checkFree)
+  CHECK_FN(checkAlloca)
+  CHECK_FN(checkStrdup)
+  CHECK_FN(checkIfNameIndex)
+  CHECK_FN(checkIfFreeNameIndex)
+  CHECK_FN(checkCXXNewOrCXXDelete)
+  CHECK_FN(checkGMalloc0)
+  

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

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

Szelethus marked 5 inline comments as done.
Szelethus added a comment.
Herald added subscribers: martong, steakhal.

I've spent a lot of time on this code, and I don't think it'd be a great idea 
to do everything in a single step. Collapsing `CallExpr, ProgramState` into 
`CallEvent` is a shockingly a non-trivial task. While I'm happy to not commit 
this patch before uploading a solution to that problem, I'd prefer (and I think 
you would too!) to do them in followup patches.

The point of this revision was to drop in `CallDescriptionMap` -- I like to 
think that it introduces no new problems, it just doesn't solve many existing 
ones :^)

edit: I will soon address the inlines and other comments with an update


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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:865
+  if (FreeingMemFnMap.lookup(Call))
 return true;
 

baloghadamsoftware wrote:
> Maybe `return !!FreeingMemFnMap.lookup(Call)`?
I think the code changed after I wrote this comment, it is not valid anymore.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:879
+bool MallocChecker::isMemCall(const CallEvent ) const {
+  if (FreeingMemFnMap.lookup(Call) || NonFreeingMemFnMap.lookup(Call))
 return true;

baloghadamsoftware wrote:
> Same here: why not just `return FreeingMemFnMap.lookup(Call) || 
> NonFreeingMemFn.lookup(Call)`?
Here as well. Disregard these comments.


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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

I think my `strdup` comments weren't addressed >.<




Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:378-379
+
+  using CheckFn = void (MallocChecker::*)(CheckerContext , const CallExpr 
*CE,
+  ProgramStateRef State) const;
+

baloghadamsoftware wrote:
> Szelethus wrote:
> > Szelethus wrote:
> > > NoQ wrote:
> > > > Whenever i see a (`CE`, `State`) pair, it screams `CallEvent` to me. 
> > > > That said, i'm worried that `State` in these callbacks isn't 
> > > > necessarily equal to `C.getState()` (the latter, by the way, is always 
> > > > equal to the `CallEvent`'s `.getState()` - that's a relief, right?), so 
> > > > if you'll ever be in the mood to check that, that'd be great :)
> > > It should be always equal to it. I'll change it.
> > Hmmm, I tried making this take a (`CallEvent`, `CheckerContext`), but it 
> > bloated the code for little gain, since every handler function would start 
> > with the retrieval of the state and the call expression. That said, I could 
> > cascade these down to `FreeMemAux`, `MallocMemAux`, etc, to also take this 
> > pair instead, but I'll be honest, I don't see point until something 
> > actually breaks.
> This is the standard way in the checkers: almost every handler function 
> starts with the retrieval of the state from the `CheckerContext`. The only 
> reason for an extra `State` parameter is that sometimes we create more states 
> in the lower level functions but only add the final one to the 
> `CheckerContext` as a new transition. Does something like this happen here?
I agree with @baloghadamsoftware here. If you pass `State` explicitly, it looks 
like an indication for the reader that this `State` may be different from 
`C.getState()`. If in practice they're the same, i'd rather do `C.getState()` 
in every method than confuse the reader.

Also do you remember what makes us query `CallExpr` so often? Given that 
`CallEvent` includes `CallExpr`, we should be able to expose everything we need 
as `CallEvent` methods. Say, we should be able to replace `MallocMemAux(C, CE, 
CE->getArg(0), ...)` with `MallocMemAux(C, Call, Call.getArgExpr(0), ...)` and 
only do `Call.getOriginExpr()` once when we report a bug.


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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:378-379
+
+  using CheckFn = void (MallocChecker::*)(CheckerContext , const CallExpr 
*CE,
+  ProgramStateRef State) const;
+

Szelethus wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Whenever i see a (`CE`, `State`) pair, it screams `CallEvent` to me. That 
> > > said, i'm worried that `State` in these callbacks isn't necessarily equal 
> > > to `C.getState()` (the latter, by the way, is always equal to the 
> > > `CallEvent`'s `.getState()` - that's a relief, right?), so if you'll ever 
> > > be in the mood to check that, that'd be great :)
> > It should be always equal to it. I'll change it.
> Hmmm, I tried making this take a (`CallEvent`, `CheckerContext`), but it 
> bloated the code for little gain, since every handler function would start 
> with the retrieval of the state and the call expression. That said, I could 
> cascade these down to `FreeMemAux`, `MallocMemAux`, etc, to also take this 
> pair instead, but I'll be honest, I don't see point until something actually 
> breaks.
This is the standard way in the checkers: almost every handler function starts 
with the retrieval of the state from the `CheckerContext`. The only reason for 
an extra `State` parameter is that sometimes we create more states in the lower 
level functions but only add the final one to the `CheckerContext` as a new 
transition. Does something like this happen here?



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:865
+  if (FreeingMemFnMap.lookup(Call))
 return true;
 

Maybe `return !!FreeingMemFnMap.lookup(Call)`?



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:879
+bool MallocChecker::isMemCall(const CallEvent ) const {
+  if (FreeingMemFnMap.lookup(Call) || NonFreeingMemFnMap.lookup(Call))
 return true;

Same here: why not just `return FreeingMemFnMap.lookup(Call) || 
NonFreeingMemFn.lookup(Call)`?


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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

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

Szelethus added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1487
 
-  if (Att->getModule()->getName() !=
-  MemFunctionInfo.CD_malloc.getFunctionName())
+  if (Att->getModule()->getName() != "malloc")
 return nullptr;

NoQ wrote:
> Szelethus wrote:
> > Ugh, no idea whats happening here, I'll be honest. I sweat to dig a bit 
> > deeper to find out, though I didn't want to bother delaying the publishing 
> > this patch for a feature not documented, or used anywhere.
> I think this is about the (lack of) support for `__attribute__((malloc))`.
This should be an NFC change, and this part of the code will have to be 
rewritten anyways if we want a more serious annotation guided analysis.


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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

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

Szelethus updated this revision to Diff 222794.
Szelethus marked an inline comment as done.
Szelethus added a comment.

Rebase.


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

https://reviews.llvm.org/D68165

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
@@ -60,6 +60,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
@@ -91,8 +92,6 @@
   AF_InnerBuffer
 };
 
-struct MemFunctionInfoTy;
-
 } // end of anonymous namespace
 
 /// Print names of allocators and deallocators.
@@ -261,47 +260,6 @@
 
 REGISTER_MAP_WITH_PROGRAMSTATE(ReallocPairs, SymbolRef, ReallocPair)
 
-//===--===//
-// Kinds of memory operations, information about resource managing functions.
-//===--===//
-
-namespace {
-
-struct MemFunctionInfoTy {
-  /// The value of the MallocChecker:Optimistic is stored in this variable.
-  ///
-  /// In pessimistic mode, the checker assumes that it does not know which
-  /// functions might free the memory.
-  /// In optimistic mode, the checker assumes that all user-defined functions
-  /// which might free a pointer are annotated.
-  DefaultBool ShouldIncludeOwnershipAnnotatedFunctions;
-
-  CallDescription CD_alloca{{"alloca"}, 1}, CD_win_alloca{{"_alloca"}, 1},
-  CD_malloc{{"malloc"}, 1}, CD_BSD_malloc{{"malloc"}, 3},
-  CD_free{{"free"}, 1}, CD_realloc{{"realloc"}, 2},
-  CD_calloc{{"calloc"}, 2}, CD_valloc{{"valloc"}, 1},
-  CD_reallocf{{"reallocf"}, 2}, CD_strndup{{"strndup"}, 2},
-  CD_strdup{{"strdup"}, 1}, CD_win_strdup{{"_strdup"}, 1},
-  CD_kmalloc{{"kmalloc"}, 2}, CD_if_nameindex{{"if_nameindex"}, 1},
-  CD_if_freenameindex{{"if_freenameindex"}, 1}, CD_wcsdup{{"wcsdup"}, 1},
-  CD_win_wcsdup{{"_wcsdup"}, 1}, CD_kfree{{"kfree"}, 2},
-  CD_g_malloc{{"g_malloc"}, 1}, CD_g_malloc0{{"g_malloc0"}, 1},
-  CD_g_realloc{{"g_realloc"}, 2}, CD_g_try_malloc{{"g_try_malloc"}, 1},
-  CD_g_try_malloc0{{"g_try_malloc0"}, 1},
-  CD_g_try_realloc{{"g_try_realloc"}, 2}, CD_g_free{{"g_free"}, 1},
-  CD_g_memdup{{"g_memdup"}, 2}, CD_g_malloc_n{{"g_malloc_n"}, 2},
-  CD_g_malloc0_n{{"g_malloc0_n"}, 2}, CD_g_realloc_n{{"g_realloc_n"}, 3},
-  CD_g_try_malloc_n{{"g_try_malloc_n"}, 2},
-  CD_g_try_malloc0_n{{"g_try_malloc0_n"}, 2},
-  CD_g_try_realloc_n{{"g_try_realloc_n"}, 3};
-
-  bool isMemFunction(const CallEvent ) const;
-  bool isCMemFunction(const CallEvent ) const;
-  bool isCMemFreeFunction(const CallEvent ) const;
-  bool isCMemAllocFunction(const CallEvent ) const;
-};
-} // end of anonymous namespace
-
 /// Tells if the callee is one of the builtin new/delete operators, including
 /// placement operators and other standard overloads.
 static bool isStandardNewDelete(const FunctionDecl *FD);
@@ -325,7 +283,11 @@
  check::PreStmt, check::PostStmt,
  check::PostObjCMessage, check::Location, eval::Assume> {
 public:
-  MemFunctionInfoTy MemFunctionInfo;
+  /// In pessimistic mode, the checker assumes that it does not know which
+  /// functions might free the memory.
+  /// In optimistic mode, the checker assumes that all user-defined functions
+  /// which might free a pointer are annotated.
+  DefaultBool ShouldIncludeOwnershipAnnotatedFunctions;
 
   /// Many checkers are essentially built into this one, so enabling them will
   /// make MallocChecker perform additional modeling and reporting.
@@ -385,6 +347,74 @@
   mutable std::unique_ptr BT_OffsetFree[CK_NumCheckKinds];
   mutable std::unique_ptr BT_UseZerroAllocated[CK_NumCheckKinds];
 
+#define CHECK_FN(NAME) \
+  void NAME(CheckerContext , const CallExpr *CE, ProgramStateRef State) const;
+
+  template 
+  void checkRealloc(CheckerContext , const CallExpr *CE,
+ProgramStateRef State) const;
+
+  CHECK_FN(checkBasicAlloc)
+  CHECK_FN(checkKernelMalloc)
+  CHECK_FN(checkCalloc)
+  CHECK_FN(checkFree)
+  CHECK_FN(checkAlloca)
+  CHECK_FN(checkStrdup)
+  CHECK_FN(checkIfNameIndex)
+  CHECK_FN(checkIfFreeNameIndex)
+  CHECK_FN(checkCXXNewOrCXXDelete)
+  CHECK_FN(checkGMalloc0)
+  CHECK_FN(checkGMemdup)
+  CHECK_FN(checkGMallocN)
+  CHECK_FN(checkGMallocN0)
+  CHECK_FN(checkReallocN)
+  

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

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

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



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:378-379
+
+  using CheckFn = void (MallocChecker::*)(CheckerContext , const CallExpr 
*CE,
+  ProgramStateRef State) const;
+

Szelethus wrote:
> NoQ wrote:
> > Whenever i see a (`CE`, `State`) pair, it screams `CallEvent` to me. That 
> > said, i'm worried that `State` in these callbacks isn't necessarily equal 
> > to `C.getState()` (the latter, by the way, is always equal to the 
> > `CallEvent`'s `.getState()` - that's a relief, right?), so if you'll ever 
> > be in the mood to check that, that'd be great :)
> It should be always equal to it. I'll change it.
Hmmm, I tried making this take a (`CallEvent`, `CheckerContext`), but it 
bloated the code for little gain, since every handler function would start with 
the retrieval of the state and the call expression. That said, I could cascade 
these down to `FreeMemAux`, `MallocMemAux`, etc, to also take this pair 
instead, but I'll be honest, I don't see point until something actually breaks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:397-398
+  CallDescriptionMap NonFreeingMemFnMap{
+  {{"alloca", 1}, ::checkAlloca},
+  {{"_alloca", 1}, ::checkAlloca},
+  {{"malloc", 1}, ::checkMalloc},

Szelethus wrote:
> NoQ wrote:
> > I think `alloca` deserves `CDF_MaybeBuiltin`. This would also probably 
> > allow you to remove the second line.
> Actually, `BuiltinFunctionChecker` uses `evalCall` to create an 
> `AllocaRegion` for `__builtin_alloca`. I spent an hour when writing this 
> patch to track a crash down when I initially made this `CDF_MaybeBuiltin` :)
Oh misery.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

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

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



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:378-379
+
+  using CheckFn = void (MallocChecker::*)(CheckerContext , const CallExpr 
*CE,
+  ProgramStateRef State) const;
+

NoQ wrote:
> Whenever i see a (`CE`, `State`) pair, it screams `CallEvent` to me. That 
> said, i'm worried that `State` in these callbacks isn't necessarily equal to 
> `C.getState()` (the latter, by the way, is always equal to the `CallEvent`'s 
> `.getState()` - that's a relief, right?), so if you'll ever be in the mood to 
> check that, that'd be great :)
It should be always equal to it. I'll change it.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:397-398
+  CallDescriptionMap NonFreeingMemFnMap{
+  {{"alloca", 1}, ::checkAlloca},
+  {{"_alloca", 1}, ::checkAlloca},
+  {{"malloc", 1}, ::checkMalloc},

NoQ wrote:
> I think `alloca` deserves `CDF_MaybeBuiltin`. This would also probably allow 
> you to remove the second line.
Actually, `BuiltinFunctionChecker` uses `evalCall` to create an `AllocaRegion` 
for `__builtin_alloca`. I spent an hour when writing this patch to track a 
crash down when I initially made this `CDF_MaybeBuiltin` :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

[Artem Dergachev via Phabricator via cfe-commits]

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



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:378-379
+
+  using CheckFn = void (MallocChecker::*)(CheckerContext , const CallExpr 
*CE,
+  ProgramStateRef State) const;
+

Whenever i see a (`CE`, `State`) pair, it screams `CallEvent` to me. That said, 
i'm worried that `State` in these callbacks isn't necessarily equal to 
`C.getState()` (the latter, by the way, is always equal to the `CallEvent`'s 
`.getState()` - that's a relief, right?), so if you'll ever be in the mood to 
check that, that'd be great :)



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:397-398
+  CallDescriptionMap NonFreeingMemFnMap{
+  {{"alloca", 1}, ::checkAlloca},
+  {{"_alloca", 1}, ::checkAlloca},
+  {{"malloc", 1}, ::checkMalloc},

I think `alloca` deserves `CDF_MaybeBuiltin`. This would also probably allow 
you to remove the second line.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:402-404
+  {{"strndup", 2}, ::checkStrdup},
+  {{"strdup", 1}, ::checkStrdup},
+  {{"_strdup", 1}, ::checkStrdup},

These are also builtins.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:407-408
+  {{"if_nameindex", 1}, ::checkIfNameIndex},
+  {{"wcsdup", 1}, ::checkStrdup},
+  {{"_wcsdup", 1}, ::checkStrdup},
+  {{"g_malloc", 1}, ::checkMalloc},

And these too.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1487
 
-  if (Att->getModule()->getName() !=
-  MemFunctionInfo.CD_malloc.getFunctionName())
+  if (Att->getModule()->getName() != "malloc")
 return nullptr;

Szelethus wrote:
> Ugh, no idea whats happening here, I'll be honest. I sweat to dig a bit 
> deeper to find out, though I didn't want to bother delaying the publishing 
> this patch for a feature not documented, or used anywhere.
I think this is about the (lack of) support for `__attribute__((malloc))`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

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

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



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:890
 
-bool MemFunctionInfoTy::isCMemAllocFunction(const CallEvent ) const {
-  if (Call.isCalled(CD_malloc, CD_realloc, CD_reallocf, CD_calloc, CD_valloc,
-CD_strdup, CD_win_strdup, CD_strndup, CD_wcsdup,
-CD_win_wcsdup, CD_kmalloc, CD_g_malloc, CD_g_malloc0,
-CD_g_realloc, CD_g_try_malloc, CD_g_try_malloc0,
-CD_g_try_realloc, CD_g_memdup, CD_g_malloc_n,
-CD_g_malloc0_n, CD_g_realloc_n, CD_g_try_malloc_n,
-CD_g_try_malloc0_n, CD_g_try_realloc_n))
-return true;
-
-  if (Call.isCalled(CD_if_nameindex))
-return true;
-
-  if (Call.isCalled(CD_alloca, CD_win_alloca))
+bool MallocChecker::isMemCall(const CallEvent ) const {
+  if (FreeingMemFnMap.lookup(Call) || NonFreeingMemFnMap.lookup(Call))

It may be more precise to call this `isCMemCall`, because that's the question 
it answers.



Comment at: clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp:1487
 
-  if (Att->getModule()->getName() !=
-  MemFunctionInfo.CD_malloc.getFunctionName())
+  if (Att->getModule()->getName() != "malloc")
 return nullptr;

Ugh, no idea whats happening here, I'll be honest. I sweat to dig a bit deeper 
to find out, though I didn't want to bother delaying the publishing this patch 
for a feature not documented, or used anywhere.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68165



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

[PATCH] D68165: [analyzer][MallocChecker][NFC] Split checkPostCall up, deploy CallDescriptionMap

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

Szelethus created this revision.
Szelethus added reviewers: NoQ, xazax.hun, rnkovacs, Charusso, 
baloghadamsoftware, dcoughlin.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, gamesh411, dkrupp, donat.nagy, 
mikhail.ramalho, a.sidorin, szepet, whisperity.
Szelethus added a parent revision: D68163: [analyzer][MallocChecker][NFC] 
Change the use of IdentifierInfo* to CallDescription.

Since its important to know whether a function frees memory (even if its a 
reallocating function!), I used two `CallDescriptionMap`s to merge all 
`CallDescription`s into it. `MemFunctionInfoTy` no longer makes sense, it may 
never have, but for now, it would be more of a distraction then anything else.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68165

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
@@ -60,6 +60,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/ProgramStateTrait.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState_Fwd.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
@@ -91,8 +92,6 @@
   AF_InnerBuffer
 };
 
-struct MemFunctionInfoTy;
-
 } // end of anonymous namespace
 
 /// Print names of allocators and deallocators.
@@ -265,45 +264,6 @@
 // Kinds of memory operations, information about resource managing functions.
 //===--===//
 
-namespace {
-
-struct MemFunctionInfoTy {
-  /// The value of the MallocChecker:Optimistic is stored in this variable.
-  ///
-  /// In pessimistic mode, the checker assumes that it does not know which
-  /// functions might free the memory.
-  /// In optimistic mode, the checker assumes that all user-defined functions
-  /// which might free a pointer are annotated.
-  DefaultBool ShouldIncludeOwnershipAnnotatedFunctions;
-
-  CallDescription CD_alloca{{"alloca"}, 1}, CD_win_alloca{{"_alloca"}, 1},
-  CD_malloc{{"malloc"}, 1}, CD_free{{"free"}, 1},
-  CD_realloc{{"realloc"}, 2}, CD_calloc{{"calloc"}, 2},
-  CD_valloc{{"valloc"}, 1}, CD_reallocf{{"reallocf"}, 2},
-  CD_strndup{{"strndup"}, 2}, CD_strdup{{"strdup"}, 1},
-  CD_win_strdup{{"_strdup"}, 1}, CD_kmalloc{{"kmalloc"}, 2},
-  CD_if_nameindex{{"if_nameindex"}, 1},
-  CD_if_freenameindex{{"if_freenameindex"}, 1}, CD_wcsdup{{"wcsdup"}, 1},
-  CD_win_wcsdup{{"_wcsdup"}, 1}, CD_kfree{{"kfree"}, 2},
-  CD_g_malloc{{"g_malloc"}, 1}, CD_g_malloc0{{"g_malloc0"}, 1},
-  CD_g_realloc{{"g_realloc"}, 2}, CD_g_try_malloc{{"g_try_malloc"}, 1},
-  CD_g_try_malloc0{{"g_try_malloc0"}, 1},
-  CD_g_try_realloc{{"g_try_realloc"}, 2}, CD_g_free{{"g_free"}, 1},
-  CD_g_memdup{{"g_memdup"}, 2}, CD_g_malloc_n{{"g_malloc_n"}, 2},
-  CD_g_malloc0_n{{"g_malloc0_n"}, 2}, CD_g_realloc_n{{"g_realloc_n"}, 3},
-  CD_g_try_malloc_n{{"g_try_malloc_n"}, 2},
-  CD_g_try_malloc0_n{{"g_try_malloc0_n"}, 2},
-  CD_g_try_realloc_n{{"g_try_realloc_n"}, 3};
-
-  void initIdentifierInfo(ASTContext ) const;
-
-  bool isMemFunction(const CallEvent ) const;
-  bool isCMemFunction(const CallEvent ) const;
-  bool isCMemFreeFunction(const CallEvent ) const;
-  bool isCMemAllocFunction(const CallEvent ) const;
-};
-} // end of anonymous namespace
-
 /// Tells if the callee is one of the builtin new/delete operators, including
 /// placement operators and other standard overloads.
 static bool isStandardNewDelete(const FunctionDecl *FD);
@@ -327,7 +287,11 @@
  check::PreStmt, check::PostStmt,
  check::PostObjCMessage, check::Location, eval::Assume> {
 public:
-  MemFunctionInfoTy MemFunctionInfo;
+  /// In pessimistic mode, the checker assumes that it does not know which
+  /// functions might free the memory.
+  /// In optimistic mode, the checker assumes that all user-defined functions
+  /// which might free a pointer are annotated.
+  DefaultBool ShouldIncludeOwnershipAnnotatedFunctions;
 
   /// Many checkers are essentially built into this one, so enabling them will
   /// make MallocChecker perform additional modeling and reporting.
@@ -387,6 +351,74 @@
   mutable std::unique_ptr BT_OffsetFree[CK_NumCheckKinds];
   mutable std::unique_ptr BT_UseZerroAllocated[CK_NumCheckKinds];
 
+#define CHECK_FN(NAME) \
+  void NAME(CheckerContext , const CallExpr *CE, ProgramStateRef State) const;
+
+  template 
+  void checkRealloc(CheckerContext , const CallExpr *CE,
+ProgramStateRef State)