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