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

[David Malcolm <dmalcolm@redhat.com>]

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?

(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".

> 
> 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)

I can also imagine a
  sets_errno_on_failure
attribute being useful (and perhaps a "doesnt_touch_errno"???)

> 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...]