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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
