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

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

balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Errno.h:22
+namespace ento {
+namespace errno_check {
+
----------------
steakhal wrote:
> I think we can settle on something better. What about calling it simply `errno`?
Just `errno` may not work because it collides with the "real" errno (that is a macro).


================
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.
These work now if no ErrnoRegion is available. Returning `Optional` seems not better than a required check before the call. I think the current version is not the best: It can be possible to make assumptions using `assume` on the returned `SVal` value, but this must not be done on a non-existing errno value. For this case probably `isErrnoAvailable` is required to be used.

The `isErrnoAvailable` can be useful for a checker that does special things only if there is a working errno modeling.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:35
+// FIXME: Check if there are other similar function names.
+llvm::StringRef ErrnoLocationFuncNames[] = {"__errno_location"};
+
----------------
steakhal wrote:
> `compiler-rt/lib/sanitizer_common/sanitizer_errno.h` has a list of the possible names for this API.:
> `__error`, `__errno`, `___errno`, `_errno` or `__errno_location`
> 
> That being said, you should `eval::Call` all of these, and bind the return value to the memorized errno region.
> At this point, this should be a `CallDescriptionSet` within the checker.
The list is needed to build `CallDescription`s and to lookup the errno location function. A `CallDescriptionSet` can not be used to get the function names from it. Is there a way to build a `initializer_list` of `CallDescription` objects in compile time (from a string array)?


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ErrnoChecker.cpp:99
+               FD->isExternC() && FD->getNumParams() == 0 &&
+               FD->getReturnType() == ACtx.getPointerType(ACtx.IntTy);
+      return false;
----------------
steakhal wrote:
> I don't think any of these actually return the canonic versions.
`ACtx.IntTy` and value from `getPointerType` should be canonical (it is a `CanQualType`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120310