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