Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 7 October 2016 at 10:33, Prathamesh Kulkarni
 wrote:
> On 22 September 2016 at 23:15, Joseph Myers  wrote:
>> 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 *
>>> since vec 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 * 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 __gcc_vec_int__;" or similar, and the code looks up
>> __gcc_vec_int__.
> Thanks for the suggestions. To keep it simple, instead of vec,
> I made %Z take two args: int *v, unsigned len, and prints elements in
> v having length == len.
> Is that OK ?
>
> Bootstrapped+tested on x86_64-unknown-linux-gnu.
> As pointed out earlier in the thread, the patch can give false positives 
> because
> it only checks whether parameters are qualified with restrict, not how
> parameters
> are used inside the function. For instance it warned for example 10
> mentioned in n1570
> under section 6.7.3.1 - "Formal definition of restrict".
> Should we keep the warning in Wall or keep it in Wextra ?
> The attached patch enables it with Wall.
Ping for c, c-family changes:
https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00446.html

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

Re: PR35503 - warn for restrict pointer

[Jason Merrill]

On Sat, Oct 8, 2016 at 1:07 PM, Prathamesh Kulkarni
 wrote:
> On 7 October 2016 at 17:49, David Malcolm  wrote:
>> On Fri, 2016-10-07 at 10:33 +0530, Prathamesh Kulkarni wrote:
>>> On 22 September 2016 at 23:15, Joseph Myers 
>>> wrote:
>>> > 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 *
>>> > > since vec 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 * 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 __gcc_vec_int__;" or similar, and the code looks up
>>> > __gcc_vec_int__.
>>> Thanks for the suggestions. To keep it simple, instead of vec,
>>> I made %Z take two args: int *v, unsigned len, and prints elements in
>>> v having length == len.
>>> Is that OK ?
>>>
>>> Bootstrapped+tested on x86_64-unknown-linux-gnu.
>>> As pointed out earlier in the thread, the patch can give false
>>> positives because
>>> it only checks whether parameters are qualified with restrict, not
>>> how
>>> parameters
>>> are used inside the function. For instance it warned for example 10
>>> mentioned in n1570
>>> under section 6.7.3.1 - "Formal definition of restrict".
>>> Should we keep the warning in Wall or keep it in Wextra ?
>>> The attached patch enables it with Wall.
>>>
>>
>> This needs a ChangeLog.
>>
>> The changes to diagnostic-core.h and diagnostic.c are OK for trunk,
>> given a suitable ChangeLog (and could be split into a separate patch if
>> you like).
> Thanks, I committed diagnostic.c and diagnostic-core.h changes (with 
> ChangeLog)
> in r240891.

The C++ changes are also OK.

Jason

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 7 October 2016 at 17:49, David Malcolm  wrote:
> On Fri, 2016-10-07 at 10:33 +0530, Prathamesh Kulkarni wrote:
>> On 22 September 2016 at 23:15, Joseph Myers 
>> wrote:
>> > 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 *
>> > > since vec 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 * 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 __gcc_vec_int__;" or similar, and the code looks up
>> > __gcc_vec_int__.
>> Thanks for the suggestions. To keep it simple, instead of vec,
>> I made %Z take two args: int *v, unsigned len, and prints elements in
>> v having length == len.
>> Is that OK ?
>>
>> Bootstrapped+tested on x86_64-unknown-linux-gnu.
>> As pointed out earlier in the thread, the patch can give false
>> positives because
>> it only checks whether parameters are qualified with restrict, not
>> how
>> parameters
>> are used inside the function. For instance it warned for example 10
>> mentioned in n1570
>> under section 6.7.3.1 - "Formal definition of restrict".
>> Should we keep the warning in Wall or keep it in Wextra ?
>> The attached patch enables it with Wall.
>>
>> Thanks,
>> Prathamesh
>
> This needs a ChangeLog.
>
> The changes to diagnostic-core.h and diagnostic.c are OK for trunk,
> given a suitable ChangeLog (and could be split into a separate patch if
> you like).
Thanks, I committed diagnostic.c and diagnostic-core.h changes (with ChangeLog)
in r240891.

Thanks,
Prathamesh

Re: PR35503 - warn for restrict pointer

[David Malcolm]

On Fri, 2016-10-07 at 10:33 +0530, Prathamesh Kulkarni wrote:
> On 22 September 2016 at 23:15, Joseph Myers 
> wrote:
> > 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 *
> > > since vec 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 * 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 __gcc_vec_int__;" or similar, and the code looks up
> > __gcc_vec_int__.
> Thanks for the suggestions. To keep it simple, instead of vec,
> I made %Z take two args: int *v, unsigned len, and prints elements in
> v having length == len.
> Is that OK ?
> 
> Bootstrapped+tested on x86_64-unknown-linux-gnu.
> As pointed out earlier in the thread, the patch can give false
> positives because
> it only checks whether parameters are qualified with restrict, not
> how
> parameters
> are used inside the function. For instance it warned for example 10
> mentioned in n1570
> under section 6.7.3.1 - "Formal definition of restrict".
> Should we keep the warning in Wall or keep it in Wextra ?
> The attached patch enables it with Wall.
> 
> Thanks,
> Prathamesh

This needs a ChangeLog.

The changes to diagnostic-core.h and diagnostic.c are OK for trunk,
given a suitable ChangeLog (and could be split into a separate patch if
you like).

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 22 September 2016 at 23:15, Joseph Myers  wrote:
> 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 *
>> since vec 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 * 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 __gcc_vec_int__;" or similar, and the code looks up
> __gcc_vec_int__.
Thanks for the suggestions. To keep it simple, instead of vec,
I made %Z take two args: int *v, unsigned len, and prints elements in
v having length == len.
Is that OK ?

Bootstrapped+tested on x86_64-unknown-linux-gnu.
As pointed out earlier in the thread, the patch can give false positives because
it only checks whether parameters are qualified with restrict, not how
parameters
are used inside the function. For instance it warned for example 10
mentioned in n1570
under section 6.7.3.1 - "Formal definition of restrict".
Should we keep the warning in Wall or keep it in Wextra ?
The attached patch enables it with Wall.

Thanks,
Prathamesh
>
> --
> Joseph S. Myers
> jos...@codesourcery.com
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 491c637..751af90 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -48,6 +48,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.  */
 
@@ -13156,4 +13157,59 @@ 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 *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;
+  int *arg_positions = XNEWVEC (int, args->length ());
+  unsigned arg_positions_len = 0;
+
+  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[arg_positions_len++] = (i + 1);
+   }
+}
+
+  if (arg_positions_len == 0)
+{
+  free (arg_positions);
+  return;
+}
+
+  for (unsigned i = 0; i < arg_positions_len; i++)
+{
+  unsigned pos = arg_positions[i];
+  tree arg = (*args)[pos - 1];
+  if (EXPR_HAS_LOCATION (arg))
+   richloc.add_range (EXPR_LOCATION (arg), false);
+}
+
+  warning_at_rich_loc_n (, OPT_Wrestrict, arg_positions_len,
+"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, arg_positions_len);
+
+  free (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 c88619b..45b7439 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -923,6 +923,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 *);
 
 /* 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..8a4bf6f 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_I,   BADLEN,  BADLEN,  

Re: PR35503 - warn for restrict pointer

[Joseph Myers]

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 *
> since vec 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 * 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 __gcc_vec_int__;" or similar, and the code looks up 
__gcc_vec_int__.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 20 September 2016 at 18:31, Joseph Myers  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 *.
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 *
since vec 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 * 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
> jos...@codesourcery.com
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 *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 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 (, 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, _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 *);
 
 /* 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
--- 

Re: PR35503 - warn for restrict pointer

[Joseph Myers]

On Tue, 20 Sep 2016, Martin Sebor wrote:

> > That's much harder to support in format checking (which expects one length
> > modifier, not a combination like that).
> 
> I haven't looked into it detail but since the format checker supports
> one-to-two character sequences of length modifiers (h or hh, etc) it
> should be possible to extend it to handle one-to-three character
> sequences (h, hV, or hhV, etc.)  The V character would not be a new
> length modifier on its own but instead be recognized as part of
> a longer length modifier sequence.  Do you see a problem with that?

You could arrange for length modifiers to have extra variants like that 
(at present they have single and double variants listed), but it's clearly 
more complicated than a modifier that's used on its own.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: PR35503 - warn for restrict pointer

[Martin Sebor]

On 09/20/2016 07:00 AM, Joseph Myers wrote:

On Tue, 20 Sep 2016, Martin Sebor wrote:


That said, since this specifier formats a vec, it seems that
it might be useful to be able to format vectors of other elements,
such as short and long.  With that in mind, would adding a new V
length modifier instead be a more regular way to extend the pretty
printer to these types?  The V length modifier would have to be
accepted in conjunction with other length modifiers (h, l, etc
and type specifiers (d, i, o, etc.).  E.g, "%hVu" would accept


That's much harder to support in format checking (which expects one length
modifier, not a combination like that).


I haven't looked into it detail but since the format checker supports
one-to-two character sequences of length modifiers (h or hh, etc) it
should be possible to extend it to handle one-to-three character
sequences (h, hV, or hhV, etc.)  The V character would not be a new
length modifier on its own but instead be recognized as part of
a longer length modifier sequence.  Do you see a problem with that?

Martin

Re: PR35503 - warn for restrict pointer

[Joseph Myers]

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*).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: PR35503 - warn for restrict pointer

[Joseph Myers]

On Tue, 20 Sep 2016, Martin Sebor wrote:

> That said, since this specifier formats a vec, it seems that
> it might be useful to be able to format vectors of other elements,
> such as short and long.  With that in mind, would adding a new V
> length modifier instead be a more regular way to extend the pretty
> printer to these types?  The V length modifier would have to be
> accepted in conjunction with other length modifiers (h, l, etc
> and type specifiers (d, i, o, etc.).  E.g, "%hVu" would accept

That's much harder to support in format checking (which expects one length 
modifier, not a combination like that).

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 20 September 2016 at 08:57, Martin Sebor  wrote:
>> and used
>> pp_format for formatting arg_positions by adding specifier %I (name
>> chosen arbitrarily).
>> Does that look OK ?
>
>
> 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':
>
> The comment just above pp_format that lists the format specifiers
> understood by the function should be updated.  There probably (I
> hope) is more formal documentation of the format specifiers
> somewhere else that should be updated as well.
>
> That said, since this specifier formats a vec, it seems that
> it might be useful to be able to format vectors of other elements,
> such as short and long.  With that in mind, would adding a new V
> length modifier instead be a more regular way to extend the pretty
> printer to these types?  The V length modifier would have to be
> accepted in conjunction with other length modifiers (h, l, etc
> and type specifiers (d, i, o, etc.).  E.g, "%hVu" would accept
> a vec*  as an argument and format it as a sequence
> of decimal numbers, and "%lVx" would do the same for vec*
> formatting them in hex.
Hi,
Thanks everyone for the suggestions.

This patch does the following:
a) adds warning_at_rich_loc_n(). Somehow I missed David's clear explanation of
warning_at_rich_loc_n earlier :(

b) clears TREE_VISITED flag in C/C++ FE after we are done with
warn_for_restrict().
I assumed that TREE_VISITED would be treated as a "scratch" flag and any pass
wishing to use it must set it first, so didn't bother clearing at first.

c) It appears '%I' is already used for some format specifier in
c-family/c-format.c ?
I changed name to %Z instead. Martin suggested a better idea above to
implement %Z as a length modifier so we can extend it to print vec of other
types, like %hZx would print vec formatted in hex.
I am not sure though if it would be a simple fix, and left it as a FIXME
in the patch adding %Z just to print vec, maybe we could revisit it later ?
Could someone please take a look at the change to c-format.c, I am not sure
if I have added that correctly.

Thanks,
Prathamesh
>
> Martin
>
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 *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 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 (, 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, _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 *);
 
 /* 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..b27d34f 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,  

Re: PR35503 - warn for restrict pointer

[Pedro Alves]

On 09/18/2016 06:16 PM, Prathamesh Kulkarni wrote:
> +  warning_at_rich_loc (, OPT_Wrestrict, "passing argument %i to"
> +" restrict-qualified parameter aliases with argument%s 
> %I",
> +param_pos + 1, (arg_positions.length() > 1) ? "s" : "",
> +_positions); 
> +}

This way of building a plural string is not i18n friendly:

 https://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html

I see that there are _n suffixed versions of a few diagnostic
functions, like inform_n, warning_n, etc. that handle plurals
by calling ngettext in diagnostic_n_impl, but it looks like a
warning_at_rich_loc_n variant of warning_at_rich_loc is missing.

Thanks,
Pedro Alves

Re: PR35503 - warn for restrict pointer

[Martin Sebor]

and used
pp_format for formatting arg_positions by adding specifier %I (name
chosen arbitrarily).
Does that look OK ?


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':

The comment just above pp_format that lists the format specifiers
understood by the function should be updated.  There probably (I
hope) is more formal documentation of the format specifiers
somewhere else that should be updated as well.

That said, since this specifier formats a vec, it seems that
it might be useful to be able to format vectors of other elements,
such as short and long.  With that in mind, would adding a new V
length modifier instead be a more regular way to extend the pretty
printer to these types?  The V length modifier would have to be
accepted in conjunction with other length modifiers (h, l, etc
and type specifiers (d, i, o, etc.).  E.g, "%hVu" would accept
a vec*  as an argument and format it as a sequence
of decimal numbers, and "%lVx" would do the same for vec*
formatting them in hex.

Martin

Re: PR35503 - warn for restrict pointer

[Joseph Myers]

On Mon, 19 Sep 2016, David Malcolm wrote:

> But does the new specifier need to be added to c-family/c-format.c, so
> that stage 2 and 3 compilers do know about it?

Yes.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: PR35503 - warn for restrict pointer

[David Malcolm]

On Sun, 2016-09-18 at 22:46 +0530, Prathamesh Kulkarni wrote:
> On 2 September 2016 at 23:14, David Malcolm 
> 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 *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 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 (_obstack);
> > +  char *fmt = (char *) obstack_alloc (_obstack, 0);
> > +
> > +  char num[32];
> > +  sprintf (num, "%u", param_pos + 1);
> > +
> > +  obstack_grow (_obstack, "passing argument ",
> > +   strlen ("passing argument "));
> > +  obstack_grow (_obstack, num, strlen (num));
> > +  obstack_grow (_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 (_obstack, 's');
> > +  obstack_1grow (_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 (_obstack, num, strlen (num));
> > +
> > +  if (i < arg_positions.length () - 1)
> > +   obstack_grow (_obstack, ", ",  strlen (", "));
> > +}
> > +
> > +  obstack_1grow (_obstack, 0);
> > +  warning_at_rich_loc (, OPT_Wrestrict, fmt);
> > +  obstack_free (_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 (, 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 (, 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,
> > 
> >  _positions);
> > 
> > 
> > and have %FIXME (or somesuch) consume _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
> > 

Re: PR35503 - warn for restrict pointer

[David Malcolm]

On Mon, 2016-09-19 at 14:21 -0400, Jason Merrill wrote:
> On Sun, Sep 18, 2016 at 1:16 PM, Prathamesh Kulkarni
>  wrote:
> > 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*)’:
> > ../../gcc/gcc/c-family/c-common.c:13132:24: warning: unknown
> > conversion type character ‘I’ in format [-Wformat=]
> 
> This warning (in stage 1) happens whenever we add a new specifier,
> don't worry about it.

Right: there's no way the stage 1 compiler could know about a new
specifier.

But does the new specifier need to be added to c-family/c-format.c, so
that stage 2 and 3 compilers do know about it?

Re: PR35503 - warn for restrict pointer

[Jason Merrill]

On Sun, Sep 18, 2016 at 1:16 PM, Prathamesh Kulkarni
 wrote:
> 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*)’:
> ../../gcc/gcc/c-family/c-common.c:13132:24: warning: unknown
> conversion type character ‘I’ in format [-Wformat=]

This warning (in stage 1) happens whenever we add a new specifier,
don't worry about it.

+ FOR_EACH_VEC_SAFE_ELT (args, i, arg)
+  TREE_VISITED (arg) = 0;

Hmm, so you're clearing TREE_VISITED before you check the arguments
and leaving it set afterward?  That seems like a problem, a lot of
code assumes that TREE_VISITED has been cleared already and clears it
after.

Jason

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 2 September 2016 at 23:14, David Malcolm  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 *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 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 (_obstack);
> +  char *fmt = (char *) obstack_alloc (_obstack, 0);
> +
> +  char num[32];
> +  sprintf (num, "%u", param_pos + 1);
> +
> +  obstack_grow (_obstack, "passing argument ",
> +   strlen ("passing argument "));
> +  obstack_grow (_obstack, num, strlen (num));
> +  obstack_grow (_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 (_obstack, 's');
> +  obstack_1grow (_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 (_obstack, num, strlen (num));
> +
> +  if (i < arg_positions.length () - 1)
> +   obstack_grow (_obstack, ", ",  strlen (", "));
> +}
> +
> +  obstack_1grow (_obstack, 0);
> +  warning_at_rich_loc (, OPT_Wrestrict, fmt);
> +  obstack_free (_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 (, 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 (, 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,
>
>  _positions);
>
>
> and have %FIXME (or somesuch) consume _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 
> 

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 1 September 2016 at 21:28, Martin Sebor  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 ?
>
>
> Awesome!  I've wished for years for a warning like this!
Thanks for experimenting with the patch!
>
> I'm curious if you've tested other examples from 6.7.3.1 of C11
> besides Example 3.  Example 4 seems like something GCC should be
> able to detect but I didn't get a warning with the patch.
Oops, I wasn't aware about example 4, and only implemented the warning
for function call. IIUC, the assignment p = q will result in undefined behavior
if q is qualified with restrict and p is at an outer-scope relative to q ?
>
> I would expect the warning to be especially valuable with string
> manipulation functions like memcpy that have undefined behavior
> for overlapping regions, such as in:
>
>   char a[4];
>
>   void g (void)
>   {
> __builtin_memcpy (a, a + 1, 3);
>   }
>
> But here, too, I didn't get a warning.  I understand that this
> case cannot be handled for arbitrary functions whose semantics
> aren't known but with standard functions for which GCC provides
> intrinsics the effects are known and aliasing violations can in
> common cases be detected.
Indeed, thanks for the suggestions. I will add support for some
standard functions in a follow-up patch.

Thanks,
Prathamesh
>
> Martin

Re: PR35503 - warn for restrict pointer

[Manuel López-Ibáñez]

On 02/09/16 18:44, David Malcolm wrote:

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 (, 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,

 _positions);


Yes, building up diagnostic messages from pieces is discouraged: 
https://gcc.gnu.org/codingconventions.html#Diagnostics



and have %FIXME (or somesuch) consume _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.


Is it possible to pass template arguments through ... ? And how does va_arg 
know the type of the particular template passed?



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??).


I'm surprised we don't have a function pp_vec to print/debug a vec<>, but 
perhaps it is simpler to convert arg_pos to a 'char *' and use %s instead of 
%FIXME or call-backs.


Cheers,

Manuel.

Re: PR35503 - warn for restrict pointer

[David Malcolm]

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 *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 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 (_obstack);
+  char *fmt = (char *) obstack_alloc (_obstack, 0);
+
+  char num[32];
+  sprintf (num, "%u", param_pos + 1);
+
+  obstack_grow (_obstack, "passing argument ",
+   strlen ("passing argument "));
+  obstack_grow (_obstack, num, strlen (num));
+  obstack_grow (_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 (_obstack, 's');
+  obstack_1grow (_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 (_obstack, num, strlen (num));
+
+  if (i < arg_positions.length () - 1)
+   obstack_grow (_obstack, ", ",  strlen (", "));
+}
+
+  obstack_1grow (_obstack, 0);
+  warning_at_rich_loc (, OPT_Wrestrict, fmt);
+  obstack_free (_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 (, 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 (, 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,

 _positions);


and have %FIXME (or somesuch) consume _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.

[...]

[1] which seems to be locale-dependent itself:
https://en.wikipedia.org/wiki/Serial_comma#Other_languages

Re: PR35503 - warn for restrict pointer

[Martin Sebor]

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 ?


Awesome!  I've wished for years for a warning like this!

I'm curious if you've tested other examples from 6.7.3.1 of C11
besides Example 3.  Example 4 seems like something GCC should be
able to detect but I didn't get a warning with the patch.

I would expect the warning to be especially valuable with string
manipulation functions like memcpy that have undefined behavior
for overlapping regions, such as in:

  char a[4];

  void g (void)
  {
__builtin_memcpy (a, a + 1, 3);
  }

But here, too, I didn't get a warning.  I understand that this
case cannot be handled for arbitrary functions whose semantics
aren't known but with standard functions for which GCC provides
intrinsics the effects are known and aliasing violations can in
common cases be detected.

Martin

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 1 September 2016 at 12:25, Richard Biener  wrote:
> On Tue, 30 Aug 2016, Tom de Vries wrote:
>
>> On 30/08/16 17:34, Prathamesh Kulkarni wrote:
>> > On 30 August 2016 at 20:24, Tom de Vries  wrote:
>> > > On 26/08/16 13:39, Prathamesh Kulkarni wrote:
>> > > >
>> > > > Hi,
>> > > > The following patch adds option -Wrestrict that warns when an argument
>> > > > is passed to a restrict qualified parameter and it aliases with
>> > > > another argument.
>> > > >
>> > > > eg:
>> > > > int foo (const char *__restrict buf, const char *__restrict fmt, ...);
>> > > >
>> > > > void f(void)
>> > > > {
>> > > >   char buf[100] = "hello";
>> > > >   foo (buf, "%s-%s", buf, "world");
>> > > > }
>> > >
>> > >
>> > > Does -Wrestrict generate a warning for this example?
>> > >
>> > > ...
>> > > void h(int n, int * restrict p, int * restrict q, int * restrict r)
>> > > {
>> > >   int i;
>> > >   for (i = 0; i < n; i++)
>> > > p[i] = q[i] + r[i];
>> > > }
>> > >
>> > > h (100, a, b, b)
>> > > ...
>> > >
>> > > [ Note that this is valid C, and does not violate the restrict 
>> > > definition.
>> > > ]
>> > >
>> > > If -Wrestrict indeed generates a warning, then we should be explicit in
>> > > the
>> > > documentation that while the warning triggers on this type of example, 
>> > > the
>> > > code is correct.
>> > I am afraid it would warn for the above case. The patch just checks if
>> > the parameter is qualified
>> > with restrict, and if the corresponding argument has aliases in the
>> > call (by calling operand_equal_p).
>>
>> > Is such code common in practice ?
>>
>> [ The example is from the definition of restrict in the c99 standard. ]
>>
>> According to the definition of restrict, only the restrict on p is required 
>> to
>> know that p doesn't alias with q and that p doesn't alias with r.
>>
>> AFAIK the current implementation of gcc already generates optimal code if
>> restrict is only on p. But earlier versions (and possibly other compilers as
>> well?) need the restrict on q and r as well.
>>
>> So I expect this code to occur.
>>
>> > If it is, I wonder if we should keep
>> > the warning in -Wall ?
>> >
>>
>> Hmm, I wonder if we can use the const keyword to silence the warning.
>>
>> So if we generate a warning for:
>> ...
>> void h(int n, int * restrict p, int * restrict q, int * restrict r)
>> {
>>   int i;
>>   for (i = 0; i < n; i++)
>> p[i] = q[i] + r[i];
>> }
>> h (100, a, b, b)
>> ...
>>
>> but not for:
>> ...
>> void h(int n, int * restrict p, const int * restrict q, const int * restrict
>> r)
>> {
>>   int i;
>>   for (i = 0; i < n; i++)
>> p[i] = q[i] + r[i];
>> }
>> h (100, a, b, b)
>> ...
>>
>> Then there's an easy way to rewrite the example such that the warning doesn't
>> trigger, without having to remove the restrict.
>
> Note that I've seen restrict being used even for
>
> void h(int n, int * restrict p, int * restrict q)
> {
>   int i;
>   for (i = 0; i < n; i++)
> p[2*i] = p[2*i] + q[2*i + 1];
> }
>
> thus where the actual accesses do not alias (the pointers are used
> to access every other element).  I think the definition of "object"
> (based on accessed lvalues) makes this example valid.  So we
> shouldn't warn about
>
> h (n, p, p)
>
> but we could warn about
>
> h (n, p, p + 1)
>
> and yes, only one of the pointers need to be restrict qualified.
>
> Note that as with all other alias warnings if you want to avoid
> false positives and rely on conservative analysis then you can
> as well simply avoid taking advantate of the bug in the code
> (as we do for TBAA and trivial must-alias cases).  If you allow
> false positives then you ultimatively end up with a mess like
> our existing -Wstrict-aliasing warnings (which are IMHO quite
> useless).
>
> Note that nowhere in GCC we implement anything closely matching
> the formal definition of restrict as writte in the C standard --
> only in fronted code could we properly derive the 'based-on'
> property and note lvalues affected by restrict.  Currently we
> are restricted to looking at restrict qualified parameters
> because of this.
Hi,
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 

Re: PR35503 - warn for restrict pointer

[Richard Biener]

On Tue, 30 Aug 2016, Tom de Vries wrote:

> On 30/08/16 17:34, Prathamesh Kulkarni wrote:
> > On 30 August 2016 at 20:24, Tom de Vries  wrote:
> > > On 26/08/16 13:39, Prathamesh Kulkarni wrote:
> > > > 
> > > > Hi,
> > > > The following patch adds option -Wrestrict that warns when an argument
> > > > is passed to a restrict qualified parameter and it aliases with
> > > > another argument.
> > > > 
> > > > eg:
> > > > int foo (const char *__restrict buf, const char *__restrict fmt, ...);
> > > > 
> > > > void f(void)
> > > > {
> > > >   char buf[100] = "hello";
> > > >   foo (buf, "%s-%s", buf, "world");
> > > > }
> > > 
> > > 
> > > Does -Wrestrict generate a warning for this example?
> > > 
> > > ...
> > > void h(int n, int * restrict p, int * restrict q, int * restrict r)
> > > {
> > >   int i;
> > >   for (i = 0; i < n; i++)
> > > p[i] = q[i] + r[i];
> > > }
> > > 
> > > h (100, a, b, b)
> > > ...
> > > 
> > > [ Note that this is valid C, and does not violate the restrict definition.
> > > ]
> > > 
> > > If -Wrestrict indeed generates a warning, then we should be explicit in
> > > the
> > > documentation that while the warning triggers on this type of example, the
> > > code is correct.
> > I am afraid it would warn for the above case. The patch just checks if
> > the parameter is qualified
> > with restrict, and if the corresponding argument has aliases in the
> > call (by calling operand_equal_p).
> 
> > Is such code common in practice ?
> 
> [ The example is from the definition of restrict in the c99 standard. ]
> 
> According to the definition of restrict, only the restrict on p is required to
> know that p doesn't alias with q and that p doesn't alias with r.
> 
> AFAIK the current implementation of gcc already generates optimal code if
> restrict is only on p. But earlier versions (and possibly other compilers as
> well?) need the restrict on q and r as well.
> 
> So I expect this code to occur.
> 
> > If it is, I wonder if we should keep
> > the warning in -Wall ?
> > 
> 
> Hmm, I wonder if we can use the const keyword to silence the warning.
> 
> So if we generate a warning for:
> ...
> void h(int n, int * restrict p, int * restrict q, int * restrict r)
> {
>   int i;
>   for (i = 0; i < n; i++)
> p[i] = q[i] + r[i];
> }
> h (100, a, b, b)
> ...
> 
> but not for:
> ...
> void h(int n, int * restrict p, const int * restrict q, const int * restrict
> r)
> {
>   int i;
>   for (i = 0; i < n; i++)
> p[i] = q[i] + r[i];
> }
> h (100, a, b, b)
> ...
> 
> Then there's an easy way to rewrite the example such that the warning doesn't
> trigger, without having to remove the restrict.

Note that I've seen restrict being used even for

void h(int n, int * restrict p, int * restrict q)
{
  int i;
  for (i = 0; i < n; i++)
p[2*i] = p[2*i] + q[2*i + 1];
}

thus where the actual accesses do not alias (the pointers are used
to access every other element).  I think the definition of "object"
(based on accessed lvalues) makes this example valid.  So we
shouldn't warn about

h (n, p, p)

but we could warn about

h (n, p, p + 1)

and yes, only one of the pointers need to be restrict qualified.

Note that as with all other alias warnings if you want to avoid
false positives and rely on conservative analysis then you can
as well simply avoid taking advantate of the bug in the code
(as we do for TBAA and trivial must-alias cases).  If you allow
false positives then you ultimatively end up with a mess like
our existing -Wstrict-aliasing warnings (which are IMHO quite
useless).

Note that nowhere in GCC we implement anything closely matching
the formal definition of restrict as writte in the C standard --
only in fronted code could we properly derive the 'based-on'
property and note lvalues affected by restrict.  Currently we
are restricted to looking at restrict qualified parameters
because of this.

Richard.

Re: PR35503 - warn for restrict pointer

[Fabien Chêne]

2016-08-30 17:34 GMT+02:00 Prathamesh Kulkarni :
> On 30 August 2016 at 20:24, Tom de Vries  wrote:
>> On 26/08/16 13:39, Prathamesh Kulkarni wrote:
>>>
>>> Hi,
>>> The following patch adds option -Wrestrict that warns when an argument
>>> is passed to a restrict qualified parameter and it aliases with
>>> another argument.
>>>
>>> eg:
>>> int foo (const char *__restrict buf, const char *__restrict fmt, ...);
>>>
>>> void f(void)
>>> {
>>>   char buf[100] = "hello";
>>>   foo (buf, "%s-%s", buf, "world");
>>> }
>>
>>
>> Does -Wrestrict generate a warning for this example?
>>
>> ...
>> void h(int n, int * restrict p, int * restrict q, int * restrict r)
>> {
>>   int i;
>>   for (i = 0; i < n; i++)
>> p[i] = q[i] + r[i];
>> }
>>
>> h (100, a, b, b)
>> ...
>>
>> [ Note that this is valid C, and does not violate the restrict definition. ]
>>
>> If -Wrestrict indeed generates a warning, then we should be explicit in the
>> documentation that while the warning triggers on this type of example, the
>> code is correct.
> I am afraid it would warn for the above case. The patch just checks if
> the parameter is qualified
> with restrict, and if the corresponding argument has aliases in the
> call (by calling operand_equal_p).
> Is such code common in practice ? If it is, I wonder if we should keep
> the warning in -Wall ?
>
> To avoid such false positives, we would need to track which arguments
> are modified by the call.
> I suppose that could be done with ipa mod/ref analysis (and moving the
> warning to middle-end) ?
> I tried looking into gcc sources but couldn't find where it's implemented.

I don't know either which pass can be used, but I guess it should be doable.
If so, a first step would be to determine useless restrict-qualified
args (and possibly warn for them), and then perform the current
-Wrestrict analysis only on non-useless restrict-qualified args ?

--
Fabien

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 30 August 2016 at 18:49, David Malcolm  wrote:
> On Tue, 2016-08-30 at 17:08 +0530, Prathamesh Kulkarni wrote:
>> On 30 August 2016 at 05:34, David Malcolm 
>> wrote:
>> > On Mon, 2016-08-29 at 20:01 -0400, David Malcolm wrote:
>> > > On Mon, 2016-08-29 at 19:55 -0400, David Malcolm wrote:
>> > > [...]
>> > > > Assuming you have the location_t values available, you can
>> > > > create a
>> > > > rich_location for the primary range, and then add secondary
>> > > > ranges
>> > > > like
>> > > > this:
>> > > >
>> > > >   rich_location richloc (loc_of_arg1);
>> > >
>> > > Oops, the above should be:
>> > >
>> > > rich_location richloc (line_table, loc_of_arg1);
>> > >
>> > > or:
>> > >
>> > > gcc_rich_location (loc_of_arg1);
>> > and this should be:
>> >
>> >  gcc_rich_location richloc (loc_of_arg1);
>> > > which does the same thing (#include "gcc-rich-location.h").
>> >
>> > Clearly I need to sleep :)
>> Hi David,
>> Thanks for the suggestions. I can now see multiple source ranges for
>> pr35503-2.c (included in patch).
>> Output shows: http://pastebin.com/FNAVDU8A
>> (Posted pastebin link to avoid mangling by the mailer)
>
> The underlines look great, thanks for working on this.
Thanks -;)
>
>> However the test for underline fails:
>> FAIL: c-c++-common/pr35503-2.c  -Wc++-compat   expected multiline
>> pattern lines 12-13 not found: "\s*f \(, , ,
>> \);.*\n  \^~ ~~  ~~ .*\n"
>> I have attached gcc.log for the test-case. Presumably I have written
>> the test-case incorrectly.
>> Could you please have a look at it ?
>
> (I hope this doesn't get too badly mangled by Evolution...)
>
> I think you have an extra trailing space on the line containing the
> expected underlines within the multiline directive:
>
> +/* { dg-begin-multiline-output "" }
> +   f (, , , );
> +  ^~ ~~  ~~
> ^ EXTRA SPACE HERE
> +   { dg-end-multiline-output "" } */
> +}
>
> as the actual output is:
>
>f (, , , );
>   ^~ ~~  ~~
>   ^ LINE ENDS HERE, with no trailing
> space present
>
> This space shows up in the error here:
>
> FAIL: c-c++-common/pr35503-2.c  -Wc++-compat   expected multiline
> pattern lines 12-13 not found: "\s*f \(, , ,
> \);.*\n  \^~ ~~  ~~ .*\n"
>  ^ EXTRA SPACE
>
> BTW, the .* at the end of the pattern means it's ok to have additional
> material in the actual output that isn't matched (e.g. for comments
> containing dg- directives [1] ) but it doesn't work the other way
> around: all of the text within the dg-begin/end-multiline directives
> has to be in the actual output.
>
>
> [1] so you can have e.g.:
>
>   f (, , , ); /* { dg-warning "passing argument 1 to 
> restrict-qualified parameter aliases with arguments 3, 4" } */
>
> and:
>
> /* { dg-begin-multiline-output "" }
>f (, , , );
>   ^~ ~~  ~~
>{ dg-end-multiline-output "" } */
>
> where the actual output will look like:
>
> pr35503-2.c:8:6: warning: passing argument 1 to restrict-qualified parameter 
> aliases with arguments 3, 4 [-Wrestrict]
>f (, , , ); /* { dg-warning "passing argument 1 to 
> restrict-qualified parameter aliases with arguments 3, 4" } */
>  ^~ ~~  ~~
>
> and you can omit the copy of the dg-warning directive in the expected
> multiline output (which would otherwise totally confuse DejaGnu).
> Doing so avoids having to specify the line number.
Thanks for the detailed explanation! The test-case is now fixed.

Regards,
Prathamesh
>
>> Thanks,
>> Prathamesh
>> >
>> > > >   richloc.add_range (loc_of_arg3, false);  /* false here =
>> > > > don't
>> > > > draw
>> > > > a
>> > > > caret, just the underline */
>> > > >   richloc.add_range (loc_of_arg4, false);
>> > > >   warning_at_rich_loc (, OPT_Wrestrict, etc...
>> > > >
>> > > > See line-map.h for more information on rich_location.
>> > >
>> > > [...]

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Prathamesh Kulkarni]

On 31 August 2016 at 03:45, Mike Stump  wrote:
> On Aug 30, 2016, at 4:57 AM, Prathamesh Kulkarni 
>  wrote:
>>
>> On 30 August 2016 at 17:11, Eric Gallager  wrote:
>>> On 8/29/16, Jason Merrill  wrote:
 On Mon, Aug 29, 2016 at 10:28 AM, Marek Polacek  wrote:
> On Mon, Aug 29, 2016 at 09:20:53AM -0400, Eric Gallager wrote:
>> I tried this patch on my fork of gdb-binutils and got a few warnings
>> from it. Would it be possible to have the caret point to the argument
>> mentioned, instead of the function name? And also print the option
>> name? E.g., instead of the current:
>>
>> or32-opc.c: In function ‘or32_print_register’:
>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>> parameter aliases with argument 3
>>   sprintf (disassembled, "%sr%d", disassembled, regnum);
>>   ^~~
>>
>> could it look like:
>>
>> or32-opc.c: In function ‘or32_print_register’:
>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>> parameter aliases with argument 3 [-Wrestrict]
>>   sprintf (disassembled, "%sr%d", disassembled, regnum);
>>^~~~
>>
>> instead?
>
> I didn't try to implement it, but I think this should be fairly easy to
> achieve in the C FE, because c_parser_postfix_expression_after_primary
> has arg_loc, which is a vector of parameter locations.

 The C++ FE doesn't have this currently, but it could be added without
 too much trouble: in cp_parser_parenthesized_expression_list, extract
 the locations from the cp_expr return value of
 cp_parser_assignment_expression, and then pass the locations back up
 to cp_parser_postfix_expression.

 Jason

>>>
>>>
>>> On the topic of how to get this warning working with various
>>> frontends, is there any reason why the Objective C frontend doesn't
>>> handle -Wrestrict? Currently when trying to use it, it just says:
>>>
>>> cc1obj: warning: command line option '-Wrestrict' is valid for C/C++
>>> but not for ObjC
>> Hi Eric,
>> I am not sure if restrict is valid for ObjC/Obj-C++ and hence didn't
>> add the option for these front-ends.
>> If it is valid, I will enable the option for ObjC and Obj-C++.
>
> This is wrong, C/C++ options should always be ObjC/ObjC++ options.
Thanks, I will add the warning for ObjC and ObjC++.

Thanks,
Prathamesh
>

Re: PR35503 - warn for restrict pointer

[Tom de Vries]

On 30/08/16 16:54, Tom de Vries wrote:

On 26/08/16 13:39, Prathamesh Kulkarni wrote:

Hi,
The following patch adds option -Wrestrict that warns when an argument
is passed to a restrict qualified parameter and it aliases with
another argument.

eg:
int foo (const char *__restrict buf, const char *__restrict fmt, ...);

void f(void)
{
  char buf[100] = "hello";
  foo (buf, "%s-%s", buf, "world");
}


Does -Wrestrict generate a warning for this example?

...
void h(int n, int * restrict p, int * restrict q, int * restrict r)
{
  int i;
  for (i = 0; i < n; i++)
p[i] = q[i] + r[i];
}

h (100, a, b, b)
...

[ Note that this is valid C, and does not violate the restrict
definition. ]

If -Wrestrict indeed generates a warning, then we should be explicit in
the documentation that while the warning triggers on this type of
example, the code is correct.


So, what I had in mind is something like this:
...
Warn when an argument passed to a restrict-qualified parameter pointing 
to a non-const type aliases with another argument.


The warning will trigger for the following illegal use of restrict:

  void foo (int *restrict a, int *b) { *a = 1; *b = 2; }
  void foo2 (void) { int o; foo (, ); }

The use of restrict is illegal because in the lifetime of the scope of 
the restrict pointer 'a' both:

- the object 'o' that the restrict pointer 'a' points to is modified,
  and
- the object 'o' is accessed through pointer 'b', which is not based on
  restrict pointer 'a'.

However, the warning will also trigger for the following legal use of 
restrict:


  int foo (int *restrict a, int *b) { return *a + *b; }
  int foo2 (void) { int o = 1; return foo (, ); }

The use of restrict is legal, because the object 'o' that the restrict 
pointer 'a' points to is not modified  in the lifetime of the scope of 
the restrict pointer 'a'. The warning can be silenced by changing the 
type of 'a' to 'const int *restrict'.

...

Thanks,
- Tom

Re: PR35503 - warn for restrict pointer

[David Malcolm]

On Tue, 2016-08-30 at 23:26 +0530, Prathamesh Kulkarni wrote:
[...]

> The attached (untested) patch silences the warning if parameter is
> const qualified.
> I will give it some testing after resolving a different issue.

I've now removed the hard-coded limit of 3 ranges per rich_location, so
you should now be able to add an arbitrary number of underlined ranges
to the diagnostic (r239879 removes the limit).

Dave

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Mike Stump]

On Aug 30, 2016, at 4:57 AM, Prathamesh Kulkarni 
 wrote:
> 
> On 30 August 2016 at 17:11, Eric Gallager  wrote:
>> On 8/29/16, Jason Merrill  wrote:
>>> On Mon, Aug 29, 2016 at 10:28 AM, Marek Polacek  wrote:
 On Mon, Aug 29, 2016 at 09:20:53AM -0400, Eric Gallager wrote:
> I tried this patch on my fork of gdb-binutils and got a few warnings
> from it. Would it be possible to have the caret point to the argument
> mentioned, instead of the function name? And also print the option
> name? E.g., instead of the current:
> 
> or32-opc.c: In function ‘or32_print_register’:
> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
> parameter aliases with argument 3
>   sprintf (disassembled, "%sr%d", disassembled, regnum);
>   ^~~
> 
> could it look like:
> 
> or32-opc.c: In function ‘or32_print_register’:
> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
> parameter aliases with argument 3 [-Wrestrict]
>   sprintf (disassembled, "%sr%d", disassembled, regnum);
>^~~~
> 
> instead?
 
 I didn't try to implement it, but I think this should be fairly easy to
 achieve in the C FE, because c_parser_postfix_expression_after_primary
 has arg_loc, which is a vector of parameter locations.
>>> 
>>> The C++ FE doesn't have this currently, but it could be added without
>>> too much trouble: in cp_parser_parenthesized_expression_list, extract
>>> the locations from the cp_expr return value of
>>> cp_parser_assignment_expression, and then pass the locations back up
>>> to cp_parser_postfix_expression.
>>> 
>>> Jason
>>> 
>> 
>> 
>> On the topic of how to get this warning working with various
>> frontends, is there any reason why the Objective C frontend doesn't
>> handle -Wrestrict? Currently when trying to use it, it just says:
>> 
>> cc1obj: warning: command line option '-Wrestrict' is valid for C/C++
>> but not for ObjC
> Hi Eric,
> I am not sure if restrict is valid for ObjC/Obj-C++ and hence didn't
> add the option for these front-ends.
> If it is valid, I will enable the option for ObjC and Obj-C++.

This is wrong, C/C++ options should always be ObjC/ObjC++ options.

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 30 August 2016 at 22:58, Tom de Vries  wrote:
> On 30/08/16 17:34, Prathamesh Kulkarni wrote:
>>
>> On 30 August 2016 at 20:24, Tom de Vries  wrote:
>>>
>>> On 26/08/16 13:39, Prathamesh Kulkarni wrote:


 Hi,
 The following patch adds option -Wrestrict that warns when an argument
 is passed to a restrict qualified parameter and it aliases with
 another argument.

 eg:
 int foo (const char *__restrict buf, const char *__restrict fmt, ...);

 void f(void)
 {
   char buf[100] = "hello";
   foo (buf, "%s-%s", buf, "world");
 }
>>>
>>>
>>>
>>> Does -Wrestrict generate a warning for this example?
>>>
>>> ...
>>> void h(int n, int * restrict p, int * restrict q, int * restrict r)
>>> {
>>>   int i;
>>>   for (i = 0; i < n; i++)
>>> p[i] = q[i] + r[i];
>>> }
>>>
>>> h (100, a, b, b)
>>> ...
>>>
>>> [ Note that this is valid C, and does not violate the restrict
>>> definition. ]
>>>
>>> If -Wrestrict indeed generates a warning, then we should be explicit in
>>> the
>>> documentation that while the warning triggers on this type of example,
>>> the
>>> code is correct.
>>
>> I am afraid it would warn for the above case. The patch just checks if
>> the parameter is qualified
>> with restrict, and if the corresponding argument has aliases in the
>> call (by calling operand_equal_p).
>
>
>> Is such code common in practice ?
>
>
> [ The example is from the definition of restrict in the c99 standard. ]
>
> According to the definition of restrict, only the restrict on p is required
> to know that p doesn't alias with q and that p doesn't alias with r.
>
> AFAIK the current implementation of gcc already generates optimal code if
> restrict is only on p. But earlier versions (and possibly other compilers as
> well?) need the restrict on q and r as well.
>
> So I expect this code to occur.
>
>> If it is, I wonder if we should keep
>> the warning in -Wall ?
>>
>
> Hmm, I wonder if we can use the const keyword to silence the warning.
>
> So if we generate a warning for:
> ...
> void h(int n, int * restrict p, int * restrict q, int * restrict r)
> {
>   int i;
>   for (i = 0; i < n; i++)
> p[i] = q[i] + r[i];
> }
> h (100, a, b, b)
> ...
>
> but not for:
> ...
> void h(int n, int * restrict p, const int * restrict q, const int * restrict
> r)
> {
>   int i;
>   for (i = 0; i < n; i++)
> p[i] = q[i] + r[i];
> }
> h (100, a, b, b)
> ...
>
> Then there's an easy way to rewrite the example such that the warning
> doesn't trigger, without having to remove the restrict.
The attached (untested) patch silences the warning if parameter is
const qualified.
I will give it some testing after resolving a different issue.

Thanks,
Prathamesh
>
> Thanks,
> - Tom
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 3feb910..4239cef 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,86 @@ 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 *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 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 (_obstack);
+  char *fmt = (char *) obstack_alloc (_obstack, 0);
+
+  char num[32];
+  sprintf (num, "%u", param_pos + 1);
+
+  obstack_grow (_obstack, "passing argument ",
+   strlen ("passing argument "));
+  obstack_grow (_obstack, num, strlen (num));
+  obstack_grow (_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 (_obstack, 's');
+  obstack_1grow (_obstack, ' ');
+
+  unsigned pos;
+  unsigned ranges_added = 1;
+
+  FOR_EACH_VEC_ELT (arg_positions, i, pos)
+{
+  /* FIXME: Fow now, only 3 ranges can be added. Remove 

Re: PR35503 - warn for restrict pointer

[Tom de Vries]

On 30/08/16 17:34, Prathamesh Kulkarni wrote:

On 30 August 2016 at 20:24, Tom de Vries  wrote:

On 26/08/16 13:39, Prathamesh Kulkarni wrote:


Hi,
The following patch adds option -Wrestrict that warns when an argument
is passed to a restrict qualified parameter and it aliases with
another argument.

eg:
int foo (const char *__restrict buf, const char *__restrict fmt, ...);

void f(void)
{
  char buf[100] = "hello";
  foo (buf, "%s-%s", buf, "world");
}



Does -Wrestrict generate a warning for this example?

...
void h(int n, int * restrict p, int * restrict q, int * restrict r)
{
  int i;
  for (i = 0; i < n; i++)
p[i] = q[i] + r[i];
}

h (100, a, b, b)
...

[ Note that this is valid C, and does not violate the restrict definition. ]

If -Wrestrict indeed generates a warning, then we should be explicit in the
documentation that while the warning triggers on this type of example, the
code is correct.

I am afraid it would warn for the above case. The patch just checks if
the parameter is qualified
with restrict, and if the corresponding argument has aliases in the
call (by calling operand_equal_p).



Is such code common in practice ?


[ The example is from the definition of restrict in the c99 standard. ]

According to the definition of restrict, only the restrict on p is 
required to know that p doesn't alias with q and that p doesn't alias 
with r.


AFAIK the current implementation of gcc already generates optimal code 
if restrict is only on p. But earlier versions (and possibly other 
compilers as well?) need the restrict on q and r as well.


So I expect this code to occur.


If it is, I wonder if we should keep
the warning in -Wall ?



Hmm, I wonder if we can use the const keyword to silence the warning.

So if we generate a warning for:
...
void h(int n, int * restrict p, int * restrict q, int * restrict r)
{
  int i;
  for (i = 0; i < n; i++)
p[i] = q[i] + r[i];
}
h (100, a, b, b)
...

but not for:
...
void h(int n, int * restrict p, const int * restrict q, const int * 
restrict r)

{
  int i;
  for (i = 0; i < n; i++)
p[i] = q[i] + r[i];
}
h (100, a, b, b)
...

Then there's an easy way to rewrite the example such that the warning 
doesn't trigger, without having to remove the restrict.


Thanks,
- Tom

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 30 August 2016 at 20:24, Tom de Vries  wrote:
> On 26/08/16 13:39, Prathamesh Kulkarni wrote:
>>
>> Hi,
>> The following patch adds option -Wrestrict that warns when an argument
>> is passed to a restrict qualified parameter and it aliases with
>> another argument.
>>
>> eg:
>> int foo (const char *__restrict buf, const char *__restrict fmt, ...);
>>
>> void f(void)
>> {
>>   char buf[100] = "hello";
>>   foo (buf, "%s-%s", buf, "world");
>> }
>
>
> Does -Wrestrict generate a warning for this example?
>
> ...
> void h(int n, int * restrict p, int * restrict q, int * restrict r)
> {
>   int i;
>   for (i = 0; i < n; i++)
> p[i] = q[i] + r[i];
> }
>
> h (100, a, b, b)
> ...
>
> [ Note that this is valid C, and does not violate the restrict definition. ]
>
> If -Wrestrict indeed generates a warning, then we should be explicit in the
> documentation that while the warning triggers on this type of example, the
> code is correct.
I am afraid it would warn for the above case. The patch just checks if
the parameter is qualified
with restrict, and if the corresponding argument has aliases in the
call (by calling operand_equal_p).
Is such code common in practice ? If it is, I wonder if we should keep
the warning in -Wall ?

To avoid such false positives, we would need to track which arguments
are modified by the call.
I suppose that could be done with ipa mod/ref analysis (and moving the
warning to middle-end) ?
I tried looking into gcc sources but couldn't find where it's implemented.

Thanks,
Prathamesh
>
> Thanks,
> - Tom

Re: PR35503 - warn for restrict pointer

[Tom de Vries]

On 26/08/16 13:39, Prathamesh Kulkarni wrote:

Hi,
The following patch adds option -Wrestrict that warns when an argument
is passed to a restrict qualified parameter and it aliases with
another argument.

eg:
int foo (const char *__restrict buf, const char *__restrict fmt, ...);

void f(void)
{
  char buf[100] = "hello";
  foo (buf, "%s-%s", buf, "world");
}


Does -Wrestrict generate a warning for this example?

...
void h(int n, int * restrict p, int * restrict q, int * restrict r)
{
  int i;
  for (i = 0; i < n; i++)
p[i] = q[i] + r[i];
}

h (100, a, b, b)
...

[ Note that this is valid C, and does not violate the restrict definition. ]

If -Wrestrict indeed generates a warning, then we should be explicit in 
the documentation that while the warning triggers on this type of 
example, the code is correct.


Thanks,
- Tom

Re: PR35503 - warn for restrict pointer

[David Malcolm]

On Tue, 2016-08-30 at 17:08 +0530, Prathamesh Kulkarni wrote:
> On 30 August 2016 at 05:34, David Malcolm 
> wrote:
> > On Mon, 2016-08-29 at 20:01 -0400, David Malcolm wrote:
> > > On Mon, 2016-08-29 at 19:55 -0400, David Malcolm wrote:
> > > [...]
> > > > Assuming you have the location_t values available, you can
> > > > create a
> > > > rich_location for the primary range, and then add secondary
> > > > ranges
> > > > like
> > > > this:
> > > > 
> > > >   rich_location richloc (loc_of_arg1);
> > > 
> > > Oops, the above should be:
> > > 
> > > rich_location richloc (line_table, loc_of_arg1);
> > > 
> > > or:
> > > 
> > > gcc_rich_location (loc_of_arg1);
> > and this should be:
> > 
> >  gcc_rich_location richloc (loc_of_arg1);
> > > which does the same thing (#include "gcc-rich-location.h").
> > 
> > Clearly I need to sleep :)
> Hi David,
> Thanks for the suggestions. I can now see multiple source ranges for
> pr35503-2.c (included in patch).
> Output shows: http://pastebin.com/FNAVDU8A
> (Posted pastebin link to avoid mangling by the mailer)

The underlines look great, thanks for working on this.

> However the test for underline fails:
> FAIL: c-c++-common/pr35503-2.c  -Wc++-compat   expected multiline
> pattern lines 12-13 not found: "\s*f \(, , ,
> \);.*\n  \^~ ~~  ~~ .*\n"
> I have attached gcc.log for the test-case. Presumably I have written
> the test-case incorrectly.
> Could you please have a look at it ?

(I hope this doesn't get too badly mangled by Evolution...)

I think you have an extra trailing space on the line containing the
expected underlines within the multiline directive:

+/* { dg-begin-multiline-output "" }
+   f (, , , );
+  ^~ ~~  ~~ 
^ EXTRA SPACE HERE
+   { dg-end-multiline-output "" } */
+}

as the actual output is:

   f (, , , );
  ^~ ~~  ~~
  ^ LINE ENDS HERE, with no trailing
space present

This space shows up in the error here:

FAIL: c-c++-common/pr35503-2.c  -Wc++-compat   expected multiline
pattern lines 12-13 not found: "\s*f \(, , ,
\);.*\n  \^~ ~~  ~~ .*\n"
 ^ EXTRA SPACE

BTW, the .* at the end of the pattern means it's ok to have additional
material in the actual output that isn't matched (e.g. for comments
containing dg- directives [1] ) but it doesn't work the other way
around: all of the text within the dg-begin/end-multiline directives
has to be in the actual output.


[1] so you can have e.g.:

  f (, , , ); /* { dg-warning "passing argument 1 to 
restrict-qualified parameter aliases with arguments 3, 4" } */

and:

/* { dg-begin-multiline-output "" }
   f (, , , );
  ^~ ~~  ~~
   { dg-end-multiline-output "" } */

where the actual output will look like:

pr35503-2.c:8:6: warning: passing argument 1 to restrict-qualified parameter 
aliases with arguments 3, 4 [-Wrestrict]
   f (, , , ); /* { dg-warning "passing argument 1 to 
restrict-qualified parameter aliases with arguments 3, 4" } */
 ^~ ~~  ~~

and you can omit the copy of the dg-warning directive in the expected
multiline output (which would otherwise totally confuse DejaGnu).
Doing so avoids having to specify the line number.

> Thanks,
> Prathamesh
> > 
> > > >   richloc.add_range (loc_of_arg3, false);  /* false here =
> > > > don't
> > > > draw
> > > > a
> > > > caret, just the underline */
> > > >   richloc.add_range (loc_of_arg4, false);
> > > >   warning_at_rich_loc (, OPT_Wrestrict, etc...
> > > > 
> > > > See line-map.h for more information on rich_location.
> > > 
> > > [...]

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Prathamesh Kulkarni]

On 30 August 2016 at 17:11, Eric Gallager  wrote:
> On 8/29/16, Jason Merrill  wrote:
>> On Mon, Aug 29, 2016 at 10:28 AM, Marek Polacek  wrote:
>>> On Mon, Aug 29, 2016 at 09:20:53AM -0400, Eric Gallager wrote:
 I tried this patch on my fork of gdb-binutils and got a few warnings
 from it. Would it be possible to have the caret point to the argument
 mentioned, instead of the function name? And also print the option
 name? E.g., instead of the current:

 or32-opc.c: In function ‘or32_print_register’:
 or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
 parameter aliases with argument 3
sprintf (disassembled, "%sr%d", disassembled, regnum);
^~~

 could it look like:

 or32-opc.c: In function ‘or32_print_register’:
 or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
 parameter aliases with argument 3 [-Wrestrict]
sprintf (disassembled, "%sr%d", disassembled, regnum);
 ^~~~

 instead?
>>>
>>> I didn't try to implement it, but I think this should be fairly easy to
>>> achieve in the C FE, because c_parser_postfix_expression_after_primary
>>> has arg_loc, which is a vector of parameter locations.
>>
>> The C++ FE doesn't have this currently, but it could be added without
>> too much trouble: in cp_parser_parenthesized_expression_list, extract
>> the locations from the cp_expr return value of
>> cp_parser_assignment_expression, and then pass the locations back up
>> to cp_parser_postfix_expression.
>>
>> Jason
>>
>
>
> On the topic of how to get this warning working with various
> frontends, is there any reason why the Objective C frontend doesn't
> handle -Wrestrict? Currently when trying to use it, it just says:
>
> cc1obj: warning: command line option '-Wrestrict' is valid for C/C++
> but not for ObjC
Hi Eric,
I am not sure if restrict is valid for ObjC/Obj-C++ and hence didn't
add the option for these front-ends.
If it is valid, I will enable the option for ObjC and Obj-C++.

Thanks,
Prathamesh

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Eric Gallager]

On 8/29/16, Jason Merrill  wrote:
> On Mon, Aug 29, 2016 at 10:28 AM, Marek Polacek  wrote:
>> On Mon, Aug 29, 2016 at 09:20:53AM -0400, Eric Gallager wrote:
>>> I tried this patch on my fork of gdb-binutils and got a few warnings
>>> from it. Would it be possible to have the caret point to the argument
>>> mentioned, instead of the function name? And also print the option
>>> name? E.g., instead of the current:
>>>
>>> or32-opc.c: In function ‘or32_print_register’:
>>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>>> parameter aliases with argument 3
>>>sprintf (disassembled, "%sr%d", disassembled, regnum);
>>>^~~
>>>
>>> could it look like:
>>>
>>> or32-opc.c: In function ‘or32_print_register’:
>>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>>> parameter aliases with argument 3 [-Wrestrict]
>>>sprintf (disassembled, "%sr%d", disassembled, regnum);
>>> ^~~~
>>>
>>> instead?
>>
>> I didn't try to implement it, but I think this should be fairly easy to
>> achieve in the C FE, because c_parser_postfix_expression_after_primary
>> has arg_loc, which is a vector of parameter locations.
>
> The C++ FE doesn't have this currently, but it could be added without
> too much trouble: in cp_parser_parenthesized_expression_list, extract
> the locations from the cp_expr return value of
> cp_parser_assignment_expression, and then pass the locations back up
> to cp_parser_postfix_expression.
>
> Jason
>


On the topic of how to get this warning working with various
frontends, is there any reason why the Objective C frontend doesn't
handle -Wrestrict? Currently when trying to use it, it just says:

cc1obj: warning: command line option '-Wrestrict' is valid for C/C++
but not for ObjC

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 30 August 2016 at 05:34, David Malcolm  wrote:
> On Mon, 2016-08-29 at 20:01 -0400, David Malcolm wrote:
>> On Mon, 2016-08-29 at 19:55 -0400, David Malcolm wrote:
>> [...]
>> > Assuming you have the location_t values available, you can create a
>> > rich_location for the primary range, and then add secondary ranges
>> > like
>> > this:
>> >
>> >   rich_location richloc (loc_of_arg1);
>>
>> Oops, the above should be:
>>
>> rich_location richloc (line_table, loc_of_arg1);
>>
>> or:
>>
>> gcc_rich_location (loc_of_arg1);
> and this should be:
>
>  gcc_rich_location richloc (loc_of_arg1);
>> which does the same thing (#include "gcc-rich-location.h").
>
> Clearly I need to sleep :)
Hi David,
Thanks for the suggestions. I can now see multiple source ranges for
pr35503-2.c (included in patch).
Output shows: http://pastebin.com/FNAVDU8A
(Posted pastebin link to avoid mangling by the mailer)

However the test for underline fails:
FAIL: c-c++-common/pr35503-2.c  -Wc++-compat   expected multiline
pattern lines 12-13 not found: "\s*f \(, , ,
\);.*\n  \^~ ~~  ~~ .*\n"
I have attached gcc.log for the test-case. Presumably I have written
the test-case incorrectly.
Could you please have a look at it ?

Thanks,
Prathamesh
>
>> >   richloc.add_range (loc_of_arg3, false);  /* false here = don't
>> > draw
>> > a
>> > caret, just the underline */
>> >   richloc.add_range (loc_of_arg4, false);
>> >   warning_at_rich_loc (, OPT_Wrestrict, etc...
>> >
>> > See line-map.h for more information on rich_location.
>>
>> [...]
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 3feb910..4239cef 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,86 @@ 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 *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 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 (_obstack);
+  char *fmt = (char *) obstack_alloc (_obstack, 0);
+
+  char num[32];
+  sprintf (num, "%u", param_pos + 1);
+
+  obstack_grow (_obstack, "passing argument ",
+   strlen ("passing argument "));
+  obstack_grow (_obstack, num, strlen (num));
+  obstack_grow (_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 (_obstack, 's');
+  obstack_1grow (_obstack, ' ');
+
+  unsigned pos;
+  unsigned ranges_added = 1;
+
+  FOR_EACH_VEC_ELT (arg_positions, i, pos)
+{
+  /* FIXME: Fow now, only 3 ranges can be added. Remove this once
+the restriction is lifted.  */
+  if (ranges_added < 3)
+   {
+ tree arg = (*args)[pos];
+ if (EXPR_HAS_LOCATION (arg))
+   {
+ richloc.add_range (EXPR_LOCATION (arg), false);
+ ranges_added++;
+   }
+   }
+
+  sprintf (num, "%u", pos + 1);
+  obstack_grow (_obstack, num, strlen (num));
+
+  if (i < arg_positions.length () - 1)
+   obstack_grow (_obstack, ", ",  strlen (", "));
+}
+
+  obstack_1grow (_obstack, 0);
+  warning_at_rich_loc (, OPT_Wrestrict, fmt);
+  obstack_free (_obstack, fmt);
+}
+
 #include "gt-c-family-c-common.h"
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index bc22baa..cdb762e 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -920,6 +920,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 *);
 
 /* 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 a5358ed..a029a86 100644
--- 

Re: PR35503 - warn for restrict pointer

[David Malcolm]

On Mon, 2016-08-29 at 20:01 -0400, David Malcolm wrote:
> On Mon, 2016-08-29 at 19:55 -0400, David Malcolm wrote:
> [...]
> > Assuming you have the location_t values available, you can create a
> > rich_location for the primary range, and then add secondary ranges
> > like
> > this:
> > 
> >   rich_location richloc (loc_of_arg1);
> 
> Oops, the above should be:
> 
> rich_location richloc (line_table, loc_of_arg1);
> 
> or:
> 
> gcc_rich_location (loc_of_arg1);
and this should be:

 gcc_rich_location richloc (loc_of_arg1);
> which does the same thing (#include "gcc-rich-location.h").

Clearly I need to sleep :)

> >   richloc.add_range (loc_of_arg3, false);  /* false here = don't
> > draw
> > a
> > caret, just the underline */
> >   richloc.add_range (loc_of_arg4, false);
> >   warning_at_rich_loc (, OPT_Wrestrict, etc...
> > 
> > See line-map.h for more information on rich_location.
> 
> [...]

Re: PR35503 - warn for restrict pointer

[David Malcolm]

On Mon, 2016-08-29 at 19:55 -0400, David Malcolm wrote:
[...]
> Assuming you have the location_t values available, you can create a
> rich_location for the primary range, and then add secondary ranges
> like
> this:
> 
>   rich_location richloc (loc_of_arg1);

Oops, the above should be:

rich_location richloc (line_table, loc_of_arg1);

or:

gcc_rich_location (loc_of_arg1);

which does the same thing (#include "gcc-rich-location.h").

>   richloc.add_range (loc_of_arg3, false);  /* false here = don't draw
> a
> caret, just the underline */
>   richloc.add_range (loc_of_arg4, false);
>   warning_at_rich_loc (, OPT_Wrestrict, etc...
> 
> See line-map.h for more information on rich_location.

[...]

Re: PR35503 - warn for restrict pointer

[David Malcolm]

On Tue, 2016-08-30 at 03:23 +0530, Prathamesh Kulkarni wrote:
> On 29 August 2016 at 19:59, Marek Polacek  wrote:
> > On Mon, Aug 29, 2016 at 04:25:25PM +0200, Tobias Burnus wrote:
> > > Prathamesh Kulkarni wrote:
> > > > Attachment: pr35503-3.txt
> > > 
> > > I tried the patch - and it found a bug in our code; nice!
> > > 
> > > 
> > > (a) Regarding the [-Werror] output:
> > > 
> > >error: passing argument 24 to restrict qualified parameter
> > > aliases with argument 29 [-Werror]
> > > 
> > > Shouldn't that output "[-Werror=restrict]" instead of a bare "[
> > > -Werror]? Namely, instead of
> > > 
> > > + warning_at (loc, 0,
> > > + "passing argument %u to restrict qualified
> > > parameter aliases with "
> > > + "argument %u", param_pos + 1, i + 1);
> > > 
> > > I think one gets this with
> > > warning_at (loc, OPT_Wrestrict, ...
> > 
> > Yes, this needs to be fixed in the patch.
> Hi,
> Thanks for all the suggestions, I have tried to incorporate them in
> the attached version.
> To avoid duplicating the warnings for multiple aliased arguments,
> I am using TREE_VISITED to mark all the aliased arguments of the
> current
> arg, which makes diagnostic emitted only once per same set of
> aliases.
> Is using TREE_VISITED ok as in the patch ?
> 
> eg:
> void f(int *__restrict x, int *y, int *__restrict z, int *w);
> 
> void foo(int a, int b)
> {
>   f (, , , );
> }
> 
> Output:
> test-2.c: In function ‘foo’:
> test-2.c:5:6: warning: passing argument 1 to restrict-qualified
> parameter aliases with arguments 3, 4 [-Wrestrict]
>f (, , , );
>^~
> 
> Using EXPR_LOCATION (arg), helps underline the argument for both C
> and C++
> as in the example above. However in case of variadic functions, it
> appears C++FE doesn't set EXPR_LOCATION,
> so I am falling back to using input_location if EXPR_LOCATION is
> unknown.

You may be able to use EXPR_LOC_OR_LOC for this.

> However it gives the location correctly for non-variadic functions.
> For setting locations correctly I will try the approach suggested by
> Jason in
> https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01987.html
> 
> I was wondering how to underline all the aliased arguments ?
> I would like to see the output for above eg as:
> 
> test-2.c: In function ‘foo’:
> test-2.c:5:6: warning: passing argument 1 to restrict-qualified
> parameter aliases with arguments 3, 4 [-Wrestrict]
>f (, , , );
>^~~~   ~~
> 
> I would be grateful for suggestions for the same.
> Bootstrap+test in progress on x86_64-unknown-linux-gnu.


The above came through email a bit mangled; presumably you mean:

   f (, , , );
  ^~  ~~  ~~ 

(hopefully Evolution won't mangle the above).

Assuming you have the location_t values available, you can create a
rich_location for the primary range, and then add secondary ranges like
this:

  rich_location richloc (loc_of_arg1);
  richloc.add_range (loc_of_arg3, false);  /* false here = don't draw a
caret, just the underline */
  richloc.add_range (loc_of_arg4, false);
  warning_at_rich_loc (, OPT_Wrestrict, etc...

See line-map.h for more information on rich_location.

Currently we can only have up to 3 ranges in a rich_location; I have a
patch in one of my working copies that fixes that so we can have an
arbitrary number; I'll try to dig that out as it's clearly relevant
here.

It's a good idea to have a testcase that verifies the underlines.

Use:

  /* { dg-options "-fdiagnostics-show-caret" } */

and then use:

  /* { dg-begin-multiline-output "" }
quote the expected output here, omitting trailing dg-directives
 { dg-end-multiline-output "" } */

e.g.
  /* { dg-begin-multiline-output "" }
   f (, , , );
  ^~  ~~  ~~ 
 { dg-end-multiline-output "" } */

Please use identifiers that are longer than one char in such a test, since 
otherwise there's a chance that we're not properly underlining all of the 
pertinent range (i.e. testing the start != finish case).

Hope the above is helpful
Dave

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 29 August 2016 at 19:59, Marek Polacek  wrote:
> On Mon, Aug 29, 2016 at 04:25:25PM +0200, Tobias Burnus wrote:
>> Prathamesh Kulkarni wrote:
>> > Attachment: pr35503-3.txt
>>
>> I tried the patch - and it found a bug in our code; nice!
>>
>>
>> (a) Regarding the [-Werror] output:
>>
>>error: passing argument 24 to restrict qualified parameter aliases with 
>> argument 29 [-Werror]
>>
>> Shouldn't that output "[-Werror=restrict]" instead of a bare "[-Werror]? 
>> Namely, instead of
>>
>> + warning_at (loc, 0,
>> + "passing argument %u to restrict qualified parameter 
>> aliases with "
>> + "argument %u", param_pos + 1, i + 1);
>>
>> I think one gets this with
>> warning_at (loc, OPT_Wrestrict, ...
>
> Yes, this needs to be fixed in the patch.
Hi,
Thanks for all the suggestions, I have tried to incorporate them in
the attached version.
To avoid duplicating the warnings for multiple aliased arguments,
I am using TREE_VISITED to mark all the aliased arguments of the current
arg, which makes diagnostic emitted only once per same set of aliases.
Is using TREE_VISITED ok as in the patch ?

eg:
void f(int *__restrict x, int *y, int *__restrict z, int *w);

void foo(int a, int b)
{
  f (, , , );
}

Output:
test-2.c: In function ‘foo’:
test-2.c:5:6: warning: passing argument 1 to restrict-qualified
parameter aliases with arguments 3, 4 [-Wrestrict]
   f (, , , );
   ^~

Using EXPR_LOCATION (arg), helps underline the argument for both C and C++
as in the example above. However in case of variadic functions, it
appears C++FE doesn't set EXPR_LOCATION,
so I am falling back to using input_location if EXPR_LOCATION is unknown.
However it gives the location correctly for non-variadic functions.
For setting locations correctly I will try the approach suggested by Jason in
https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01987.html

I was wondering how to underline all the aliased arguments ?
I would like to see the output for above eg as:

test-2.c: In function ‘foo’:
test-2.c:5:6: warning: passing argument 1 to restrict-qualified
parameter aliases with arguments 3, 4 [-Wrestrict]
   f (, , , );
   ^~~~   ~~

I would be grateful for suggestions for the same.
Bootstrap+test in progress on x86_64-unknown-linux-gnu.

Thanks,
Prathamesh
>
> Marek
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 3feb910..abb92b3 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -13057,4 +13057,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 *args)
+{
+  tree arg = (*args)[param_pos];
+  if (TREE_VISITED (arg))
+return;
+
+  location_t loc = (EXPR_LOCATION (arg) != UNKNOWN_LOCATION)
+  ? EXPR_LOCATION (arg)
+  : input_location;
+
+  if (operand_equal_p (arg, null_pointer_node, 0))
+return;
+
+  unsigned i;
+  tree current_arg;
+  auto_vec 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 (_obstack);
+  char *fmt = (char *) obstack_alloc (_obstack, 0);
+
+  char num[32];
+  sprintf (num, "%u", param_pos + 1);
+
+  obstack_grow (_obstack, "passing argument ",
+   strlen ("passing argument "));
+  obstack_grow (_obstack, num, strlen (num));
+  obstack_grow (_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 (_obstack, 's');
+  obstack_1grow (_obstack, ' ');
+
+  unsigned pos;
+  FOR_EACH_VEC_ELT (arg_positions, i, pos)
+{
+  sprintf (num, "%u", pos + 1);
+  obstack_grow (_obstack, num, strlen (num));
+
+  if (i < arg_positions.length () - 1)
+   obstack_grow (_obstack, ", ",  strlen (", "));
+}
+
+  obstack_1grow (_obstack, 0);
+  warning_at (loc, OPT_Wrestrict, fmt);
+  obstack_free (_obstack, fmt);
+}
+
 #include "gt-c-family-c-common.h"
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index bc22baa..cdb762e 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -920,6 +920,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 

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Jason Merrill]

On Mon, Aug 29, 2016 at 10:28 AM, Marek Polacek  wrote:
> On Mon, Aug 29, 2016 at 09:20:53AM -0400, Eric Gallager wrote:
>> I tried this patch on my fork of gdb-binutils and got a few warnings
>> from it. Would it be possible to have the caret point to the argument
>> mentioned, instead of the function name? And also print the option
>> name? E.g., instead of the current:
>>
>> or32-opc.c: In function ‘or32_print_register’:
>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>> parameter aliases with argument 3
>>sprintf (disassembled, "%sr%d", disassembled, regnum);
>>^~~
>>
>> could it look like:
>>
>> or32-opc.c: In function ‘or32_print_register’:
>> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
>> parameter aliases with argument 3 [-Wrestrict]
>>sprintf (disassembled, "%sr%d", disassembled, regnum);
>> ^~~~
>>
>> instead?
>
> I didn't try to implement it, but I think this should be fairly easy to
> achieve in the C FE, because c_parser_postfix_expression_after_primary
> has arg_loc, which is a vector of parameter locations.

The C++ FE doesn't have this currently, but it could be added without
too much trouble: in cp_parser_parenthesized_expression_list, extract
the locations from the cp_expr return value of
cp_parser_assignment_expression, and then pass the locations back up
to cp_parser_postfix_expression.

Jason

Re: PR35503 - warn for restrict pointer

[Marek Polacek]

On Mon, Aug 29, 2016 at 04:25:25PM +0200, Tobias Burnus wrote:
> Prathamesh Kulkarni wrote:
> > Attachment: pr35503-3.txt
> 
> I tried the patch - and it found a bug in our code; nice!
> 
> 
> (a) Regarding the [-Werror] output:
> 
>error: passing argument 24 to restrict qualified parameter aliases with 
> argument 29 [-Werror]
> 
> Shouldn't that output "[-Werror=restrict]" instead of a bare "[-Werror]? 
> Namely, instead of
> 
> + warning_at (loc, 0,
> + "passing argument %u to restrict qualified parameter 
> aliases with "
> + "argument %u", param_pos + 1, i + 1);
> 
> I think one gets this with
> warning_at (loc, OPT_Wrestrict, ...
 
Yes, this needs to be fixed in the patch.

Marek

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Marek Polacek]

On Mon, Aug 29, 2016 at 09:20:53AM -0400, Eric Gallager wrote:
> I tried this patch on my fork of gdb-binutils and got a few warnings
> from it. Would it be possible to have the caret point to the argument
> mentioned, instead of the function name? And also print the option
> name? E.g., instead of the current:
> 
> or32-opc.c: In function ‘or32_print_register’:
> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
> parameter aliases with argument 3
>sprintf (disassembled, "%sr%d", disassembled, regnum);
>^~~
> 
> could it look like:
> 
> or32-opc.c: In function ‘or32_print_register’:
> or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
> parameter aliases with argument 3 [-Wrestrict]
>sprintf (disassembled, "%sr%d", disassembled, regnum);
> ^~~~
> 
> instead?

I didn't try to implement it, but I think this should be fairly easy to
achieve in the C FE, because c_parser_postfix_expression_after_primary
has arg_loc, which is a vector of parameter locations.

Marek

Re: PR35503 - warn for restrict pointer

[Tobias Burnus]

Prathamesh Kulkarni wrote:
> Attachment: pr35503-3.txt

I tried the patch - and it found a bug in our code; nice!


(a) Regarding the [-Werror] output:

   error: passing argument 24 to restrict qualified parameter aliases with 
argument 29 [-Werror]

Shouldn't that output "[-Werror=restrict]" instead of a bare "[-Werror]? 
Namely, instead of

+   warning_at (loc, 0,
+   "passing argument %u to restrict qualified parameter 
aliases with "
+   "argument %u", param_pos + 1, i + 1);

I think one gets this with
warning_at (loc, OPT_Wrestrict, ...


 * * *

(b) I get:


file.cc:5582:41: error: passing argument 24 to restrict qualified parameter 
aliases with argument 29 [-Werror]
out, out2);
 ^
file.cc:5582:41: error: passing argument 29 to restrict qualified parameter 
aliases with argument 24 [-Werror]


Thus, the message is kind of shown twice, one for 24 and once for 29;
but only one has a "^" possition, pointing after the 31st argument.


(i) I had expected that the message is shown only once. The issue here that
both the 24th and the 29th argument have the restrict qualifier. A simple

+  if (i < param_pos)
+   continue;

in warn_for_restrict would have silenced the second call, but would have
missed the output if only the 29th argument had the qualifier and not
also the 24th argument. Thus, one needs some more logic.


(ii) It would be great if the "^" wouldn't point to the end of the
argument list but to the one of the aliasing arguments - or even better
to "^~~" to the first argument and to "~" to the second.

I have to admit that I don't know how to use rich_location, but I am sure
digging in the source would help; possibly, also David or Marek can give
you a hint.

Cheers,

Tobias

Re: PR35503 - warn for restrict pointer

[Marek Polacek]

On Sun, Aug 28, 2016 at 06:32:59PM +0530, Prathamesh Kulkarni wrote:
> On 26 August 2016 at 21:25, Jason Merrill  wrote:
> > On Fri, Aug 26, 2016 at 7:39 AM, Prathamesh Kulkarni
> >  wrote:
> >> However with C++FE it appears TYPE_RESTRICT is not set for the
> >> parameters (buf and fmt)
> >> and hence the warning doesn't get emitted for C++.
> >> C FE sets TYPE_RESTRICT for them. I am not sure how to workaround this
> >> issue, and would be grateful for suggestions.
> >
> > In the C++ FE you can see TYPE_RESTRICT on the types of the
> > DECL_ARGUMENTS of the function.
> Thanks for the suggestions, I could indeed see TYPE_RESTRICT set on
> DECL_ARGUMENTS.
> The attached patch warns for both C and C++ with -Wrestrict, and I
> have put it under -Wall.
> I suppose we don't want to warn for null pointer args ?
> for eg:
> void f(int *restrict x, int *y);
> 
> void foo(void)
> {
>   f(NULL, NULL) ?
> }
> 
> However I suppose warning for pointer constants other than zero is desirable ?
> I am not sure whether restrict is valid for obj-c/obj-c++, so I have
> just kept it to C and C++
> in the patch. Should the warning also be included for obj-c and obj-c++ FE's ?
> Boostrapped and tested on x86_64-unknown-linux-gnu.
> OK to commit ?
> 
> Thanks,
> Prathamesh
> >
> > Jason

> 2016-08-28  Prathamesh Kulkarni  
> 
>   PR c/35503
>   * doc/invoke.texi: Document -Wrestrict.
> c-family/
>   * c-common.c (warn_for_restrict): New function.
>   * c-common.h (warn_for_restrict): Declare it.
>   * c.opt: New option -Wrestrit.

Typo.

> c/
>   * c-parser.c (c_parser_postfix_expression_after_primary): Call
>   warn_for_restrict.
> cp/
>   * parser.c (cp_parser_postfix_expression): Likewise.

This is a ChangeLog entry in another directory, so Likewise wouldn't work here.

> testsuite/
>   * c-c++-common/pr35503.c: New test-case.
> 
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index 3feb910..ec8778f 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -13057,4 +13057,27 @@ 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 (location_t loc, unsigned param_pos, vec 
> *args)
> +{
> +  tree arg = (*args)[param_pos];
> +  if (operand_equal_p (arg, null_pointer_node, 0))
> +return;
> +
> +  for (unsigned i = 0; i < args->length (); ++i)

Use FOR_EACH_VEC_ELT?

> +{
> +  if (i == param_pos)
> + continue;
> +
> +  tree current_arg = (*args)[i];
> +  if (operand_equal_p (arg, current_arg, 0))
> + warning_at (loc, 0,
> + "passing argument %u to restrict qualified parameter 
> aliases with "

I think this should be "restrict-qualified".

> + "argument %u", param_pos + 1, i + 1);
> +}
> +}
> +
>  #include "gt-c-family-c-common.h"
> diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
> index bc22baa..0526ad5 100644
> --- a/gcc/c-family/c-common.h
> +++ b/gcc/c-family/c-common.h
> @@ -920,6 +920,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 (location_t, unsigned, vec *);
>  
>  /* 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 a5358ed..a029a86 100644
> --- a/gcc/c-family/c.opt
> +++ b/gcc/c-family/c.opt
> @@ -1012,6 +1012,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 C++ Var(warn_restrict) Warning LangEnabledBy(C C++,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 fe0c95f..54e1e87 100644
> --- a/gcc/c/c-parser.c
> +++ b/gcc/c/c-parser.c
> @@ -8369,6 +8369,17 @@ 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 param_pos = 0;
> +   for (tree t = TYPE_ARG_TYPES (TREE_TYPE (expr.value)); t; t = 
> TREE_CHAIN (t), param_pos++) 
> + {
> +   tree type = TREE_VALUE (t);
> +   if (POINTER_TYPE_P (type) && TYPE_RESTRICT (type))
> + warn_for_restrict (expr_loc, param_pos, exprlist);  
> + 

Re: PR35503 - warn for restrict pointer (-Wrestrict)

[Eric Gallager]

On 8/28/16, Prathamesh Kulkarni  wrote:
> On 26 August 2016 at 21:25, Jason Merrill  wrote:
>> On Fri, Aug 26, 2016 at 7:39 AM, Prathamesh Kulkarni
>>  wrote:
>>> However with C++FE it appears TYPE_RESTRICT is not set for the
>>> parameters (buf and fmt)
>>> and hence the warning doesn't get emitted for C++.
>>> C FE sets TYPE_RESTRICT for them. I am not sure how to workaround this
>>> issue, and would be grateful for suggestions.
>>
>> In the C++ FE you can see TYPE_RESTRICT on the types of the
>> DECL_ARGUMENTS of the function.
> Thanks for the suggestions, I could indeed see TYPE_RESTRICT set on
> DECL_ARGUMENTS.
> The attached patch warns for both C and C++ with -Wrestrict, and I
> have put it under -Wall.
> I suppose we don't want to warn for null pointer args ?
> for eg:
> void f(int *restrict x, int *y);
>
> void foo(void)
> {
>   f(NULL, NULL) ?
> }
>
> However I suppose warning for pointer constants other than zero is desirable
> ?
> I am not sure whether restrict is valid for obj-c/obj-c++, so I have
> just kept it to C and C++
> in the patch. Should the warning also be included for obj-c and obj-c++ FE's
> ?
> Boostrapped and tested on x86_64-unknown-linux-gnu.
> OK to commit ?
>
> Thanks,
> Prathamesh
>


I tried this patch on my fork of gdb-binutils and got a few warnings
from it. Would it be possible to have the caret point to the argument
mentioned, instead of the function name? And also print the option
name? E.g., instead of the current:

or32-opc.c: In function ‘or32_print_register’:
or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
parameter aliases with argument 3
   sprintf (disassembled, "%sr%d", disassembled, regnum);
   ^~~

could it look like:

or32-opc.c: In function ‘or32_print_register’:
or32-opc.c:956:3: warning: passing argument 1 to restrict qualified
parameter aliases with argument 3 [-Wrestrict]
   sprintf (disassembled, "%sr%d", disassembled, regnum);
^~~~

instead?
Just an idea. Either way, I hope this patch goes in.
Thanks,
Eric

Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni]

On 26 August 2016 at 21:25, Jason Merrill  wrote:
> On Fri, Aug 26, 2016 at 7:39 AM, Prathamesh Kulkarni
>  wrote:
>> However with C++FE it appears TYPE_RESTRICT is not set for the
>> parameters (buf and fmt)
>> and hence the warning doesn't get emitted for C++.
>> C FE sets TYPE_RESTRICT for them. I am not sure how to workaround this
>> issue, and would be grateful for suggestions.
>
> In the C++ FE you can see TYPE_RESTRICT on the types of the
> DECL_ARGUMENTS of the function.
Thanks for the suggestions, I could indeed see TYPE_RESTRICT set on
DECL_ARGUMENTS.
The attached patch warns for both C and C++ with -Wrestrict, and I
have put it under -Wall.
I suppose we don't want to warn for null pointer args ?
for eg:
void f(int *restrict x, int *y);

void foo(void)
{
  f(NULL, NULL) ?
}

However I suppose warning for pointer constants other than zero is desirable ?
I am not sure whether restrict is valid for obj-c/obj-c++, so I have
just kept it to C and C++
in the patch. Should the warning also be included for obj-c and obj-c++ FE's ?
Boostrapped and tested on x86_64-unknown-linux-gnu.
OK to commit ?

Thanks,
Prathamesh
>
> Jason
2016-08-28  Prathamesh Kulkarni  

PR c/35503
* doc/invoke.texi: Document -Wrestrict.
c-family/
* c-common.c (warn_for_restrict): New function.
* c-common.h (warn_for_restrict): Declare it.
* c.opt: New option -Wrestrit.
c/
* c-parser.c (c_parser_postfix_expression_after_primary): Call
warn_for_restrict.
cp/
* parser.c (cp_parser_postfix_expression): Likewise.
testsuite/
* c-c++-common/pr35503.c: New test-case.

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 3feb910..ec8778f 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -13057,4 +13057,27 @@ 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 (location_t loc, unsigned param_pos, vec *args)
+{
+  tree arg = (*args)[param_pos];
+  if (operand_equal_p (arg, null_pointer_node, 0))
+return;
+
+  for (unsigned i = 0; i < args->length (); ++i)
+{
+  if (i == param_pos)
+   continue;
+
+  tree current_arg = (*args)[i];
+  if (operand_equal_p (arg, current_arg, 0))
+   warning_at (loc, 0,
+   "passing argument %u to restrict qualified parameter 
aliases with "
+   "argument %u", param_pos + 1, i + 1);
+}
+}
+
 #include "gt-c-family-c-common.h"
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index bc22baa..0526ad5 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -920,6 +920,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 (location_t, unsigned, vec *);
 
 /* 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 a5358ed..a029a86 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1012,6 +1012,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 C++ Var(warn_restrict) Warning LangEnabledBy(C C++,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 fe0c95f..54e1e87 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -8369,6 +8369,17 @@ 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 param_pos = 0;
+ for (tree t = TYPE_ARG_TYPES (TREE_TYPE (expr.value)); t; t = 
TREE_CHAIN (t), param_pos++) 
+   {
+ tree type = TREE_VALUE (t);
+ if (POINTER_TYPE_P (type) && TYPE_RESTRICT (type))
+   warn_for_restrict (expr_loc, param_pos, exprlist);  
+   }
+   }
+
  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 690e928..4710a08 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -6878,6 +6878,20 @@ cp_parser_postfix_expression (cp_parser *parser, bool 
address_p, bool cast_p,
warn_for_memset (input_location, arg0, arg2, literal_mask);
 

Re: PR35503 - warn for restrict pointer

[Jason Merrill]

On Fri, Aug 26, 2016 at 7:39 AM, Prathamesh Kulkarni
 wrote:
> However with C++FE it appears TYPE_RESTRICT is not set for the
> parameters (buf and fmt)
> and hence the warning doesn't get emitted for C++.
> C FE sets TYPE_RESTRICT for them. I am not sure how to workaround this
> issue, and would be grateful for suggestions.

In the C++ FE you can see TYPE_RESTRICT on the types of the
DECL_ARGUMENTS of the function.

Jason

Re: PR35503 - warn for restrict pointer

[Joseph Myers]

Arguments passed to diagnostic functions should not end with '\n'.

-- 
Joseph S. Myers
jos...@codesourcery.com