[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Nice work!



================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:159
+// Note: This pointer has type 'const MemRegion *'
+REGISTER_TRAIT_WITH_PROGRAMSTATE(EnvPtrRegion, const void *)
+
----------------
Why is it `const void *`? Why can't we use `const MemRegion *` and get rid of the ugly reinterpret_cast?  There must be a reason ,which I'd like to see documented in the comments. 


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:163
+// memory region returned by previous call of this function
+REGISTER_MAP_WITH_PROGRAMSTATE(PreviousCallResultMap, const char *,
+                               const MemRegion *)
----------------
I think we could use the canonical `FunctionDecl*` as the key instead of `const char *`. Then all the `eval` functions like `evalGetenv` and alike could be substituted with one simple function.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:221-224
+  const auto *SymRegOfRetVal = dyn_cast<SymbolicRegion>(RetVal.getAsRegion());
+  State = State->set<PreviousCallResultMap>(
+      FunctionName.data(),
+      const_cast<MemRegion *>(SymRegOfRetVal->getBaseRegion()));
----------------
Would it be possible to add a `NoteTag` here and eliminate the `PreviousCallVisitor`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:342
+
+void InvalidPtrChecker::checkDeadSymbols(SymbolReaper &SymReaper,
+                                         CheckerContext &C) const {
----------------
I think we should delete the code of this callback, since we can't use it.


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

https://reviews.llvm.org/D97699