On Tue, 16 Nov 2021 at 03:42, David Malcolm <dmalc...@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 ?
>
> 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 ?
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...]
>
