On Tue, 8 Oct 2013, Jeff Law wrote:
On 10/07/13 08:17, Marc Glisse wrote:I'm going to assume this is correct -- it looks sane, but I've never really done much with the attribute tables.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.gli...@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 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.
Presumably g() is testing the fold-const.c and h() tests the tree-vrp changes, right?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" } } */
Yes.
This is OK for the trunk, please install.
Thank you. -- Marc Glisse
