[PATCH] D120310: [clang][analyzer] Add modeling of 'errno'.

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

balazske marked an inline comment as done.
balazske added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/Errno.h:22
+namespace ento {
+namespace errno_check {
+
----------------
steakhal wrote:
> balazske wrote:
> > 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).
> Ah, I see. Ugly macros!
> Then why not simply leave it in the `clang::ento` namespace?
It is better to have separate namespaces for the different inter-checker API's. Probably more functions will be added later to the `errno_modeling`.


================
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:
> NoQ wrote:
> > balazske wrote:
> > > 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.
> > > 
> > `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.
> Yeah, probably the `Optional` is a better alternative.
> 
> However, I'd like to explain my reasoning behind my previous suggestion:
> `Unknown` can/should model any values that the analyzer cannot currently reason about: e.g. floating-point numbers.
> In this case, we cannot reason about the `errno`, thus I recommended it.
> 
> My point is, that we shouldn't regress our API for a marginal problem. And according to my reasoning, this seems to be a marginal issue.
> Using `Optional` here is better than the current implementation, but not by much.
> It would still result in a suboptimal coding pattern, where we guard each access to get/set `errno` by an `if` statement. In the cases where we would use the `getOr()` getter, I'm expecting to see the users pass the `UnknownVal()` as the fallback value in most cases anyway.
I have removed now `isErrnoAvailable` and the get function returns `Optional`. I am not sure if this is the best option, if more functions will be added (to get the errno location, and get and set an "errno state").


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120310