Re: New attribute: returns_nonnull

[Marc Glisse <marc dot glisse at inria dot fr>]

On Tue, 8 Oct 2013, Jeff Law wrote:

> On 10/07/13 08:17, Marc Glisse wrote:
> > Hello,
> >
> > this patch adds an attribute to let the compiler know that a function
> > never returns NULL. I saw some ECF_* flags, but the attribute seems
> > sufficient. I considered using nonnull(0), but then it would have been
> > confusing that the version of nonnull without arguments applies only to
> > parameters and not the return value.
> >
> > 2013-10-08  Marc Glisse <marc.glisse@inria.fr>
> >
> >      PR tree-optimization/20318
> > gcc/c-family/
> >      * c-common.c (handle_returns_nonnull_attribute): New function.
> >      (c_common_attribute_table): Add returns_nonnull.
> >
> > gcc/
> >      * doc/extend.texi (returns_nonnull): New function attribute.
> >      * fold-const.c (tree_expr_nonzero_warnv_p): Look for returns_nonnull
> >      attribute.
> >      * tree-vrp.c (gimple_stmt_nonzero_warnv_p): Likewise.
> >      (stmt_interesting_for_vrp): Accept all GIMPLE_CALL.
> >
> > gcc/testsuite/
> >      * c-c++-common/pr20318.c: New file.
> >      * gcc.dg/tree-ssa/pr20318.c: New file.
> >
> > --
> > Marc Glisse
> >
> > p12
> >
> >
> > Index: c-family/c-common.c
> > ===================================================================
> > --- c-family/c-common.c	(revision 203241)
> > +++ c-family/c-common.c	(working copy)
> > @@ -740,20 +741,22 @@ const struct attribute_spec c_common_att
> >     { "*tm regparm",            0, 0, false, true, true,
> >   			      ignore_attribute, false },
> >     { "no_split_stack",	      0, 0, true,  false, false,
> >   			      handle_no_split_stack_attribute, false },
> >     /* For internal use (marking of builtins and runtime functions) only.
> >        The name contains space to prevent its usage in source code.  */
> >     { "fn spec",	 	      1, 1, false, true, true,
> >   			      handle_fnspec_attribute, false },
> >     { "warn_unused",            0, 0, false, false, false,
> >   			      handle_warn_unused_attribute, false },
> > +  { "returns_nonnull",        0, 0, false, true, true,
> > +			      handle_returns_nonnull_attribute, false },
> >     { NULL,                     0, 0, false, false, false, NULL, false }
> >   };
> I'm going to assume this is correct -- it looks sane, but I've never really 
> done much with the attribute tables.

I looked at nonnull and noreturn, and the second one says that it is wrong 
and should be like nonnull, so I mostly copied from nonnull. I can't say I 
really understand what it is doing, but I was happy that everything worked 
so nicely.

> > +
> > +/* Handle a "returns_nonnull" attribute; arguments as in
> > +   struct attribute_spec.handler.  */
> > +
> > +static tree
> > +handle_returns_nonnull_attribute (tree *node, tree, tree, int,
> > +				  bool *no_add_attrs)
> > +{
> > +  // Even without a prototype we still have a return type we can check.
> > +  if (TREE_CODE (TREE_TYPE (*node)) != POINTER_TYPE)
> > +    {
> > +      error ("returns_nonnull attribute on a function not returning a 
> > pointer");
> > +      *no_add_attrs = true;
> > +    }
> > +  return NULL_TREE;
> > +}
> Glad to see you checked this and have a test for it.

I am slowly starting to understand how reviewers think ;-)

> Not required for approval, but an "extra credit" -- a warning if a NULL value 
> flows into a return statement in a function with this marking.
>
> Similarly, not required for approval, but it'd be real cool if we could 
> back-propagate the non-null return value attribute.  ie, any value flowing 
> into the return statement of one of these functions can be assumed to be 
> non-zero, which may help eliminate more null pointer checks in the decorated 
> function.  I guess ultimately we'd have to see if noting this actually helps 
> any real code.
>
> Also not required for approval, but adding returns_nonnull markups to 
> appropriate functions in gcc itself.

I completely agree that returns_nonnull has more uses. The warning is the 
first one (not sure where to add such a check though), and maybe we should 
have a sanitizer option to check the nonnull and returns_nonnull 
attributes, although I don't know if that should be in the caller, in the 
callee, or both.

> From an optimization POV, I guess there is also the inlining that could be 
improved so it doesn't lose the nonnull property.

The main request I am aware of is PR21856, but I am not familiar enough 
with java to handle it. Do you have suggestions of functions which should 
get the attribute? memcpy could, but that's really a special case since it 
returns its argument, which is itself non-null.

I'll open a new PR about all these. I mostly implemented returns_nonnull 
because I'd just done the same for operator new and thus I knew exactly 
where the optimization was, I can't promise I'll manage much of the rest.


> > Index: testsuite/gcc.dg/tree-ssa/pr20318.c
> > ===================================================================
> > --- testsuite/gcc.dg/tree-ssa/pr20318.c	(revision 0)
> > +++ testsuite/gcc.dg/tree-ssa/pr20318.c	(working copy)
> > @@ -0,0 +1,19 @@
> > +/* { dg-do compile { target { ! keeps_null_pointer_checks } } } */
> > +/* { dg-options "-O2 -fdump-tree-original -fdump-tree-vrp1" } */
> > +
> > +extern int* f(int) __attribute__((returns_nonnull));
> > +extern void eliminate ();
> > +void g () {
> > +  if (f (2) == 0)
> > +    eliminate ();
> > +}
> > +void h () {
> > +  int *p = f (2);
> > +  if (p == 0)
> > +    eliminate ();
> > +}
> > +
> > +/* { dg-final { scan-tree-dump-times "== 0" 1 "original" } } */
> > +/* { dg-final { scan-tree-dump-times "Folding predicate\[^\\n\]*to 0" 1 
> > "vrp1" } } */
> > +/* { dg-final { cleanup-tree-dump "original" } } */
> > +/* { dg-final { cleanup-tree-dump "vrp1" } } */
> Presumably g() is testing the fold-const.c and h() tests the tree-vrp 
> changes, right?

Yes.

> This is OK for the trunk, please install.

Thank you.

--
Marc Glisse