steakhal added a comment.
It looks everything fine, given that the `alpha.unix.Errno` will depend on the
`apimodeling.StdCLibraryFunctions`.
This should do the expected errno state propagation by which the `Errno`
checker can report diagnostics - WHILE the `StdCLibraryFunctions` still won't
report diagnostics.
This means that the Errno part /diagnostics can be separately disabled from the
diagnostics of `StdCLibraryFunctionArgsChecker` and other frontend checkers of
the `StdCLibraryFunctionsChecker` modeling part.
Excellent.
Please demonstrate that indeed disabling separately `Errno` or
`StdCLibraryFunctionArgsChecker` will make the respective reports disappear.
---
I can see that in many cases you introduced an additional `.Case()`, which will
result in a state-split.
I'm curious what runtime implications this change introduces. Please measure
this.
I must also admit that I haven't checked the parameters of `Case()` in all
cases; so I would encourage you to double-check those to make sure they are in
line with the API specification.
Given these changes are implemented, I think it's ready to land. Awesome job,
really!
Sorry about reject, I just saw it's green and I still had some thoughts about
this.
================
Comment at:
clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:68-69
+ return "may be undefined after the call and should not be used";
+ };
+}
+
----------------
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:444
+ DefinedOrUnknownSVal Cond =
+ SVB.evalBinOpNN(State, Op, ErrnoSVal, ZeroVal,
SVB.getConditionType())
+ .castAs<DefinedOrUnknownSVal>();
----------------
nit: we should not use the `NN` and other suffixed versions of this API. The
generic one should suffice.
Actually, I plan to refactor all code and make those private.
================
Comment at: clang/test/Analysis/errno-stdlibraryfunctions-notes.c:15-23
+void test1() {
+ access("path", 0); // no note here
+ access("path", 0);
+ // expected-note@-1{{Assuming that function 'access' is successful, in this
case the value 'errno' may be undefined after the call and should not be used}}
+ if (errno != 0) {
+ // expected-warning@-1{{An undefined value may be read from 'errno'}}
+ // expected-note@-2{{An undefined value may be read from 'errno'}}
----------------
martong wrote:
> balazske wrote:
> > steakhal wrote:
> > > I must say that I dislike that one cannot disable this warning unless
> > > they disable the whole stdlibrarymodeling. It's getting bigger and
> > > bigger, thus increasing the value it provides. Yet, not everybody wants
> > > to follow SEI CERT guidelines - and I would expect plenty of users in
> > > that category, who might be surprised that reading `errno` is a bug.
> > >
> > > Just imagine in the example that between the two `access()` calls, you
> > > set `errno` to zero.
> > > The code would look reasonable.
> > > 1) reset errno
> > > 2) do some syscall
> > > 3) check errno
> > >
> > > I would expect this to be a good practice. We should at least have an
> > > option for suppressing reports for such scenarios. IMO it should be
> > > rather opt-in, than opt-out, but it might be my personal preference.
> > > WDYT @martong @xazax.hun
> > The CERT rule says that there is no guarantee that `errno` is not changed
> > if the function is successful, regardless if it was 0 or not. There are
> > some functions (that have //in-band return value//) where it is required to
> > set `errno` to 0, there can be another checker (or extend `ErrnoChecker`)
> > to check this.
> >
> > The new warning is turned on when `alpha.unix.Errno` is turned on,
> > otherwise it should not appear.
> > I must say that I dislike that one cannot disable this warning unless they
> > disable the whole stdlibrarymodeling.
>
> Yeah, good point. Perhaps we should have a new checker option that
> enables/disables errno related reports. Should this be part of the
> ErrnoChecker (or the StdLibraryFunctionsChecker)?
> The new warning is turned on when alpha.unix.Errno is turned on, otherwise it
> should not appear.
Oh, I see. I might overreacted this part.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125400/new/
https://reviews.llvm.org/D125400
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
