On Thu, 2016-09-01 at 14:55 +0530, Prathamesh Kulkarni wrote:
[...]
> The attached version passes bootstrap+test on ppc64le-linux-gnu.
> Given that it only looks if parameters are restrict qualified and not
> how they're used inside the callee,
> this can have false positives as in above test-cases.
> Should the warning be put in Wextra rather than Wall (I have left it
> in Wall in the patch) or only enabled with -Wrestrict ?
>
> Thanks,
> Prathamesh
> >
> > Richard.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 3feb910..a3dae34 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -47,6 +47,7 @@ along with GCC; see the file COPYING3. If not see
#include "gimplify.h"
#include "substring-locations.h"
#include "spellcheck.h"
+#include "gcc-rich-location.h"
cpp_reader *parse_in; /* Declared in c-pragma.h. */
@@ -13057,4 +13058,76 @@ diagnose_mismatched_attributes (tree olddecl,
tree newdecl)
return warned;
}
+/* Warn if an argument at position param_pos is passed to a
+ restrict-qualified param, and it aliases with another argument. */
+
+void
+warn_for_restrict (unsigned param_pos, vec<tree, va_gc> *args)
+{
+ tree arg = (*args)[param_pos];
+ if (TREE_VISITED (arg) || operand_equal_p (arg, null_pointer_node,
0))
+ return;
+
+ location_t loc = EXPR_LOC_OR_LOC (arg, input_location);
+ gcc_rich_location richloc (loc);
+
+ unsigned i;
+ tree current_arg;
+ auto_vec<unsigned> arg_positions;
+
+ FOR_EACH_VEC_ELT (*args, i, current_arg)
+ {
+ if (i == param_pos)
+ continue;
+
+ tree current_arg = (*args)[i];
+ if (operand_equal_p (arg, current_arg, 0))
+ {
+ TREE_VISITED (current_arg) = 1;
+ arg_positions.safe_push (i);
+ }
+ }
+
+ if (arg_positions.is_empty ())
+ return;
+
+ struct obstack fmt_obstack;
+ gcc_obstack_init (&fmt_obstack);
+ char *fmt = (char *) obstack_alloc (&fmt_obstack, 0);
+
+ char num[32];
+ sprintf (num, "%u", param_pos + 1);
+
+ obstack_grow (&fmt_obstack, "passing argument ",
+ strlen ("passing argument "));
+ obstack_grow (&fmt_obstack, num, strlen (num));
+ obstack_grow (&fmt_obstack,
+ " to restrict-qualified parameter aliases with
argument",
+ strlen (" to restrict-qualified parameter "
+ "aliases with argument"));
+
+ /* make argument plural and append space. */
+ if (arg_positions.length () > 1)
+ obstack_1grow (&fmt_obstack, 's');
+ obstack_1grow (&fmt_obstack, ' ');
+
+ unsigned pos;
+ FOR_EACH_VEC_ELT (arg_positions, i, pos)
+ {
+ tree arg = (*args)[pos];
+ if (EXPR_HAS_LOCATION (arg))
+ richloc.add_range (EXPR_LOCATION (arg), false);
+
+ sprintf (num, "%u", pos + 1);
+ obstack_grow (&fmt_obstack, num, strlen (num));
+
+ if (i < arg_positions.length () - 1)
+ obstack_grow (&fmt_obstack, ", ", strlen (", "));
+ }
+
+ obstack_1grow (&fmt_obstack, 0);
+ warning_at_rich_loc (&richloc, OPT_Wrestrict, fmt);
+ obstack_free (&fmt_obstack, fmt);
+}
Thanks for working on this.
I'm not a fan of how the patch builds "fmt" here. If nothing else,
this perhaps should be:
warning_at_rich_loc (&richloc, OPT_Wrestrict, "%s", fmt);
but building up strings like the patch does makes localization
difficult.
Much better would be to have the formatting be done inside the
diagnostics subsystem's call into pp_format, with something like this:
warning_at_rich_loc_n (&richloc, OPT_Wrestrict,
arg_positions
.length (),
"passing argument %i to restrict
-qualified"
" parameter aliases with argument
%FIXME",
"passing argument %i to restrict
-qualified"
" parameter aliases with arguments
%FIXME",
param_pos + 1,
&arg_positions);
and have %FIXME (or somesuch) consume &arg_positions in the va_arg,
printing the argument numbers there. Doing it this way also avoids
building the string for the case where Wrestrict is disabled, since the
pp_format only happens after we know we're going to print the warning.
Assuming that there isn't yet a pre-canned way to print a set of
argument numbers that I've missed, the place to add the %FIXME-handling
would presumably be in default_tree_printer in tree-diagnostic.c -
though it's obviously nothing to do with trees. (Or if this is too
single-purpose, perhaps there's a need to temporarily inject one-time
callbacks for consuming custom args??).
We can then have a fun discussion about the usage of the Oxford comma :) [1]
IIRC we don't yet have warning_at_rich_loc_n, but it would be fairly easy to
add.
[...]
[1] which seems to be locale-dependent itself:
https://en.wikipedia.org/wiki/Serial_comma#Other_languages
