[PATCH] D84316: [analyzer][NFC] Split CStringChecker to modeling and reporting

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

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

Yay! This checker has become a major headache as the analyzer grew. Not on a RetainCount scale, but on a MallocChecker one. Cheap shots, I know :)

The patch looks great! I have some miscellaneous comments, but nothing blocking.

>> I don't think i understand having `unix.cstring.CStringChecker` as one more entry in `Checkers.td`. Do you expect there to be a situation when enabling `CStringModeling` without `CStringChecker` actually makes sense?
> 
> You seem to be right.
>  Enabling only the cstring modeling part does not make much sense without enabling //at least// the CStringChecker to model the cstring manipulation functions - even if the reporting is disabled of such functions.
> 
>> If not, why not keep them agglutinated?
> 
> I wanted to have a separate class for bookkeeping while minimalizing the necessary changes.
>  What do you think would be the best way to organize this separation?
> 
> Few notes:
> 
> - Checkers are implemented in the anonymous namespace, so only the given file has access to them.
> - I wanted to separate the bookkeeping logic from the reporting/function modeling logic in different files.
> - I like the fact that after the change the CStringChecker implements only the evalCall checker callback.
> 
>   Let me know if I misunderstood something.

Mind that that entry does a lot more then give a flag to the user -- It generates code for a lot of the checker machinery as well. Since CStringModeling still uses the checker callbacks to set up the proper string length, it is a necessity. (strong) Checker dependencies are the exact solution to the the problem where a checker cannot run without another (as I understand it, its not only about making sense, you really cant make CStringChecker work without CStringModeling).



================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:429
 
 def CStringModeling : Checker<"CStringModeling">,
+  HelpText<"Responsible for essential modeling of cstring lengths. "
----------------
What other aspects of c strings needs to be modeled? Is it only length? If so, how about we rename the checker to `CStringLengthModeling`?


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:495
   ]>,
-  Dependencies<[CStringModeling]>,
+  Dependencies<[CStringChecker]>,
   Documentation<NotDocumented>,
----------------
I dug around a bit, and found this commit as to why this was needed: rGe56167e8f87acf87a9de3d383752e18a738cf056. So this dependency is appropriate.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringChecker.cpp:74
 enum class ConcatFnKind { none = 0, strcat = 1, strlcat = 2 };
-class CStringChecker : public Checker< eval::Call,
-                                         check::PreStmt<DeclStmt>,
-                                         check::LiveSymbols,
-                                         check::DeadSymbols,
-                                         check::RegionChanges
-                                         > {
+class CStringChecker : public Checker<eval::Call> {
   mutable std::unique_ptr<BugType> BT_Null, BT_Bounds, BT_Overlap,
----------------
This is somewhat orthogonal to the patch, but shouldn't precondition violations be reported at `preCall`?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/CStringLength.cpp:175-181
+void ento::registerCStringModeling(CheckerManager &Mgr) {
+  Mgr.registerChecker<CStringModeling>();
+}
+
+bool ento::shouldRegisterCStringModeling(const CheckerManager &) {
+  return true;
+}
----------------
We traditionally put these on the bottom of the file -- I don't think this would upset the structure too much :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84316