[PATCH] D45458: MallocChecker refactoring of calls checkers

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D45458#1062342, @NoQ wrote:

> @xazax.hun: do you think the approach you took in the `Valist` checker is 
> applicable here? Did you like how it ended up working? Cause i'd love to see 
> `CallDescription` and initializer lists used for readability here. And i'd 
> love to see branches replaced with map lookups. And i'm not sure if anybody 
> has written code that has all the stuff that i'd love to see.


Overall the `CallDescription` interface works really well if you do not need 
anything more advanced, e.g. supporting overloads. I can imagine that its 
current implementation is slightly less efficient since the lazy initialization 
logic of the identifiers are repeated for every identifier but that also makes 
it less bug-prone (do not have the implicit assumption that a set of 
identifiers should always exist together).

It would be great to extend the interface in the future, but one of the reasons 
I was reluctant to do so yet is performance. E.g.: having a simple check that a 
checker is interested only in global C functions is better than having this 
check repeated in every call description object in an array, or making it 
possible to do a StringSwitch like optimization with multiple statically known 
CallDescription objects would also be awesome.

But I think most of the time these tests would not be used on the hot paths 
anyways, so as long its current expressive power is sufficient, I prefer code 
that is utilizing this tool. It tends to make the code slightly shorter.


Repository:
  rC Clang

https://reviews.llvm.org/D45458



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

[PATCH] D45458: MallocChecker refactoring of calls checkers

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a reviewer: xazax.hun.
NoQ added a subscriber: xazax.hun.
NoQ added a comment.
Herald added a subscriber: rnkovacs.

@xazax.hun: do you think the approach you took in the `Valist` checker is 
applicable here? Did you like how it ended up working? Cause i'd love to see 
`CallDescription` and initializer lists used for readability here. And i'd love 
to see branches replaced with map lookups. And i'm not sure if anybody has 
written code that has all the stuff that i'd love to see.


Repository:
  rC Clang

https://reviews.llvm.org/D45458



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

[PATCH] D45458: MallocChecker refactoring of calls checkers

[David CARLIER via Phabricator via cfe-commits]

devnexen added a comment.

Splitting from https://reviews.llvm.org/D45149 as requested.


Repository:
  rC Clang

https://reviews.llvm.org/D45458



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

[PATCH] D45458: MallocChecker refactoring of calls checkers

[David CARLIER via Phabricator via cfe-commits]

devnexen created this revision.
devnexen added reviewers: NoQ, thakis.
Herald added a subscriber: cfe-commits.

- Splitting some by family to reduce the list of function identifiers.


Repository:
  rC Clang

https://reviews.llvm.org/D45458

Files:
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Index: lib/StaticAnalyzer/Checkers/MallocChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -171,18 +171,14 @@
 {
 public:
   MallocChecker()
-  : 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), II_if_freenameindex(nullptr),
-II_wcsdup(nullptr), II_win_wcsdup(nullptr), II_g_malloc(nullptr),
-II_g_malloc0(nullptr), II_g_realloc(nullptr), II_g_try_malloc(nullptr), 
-II_g_try_malloc0(nullptr), II_g_try_realloc(nullptr), 
-II_g_free(nullptr), II_g_memdup(nullptr), II_g_malloc_n(nullptr), 
-II_g_malloc0_n(nullptr), II_g_realloc_n(nullptr), 
-II_g_try_malloc_n(nullptr), II_g_try_malloc0_n(nullptr), 
-II_g_try_realloc_n(nullptr) {}
+  : II_kmalloc(nullptr), II_if_nameindex(nullptr),
+II_if_freenameindex(nullptr), II_valloc(nullptr),
+II_realloc(nullptr), II_calloc(nullptr),
+II_reallocarray(nullptr), II_recallocarray(nullptr),
+II_strndup(nullptr), II_g_memdup(nullptr),
+II_g_malloc0(nullptr), II_g_try_malloc0(nullptr),
+II_g_malloc0_n(nullptr), II_g_try_malloc0_n(nullptr),
+II_reallocf(nullptr), II_malloc(nullptr) {}
 
   /// In pessimistic mode, the checker assumes that it does not know which
   /// functions might free the memory.
@@ -242,18 +238,27 @@
   mutable std::unique_ptr BT_MismatchedDealloc;
   mutable std::unique_ptr BT_OffsetFree[CK_NumCheckKinds];
   mutable std::unique_ptr BT_UseZerroAllocated[CK_NumCheckKinds];
-  mutable IdentifierInfo *II_alloca, *II_win_alloca, *II_malloc, *II_free,
- *II_realloc, *II_calloc, *II_valloc, *II_reallocf,
- *II_strndup, *II_strdup, *II_win_strdup, *II_kmalloc,
- *II_if_nameindex, *II_if_freenameindex, *II_wcsdup,
- *II_win_wcsdup, *II_g_malloc, *II_g_malloc0, 
- *II_g_realloc, *II_g_try_malloc, *II_g_try_malloc0, 
- *II_g_try_realloc, *II_g_free, *II_g_memdup, 
- *II_g_malloc_n, *II_g_malloc0_n, *II_g_realloc_n, 
- *II_g_try_malloc_n, *II_g_try_malloc0_n, 
- *II_g_try_realloc_n;
   mutable Optional KernelZeroFlagVal;
 
+  mutable llvm::SmallSet AllocFunctions;
+  mutable llvm::SmallSet FreeFunctions;
+
+  mutable llvm::SmallSet AllocaFunctions;
+  mutable llvm::SmallSet SimpleMallocFunctions;
+
+  mutable llvm::SmallSet NZeroMallocFunctions;
+
+  mutable llvm::SmallSet NReallocFunctions;
+  mutable llvm::SmallSet ReallocFunctions;
+
+  mutable llvm::SmallSet StrdupFunctions;
+
+  mutable IdentifierInfo *II_kmalloc, *II_if_nameindex, *II_if_freenameindex,
+ *II_valloc, *II_realloc, *II_calloc, *II_reallocarray,
+ *II_recallocarray, *II_strndup, *II_g_memdup,
+ *II_g_malloc0, *II_g_try_malloc0, *II_g_malloc0_n,
+ *II_g_try_malloc0_n, *II_reallocf, *II_malloc;
+
   void initIdentifierInfo(ASTContext &C) const;
 
   /// \brief Determine family of a deallocation expression.
@@ -345,7 +350,8 @@
   ProgramStateRef ReallocMemAux(CheckerContext &C, const CallExpr *CE,
 bool FreesMemOnFailure,
 ProgramStateRef State, 
-bool SuffixWithN = false) const;
+bool SuffixWithN = false,
+bool ZeroVal = false) const;
   static SVal evalMulForBufferSize(CheckerContext &C, const Expr *Blocks,
const Expr *BlockBytes);
   static ProgramStateRef CallocMem(CheckerContext &C, const CallExpr *CE,
@@ -575,42 +581,86 @@
 } // end anonymous namespace
 
 void MallocChecker::initIdentifierInfo(ASTContext &Ctx) const {
-  if (II_malloc)
+  if (!AllocFunctions.empty())
 return;
-  II_alloca = &Ctx.Idents.get("alloca");
-  II_malloc = &Ctx.Idents.get("malloc");
-  II_free = &Ctx.Idents.get("free");
-  II_realloc = &Ctx.Idents.get("realloc");
-  II_reallocf = &Ctx.Idents.get("reallocf");
-  II_calloc = &Ctx.Idents.get("calloc");
-  II_valloc = &Ctx.Idents.get("valloc");
-  II_strdup = &Ctx.Idents.get("strdup");
-  II_strndup = &Ctx.Idents.get("strndup");
-  II_wcsdup = &Ctx.Idents.get("wcsdup");
-