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

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 2 inline comments as done.
balazske added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Checkers/Checkers.td:545
+  Dependencies<[ErrnoModeling]>,
+  Documentation<HasAlphaDocumentation>;
+
----------------
steakhal wrote:
> Then we should have documentation, and examples for it.
Yes this is missing. But currently the errno value is not set by any non-debug checker, if there is any real example in the documentation it will not work (until the function is modeled correctly). Adding errno support for the functions (or some of them) is a separate change, we can add documentation here and commit the changes together (as close as possible).


================
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:
> NoQ wrote:
> > 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()`?
> ah true
This assert is not needed, the errno value has always a place that is a `MemRegion` according to how the `ErrnoModeling` works.


================
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;
----------------
NoQ wrote:
> 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.
The "cached" errno values are removed, this function is not needed.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoModeling.h:24
 
+enum ErrnoCheckState : unsigned {
+  Errno_Irrelevant = 0,
----------------
steakhal wrote:
> Did you consider enum classes?
Originally it was enum class but I had to change it for some reason. Probably it did not have a default value.


================
Comment at: clang/test/Analysis/errno.c:170
+    clang_analyzer_eval(errno); // expected-warning{{TRUE}}
+    something();
+    clang_analyzer_eval(errno); // expected-warning{{UNKNOWN}}
----------------
steakhal wrote:
> So this is the case when `errno` gets indirectly invalidated by some opaque function call.  I see.
> However, it will test that the value associated with the errno region gets invalidated, that's fine. However, you should test if the metadata (the enum value) attached to memregion also gets invalidated.
> Please make sure you have tests for that as well.
> A `ErrnoTesterChecker_dumpCheckState()` should be perfect for this.
The check is now done indirectly by checking that no warning is produced.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122150