[PATCH] D122150: [clang][analyzer] Add checker for bad use of 'errno'.

[Artem Dergachev via Phabricator via cfe-commits]

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