Re: _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h)

[enh <enh@google.com>]

On Wed, Aug 9, 2023 at 12:26 AM Martin Uecker <muecker@gwdg.de> wrote:
>
> Am Dienstag, dem 08.08.2023 um 17:14 -0700 schrieb enh:
> > (bionic maintainer here, mostly by accident...)
> >
> > yeah, we tried the GCC attributes once before with _disastrous_
> > results (because GCC saw it as an excuse to delete explicit null
> > checks, it broke all kinds of things).
>
> Thanks! I am currently exploring different options and what
> could be done to improve the situation from the language
> as well as compile side.  It looks we have some partial
> solution but they do not work quite right or do not
> fit together perfectly.
>
>
> >  the clang attributes are
> > "better" in that they don't confuse two entirely separate notions ...
> > but they're not "good" because the analysis is so limited. i think we
> > were hoping for something more like the "used but not set" kind of
> > diagnostic, but afaict it really only catches _direct_ use of a
> > literal nullptr. so:
> >
> >   foo(nullptr); // caught
> >
> >   void* p = nullptr;
> >   foo(p); // not caught
> >
> > without getting on to anything fancy like:
> >
> >   void* p;
> >   if (a) p = bar();
> >   foo(p);
> >
> > we've done all the annotations anyway, but we're not expecting to
> > actually catch many bugs with them, and in fact did not catch any real
> > bugs in AOSP while adding the annotations. (though we did find several
> > "you're not _wrong_, but ..." bits of code :-) )
> >
> > what i really want for christmas is the annotation that lets me say
> > "this size_t argument tells you the size of this array argument" and
> > actually does something usefully fortify-like with that.
> >
>
> What is your opinion about the access attribute?
>
> https://godbolt.org/z/PPfajefvP
>
> it is a bit cumbersome to use, but one can use [static]
> instead, which gives you the same static warnings.

yeah, "that's hard to read" was actually my first reaction. maybe it's
just because i'm a libc maintainer used to the printf_like attribute,
but i actually find the regular __attribute__() style more readable.
you probably need to ask people who _consume_ more headers than they
write :-)

> [static] does not work with __builtin_dynamic_object_size,
> but maybe this could be changed (there is a bug filed.)
>
> I am not sure whether [static] should imply [[gnu::nonnull]]
> which would then also trigger the optimization. I think
> clang uses it for optimization.

yeah, that was what made us revert the old gcc nonnull annotations ---
we don't have any use case for "please use this to omit checks"
because we're generally dealing with public API. having the compiler
dead-code eliminate our attempts to return sensible errors was counter
to our goals, and seems like it would be in any place where we'd use a
bounds annotation too --- it'll be those worried about security (who
want to fail fast, and _before_ adding a potentially caller-controlled
index to an array base address!) who i'd expect to see using this kind
of thing first.

> Martin
>
>
> >  i've seen
> > proposals like https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/
> > but i haven't seen anything i can use yet, so you -- who do use GCC
> > which aiui has something along these lines already -- will find out
> > what's good/bad about it before i do...
>
>
>
> >
> > On Tue, Aug 8, 2023 at 3:01 AM Martin Uecker <muecker@gwdg.de> wrote:
> > >
> > > Am Montag, dem 10.07.2023 um 22:16 +0200 schrieb Alejandro Colomar via Gcc:
> > > > On 7/10/23 22:14, Alejandro Colomar wrote:
> > > > > [CC += Andrew]
> > > > >
> > > > > Hi Xi, Andrew,
> > > > >
> > > > > On 7/10/23 20:41, Xi Ruoyao wrote:
> > > > > > Maybe we should have a weaker version of nonnull which only performs the
> > > > > > diagnostic, not the optimization.  But it looks like they hate the idea:
> > > > > > https://gcc.gnu.org/PR110617.
> > > > > >
> > > > > This is the one thing that makes me use both Clang and GCC to compile,
> > > > > because while any of them would be enough to build, I want as much
> > > > > static analysis as I can get, and so I want -fanalyzer (so I need GCC),
> > > > > but I also use _Nullable (so I need Clang).
> > > > >
> > > > > If GCC had support for _Nullable, I would have in GCC the superset of
> > > > > features that I need from both in a single vendor.  Moreover, Clang's
> > > > > static analyzer is brain-damaged (sorry, but it doesn't have a simple
> > > > > command line to run it, contrary to GCC's easy -fanalyzer), so having
> > > > > GCC's analyzer get over those _Nullable qualifiers would be great.
> > > > >
> > > > > Clang's _Nullable (and _Nonnull) are not very useful outside of analyzer
> > > > > mode, as there are many cases where the compiler doesn't have enough
> > > > > information, and the analyzer can get rid of false negatives and
> > > > > positives.  See: <https://github.com/llvm/llvm-project/issues/57546>
> > > > >
> > > > > I'll back the ask for the qualifiers in GCC, for compatibility with
> > > > > Clang.
> > > >
> > > > BTW, Bionic libc is adding those qualifiers:
> > > >
> > > > <https://android-review.googlesource.com/c/platform/bionic/+/2552567/7/libc/include/netinet/ether.h#45>
> > > > <https://android-review.googlesource.com/q/owner:zijunzhao@google.com+Nullability>
> > > >
> > > >
> > >
> > > I am sceptical about these qualifiers because they do
> > > not fit well with this language mechanism.   Qualifiers take
> > > effect at accesses to objects and are discarded at lvalue
> > > conversion.  So a qualifier should ideally describe a property
> > > that is relevant for accessing objects but is not relevant
> > > for values.
> > >
> > > Also there are built-in conversion rules a qualifier should
> > > conform to.  A pointer pointing to a type without qualifier
> > > can implicitely convert to a pointer to a type with qualifier,
> > > e.g.   int*  can be converted to const int*.
> > >
> > > Together, this implies that a qualifier should add constraints
> > > to a type that are relevant to how an object is accessed.
> > >
> > > "const" and "volatile" or "_Atomic" are good examples.
> > > ("restricted" does not quite follow these rules and is
> > > considered weird and difficult to understand.)
> > >
> > > In contrast, "nonnull" and "nullable" are properties that
> > > affect the set of values of the pointer, but not how the
> > > pointer itself is accessed.
> > >
> > >
> > > Martin
> > >
> > >
> > >
> > >
> > >
>