PR35503 - warn for restrict pointer

Message ID CAAgBjMmDH2tU7HbdFhcotm0NioUs0txnjWhGcSSYnRcEMu-_=g@mail.gmail.com
State New
Headers show

Commit Message

Prathamesh Kulkarni Sept. 18, 2016, 5:16 p.m.
On 2 September 2016 at 23:14, David Malcolm <dmalcolm@redhat.com> wrote:
> On Thu, 2016-09-01 at 14:55 +0530, Prathamesh Kulkarni wrote:

>

> [...]

>

>> The attached version passes bootstrap+test on ppc64le-linux-gnu.

>> Given that it only looks if parameters are restrict qualified and not

>> how they're used inside the callee,

>> this can have false positives as in above test-cases.

>> Should the warning be put in Wextra rather than Wall (I have left it

>> in Wall in the patch)  or only enabled with -Wrestrict ?

>>

>> Thanks,

>> Prathamesh

>> >

>> > Richard.

>

>

> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c

> index 3feb910..a3dae34 100644

> --- a/gcc/c-family/c-common.c

> +++ b/gcc/c-family/c-common.c

> @@ -47,6 +47,7 @@ along with GCC; see the file COPYING3.  If not see

>  #include "gimplify.h"

>  #include "substring-locations.h"

>  #include "spellcheck.h"

> +#include "gcc-rich-location.h"

>

>  cpp_reader *parse_in;          /* Declared in c-pragma.h.  */

>

> @@ -13057,4 +13058,76 @@ diagnose_mismatched_attributes (tree olddecl,

> tree newdecl)

>    return warned;

>  }

>

> +/* Warn if an argument at position param_pos is passed to a

> +   restrict-qualified param, and it aliases with another argument.  */

> +

> +void

> +warn_for_restrict (unsigned param_pos, vec<tree, va_gc> *args)

> +{

> +  tree arg = (*args)[param_pos];

> +  if (TREE_VISITED (arg) || operand_equal_p (arg, null_pointer_node,

> 0))

> +    return;

> +

> +  location_t loc = EXPR_LOC_OR_LOC (arg, input_location);

> +  gcc_rich_location richloc (loc);

> +

> +  unsigned i;

> +  tree current_arg;

> +  auto_vec<unsigned> arg_positions;

> +

> +  FOR_EACH_VEC_ELT (*args, i, current_arg)

> +    {

> +      if (i == param_pos)

> +       continue;

> +

> +      tree current_arg = (*args)[i];

> +      if (operand_equal_p (arg, current_arg, 0))

> +       {

> +         TREE_VISITED (current_arg) = 1;

> +         arg_positions.safe_push (i);

> +       }

> +    }

> +

> +  if (arg_positions.is_empty ())

> +    return;

> +

> +  struct obstack fmt_obstack;

> +  gcc_obstack_init (&fmt_obstack);

> +  char *fmt = (char *) obstack_alloc (&fmt_obstack, 0);

> +

> +  char num[32];

> +  sprintf (num, "%u", param_pos + 1);

> +

> +  obstack_grow (&fmt_obstack, "passing argument ",

> +               strlen ("passing argument "));

> +  obstack_grow (&fmt_obstack, num, strlen (num));

> +  obstack_grow (&fmt_obstack,

> +               " to restrict-qualified parameter aliases with

> argument",

> +               strlen (" to restrict-qualified parameter "

> +                       "aliases with argument"));

> +

> +  /* make argument plural and append space.  */

> +  if (arg_positions.length () > 1)

> +    obstack_1grow (&fmt_obstack, 's');

> +  obstack_1grow (&fmt_obstack, ' ');

> +

> +  unsigned pos;

> +  FOR_EACH_VEC_ELT (arg_positions, i, pos)

> +    {

> +      tree arg = (*args)[pos];

> +      if (EXPR_HAS_LOCATION (arg))

> +       richloc.add_range (EXPR_LOCATION (arg), false);

> +

> +      sprintf (num, "%u", pos + 1);

> +      obstack_grow (&fmt_obstack, num, strlen (num));

> +

> +      if (i < arg_positions.length () - 1)

> +       obstack_grow (&fmt_obstack, ", ",  strlen (", "));

> +    }

> +

> +  obstack_1grow (&fmt_obstack, 0);

> +  warning_at_rich_loc (&richloc, OPT_Wrestrict, fmt);

> +  obstack_free (&fmt_obstack, fmt);

> +}

>

> Thanks for working on this.

>

> I'm not a fan of how the patch builds "fmt" here.  If nothing else,

> this perhaps should be:

>

>   warning_at_rich_loc (&richloc, OPT_Wrestrict, "%s", fmt);

>

> but building up strings like the patch does makes localization

> difficult.

>

> Much better would be to have the formatting be done inside the

> diagnostics subsystem's call into pp_format, with something like this:

>

>   warning_at_rich_loc_n (&richloc, OPT_Wrestrict,

>                          arg_positions

> .length (),

>                          "passing argument %i to restrict

> -qualified"

>                          " parameter aliases with argument

> %FIXME",

>                          "passing argument %i to restrict

> -qualified"

>                          " parameter aliases with arguments

> %FIXME",

>                          param_pos + 1,

>

>  &arg_positions);

>

>

> and have %FIXME (or somesuch) consume &arg_positions in the va_arg,

> printing the argument numbers there.  Doing it this way also avoids

> building the string for the case where Wrestrict is disabled, since the

> pp_format only happens after we know we're going to print the warning.

>

> Assuming that there isn't yet a pre-canned way to print a set of

> argument numbers that I've missed, the place to add the %FIXME-handling

> would presumably be in default_tree_printer in tree-diagnostic.c -

> though it's obviously nothing to do with trees. (Or if this is too

> single-purpose, perhaps there's a need to temporarily inject one-time

> callbacks for consuming custom args??).

>

> We can then have a fun discussion about the usage of the Oxford comma :) [1]

>

> IIRC we don't yet have warning_at_rich_loc_n, but it would be fairly easy to add.

Hi David,
Sorry for late response. In the attached patch, I removed obstack
building on fmt, and used
pp_format for formatting arg_positions by adding specifier %I (name
chosen arbitrarily).
Does that look OK ?

However it results in following warning during gcc build and am not
sure how to fix this:
../../gcc/gcc/c-family/c-common.c: In function ‘void
warn_for_restrict(unsigned int, vec<tree_node*, va_gc>*)’:
../../gcc/gcc/c-family/c-common.c:13132:24: warning: unknown
conversion type character ‘I’ in format [-Wformat=]

Thanks,
Prathamesh
>

> [...]

>

> [1] which seems to be locale-dependent itself:

> https://en.wikipedia.org/wiki/Serial_comma#Other_languages

Patch hide | download patch | download mbox

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index fc25686..5081637 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -47,6 +47,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "gimplify.h"
 #include "substring-locations.h"
 #include "spellcheck.h"
+#include "gcc-rich-location.h"
 
 cpp_reader *parse_in;		/* Declared in c-pragma.h.  */
 
@@ -13084,4 +13085,51 @@  diagnose_mismatched_attributes (tree olddecl, tree newdecl)
   return warned;
 }
 
+/* Warn if an argument at position param_pos is passed to a
+   restrict-qualified param, and it aliases with another argument.  */
+
+void
+warn_for_restrict (unsigned param_pos, vec<tree, va_gc> *args)
+{
+  tree arg = (*args)[param_pos];
+  if (TREE_VISITED (arg) || operand_equal_p (arg, null_pointer_node, 0))
+    return;
+
+  location_t loc = EXPR_LOC_OR_LOC (arg, input_location);
+  gcc_rich_location richloc (loc);
+
+  unsigned i;
+  tree current_arg;
+  auto_vec<int> arg_positions;
+
+  FOR_EACH_VEC_ELT (*args, i, current_arg) 
+    {
+      if (i == param_pos)
+	continue;
+
+      tree current_arg = (*args)[i];
+      if (operand_equal_p (arg, current_arg, 0))
+	{
+	  TREE_VISITED (current_arg) = 1; 
+	  arg_positions.safe_push (i + 1); 
+	}
+    }
+
+  if (arg_positions.is_empty ())
+    return;
+
+  int pos;
+  FOR_EACH_VEC_ELT (arg_positions, i, pos)
+    {
+      tree arg = (*args)[pos - 1];
+      if (EXPR_HAS_LOCATION (arg))
+	richloc.add_range (EXPR_LOCATION (arg), false);
+    }
+
+  warning_at_rich_loc (&richloc, OPT_Wrestrict, "passing argument %i to"
+		       " restrict-qualified parameter aliases with argument%s %I",
+		       param_pos + 1, (arg_positions.length() > 1) ? "s" : "",
+		       &arg_positions); 
+}
+
 #include "gt-c-family-c-common.h"
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 5bbf951..f29ea1f 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -922,6 +922,7 @@  extern void c_parse_final_cleanups (void);
 
 extern void warn_for_omitted_condop (location_t, tree);
 extern void warn_for_memset (location_t, tree, tree, int);
+extern void warn_for_restrict (unsigned, vec<tree, va_gc> *);
 
 /* These macros provide convenient access to the various _STMT nodes.  */
 
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index c55c7c3..08edea0 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1032,6 +1032,11 @@  Wduplicate-decl-specifier
 C ObjC Var(warn_duplicate_decl_specifier) Warning LangEnabledBy(C ObjC,Wall)
 Warn when a declaration has duplicate const, volatile, restrict or _Atomic specifier.
 
+Wrestrict
+C ObjC C++ ObjC++ Var(warn_restrict) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+Warn when an argument passed to a restrict-qualified parameter aliases with
+another argument.
+
 ansi
 C ObjC C++ ObjC++
 A synonym for -std=c89 (for C) or -std=c++98 (for C++).
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 58424a9..f9043ba 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -8368,6 +8368,25 @@  c_parser_postfix_expression_after_primary (c_parser *parser,
 	      warn_for_memset (expr_loc, arg0, arg2, literal_zero_mask);
 	    }
 
+	  if (TREE_CODE (expr.value) == FUNCTION_DECL && warn_restrict)
+	    {
+	      unsigned i;
+	      tree arg;
+	      FOR_EACH_VEC_SAFE_ELT (exprlist, i, arg)
+		TREE_VISITED (arg) = 0;
+
+	      unsigned param_pos = 0;
+	      function_args_iterator iter;
+	      tree t;
+	      FOREACH_FUNCTION_ARGS (TREE_TYPE (expr.value), t, iter)
+		{
+		  if (POINTER_TYPE_P (t) && TYPE_RESTRICT (t)
+		      && !TYPE_READONLY (TREE_TYPE (t)))
+		    warn_for_restrict (param_pos, exprlist);
+		  param_pos++;
+		}
+	    }
+
 	  start = expr.get_start ();
 	  finish = parser->tokens_buf[0].get_finish ();
 	  expr.value
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index fb88021..87d9ef6 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -6878,6 +6878,26 @@  cp_parser_postfix_expression (cp_parser *parser, bool address_p, bool cast_p,
 		warn_for_memset (input_location, arg0, arg2, literal_mask);
 	      }
 
+	    if (TREE_CODE (postfix_expression) == FUNCTION_DECL
+		&& warn_restrict)
+	      {
+		unsigned i;
+		tree arg;
+		FOR_EACH_VEC_SAFE_ELT (args, i, arg)
+		  TREE_VISITED (arg) = 0;
+
+		unsigned param_pos = 0;
+		for (tree decl = DECL_ARGUMENTS (postfix_expression);
+		     decl != NULL_TREE;
+		     decl = DECL_CHAIN (decl), param_pos++)
+		  {
+		    tree type = TREE_TYPE (decl);
+		    if (POINTER_TYPE_P (type) && TYPE_RESTRICT (type)
+			&& !TYPE_READONLY (TREE_TYPE (type)))
+		      warn_for_restrict (param_pos, args);
+		  }
+	      }
+
 	    if (TREE_CODE (postfix_expression) == COMPONENT_REF)
 	      {
 		tree instance = TREE_OPERAND (postfix_expression, 0);
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 8eb5eff..d24eb6f 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -288,7 +288,7 @@  Objective-C and Objective-C++ Dialects}.
 -Wparentheses -Wno-pedantic-ms-format @gol
 -Wplacement-new -Wplacement-new=@var{n} @gol
 -Wpointer-arith  -Wno-pointer-to-int-cast @gol
--Wno-pragmas -Wredundant-decls  -Wno-return-local-addr @gol
+-Wno-pragmas -Wredundant-decls -Wrestrict  -Wno-return-local-addr @gol
 -Wreturn-type  -Wsequence-point  -Wshadow  -Wno-shadow-ivar @gol
 -Wshift-overflow -Wshift-overflow=@var{n} @gol
 -Wshift-count-negative -Wshift-count-overflow -Wshift-negative-value @gol
@@ -5348,6 +5348,12 @@  compilations.
 Warn when deleting a pointer to incomplete type, which may cause
 undefined behavior at runtime.  This warning is enabled by default.
 
+@item -Wrestrict
+@opindex Wrestrict
+@opindex Wno-restrict
+Warn when an argument passed to a restrict-qualified parameter
+aliases with another argument
+
 @item -Wuseless-cast @r{(C++ and Objective-C++ only)}
 @opindex Wuseless-cast
 @opindex Wno-useless-cast
diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
index a39815e..e8bd1ef 100644
--- a/gcc/pretty-print.c
+++ b/gcc/pretty-print.c
@@ -610,6 +610,23 @@  pp_format (pretty_printer *pp, text_info *text)
 	      (pp, *text->args_ptr, precision, unsigned, "u");
 	  break;
 
+	case 'I':
+	  {
+	    vec<int> *v = va_arg (*text->args_ptr, vec<int> *);
+	    unsigned i;
+	    int pos;
+
+	    FOR_EACH_VEC_ELT ((*v), i, pos)
+	      {
+		pp_scalar (pp, "%i", pos); 
+		if (v->length () > 1 && i < (v->length () - 1))
+		  {
+		    pp_comma (pp);
+		    pp_space (pp);
+		  }
+	      }
+	    break;
+	 }
 	case 'x':
 	  if (wide)
 	    pp_scalar (pp, HOST_WIDE_INT_PRINT_HEX,
diff --git a/gcc/testsuite/c-c++-common/pr35503-1.c b/gcc/testsuite/c-c++-common/pr35503-1.c
new file mode 100644
index 0000000..25e3721
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pr35503-1.c
@@ -0,0 +1,10 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Wrestrict" } */
+
+int foo (char *__restrict buf, const char *__restrict fmt, ...);
+
+void f(void)
+{
+  char buf[100] = "hello";
+  foo (buf, "%s-%s", buf, "world"); /*  { dg-warning "passing argument 1 to restrict-qualified parameter aliases with argument 3" } */
+}
diff --git a/gcc/testsuite/c-c++-common/pr35503-2.c b/gcc/testsuite/c-c++-common/pr35503-2.c
new file mode 100644
index 0000000..bfcd944
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pr35503-2.c
@@ -0,0 +1,14 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fdiagnostics-show-caret -Wrestrict" } */
+
+void f(int *__restrict x, int *y, int *__restrict z, int *w);
+
+void foo(int alpha, int beta)
+{
+  f (&alpha, &beta, &alpha, &alpha); /* { dg-warning "passing argument 1 to restrict-qualified parameter aliases with arguments 3, 4" } */
+
+/* { dg-begin-multiline-output "" }
+   f (&alpha, &beta, &alpha, &alpha);
+      ^~~~~~         ~~~~~~  ~~~~~~
+   { dg-end-multiline-output "" } */
+}
diff --git a/gcc/testsuite/c-c++-common/pr35503-3.c b/gcc/testsuite/c-c++-common/pr35503-3.c
new file mode 100644
index 0000000..8cbacab
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pr35503-3.c
@@ -0,0 +1,9 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Wrestrict" } */
+
+void f(int *x, int *__restrict y);
+
+void foo(int a)
+{
+  f (&a, &a); /* { dg-warning "passing argument 2 to restrict-qualified parameter aliases with argument 1" } */
+}