[PATCH] D120310: [clang][analyzer] Add modeling of 'errno' (work-in-progress).

[Artem Dergachev via Phabricator via cfe-commits]

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