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