Re: [PATCH 2/6] Add returns_zero_on_success/failure attributes

[David Malcolm <dmalcolm@redhat.com>]

On Wed, 2021-11-17 at 14:53 +0530, Prathamesh Kulkarni wrote:
> On Tue, 16 Nov 2021 at 03:42, David Malcolm <dmalcolm@redhat.com>
> wrote:
> > 
> > On Mon, 2021-11-15 at 12:33 +0530, Prathamesh Kulkarni wrote:
> > > On Sun, 14 Nov 2021 at 02:07, David Malcolm via Gcc-patches
> > > <gcc-patches@gcc.gnu.org> wrote:
> > > > 
> > > > This patch adds two new attributes.  The followup patch makes
> > > > use of
> > > > the attributes in -fanalyzer.
> > 
> > [...snip...]
> > 
> > > > +/* Handle "returns_zero_on_failure" and
> > > > "returns_zero_on_success"
> > > > attributes;
> > > > +   arguments as in struct attribute_spec.handler.  */
> > > > +
> > > > +static tree
> > > > +handle_returns_zero_on_attributes (tree *node, tree name,
> > > > tree,
> > > > int,
> > > > +                                  bool *no_add_attrs)
> > > > +{
> > > > +  if (!INTEGRAL_TYPE_P (TREE_TYPE (*node)))
> > > > +    {
> > > > +      error ("%qE attribute on a function not returning an
> > > > integral type",
> > > > +            name);
> > > > +      *no_add_attrs = true;
> > > > +    }
> > > > +  return NULL_TREE;
> > > Hi David,
> > 
> > Thanks for the ideas.
> > 
> > > Just curious if a warning should be emitted if the function is
> > > marked
> > > with the attribute but it's return value isn't actually 0 ?
> > 
> > That sounds like a worthwhile extension of the idea.  It should be
> > possible to identify functions that can't return zero or non-zero
> > that
> > have been marked as being able to.
> > 
> > That said:
> > 
> > (a) if you apply the attribute to a function pointer for a
> > callback,
> > you could have an implementation of the callback that always fails
> > and
> > returns, say, -1; should the warning complain that the function has
> > the
> > "returns_zero_on_success" property and is always returning -1?
> Ah OK. In that case, emitting a diagnostic if the return value
> isn't 0, doesn't make sense for "returns_zero_on_success" since the
> function "always fails".
> Thanks for pointing out!
> > 
> > (b) the attributes introduce a concept of "success" vs "failure",
> > which
> > might be hard for a machine to determine.  It's only used later on
> > in
> > terms of the events presented to the user, so that -fanalyzer can
> > emit
> > e.g. "when 'copy_from_user' fails", which IMHO is easier to read
> > than
> > "when 'copy_from_user' returns non-zero".
> Indeed.
> > 
> > > 
> > > There are other constants like -1 or 1 that are often used to
> > > indicate
> > > error, so maybe tweak the attribute to
> > > take the integer as an argument ?
> > > Sth like returns_int_on_success(cst) /
> > > returns_int_on_failure(cst) ?
> > 
> > Those could work nicely; I like the idea of them being
> > supplementary to
> > the returns_zero_on_* ones.
> > 
> > I got the urge to bikeshed about wording; some ideas:
> >   success_return_value(CST)
> >   failure_return_value(CST)
> > or maybe additionally:
> >   success_return_range(LOWER_BOUND_CST, UPPER_BOUND_CST)
> >   failure_return_range(LOWER_BOUND_CST, UPPER_BOUND_CST)
> Extending to range is a nice idea ;-)
> Apart from success / failure, if we just had an attribute
> return_range(low_cst, high_cst), I suppose that could
> be useful for return value optimization ?

Perhaps.  All of this sounds like scope creep beyond my immediate
requirements though :)

> > 
> > I can also imagine a
> >   sets_errno_on_failure
> > attribute being useful (and perhaps a "doesnt_touch_errno"???)
> More generally, would it be a good idea to provide attributes for
> mod/ref anaylsis ?
> So sth like:
> void foo(void) __attribute__((modifies(errno)));
> which would state that foo modifies errno, but neither reads nor
> modifies any other global var.
> and
> void bar(void) __attribute__((reads(errno)))
> which would state that bar only reads errno, and doesn't modify or
> read any other global var.
> I guess that can benefit optimization, since we can have better
> context about side-effects of a function call.
> For success / failure context, we could add attributes
> modifies_on_success, modifies_on_failure ?

Likewise - sounds potentially useful, but I don't need this for this
kernel trust boundaries patch kit.

Dave

> 
> Thanks,
> Prathamesh
> > 
> > > Also, would it make sense to extend it for pointers too for
> > > returning
> > > NULL on success / failure ?
> > 
> > Possibly expressible by generalizing it to allow pointer types, or
> > by
> > adding this pair:
> > 
> >   returns_null_on_failure
> >   returns_null_on_success
> > 
> > or by using the "range" idea above.
> > 
> > In terms of scope, for the trust boundary stuff, I want to be able
> > to
> > express the idea that a call can succeed vs fail, what the success
> > vs
> > failure is in terms of nonzero vs zero, and to be able to wire up
> > the
> > heuristic that if it looks like a "copy function" (use of access
> > attributes and a size), that success/failure can mean "copies all
> > of
> > it" vs "copies none of it" (which seems to get decent test coverage
> > on
> > the Linux kernel with the copy_from/to_user fns).
> > 
> > Thanks
> > Dave
> > 
> > 
> > > 
> > > Thanks,
> > > Prathamesh
> > 
> > [...snip...]
> > 
>