diff mbox

[RFC] nonzero attribute, static array parameter

Message ID 20150509094223.300601b8@lemur
State New
Headers show

Commit Message

Martin Uecker May 9, 2015, 4:42 p.m. UTC 
Hi,

here is a tentative patch to implement a new attribute nonzero,
which is similar to nonnull, but is not a function attribute
but a type attribute.

One reason is that nonnull is awkward to use. For this reason,
clang allows the use of nonnull in function parameters, but this
is incompatible with old and current use of this attribute in gcc
(though in a rather obscure use case).
See: https://gcc.gnu.org/ml/gcc/2015-05/msg00077.html

The other reason is that a nonzero type attribute is conceptually
much simpler and at the same time more general than the existing
nonnull and nonnull_return function attributes (and could replace
both), e.g. we can define non-zero types and diagnose all stores
of known constant 0 to variables of this type, use this for 
computing better value ranges, etc.

This patch implements the attribute and adds a warning if a 
zero constant is passed as argument with nonzero type attribute.
Also infer_nonnull_range in gcc/gimple.c is extended to make use 
of the attribute (which in turn is used by ubsan).

In addition, in C the attribute is automatically added to
pointers declared as array parameters with static, e.g:

void foo(int a[static 5]);


Martin

(tested on x86_64-unknown-linux-gnu)

Comments

Marek Polacek May 15, 2015, 1:38 p.m. UTC | #1 
On Sat, May 09, 2015 at 09:42:23AM -0700, Martin Uecker wrote:
> here is a tentative patch to implement a new attribute nonzero,
> which is similar to nonnull, but is not a function attribute
> but a type attribute.
> 
> One reason is that nonnull is awkward to use. For this reason,
> clang allows the use of nonnull in function parameters, but this
> is incompatible with old and current use of this attribute in gcc
> (though in a rather obscure use case).
> See: https://gcc.gnu.org/ml/gcc/2015-05/msg00077.html
 
Sorry, I quite fail to see how such an attribute would be useful.
It seems that the nonzero warning can only ever trigger when used
on function parameters or on a return value, much as the nonnull / 
returns_nonnull attributes.  The difference is that you can use
the nonzero attribute on a particular function parameter, but the
nonnull attribute has this ability as well:

__attribute__ ((nonnull (1))) void foo (int *, int *);
void
bar (void)
{
  foo (0, 0);
}

Unlike nonnull, nonzero attribute can be attached to a typedef, but
it doesn't seem to buy you anything you couldn't do with the nonnull /
returns_nonnull attributes.

The nonzero attribute can appertain even to integer types, not only
pointer types.  Can you give an example where this would come in handy?
It doesn't seem too useful to me.

+void foo1(int x[__attribute__((nonzero))]);

This looks weird, the placement of the nonzero attribute here suggests
that the array should have at least zero elements (the same that
int x[static 0] does), but in fact it checks that the pointer passed
to foo1 is non-NULL, i.e. something that could be easily achieved
with the nonnull attribute.

> The other reason is that a nonzero type attribute is conceptually
> much simpler and at the same time more general than the existing
> nonnull and nonnull_return function attributes (and could replace
> both), e.g. we can define non-zero types and diagnose all stores
> of known constant 0 to variables of this type, use this for 
> computing better value ranges, etc.
 
Why would that be useful?  What makes integer 0 special?

	Marek
Martin Uecker July 24, 2015, 1:31 a.m. UTC | #2 
Hi Marek,

sorry for taking so long to respond.

Fri, 15 May 2015 15:38:43 +0200
Marek Polacek <polacek@redhat.com>:

> On Sat, May 09, 2015 at 09:42:23AM -0700, Martin Uecker wrote:
> > here is a tentative patch to implement a new attribute nonzero,
> > which is similar to nonnull, but is not a function attribute
> > but a type attribute.
> > 
> > One reason is that nonnull is awkward to use. For this reason,
> > clang allows the use of nonnull in function parameters, but this
> > is incompatible with old and current use of this attribute in gcc
> > (though in a rather obscure use case).
> > See: https://gcc.gnu.org/ml/gcc/2015-05/msg00077.html
>  
> Sorry, I quite fail to see how such an attribute would be useful.

First of all, it makes it much simpler to implement the desired warnings
and ubsan checks when passing NULL as an array parameters with 'static'.
It avoids implementing all this argument walking code which is needed
for nonnull. If we do not want a nonzero attribute, could we then
consider an internal attribute (e.g. "non zero")?

> It seems that the nonzero warning can only ever trigger when used
> on function parameters or on a return value, much as the nonnull / 
> returns_nonnull attributes. 

So far. But in the future this could be extended to other
cases where it makes sense.

> The difference is that you can use
> the nonzero attribute on a particular function parameter, but the
> nonnull attribute has this ability as well:
> 
> __attribute__ ((nonnull (1))) void foo (int *, int *);
> void
> bar (void)
> {
>   foo (0, 0);
> }

I know. But this is ugly, cumbersome, and fragile, and some uses
are incompatible with clang. That nonnull is mis-designed
has been pointed out before:

https://gcc.gnu.org/ml/gcc/2006-04/msg00551.html

> Unlike nonnull, nonzero attribute can be attached to a typedef, but
> it doesn't seem to buy you anything you couldn't do with the nonnull /
> returns_nonnull attributes.

It is much more expressive. A nonnull pointer _type_ seems really
useful to me. Many programming languages have something like this.

> The nonzero attribute can appertain even to integer types, not only
> pointer types.  Can you give an example where this would come in handy?
> It doesn't seem too useful to me.

One example:

void assert(__attribute__((nonzero)) int i);

would give a warning if it is already known at compile time
that the expression is zero.

> 
> +void foo1(int x[__attribute__((nonzero))]);
> 
> This looks weird, 

It does look weird. But this is already documented in this way:
https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html#Attribute-Syntax

The use of 'const' and 'static' in an array declarator as
defined by ISO C99 is already weird.

> the placement of the nonzero attribute here suggests
> that the array should have at least zero elements (the same that
> int x[static 0] does), but in fact it checks that the pointer passed
> to foo1 is non-NULL, i.e. something that could be easily achieved
> with the nonnull attribute.

In many case it would be much nicer to be able to declare a 
nonzero type and use it in interfaces instead of having to
add __attribute__((nonnull(1,2,5,8))) with exactly the right
numbers to a lot of prototypes.

> > The other reason is that a nonzero type attribute is conceptually
> > much simpler and at the same time more general than the existing
> > nonnull and nonnull_return function attributes (and could replace
> > both), e.g. we can define non-zero types and diagnose all stores
> > of known constant 0 to variables of this type, use this for 
> > computing better value ranges, etc.
>  
> Why would that be useful?  What makes integer 0 special?

0 is special in many ways. In particular it represents false
in an if clause. Also, why artificially restrict it to pointers
when it is trivial to make it work for integers too?


Martin
diff mbox

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 77d9352..2f79344 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@ 
+2015-05-09  Martin Uecker  <uecker@eecs.berkeley.edu>
+
+	* gcc/gimple.c (infer_nonnull_range): Process nonzero attribute.
+	* doc/invoki.texi
+
 2015-05-08  Jim Wilson  <jim.wilson@linaro.org>
 
 	* doc/install.texi (--enable-languages): Add missing jit and lto info.
diff --git a/gcc/c-family/ChangeLog b/gcc/c-family/ChangeLog
index 57f83c9..809ecf1 100644
--- a/gcc/c-family/ChangeLog
+++ b/gcc/c-family/ChangeLog
@@ -1,3 +1,9 @@ 
+2015-05-09  Martin Uecker  <uecker@eecs.berkeley.edu>
+
+	* c-common.c (handle_nonzero_attribute): New.
+	(check_function_nonzero): New.
+	(check_nonzero_arg): New.
+
 2015-05-08  Marek Polacek  <polacek@redhat.com>
 
 	PR c/64918
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 378f237..46fe749 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -380,6 +380,7 @@  static tree handle_deprecated_attribute (tree *, tree, tree, int,
 static tree handle_vector_size_attribute (tree *, tree, tree, int,
 					  bool *);
 static tree handle_nonnull_attribute (tree *, tree, tree, int, bool *);
+static tree handle_nonzero_attribute (tree *, tree, tree, int, bool *);
 static tree handle_nothrow_attribute (tree *, tree, tree, int, bool *);
 static tree handle_cleanup_attribute (tree *, tree, tree, int, bool *);
 static tree handle_warn_unused_result_attribute (tree *, tree, tree, int,
@@ -407,6 +408,7 @@  static tree handle_bnd_instrument (tree *, tree, tree, int, bool *);
 
 static void check_function_nonnull (tree, int, tree *);
 static void check_nonnull_arg (void *, tree, unsigned HOST_WIDE_INT);
+static void check_nonzero_arg (void *, tree, unsigned HOST_WIDE_INT);
 static bool nonnull_check_p (tree, unsigned HOST_WIDE_INT);
 static bool get_nonnull_operand (tree, unsigned HOST_WIDE_INT *);
 static int resort_field_decl_cmp (const void *, const void *);
@@ -751,6 +753,8 @@  const struct attribute_spec c_common_attribute_table[] =
 			      handle_tls_model_attribute, false },
   { "nonnull",                0, -1, false, true, true,
 			      handle_nonnull_attribute, false },
+  { "nonzero",                0, -1, false, true, false,
+                              handle_nonzero_attribute, false },
   { "nothrow",                0, 0, true,  false, false,
 			      handle_nothrow_attribute, false },
   { "may_alias",	      0, 0, false, true, false, NULL, false },
@@ -8940,6 +8944,26 @@  handle_vector_size_attribute (tree *node, tree name, tree args,
   return NULL_TREE;
 }
 
+
+/* Handle the "nonzero" attribute.  */
+static tree
+handle_nonzero_attribute (tree *node, tree ARG_UNUSED (name),
+			  tree ARG_UNUSED (args), int ARG_UNUSED (flags),
+			  bool *no_add_attrs)
+{
+  tree n = *node;
+
+  if ((TREE_CODE (n) != POINTER_TYPE)
+      && (TREE_CODE (n) != INTEGER_TYPE))
+    {
+      warning (OPT_Wattributes, "%qE attribute ignored", name);
+      *no_add_attrs = true;
+    }
+
+  return NULL_TREE;
+}
+
+
 /* Handle the "nonnull" attribute.  */
 static tree
 handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name),
@@ -9016,6 +9040,34 @@  handle_nonnull_attribute (tree *node, tree ARG_UNUSED (name),
   return NULL_TREE;
 }
 
+
+static void
+check_function_nonzero (const_tree fntype, int nargs, tree *argarray)
+{
+  if (TREE_CODE (fntype) != FUNCTION_TYPE) // C++ lambdas
+    return;
+
+  function_args_iterator iter;
+  tree type;
+  int i = 0;
+
+  FOREACH_FUNCTION_ARGS (fntype, type, iter)
+    {
+      if (i == nargs)
+	break;
+
+      tree a = lookup_attribute ("nonzero", TYPE_ATTRIBUTES (type));
+
+      if (a != NULL_TREE)
+        check_function_arguments_recurse (check_nonzero_arg, NULL,
+					  argarray[i], i + 1);
+
+      i++;
+   }
+}
+
+
+
 /* Check the argument list of a function call for null in argument slots
    that are marked as requiring a non-null pointer argument.  The NARGS
    arguments are passed in the array ARGARRAY.
@@ -9139,6 +9191,21 @@  nonnull_check_p (tree args, unsigned HOST_WIDE_INT param_num)
   return false;
 }
 
+
+/* Check that the function argument PARAM (which is operand number
+   PARAM_NUM) is non-null.  This is called by check_function_nonzero
+   via check_function_arguments_recurse.  */
+
+static void
+check_nonzero_arg (void * ARG_UNUSED (ctx), tree param,
+		   unsigned HOST_WIDE_INT param_num)
+{
+  if (integer_zerop (param))
+    warning (OPT_Wnonnull, "zero argument where non-zero required "
+	     "(argument %lu)", (unsigned long) param_num);
+}
+
+
 /* Check that the function argument PARAM (which is operand number
    PARAM_NUM) is non-null.  This is called by check_function_nonnull
    via check_function_arguments_recurse.  */
@@ -9573,8 +9640,11 @@  check_function_arguments (const_tree fntype, int nargs, tree *argarray)
   /* Check for null being passed in a pointer argument that must be
      non-null.  We also need to do this if format checking is enabled.  */
 
-  if (warn_nonnull)
+  if (warn_nonnull) {
+
     check_function_nonnull (TYPE_ATTRIBUTES (fntype), nargs, argarray);
+    check_function_nonzero (fntype, nargs, argarray);
+  }
 
   /* Check for errors in format strings.  */
 
diff --git a/gcc/c/ChangeLog b/gcc/c/ChangeLog
index 3b8f491..d410594 100644
--- a/gcc/c/ChangeLog
+++ b/gcc/c/ChangeLog
@@ -1,3 +1,8 @@ 
+2015-05-09  Martin Uecker  <uecker@eecs.berkeley.edu>
+
+	* c-decl.c (grokdeclarator): Allow attributes for pointers
+	declared as arrays. Add nonzero attribute for static.
+
 2015-05-08  Marek Polacek  <polacek@redhat.com>
 
 	PR c/64918
diff --git a/gcc/c/c-decl.c b/gcc/c/c-decl.c
index 4f6761d..4273696 100644
--- a/gcc/c/c-decl.c
+++ b/gcc/c/c-decl.c
@@ -6437,10 +6437,12 @@  grokdeclarator (const struct c_declarator *declarator,
 	    if (type_quals)
 	      type = c_build_qualified_type (type, type_quals);
 
-	    /* We don't yet implement attributes in this context.  */
-	    if (array_ptr_attrs != NULL_TREE)
-	      warning_at (loc, OPT_Wattributes,
-			  "attributes in parameter array declarator ignored");
+            if (array_parm_static)
+	        array_ptr_attrs = tree_cons (get_identifier ("nonzero"), NULL_TREE, array_ptr_attrs);
+
+            if (array_ptr_attrs != NULL_TREE)
+              type = build_type_attribute_variant (type, array_ptr_attrs);
+
 
 	    size_varies = false;
 	    array_parameter_p = true;
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 9169731..976a769 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -3698,7 +3698,8 @@  formats that may yield only a two-digit year.
 @opindex Wnonnull
 @opindex Wno-nonnull
 Warn about passing a null pointer for arguments marked as
-requiring a non-null value by the @code{nonnull} function attribute.
+requiring a non-null value by the @code{nonnull} function attribute
+or by the @code{nonzero} type attribute.
 
 @option{-Wnonnull} is included in @option{-Wall} and @option{-Wformat}.  It
 can be disabled with the @option{-Wno-nonnull} option.
@@ -5791,13 +5792,14 @@  This option does not work well with @code{FE_INVALID} exceptions enabled.
 
 This option enables instrumentation of calls, checking whether null values
 are not passed to arguments marked as requiring a non-null value by the
-@code{nonnull} function attribute.
+@code{nonnull} function attribute or by the @code{nonzero} type attribute.
 
 @item -fsanitize=returns-nonnull-attribute
 @opindex fsanitize=returns-nonnull-attribute
 
 This option enables instrumentation of return statements in functions
-marked with @code{returns_nonnull} function attribute, to detect returning
+marked with @code{returns_nonnull} function attribute or with a return
+type marked with the @code{nonzero} attribute, to detect returning
 of null values from such functions.
 
 @item -fsanitize=bool
diff --git a/gcc/gimple.c b/gcc/gimple.c
index a5c1192..426b15d 100644
--- a/gcc/gimple.c
+++ b/gcc/gimple.c
@@ -2679,6 +2679,10 @@  infer_nonnull_range (gimple stmt, tree op, bool dereference, bool attribute)
     return true;
 
   if (attribute
+      && lookup_attribute ("nonzero", TYPE_ATTRIBUTES (TREE_TYPE (op))))
+      return true;
+
+  if (attribute
       && is_gimple_call (stmt) && !gimple_call_internal_p (stmt))
     {
       tree fntype = gimple_call_fntype (stmt);
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 2b6f663..8c09f3e 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,11 @@ 
+2015-05-09  Martin Uecker  <uecker@eecs.berkeley.edu>
+
+	* c-c++-common/attr-nonzero.c: New test.
+	* c-c++-common/ubsan/nonzero-1.c: New test.
+	* c-c++-common/ubsan/nonzero-2.c: New test.
+	* gcc.dg/array-parm.c: New test.
+	* gcc.dg/ubsan/array-parm.c: New test.
+
 2015-05-08  Richard Biener  <rguenther@suse.de>
 
 	PR tree-optimization/66036
diff --git a/gcc/testsuite/c-c++-common/attr-nonzero.c b/gcc/testsuite/c-c++-common/attr-nonzero.c
new file mode 100644
index 0000000..1a6a19d
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/attr-nonzero.c
@@ -0,0 +1,10 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Wall" } */
+
+void foo(__attribute__((nonzero)) int x);
+
+void bar(void)
+{
+	foo(0);		/* { dg-warning "zero argument" } */
+}
+
diff --git a/gcc/testsuite/c-c++-common/ubsan/nonzero-1.c b/gcc/testsuite/c-c++-common/ubsan/nonzero-1.c
new file mode 100644
index 0000000..7877434
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/nonzero-1.c
@@ -0,0 +1,15 @@ 
+/* { dg-do run } */
+/* { dg-options "-fsanitize=undefined" } */
+
+typedef __attribute__((nonzero)) int* nonzero_ptr;
+
+void foo(nonzero_ptr a)
+{
+}
+
+int main(void)
+{
+	foo(0);
+}
+
+/* { dg-output "runtime error: null pointer passed as argument 1" } */
diff --git a/gcc/testsuite/c-c++-common/ubsan/nonzero-2.c b/gcc/testsuite/c-c++-common/ubsan/nonzero-2.c
new file mode 100644
index 0000000..e7331cb
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/ubsan/nonzero-2.c
@@ -0,0 +1,16 @@ 
+/* { dg-do run } */
+/* { dg-options "-fsanitize=undefined" } */
+
+typedef __attribute__((nonzero)) int* nonzero_ptr;
+
+nonzero_ptr foo(void)
+{
+	return 0;
+}
+
+int main(void)
+{
+	foo();
+}
+
+/* { dg-output "runtime error: null pointer returned from function declared to never return null" } */
diff --git a/gcc/testsuite/gcc.dg/array-parm.c b/gcc/testsuite/gcc.dg/array-parm.c
new file mode 100644
index 0000000..001fed7
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/array-parm.c
@@ -0,0 +1,12 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Wall" } */
+
+void foo0(int x[static 0]);
+void foo1(int x[__attribute__((nonzero))]);
+
+void bar(void)
+{
+	foo0(0);	/* { dg-warning "zero argument" } */
+	foo1(0);	/* { dg-warning "zero argument" } */
+}
+
diff --git a/gcc/testsuite/gcc.dg/ubsan/array-parm.c b/gcc/testsuite/gcc.dg/ubsan/array-parm.c
new file mode 100644
index 0000000..5dfa6a8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ubsan/array-parm.c
@@ -0,0 +1,13 @@ 
+/* { dg-do run } */
+/* { dg-options "-fsanitize=undefined" } */
+
+void foo(int a[static 0])
+{
+}
+
+int main(void)
+{
+	foo(0);
+}
+
+/* { dg-output "runtime error: null pointer passed as argument 1" } */