[PATCH] D97699: [analyzer] Add InvalidPtrChecker

[Zurab Tsinadze via Phabricator via cfe-commits]

zukatsinadze marked an inline comment as done and an inline comment as not done.
zukatsinadze added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:947
+
+} // end "alpha.cert.env"
+
----------------
balazske wrote:
> I have multiple issues with the documentation texts but they look not final anyway. And the package and checker names could be better. There are already checkers for CERT rules that do not reside in the cert package, it is not good to have some checkers in a `cert` package and some not. It is likely that checkers will check for more rules or parts of the rules. Checker aliases are a better solution for the cert checker names. (And now the `cert` would contain checker with name not matching a rule: `InvalidPtr`.) The name `InvalidPtr` sounds too general, even if in an `env˙ package.
Agreed. We can come back to wordsmithing later.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:227
+  if (const auto *Reg =
+          State->get<PreviousCallResultMap>(FunctionName.data())) {
+    const auto *PrevReg = *Reg;
----------------
balazske wrote:
> balazske wrote:
> > `auto` is not to be used if the type is not clearly visible in the context. And this would be exactly `auto **` here. (But better is `const MemRegion **`.) Makes the later statement (assign to `PrevReg`) "messy" (and `auto` is bad there too).
> `.data` is unnecessary here?
`.data` seems to be necessary. 

`no known conversion from 'llvm::StringRef' to 'typename ProgramStateTrait<PreviousCallResultMap>::key_type' (aka 'const char *') for 1st argument`


================
Comment at: clang/lib/StaticAnalyzer/Checkers/cert/InvalidPtrChecker.cpp:341
+  State = State->set<EnvPtrRegion>(
+      reinterpret_cast<void *>(const_cast<MemRegion *>(EnvpReg)));
+  C.addTransition(State);
----------------
balazske wrote:
> Are these casts really needed?
Yes, as steakhal mentioned above, trait is specialized for void*


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

https://reviews.llvm.org/D97699