NoQ added inline comments.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:767-768
s->getType()->isBlockPointerType());
- assert(isa<UnknownSpaceRegion>(sreg) || isa<HeapSpaceRegion>(sreg));
+ assert(isa<UnknownSpaceRegion>(sreg) || isa<HeapSpaceRegion>(sreg) ||
+ isa<GlobalSystemSpaceRegion>(sreg));
}
----------------
steakhal wrote:
> At this point, I'm not even sure if we should assert any of these.
I mean, ideally this should be just `UnknownSpaceRegion`. For everything else
we should have made a fresh `MemRegion` class. It is absurd that the same
numeric address value (represented the symbol `s`) can correspond to two (now
three) different locations in memory.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/Errno.h:24-26
+/// Returns if modeling of 'errno' is available.
+/// If not, the other functions here should not be used.
+bool isErrnoAvailable(ProgramStateRef State);
----------------
steakhal wrote:
> I don't think we need this.
> In `getErrnoValue()` we should simply return `UnknownVal` if we don't have
> 'errno.
> And in `setErrnoValue()` we should return the incoming `State` unmodified.
>
> THat being said, only a top-level `Check::BeginFunction` callback could see
> an 'errno' uninitialized, which I don't think is a real issue.
> All the rest of the callbacks would run after it's initialized, thus would
> behave as expected.
> And in case the translation unit doesn't have nor 'errno' variable nor
> 'errno_location_ish' functions, ignoring these `set/get` functions is
> actually the expected behavior.
>
> Having to check `isErrnoAvailable()` would be really an anti-pattern.
`UnknownVal` implies that it's an actual value but we don't know which one. If
the value doesn't exist we shouldn't use it. And if the user doesn't include
the appropriate header then the value really doesn't exist in the translation
unit. So flavor-wise I think we shouldn't use `UnknownVal` here; I'd rather
have an `Optional<SVal>`.
Other than that, yeah, I think this is a good suggestion.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:32
+// FIXME: Is there a system where it is not called "errno" but is a variable?
+const char *ErrnoVarName = "errno";
+// Names of functions that return a location of the "errno" value.
----------------
steakhal wrote:
> Why don't you use `StringRef` here as well?
There's [[
https://llvm.org/docs/CodingStandards.html#do-not-use-static-constructors |
that rule against static constructors/destructors ]]. We should use a lot of
`constexpr` in these situations and/or try to turn these structures into plain
C arrays as much as possible.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:66
+
+void ErrnoChecker::checkBeginFunction(CheckerContext &C) const {
+ if (!C.inTopFrame())
----------------
steakhal wrote:
> I would expect a comment stating that:
> - We inspect if we have a VarDecl naming "errno", it returns that Decl.
> - Otherwise, it will look for some common `errno_location` function names and
> return that Decl.
Ok so the first part of the function (identifying how the system headers
represent `errno`) depends entirely on the AST right? In this case you can
probably perform the AST scan only once during
`checkASTDecl<TranslationUnitDecl>` and stash the result in a global variable.
It shouldn't be re-run on every analysis. So whatever you do with
`isErrnoAvailable()`, it shouldn't accept `ProgramStateRef` at all, it can
simply query that global variable. You'll still need the state trait because
`SVal` objects only make sense within a single analysis but the AST scan can be
made only once.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:134
+ const SymbolConjured *Sym =
+ SVB.conjureSymbol(nullptr, C.getLocationContext(), ACtx.IntTy, 1);
+
----------------
steakhal wrote:
>
This is literally the first step of the analysis. The block count is //known//.
What I really want to see here is a non-trivial custom tag (i.e., the `const
void *symbolTag` parameter on some of the overloads of `conjureSymbolVal()`) to
make sure this symbol isn't treated as a duplicate of any other symbol some
other checker conjures initially. Like all tags, you can initialize it with
some address of some checker-static variable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D120310/new/
https://reviews.llvm.org/D120310
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
