NoQ added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:47-49
+ mutable bool ErrnoInitialized = false;
+ mutable Optional<Loc> ErrnoLoc;
+ mutable const MemRegion *ErrnoRegion = nullptr;
----------------
steakhal wrote:
> One should not define member variables like this.
> The analysis engine might explore exploded nodes in an unpredictable order,
> invoking the checker's handler callbacks in an indeterminate order.
> You should have registered simple value traits for this purpose, associating
> the necessary information with a given State.
In this case it's probably fine given that the variable doesn't really change
throughout a single ExplodedGraph construction. But I agree that it doesn't
really make sense to cache this data; `getErrnoLoc()` an O(1) lookup anyway.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:68-71
+ if (ErrnoLoc) {
+ ErrnoRegion = ErrnoLoc->getAsRegion();
+ assert(ErrnoRegion && "The 'errno' location should be a memory region.");
+ }
----------------
steakhal wrote:
> `Loc` always wrap a memregion of some sort.
Nope; null pointer is a `Loc` but doesn't correspond to any memory region.
I usually print out doxygen inheritance graphs:
- https://clang.llvm.org/doxygen/classclang_1_1ento_1_1SVal__inherit__graph.png
-
https://clang.llvm.org/doxygen/classclang_1_1ento_1_1SymExpr__inherit__graph.png
-
https://clang.llvm.org/doxygen/classclang_1_1ento_1_1MemRegion__inherit__graph.png
and hang them on the wall near my desk. They're really handy.
On a separate note, why not make this assertion part of `getErrnoLoc()`?
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:93
+ OS << CallD->getIdentifier()->getName();
+ OS << "' might overwrite value of 'errno'";
+ PathDiagnosticLocation Loc{
----------------
"May" is more neutral.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:97
+ C.getSourceManager()};
+ BR->addNote(OS.str(), Loc, {CallMayChangeErrno->getSourceRange()});
+ }
----------------
This path-insensitive note is attached to the exact same source location as the
warning, right? I think this should be part of the warning instead. Something
like this:
```lang=c
foo(); // (1) path note: Call to 'foo()' indicates failure only by setting
'errno'
bar(); // (2) warning: Value of 'errno' potentially overwritten by 'bar()' was
not checked
```
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:104
+void ErrnoChecker::checkBeginFunction(CheckerContext &C) const {
+ // The errno location must be refreshed at every new function.
+ ErrnoInitialized = false;
----------------
Note that `checkBeginFunction()` is every time a nested stack frame is entered.
This happens much more often than an update to the variable is needed.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122150/new/
https://reviews.llvm.org/D122150
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
