diff mbox

PR35503 - warn for restrict pointer

Message ID CAAgBjMn4TdXN901wHtgs1TKnAvGA9CHiiCbzVm1R1Tv4-mgMrw@mail.gmail.com
State New
Headers show

Commit Message

Prathamesh Kulkarni Sept. 22, 2016, 6:32 a.m. UTC
On 20 September 2016 at 18:31, Joseph Myers <joseph@codesourcery.com> wrote:
> On Tue, 20 Sep 2016, Prathamesh Kulkarni wrote:
>
>> Could someone please take a look at the change to c-format.c, I am not sure
>> if I have added that correctly.
>
> Any changes to these GCC formats also require tests to be updated
> (gcc.dg/format/gcc_diag*).
Hi,
I previously used T_C for 'Z' specifier which was incorrect because
that expected
the argument type to be char *, while the actual type expected is vec<int> *.
I have currently added the following, similar to %p, which I suppose
would at least
silence the "unknown conversion specifier" warning:

 { "Z",   1, STD_C89, { T89_V,   BADLEN,  BADLEN,  BADLEN,  BADLEN,
BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN  }, "",
 "",   NULL },

Would that be acceptable ? I am not sure how to make %Z check if the
argument has type vec<int> *
since vec<int> is not really a builtin C type.
Could you suggest me a better solution so that the format checker will check
if arg has type vec<int> * instead of checking if it's just a pointer ?
Also for testing, should I create a testcase in g++.dg since
gcc.dg/format/ tests are C-only ?

Thanks,
Prathamesh
>
> --
> Joseph S. Myers
> joseph@codesourcery.com

Comments

Joseph Myers Sept. 22, 2016, 5:45 p.m. UTC | #1
On Thu, 22 Sep 2016, Prathamesh Kulkarni wrote:

> Would that be acceptable ? I am not sure how to make %Z check if the
> argument has type vec<int> *
> since vec<int> is not really a builtin C type.
> Could you suggest me a better solution so that the format checker will check
> if arg has type vec<int> * instead of checking if it's just a pointer ?
> Also for testing, should I create a testcase in g++.dg since
> gcc.dg/format/ tests are C-only ?

If it's C++-only then it would need to be in g++.dg.

The way we handle GCC-specific types in checking these formats is that the 
code using these formats has to define typedefs which the format-checking 
code then looks up.  In most cases it can just look up names like 
location_t or tree, but for HOST_WIDE_INT it looks up 
__gcc_host_wide_int__ which the user must have defined as a typedef.  
Probably that's the way to go in this case: the user must do "typedef 
vec<int> __gcc_vec_int__;" or similar, and the code looks up 
__gcc_vec_int__.
diff mbox

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index fc25686..742f06d 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,53 @@  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_n (&richloc, OPT_Wrestrict, arg_positions.length (),
+			 "passing argument %i to restrict-qualified parameter"
+			 " aliases with argument %Z",
+			 "passing argument %i to restrict-qualified parameter"
+			 " aliases with arguments %Z",
+			 param_pos + 1, &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-format.c b/gcc/c-family/c-format.c
index bf39ee0..6db2fee 100644
--- a/gcc/c-family/c-format.c
+++ b/gcc/c-family/c-format.c
@@ -713,6 +713,7 @@  static const format_char_info gcc_tdiag_char_table[] =
   { "r",   1, STD_C89, { T89_C,   BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN  }, "",    "cR",   NULL },
   { "<>'R",0, STD_C89, NOARGUMENTS, "",      "",   NULL },
   { "m",   0, STD_C89, NOARGUMENTS, "q",     "",   NULL },
+  { "Z",   1, STD_C89, { T89_V,   BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN,  BADLEN  }, "",    "",   NULL },
   { NULL,  0, STD_C89, NOLENGTHS, NULL, NULL, NULL }
 };
 
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..3ad1f67 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -8368,6 +8368,28 @@  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++;
+		}
+
+	      FOR_EACH_VEC_SAFE_ELT (exprlist, i, arg)
+		TREE_VISITED (arg) = 0;
+	    }
+
 	  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..58fc615 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -6878,6 +6878,29 @@  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);
+		  }
+
+		FOR_EACH_VEC_SAFE_ELT (args, i, arg)
+		  TREE_VISITED (arg) = 0;
+	      }
+
 	    if (TREE_CODE (postfix_expression) == COMPONENT_REF)
 	      {
 		tree instance = TREE_OPERAND (postfix_expression, 0);
diff --git a/gcc/diagnostic-core.h b/gcc/diagnostic-core.h
index 51df150..4a1766c 100644
--- a/gcc/diagnostic-core.h
+++ b/gcc/diagnostic-core.h
@@ -65,6 +65,9 @@  extern bool warning_at (location_t, int, const char *, ...)
     ATTRIBUTE_GCC_DIAG(3,4);
 extern bool warning_at_rich_loc (rich_location *, int, const char *, ...)
     ATTRIBUTE_GCC_DIAG(3,4);
+extern bool warning_at_rich_loc_n (rich_location *, int, int, const char *,
+				  const char *, ...)
+    ATTRIBUTE_GCC_DIAG(4, 6) ATTRIBUTE_GCC_DIAG(5, 6);
 extern void error (const char *, ...) ATTRIBUTE_GCC_DIAG(1,2);
 extern void error_n (location_t, int, const char *, const char *, ...)
     ATTRIBUTE_GCC_DIAG(3,5) ATTRIBUTE_GCC_DIAG(4,5);
diff --git a/gcc/diagnostic.c b/gcc/diagnostic.c
index 585028e..167a1b5 100644
--- a/gcc/diagnostic.c
+++ b/gcc/diagnostic.c
@@ -53,6 +53,10 @@  static bool diagnostic_impl (rich_location *, int, const char *,
 static bool diagnostic_n_impl (location_t, int, int, const char *,
 			       const char *, va_list *,
 			       diagnostic_t) ATTRIBUTE_GCC_DIAG(5,0);
+static bool diagnostic_n_impl_richloc (rich_location *, int, int, const char *,
+				       const char *, va_list *,
+				       diagnostic_t) ATTRIBUTE_GCC_DIAG(5,0);
+
 static void error_recursion (diagnostic_context *) ATTRIBUTE_NORETURN;
 static void real_abort (void) ATTRIBUTE_NORETURN;
 
@@ -1064,6 +1068,22 @@  diagnostic_impl (rich_location *richloc, int opt,
   return report_diagnostic (&diagnostic);
 }
 
+/* Same as diagonostic_n_impl taking rich_location instead of location_t.  */
+static bool
+diagnostic_n_impl_richloc (rich_location *richloc, int opt, int n,
+			   const char *singular_gmsgid,
+			   const char *plural_gmsgid,
+			   va_list *ap, diagnostic_t kind)
+{
+  diagnostic_info diagnostic;
+  diagnostic_set_info_translated (&diagnostic,
+                                  ngettext (singular_gmsgid, plural_gmsgid, n),
+                                  ap, richloc, kind);
+  if (kind == DK_WARNING)
+    diagnostic.option_index = opt;
+  return report_diagnostic (&diagnostic);
+} 
+
 /* Implement inform_n, warning_n, and error_n, as documented and
    defined below.  */
 static bool
@@ -1072,14 +1092,9 @@  diagnostic_n_impl (location_t location, int opt, int n,
 		   const char *plural_gmsgid,
 		   va_list *ap, diagnostic_t kind)
 {
-  diagnostic_info diagnostic;
   rich_location richloc (line_table, location);
-  diagnostic_set_info_translated (&diagnostic,
-                                  ngettext (singular_gmsgid, plural_gmsgid, n),
-                                  ap, &richloc, kind);
-  if (kind == DK_WARNING)
-    diagnostic.option_index = opt;
-  return report_diagnostic (&diagnostic);
+  return diagnostic_n_impl_richloc (&richloc, opt, n,
+				    singular_gmsgid, plural_gmsgid, ap, kind);
 }
 
 bool
@@ -1170,6 +1185,21 @@  warning_at_rich_loc (rich_location *richloc, int opt, const char *gmsgid, ...)
   return ret;
 }
 
+/* Same as warning_at_rich_loc but for plural variant.  */
+
+bool
+warning_at_rich_loc_n (rich_location *richloc, int opt, int n,
+		       const char *singular_gmsgid, const char *plural_gmsgid, ...)
+{
+  va_list ap;
+  va_start (ap, plural_gmsgid);
+  bool ret = diagnostic_n_impl_richloc (richloc, opt, n,
+					singular_gmsgid, plural_gmsgid,
+					&ap, DK_WARNING);
+  va_end (ap);
+  return ret;
+}
+
 /* A warning at LOCATION.  Use this for code which is correct according to the
    relevant language specification but is likely to be buggy anyway.
    Returns true if the warning was printed, false if it was inhibited.  */
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..20e697d 100644
--- a/gcc/pretty-print.c
+++ b/gcc/pretty-print.c
@@ -295,6 +295,11 @@  pp_indent (pretty_printer *pp)
    %Ns: likewise, but length specified as constant in the format string.
    Flag 'q': quote formatted text (must come immediately after '%').
 
+   FIXME: It would be a better idea to add %Z as a length modifier to print
+   vec of other types. For instance %hZx would print vec<short> in hex format.
+   Currently %Z just prints vec<int> formatted in decimal.
+   %Z: vec<int>.
+
    Arguments can be used sequentially, or through %N$ resp. *N$
    notation Nth argument after the format string.  If %N$ / *N$
    notation is used, it must be used for all arguments, except %m, %%,
@@ -610,6 +615,23 @@  pp_format (pretty_printer *pp, text_info *text)
 	      (pp, *text->args_ptr, precision, unsigned, "u");
 	  break;
 
+	case 'Z':
+	  {
+	    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 (i < v->length () - 1)
+		  {
+		    pp_comma (pp);
+		    pp_space (pp);
+		  }
+	      }
+	    break;
+	 }
 	case 'x':
 	  if (wide)
 	    pp_scalar (pp, HOST_WIDE_INT_PRINT_HEX,
@@ -1424,6 +1446,17 @@  test_pp_format ()
 			    "`\33[01m\33[Kfoo\33[m\33[K' 12345678", "%qs %x",
 			    "foo", 0x12345678);
 
+  /* Verify %Z.  */
+  auto_vec<int> v;
+  v.safe_push (1);
+  v.safe_push (2);
+  v.safe_push (3);
+  ASSERT_PP_FORMAT_2 ("1, 2, 3 12345678", "%Z %x", &v, 0x12345678);
+
+  auto_vec<int> v2;
+  v2.safe_push (0);
+  ASSERT_PP_FORMAT_2 ("0 12345678", "%Z %x", &v2, 0x12345678);
+
   /* Verify that combinations work, along with unformatted text.  */
   assert_pp_format (SELFTEST_LOCATION,
 		    "the quick brown fox jumps over the lazy dog",
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" } */
+}