diff mbox

PR35503 - warn for restrict pointer

Message ID 1472838245.5595.183.camel@redhat.com
State New
Headers show

Commit Message

David Malcolm Sept. 2, 2016, 5:44 p.m. UTC 
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.


+}

Thanks for working on this.

I'm not a fan of how the patch builds "fmt" here.  If nothing else,
this perhaps should be:

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

but building up strings like the patch does makes localization
difficult.

Much better would be to have the formatting be done inside the
diagnostics subsystem's call into pp_format, with something like this:

  warning_at_rich_loc_n (&richloc, OPT_Wrestrict,
			 arg_positions
.length (),
                         "passing argument %i to restrict
-qualified"
	                 " parameter aliases with argument
%FIXME",
                         "passing argument %i to restrict
-qualified"
	                 " parameter aliases with arguments
%FIXME",
                         param_pos + 1,
                        
 &arg_positions);


and have %FIXME (or somesuch) consume &arg_positions in the va_arg,
printing the argument numbers there.  Doing it this way also avoids
building the string for the case where Wrestrict is disabled, since the
pp_format only happens after we know we're going to print the warning.

Assuming that there isn't yet a pre-canned way to print a set of
argument numbers that I've missed, the place to add the %FIXME-handling
would presumably be in default_tree_printer in tree-diagnostic.c -
though it's obviously nothing to do with trees. (Or if this is too
single-purpose, perhaps there's a need to temporarily inject one-time
callbacks for consuming custom args??).

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

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

[...]

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

Comments

Manuel López-Ibáñez Sept. 2, 2016, 7:52 p.m. UTC | #1 
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 (&richloc, OPT_Wrestrict,
> 			 arg_positions
> .length (),
>                          "passing argument %i to restrict
> -qualified"
> 	                 " parameter aliases with argument
> %FIXME",
>                          "passing argument %i to restrict
> -qualified"
> 	                 " parameter aliases with arguments
> %FIXME",
>                          param_pos + 1,
>
>  &arg_positions);

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

> and have %FIXME (or somesuch) consume &arg_positions in the va_arg,
> printing the argument numbers there.  Doing it this way also avoids
> building the string for the case where Wrestrict is disabled, since the
> pp_format only happens after we know we're going to print the warning.

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.
diff mbox

Patch

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 3feb910..a3dae34 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -47,6 +47,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "gimplify.h"
 #include "substring-locations.h"
 #include "spellcheck.h"
+#include "gcc-rich-location.h"
 
 cpp_reader *parse_in;		/* Declared in c-pragma.h.  */
 
@@ -13057,4 +13058,76 @@  diagnose_mismatched_attributes (tree olddecl,
tree newdecl)
   return warned;
 }
 
+/* Warn if an argument at position param_pos is passed to a
+   restrict-qualified param, and it aliases with another argument.  */
+
+void
+warn_for_restrict (unsigned param_pos, vec<tree, va_gc> *args)
+{
+  tree arg = (*args)[param_pos];
+  if (TREE_VISITED (arg) || operand_equal_p (arg, null_pointer_node,
0))
+    return;
+
+  location_t loc = EXPR_LOC_OR_LOC (arg, input_location);
+  gcc_rich_location richloc (loc);
+
+  unsigned i;
+  tree current_arg;
+  auto_vec<unsigned> arg_positions;
+
+  FOR_EACH_VEC_ELT (*args, i, current_arg) 
+    {
+      if (i == param_pos)
+	continue;
+
+      tree current_arg = (*args)[i];
+      if (operand_equal_p (arg, current_arg, 0))
+	{
+	  TREE_VISITED (current_arg) = 1; 
+	  arg_positions.safe_push (i);
+	}
+    }
+
+  if (arg_positions.is_empty ())
+    return;
+
+  struct obstack fmt_obstack;
+  gcc_obstack_init (&fmt_obstack);
+  char *fmt = (char *) obstack_alloc (&fmt_obstack, 0);
+
+  char num[32];
+  sprintf (num, "%u", param_pos + 1);
+
+  obstack_grow (&fmt_obstack, "passing argument ",
+		strlen ("passing argument "));
+  obstack_grow (&fmt_obstack, num, strlen (num));
+  obstack_grow (&fmt_obstack,
+		" to restrict-qualified parameter aliases with
argument",
+		strlen (" to restrict-qualified parameter "
+			"aliases with argument"));
+
+  /* make argument plural and append space.  */
+  if (arg_positions.length () > 1)
+    obstack_1grow (&fmt_obstack, 's');
+  obstack_1grow (&fmt_obstack, ' ');
+
+  unsigned pos;
+  FOR_EACH_VEC_ELT (arg_positions, i, pos)
+    {
+      tree arg = (*args)[pos];
+      if (EXPR_HAS_LOCATION (arg))
+	richloc.add_range (EXPR_LOCATION (arg), false);
+
+      sprintf (num, "%u", pos + 1);
+      obstack_grow (&fmt_obstack, num, strlen (num));
+
+      if (i < arg_positions.length () - 1)
+	obstack_grow (&fmt_obstack, ", ",  strlen (", "));
+    }
+
+  obstack_1grow (&fmt_obstack, 0);
+  warning_at_rich_loc (&richloc, OPT_Wrestrict, fmt);
+  obstack_free (&fmt_obstack, fmt);