[2/6] Add returns_zero_on_success/failure attributes

This patch adds two new attributes.  The followup patch makes use of
the attributes in -fanalyzer.

gcc/c-family/ChangeLog:
	* c-attribs.c (attr_noreturn_exclusions): Add
	"returns_zero_on_failure" and "returns_zero_on_success".
	(attr_returns_twice_exclusions): Likewise.
	(attr_returns_zero_on_exclusions): New.
	(c_common_attribute_table): Add "returns_zero_on_failure" and
	"returns_zero_on_success".
	(handle_returns_zero_on_attributes): New.

gcc/ChangeLog:
	* doc/extend.texi (Common Function Attributes): Document
	"returns_zero_on_failure" and "returns_zero_on_success".

gcc/testsuite/ChangeLog:
	* c-c++-common/attr-returns-zero-on-1.c: New test.

Signed-off-by: David Malcolm <dmalcolm@redhat.com>
---
 gcc/c-family/c-attribs.c                      | 37 ++++++++++
 gcc/doc/extend.texi                           | 16 +++++
 .../c-c++-common/attr-returns-zero-on-1.c     | 68 +++++++++++++++++++
 3 files changed, 121 insertions(+)
 create mode 100644 gcc/testsuite/c-c++-common/attr-returns-zero-on-1.c

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.
>
> gcc/c-family/ChangeLog:
>         * c-attribs.c (attr_noreturn_exclusions): Add
>         "returns_zero_on_failure" and "returns_zero_on_success".
>         (attr_returns_twice_exclusions): Likewise.
>         (attr_returns_zero_on_exclusions): New.
>         (c_common_attribute_table): Add "returns_zero_on_failure" and
>         "returns_zero_on_success".
>         (handle_returns_zero_on_attributes): New.
>
> gcc/ChangeLog:
>         * doc/extend.texi (Common Function Attributes): Document
>         "returns_zero_on_failure" and "returns_zero_on_success".
>
> gcc/testsuite/ChangeLog:
>         * c-c++-common/attr-returns-zero-on-1.c: New test.
>
> Signed-off-by: David Malcolm <dmalcolm@redhat.com>
> ---
>  gcc/c-family/c-attribs.c                      | 37 ++++++++++
>  gcc/doc/extend.texi                           | 16 +++++
>  .../c-c++-common/attr-returns-zero-on-1.c     | 68 +++++++++++++++++++
>  3 files changed, 121 insertions(+)
>  create mode 100644 gcc/testsuite/c-c++-common/attr-returns-zero-on-1.c
>
> diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
> index 100c2dabab2..9e03156de5e 100644
> --- a/gcc/c-family/c-attribs.c
> +++ b/gcc/c-family/c-attribs.c
> @@ -153,6 +153,7 @@ static tree handle_argspec_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_fnspec_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_warn_unused_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_returns_nonnull_attribute (tree *, tree, tree, int, bool *);
> +static tree handle_returns_zero_on_attributes (tree *, tree, tree, int, bool *);
>  static tree handle_omp_declare_simd_attribute (tree *, tree, tree, int,
>                                                bool *);
>  static tree handle_omp_declare_variant_attribute (tree *, tree, tree, int,
> @@ -221,6 +222,8 @@ extern const struct attribute_spec::exclusions attr_noreturn_exclusions[] =
>    ATTR_EXCL ("pure", true, true, true),
>    ATTR_EXCL ("returns_twice", true, true, true),
>    ATTR_EXCL ("warn_unused_result", true, true, true),
> +  ATTR_EXCL ("returns_zero_on_failure", true, true, true),
> +  ATTR_EXCL ("returns_zero_on_success", true, true, true),
>    ATTR_EXCL (NULL, false, false, false),
>  };
>
> @@ -235,6 +238,8 @@ attr_warn_unused_result_exclusions[] =
>  static const struct attribute_spec::exclusions attr_returns_twice_exclusions[] =
>  {
>    ATTR_EXCL ("noreturn", true, true, true),
> +  ATTR_EXCL ("returns_zero_on_failure", true, true, true),
> +  ATTR_EXCL ("returns_zero_on_success", true, true, true),
>    ATTR_EXCL (NULL, false, false, false),
>  };
>
> @@ -275,6 +280,16 @@ static const struct attribute_spec::exclusions attr_stack_protect_exclusions[] =
>    ATTR_EXCL (NULL, false, false, false),
>  };
>
> +/* Exclusions that apply to the returns_zero_on_* attributes.  */
> +static const struct attribute_spec::exclusions
> +  attr_returns_zero_on_exclusions[] =
> +{
> +  ATTR_EXCL ("noreturn", true, true, true),
> +  ATTR_EXCL ("returns_twice", true, true, true),
> +  ATTR_EXCL ("returns_zero_on_failure", true, true, true),
> +  ATTR_EXCL ("returns_zero_on_success", true, true, true),
> +  ATTR_EXCL (NULL, false, false, false),
> +};
>
>  /* Table of machine-independent attributes common to all C-like languages.
>
> @@ -493,6 +508,12 @@ const struct attribute_spec c_common_attribute_table[] =
>                               handle_warn_unused_attribute, NULL },
>    { "returns_nonnull",        0, 0, false, true, true, false,
>                               handle_returns_nonnull_attribute, NULL },
> +  { "returns_zero_on_failure",0, 0, false, true, true, false,
> +                             handle_returns_zero_on_attributes,
> +                             attr_returns_zero_on_exclusions },
> +  { "returns_zero_on_success",0, 0, false, true, true, false,
> +                             handle_returns_zero_on_attributes,
> +                             attr_returns_zero_on_exclusions },
>    { "omp declare simd",       0, -1, true,  false, false, false,
>                               handle_omp_declare_simd_attribute, NULL },
>    { "omp declare variant base", 0, -1, true,  false, false, false,
> @@ -5660,6 +5681,22 @@ handle_returns_nonnull_attribute (tree *node, tree name, tree, int,
>    return NULL_TREE;
>  }
>
> +/* 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,
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 ?

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

Also, would it make sense to extend it for pointers too for returning
NULL on success / failure ?

Thanks,
Prathamesh
> +}
> +
>  /* Handle a "designated_init" attribute; arguments as in
>     struct attribute_spec.handler.  */
>
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index e9f47519df2..5a6ef464779 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -3784,6 +3784,22 @@ function.  Examples of such functions are @code{setjmp} and @code{vfork}.
>  The @code{longjmp}-like counterpart of such function, if any, might need
>  to be marked with the @code{noreturn} attribute.
>
> +@item returns_zero_on_failure
> +@cindex @code{returns_zero_on_failure} function attribute
> +The @code{returns_zero_on_failure} attribute hints that the function
> +can succeed or fail, returning non-zero on success and zero on failure.
> +This is used by the @option{-fanalyzer} option to consider both outcomes
> +separately, which may improve how it explores error-handling paths, and
> +how such outcomes are labelled in diagnostics.  It is also a hint
> +to the human reader of the source code.
> +
> +@item returns_zero_on_success
> +@cindex @code{returns_zero_on_success} function attribute
> +The @code{returns_zero_on_success} attribute is identical to the
> +@code{returns_zero_on_failure} attribute, apart from having the
> +opposite interpretation of the return value: zero on success, non-zero
> +on failure.
> +
>  @item section ("@var{section-name}")
>  @cindex @code{section} function attribute
>  @cindex functions in arbitrary sections
> diff --git a/gcc/testsuite/c-c++-common/attr-returns-zero-on-1.c b/gcc/testsuite/c-c++-common/attr-returns-zero-on-1.c
> new file mode 100644
> index 00000000000..5475dfe61db
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/attr-returns-zero-on-1.c
> @@ -0,0 +1,68 @@
> +/* Verify the parsing of the "returns_zero_on_{sucess|failure}" attributes.  */
> +
> +/* Correct usage.  */
> +
> +extern int test_int_return_s ()
> +  __attribute__((returns_zero_on_success));
> +extern long test_long_return_f ()
> +  __attribute__((returns_zero_on_failure));
> +
> +/* Should complain if not a function.  */
> +
> +extern int not_a_function_s
> +  __attribute__((returns_zero_on_success)); /* { dg-warning "'returns_zero_on_success' attribute only applies to function types" } */
> +extern int not_a_function_f
> +  __attribute__((returns_zero_on_failure)); /* { dg-warning "'returns_zero_on_failure' attribute only applies to function types" } */
> +
> +/* Should complain if return type is not integral.  */
> +
> +extern void test_void_return_s ()
> +  __attribute__((returns_zero_on_success)); /* { dg-error "'returns_zero_on_success' attribute on a function not returning an integral type" } */
> +extern void test_void_return_f ()
> +  __attribute__((returns_zero_on_failure)); /* { dg-error "'returns_zero_on_failure' attribute on a function not returning an integral type" } */
> +
> +extern void *test_void_star_return_s ()
> +  __attribute__((returns_zero_on_success)); /* { dg-error "'returns_zero_on_success' attribute on a function not returning an integral type" } */
> +extern void *test_void_star_return_f ()
> +  __attribute__((returns_zero_on_failure)); /* { dg-error "'returns_zero_on_failure' attribute on a function not returning an integral type" } */
> +
> +/* (and this prevents mixing with returns_non_null, which requires a pointer).  */
> +
> +/* Should complain if more than one returns_* attribute.  */
> +
> +extern int test_void_returns_s_f ()
> +  __attribute__((returns_zero_on_success))
> +  __attribute__((returns_zero_on_failure)); /* { dg-warning "ignoring attribute 'returns_zero_on_failure' because it conflicts with attribute 'returns_zero_on_success'" } */
> +extern int test_void_returns_f_s ()
> +  __attribute__((returns_zero_on_failure))
> +  __attribute__((returns_zero_on_success)); /* { dg-warning "ignoring attribute 'returns_zero_on_success' because it conflicts with attribute 'returns_zero_on_failure'" } */
> +
> +/* Should complain if mixed with "noreturn".  */
> +
> +extern int test_noreturn_returns_s ()
> +  __attribute__((noreturn))
> +  __attribute__((returns_zero_on_success)); /* { dg-warning "ignoring attribute 'returns_zero_on_success' because it conflicts with attribute 'noreturn'" } */
> +extern int test_returns_s_noreturn ()
> +  __attribute__((returns_zero_on_success))
> +  __attribute__((noreturn)); /* { dg-warning "ignoring attribute 'noreturn' because it conflicts with attribute 'returns_zero_on_success'" } */
> +extern int test_noreturn_returns_f ()
> +  __attribute__((noreturn))
> +  __attribute__((returns_zero_on_failure)); /* { dg-warning "ignoring attribute 'returns_zero_on_failure' because it conflicts with attribute 'noreturn'" } */
> +extern int test_returns_f_noreturn ()
> +  __attribute__((returns_zero_on_failure))
> +  __attribute__((noreturn)); /* { dg-warning "ignoring attribute 'noreturn' because it conflicts with attribute 'returns_zero_on_failure'" } */
> +
> +/* Should complain if mixed with "returns_twice".  */
> +
> +extern int test_returns_twice_returns_s ()
> +  __attribute__((returns_twice))
> +  __attribute__((returns_zero_on_success)); /* { dg-warning "ignoring attribute 'returns_zero_on_success' because it conflicts with attribute 'returns_twice'" } */
> +extern int test_returns_s_returns_twice ()
> +  __attribute__((returns_zero_on_success))
> +  __attribute__((returns_twice)); /* { dg-warning "ignoring attribute 'returns_twice' because it conflicts with attribute 'returns_zero_on_success'" } */
> +extern int test_returns_twice_returns_f ()
> +  __attribute__((returns_twice))
> +  __attribute__((returns_zero_on_failure)); /* { dg-warning "ignoring attribute 'returns_zero_on_failure' because it conflicts with attribute 'returns_twice'" } */
> +extern int test_returns_f_returns_twice ()
> +  __attribute__((returns_zero_on_failure))
> +  __attribute__((returns_twice)); /* { dg-warning "ignoring attribute 'returns_twice' because it conflicts with attribute 'returns_zero_on_failure'" } */
> --
> 2.26.3
>

On Mon, Nov 15, 2021 at 12:33:16PM +0530, Prathamesh Kulkarni wrote:
> On Sun, 14 Nov 2021 at 02:07, David Malcolm via Gcc-patches

> > +/* 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,
> 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 ?
> 
> 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) ?
> 
> Also, would it make sense to extend it for pointers too for returning
> NULL on success / failure ?

Please also consider that in Linux we use the 'last' page for error code
returns. That is, a function returning a pointer could return '(void
*)-EFAULT' also see linux/err.h

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

On Mon, 2021-11-15 at 15:45 +0100, Peter Zijlstra wrote:
> On Mon, Nov 15, 2021 at 12:33:16PM +0530, Prathamesh Kulkarni wrote:
> > On Sun, 14 Nov 2021 at 02:07, David Malcolm via Gcc-patches
> 
> > > +/* 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,
> > 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 ?
> > 
> > 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) ?
> > 
> > Also, would it make sense to extend it for pointers too for returning
> > NULL on success / failure ?
> 
> Please also consider that in Linux we use the 'last' page for error
> code
> returns. That is, a function returning a pointer could return '(void
> *)-EFAULT' also see linux/err.h
> 

Thanks.

Am I right in thinking that such functions return non-NULL, giving
something like:

  __attribute__((returns_ptr_in_range_on_success (0x1, NULL - 4096)))
  __attribute__((returns_ptr_in_range_on_failure (NULL - 4096, NULL - 1)))
  __attribute__((returns_non_null))

as attributes?  (I have no idea if the above will parse, and I admit
these look ugly as-is, though I suppose they could be hidden behind a
macro).

Looking at include/linux/err.h I see functions:

static inline bool __must_check IS_ERR(__force const void *ptr)
{
	return IS_ERR_VALUE((unsigned long)ptr);
}

static inline bool __must_check IS_ERR_OR_NULL(__force const void *ptr)
{
	return unlikely(!ptr) || IS_ERR_VALUE((unsigned long)ptr);
}

so maybe attribute could refer to predicate functions, something like
this:

  __attribute__((return_value_success_predicate(FUNCTION_DECL)))
  __attribute__((return_value_failure_predicate(FUNCTION_DECL)))

where this case could use something like:

  __attribute__((return_value_failure_predicate(IS_ERR)))

to express the idea "this function can succeed or fail, and the given
function decl expresses whether a given return value is a failure" - or
somesuch.  The predicate function would probably have to be pure.

Obviously I'm just brainstorming here; as noted in my reply to
Prathamesh, all I need for the initial implementation of the trust
boundary work is just being able to express that zero vs non-zero
return is the success vs failure condition for a function.

Dave

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

On Wed, 17 Nov 2021, Prathamesh Kulkarni via Gcc-patches wrote:

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

Many math.h functions are const except for possibly setting errno, 
possibly raising floating-point exceptions (which might have other effects 
when using alternate exception handling) and possibly reading the rounding 
mode.  To represent that, it might be useful for such attributes to be 
able to describe state (such as the floating-point environment) that 
doesn't correspond to a C identifier.  (errno tends to be a macro, so 
referring to it as such in an attribute may be awkward as well.)

(See also <http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2825.htm> with 
some proposals for features to describe const/pure-like properties of 
functions.)

On Wed, Nov 17, 2021 at 10:43:58PM +0000, Joseph Myers wrote:
> On Wed, 17 Nov 2021, Prathamesh Kulkarni via Gcc-patches wrote:
> > 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.
> 
> Many math.h functions are const except for possibly setting errno, 
> possibly raising floating-point exceptions (which might have other effects 
> when using alternate exception handling) and possibly reading the rounding 
> mode.  To represent that, it might be useful for such attributes to be 
> able to describe state (such as the floating-point environment) that 
> doesn't correspond to a C identifier.  (errno tends to be a macro, so 
> referring to it as such in an attribute may be awkward as well.)

We need some way to describe these things in Gimple and RTL as well,
and not just on function calls: also on other expressions.  Adding
attributes that allow to describe this (partially, only per function) in
C source code does not bring us closer to where we need to be.


Segher

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

On Wed, 2021-11-17 at 22:43 +0000, Joseph Myers wrote:
> On Wed, 17 Nov 2021, Prathamesh Kulkarni via Gcc-patches wrote:
> 
> > 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.
> 
> Many math.h functions are const except for possibly setting errno, 
> possibly raising floating-point exceptions (which might have other
> effects 
> when using alternate exception handling) and possibly reading the
> rounding 
> mode.  To represent that, it might be useful for such attributes to
> be 
> able to describe state (such as the floating-point environment) that 
> doesn't correspond to a C identifier.  (errno tends to be a macro, so
> referring to it as such in an attribute may be awkward as well.)
> 
> (See also <http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2825.htm>
> with 
> some proposals for features to describe const/pure-like properties of
> functions.)
> 

Thanks for the link.

As noted in my reply to Prathamesh, these ideas sound interesting, but
this thread seems to be entering scope creep - I don't need these ideas
to implement this patch kit (but I do need the attributes specified in
the patch, or similar).  

Do the specific attributes I posted sound reasonable?  (without
necessarily going in to a full review).

If we're thinking longer term, I want the ability to express that a
function can have multiple outcomes (e.g. "success" vs "failure" or
"found" vs "not found", etc), and it might be good to have a way to
attach attributes to those outcomes.  Unfortunately the attribute
syntax is flat, but maybe there could be a two level hierarchy,
something like:

int foo (args)
  __attribute__((outcome("success")
                 __attribute__((return_value(0))))
  __attribute__((outcome("failure")
                 __attribute__((return_value_ne(0))
                 __attribute__((modifies(errno)))));

Or given that we're enamored by Lisp-ish DSLs we could go the whole hog
and have something like:

int foo (args)
  __attribute ((semantics(
    "(def-outcomes (success (return-value (eq 0))"
    "              (failure (return-value (ne 0)"
    "                        modifies (errno))))")));

which may be over-engineering things :)


Going back to the patch itself, returns_zero_on_success/failure get me
what I want to express for finding trust boundaries in the Linux
kernel, have obvious meaning to a programmer (helpful even w/o compiler
support), and could interoperate with one the more elaborate ideas in
this thread.

Hope this is constructive
Dave

On Thu, 2021-11-18 at 14:08 -0600, Segher Boessenkool wrote:
> On Wed, Nov 17, 2021 at 10:43:58PM +0000, Joseph Myers wrote:
> > On Wed, 17 Nov 2021, Prathamesh Kulkarni via Gcc-patches wrote:
> > > 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.
> > 
> > Many math.h functions are const except for possibly setting errno, 
> > possibly raising floating-point exceptions (which might have other
> > effects 
> > when using alternate exception handling) and possibly reading the
> > rounding 
> > mode.  To represent that, it might be useful for such attributes to
> > be 
> > able to describe state (such as the floating-point environment)
> > that 
> > doesn't correspond to a C identifier.  (errno tends to be a macro,
> > so 
> > referring to it as such in an attribute may be awkward as well.)
> 
> We need some way to describe these things in Gimple and RTL as well,
> and not just on function calls: also on other expressions.  Adding
> attributes that allow to describe this (partially, only per function)
> in
> C source code does not bring us closer to where we need to be.

Right, but those IR concerns are orthogonal to the needs of the patch
kit, which is a way to express certain *other* things per-function in
the C frontend.  

As noted in my other replies, this thread seems to be turning into
something of a scope-creep pile-on, when I have some specific things I
need for the rest of the patch kit, and they're unrelated to the
problems of errno or floating-point handling.

Dave

On Thu, Nov 18, 2021 at 06:45:42PM -0500, David Malcolm wrote:
> On Thu, 2021-11-18 at 14:08 -0600, Segher Boessenkool wrote:
> > We need some way to describe these things in Gimple and RTL as well,
> > and not just on function calls: also on other expressions.  Adding
> > attributes that allow to describe this (partially, only per function)
> > in
> > C source code does not bring us closer to where we need to be.
> 
> Right, but those IR concerns are orthogonal to the needs of the patch
> kit, which is a way to express certain *other* things per-function in
> the C frontend.  

My fear is that such band-aids will only make attacking the long
standing hard problems even harder.

> As noted in my other replies, this thread seems to be turning into
> something of a scope-creep pile-on, when I have some specific things I
> need for the rest of the patch kit, and they're unrelated to the
> problems of errno or floating-point handling.

I am just asking to think about the broader picture, and see how this
fits in there.


Segher

On 11/18/21 4:34 PM, David Malcolm via Gcc-patches wrote:
> On Wed, 2021-11-17 at 22:43 +0000, Joseph Myers wrote:
>> On Wed, 17 Nov 2021, Prathamesh Kulkarni via Gcc-patches wrote:
>>
>>> 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.
>>
>> Many math.h functions are const except for possibly setting errno,
>> possibly raising floating-point exceptions (which might have other
>> effects
>> when using alternate exception handling) and possibly reading the
>> rounding
>> mode.  To represent that, it might be useful for such attributes to
>> be
>> able to describe state (such as the floating-point environment) that
>> doesn't correspond to a C identifier.  (errno tends to be a macro, so
>> referring to it as such in an attribute may be awkward as well.)
>>
>> (See also <http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2825.htm>
>> with
>> some proposals for features to describe const/pure-like properties of
>> functions.)
>>
> 
> Thanks for the link.
> 
> As noted in my reply to Prathamesh, these ideas sound interesting, but
> this thread seems to be entering scope creep - I don't need these ideas
> to implement this patch kit (but I do need the attributes specified in
> the patch, or similar).
> 
> Do the specific attributes I posted sound reasonable?  (without
> necessarily going in to a full review).
> 
> If we're thinking longer term, I want the ability to express that a
> function can have multiple outcomes (e.g. "success" vs "failure" or
> "found" vs "not found", etc), and it might be good to have a way to
> attach attributes to those outcomes.  Unfortunately the attribute
> syntax is flat, but maybe there could be a two level hierarchy,
> something like:
> 
> int foo (args)
>    __attribute__((outcome("success")
>                   __attribute__((return_value(0))))
>    __attribute__((outcome("failure")
>                   __attribute__((return_value_ne(0))
>                   __attribute__((modifies(errno)))));
> 
> Or given that we're enamored by Lisp-ish DSLs we could go the whole hog
> and have something like:
> 
> int foo (args)
>    __attribute ((semantics(
>      "(def-outcomes (success (return-value (eq 0))"
>      "              (failure (return-value (ne 0)"
>      "                        modifies (errno))))")));
> 
> which may be over-engineering things :)

For a fully general solution, one that can express (nearly)
arbitrarily complex pre-conditions and invariants, I'd look
at the ideas in the C++ contracts papers.  I don't know if
any of the proposals (there were quite a few) made it possible
to specify postconditions involving function return values,
but I'd think that could be overcome by introducing some
special token like __retval.

Syntactically, one of the nice things about contracts that
I hope should be possible to implement in our attributes is
a way to refer to formal function arguments by name rather
than by their position in the argument list.  With that,
the expressivity goes up dramatically because it becomes
possible to use any C expression.

Martin

> Going back to the patch itself, returns_zero_on_success/failure get me
> what I want to express for finding trust boundaries in the Linux
> kernel, have obvious meaning to a programmer (helpful even w/o compiler
> support), and could interoperate with one the more elaborate ideas in
> this thread.
> 
> Hope this is constructive
> Dave
> 
> 
> 
> 
>