On 2 September 2016 at 23:14, David Malcolm <[email protected]> wrote:
> On Thu, 2016-09-01 at 14:55 +0530, Prathamesh Kulkarni wrote:
>
> [...]
>
>> The attached version passes bootstrap+test on ppc64le-linux-gnu.
>> Given that it only looks if parameters are restrict qualified and not
>> how they're used inside the callee,
>> this can have false positives as in above test-cases.
>> Should the warning be put in Wextra rather than Wall (I have left it
>> in Wall in the patch) or only enabled with -Wrestrict ?
>>
>> Thanks,
>> Prathamesh
>> >
>> > Richard.
>
>
> diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> index 3feb910..a3dae34 100644
> --- a/gcc/c-family/c-common.c
> +++ b/gcc/c-family/c-common.c
> @@ -47,6 +47,7 @@ along with GCC; see the file COPYING3. If not see
> #include "gimplify.h"
> #include "substring-locations.h"
> #include "spellcheck.h"
> +#include "gcc-rich-location.h"
>
> cpp_reader *parse_in; /* Declared in c-pragma.h. */
>
> @@ -13057,4 +13058,76 @@ diagnose_mismatched_attributes (tree olddecl,
> tree newdecl)
> return warned;
> }
>
> +/* Warn if an argument at position param_pos is passed to a
> + restrict-qualified param, and it aliases with another argument. */
> +
> +void
> +warn_for_restrict (unsigned param_pos, vec<tree, va_gc> *args)
> +{
> + tree arg = (*args)[param_pos];
> + if (TREE_VISITED (arg) || operand_equal_p (arg, null_pointer_node,
> 0))
> + return;
> +
> + location_t loc = EXPR_LOC_OR_LOC (arg, input_location);
> + gcc_rich_location richloc (loc);
> +
> + unsigned i;
> + tree current_arg;
> + auto_vec<unsigned> arg_positions;
> +
> + FOR_EACH_VEC_ELT (*args, i, current_arg)
> + {
> + if (i == param_pos)
> + continue;
> +
> + tree current_arg = (*args)[i];
> + if (operand_equal_p (arg, current_arg, 0))
> + {
> + TREE_VISITED (current_arg) = 1;
> + arg_positions.safe_push (i);
> + }
> + }
> +
> + if (arg_positions.is_empty ())
> + return;
> +
> + struct obstack fmt_obstack;
> + gcc_obstack_init (&fmt_obstack);
> + char *fmt = (char *) obstack_alloc (&fmt_obstack, 0);
> +
> + char num[32];
> + sprintf (num, "%u", param_pos + 1);
> +
> + obstack_grow (&fmt_obstack, "passing argument ",
> + strlen ("passing argument "));
> + obstack_grow (&fmt_obstack, num, strlen (num));
> + obstack_grow (&fmt_obstack,
> + " to restrict-qualified parameter aliases with
> argument",
> + strlen (" to restrict-qualified parameter "
> + "aliases with argument"));
> +
> + /* make argument plural and append space. */
> + if (arg_positions.length () > 1)
> + obstack_1grow (&fmt_obstack, 's');
> + obstack_1grow (&fmt_obstack, ' ');
> +
> + unsigned pos;
> + FOR_EACH_VEC_ELT (arg_positions, i, pos)
> + {
> + tree arg = (*args)[pos];
> + if (EXPR_HAS_LOCATION (arg))
> + richloc.add_range (EXPR_LOCATION (arg), false);
> +
> + sprintf (num, "%u", pos + 1);
> + obstack_grow (&fmt_obstack, num, strlen (num));
> +
> + if (i < arg_positions.length () - 1)
> + obstack_grow (&fmt_obstack, ", ", strlen (", "));
> + }
> +
> + obstack_1grow (&fmt_obstack, 0);
> + warning_at_rich_loc (&richloc, OPT_Wrestrict, fmt);
> + obstack_free (&fmt_obstack, fmt);
> +}
>
> Thanks for working on this.
>
> I'm not a fan of how the patch builds "fmt" here. If nothing else,
> this perhaps should be:
>
> warning_at_rich_loc (&richloc, OPT_Wrestrict, "%s", fmt);
>
> but building up strings like the patch does makes localization
> difficult.
>
> Much better would be to have the formatting be done inside the
> diagnostics subsystem's call into pp_format, with something like this:
>
> warning_at_rich_loc_n (&richloc, OPT_Wrestrict,
> arg_positions
> .length (),
> "passing argument %i to restrict
> -qualified"
> " parameter aliases with argument
> %FIXME",
> "passing argument %i to restrict
> -qualified"
> " parameter aliases with arguments
> %FIXME",
> param_pos + 1,
>
> &arg_positions);
>
>
> and have %FIXME (or somesuch) consume &arg_positions in the va_arg,
> printing the argument numbers there. Doing it this way also avoids
> building the string for the case where Wrestrict is disabled, since the
> pp_format only happens after we know we're going to print the warning.
>
> Assuming that there isn't yet a pre-canned way to print a set of
> argument numbers that I've missed, the place to add the %FIXME-handling
> would presumably be in default_tree_printer in tree-diagnostic.c -
> though it's obviously nothing to do with trees. (Or if this is too
> single-purpose, perhaps there's a need to temporarily inject one-time
> callbacks for consuming custom args??).
>
> We can then have a fun discussion about the usage of the Oxford comma :) [1]
>
> IIRC we don't yet have warning_at_rich_loc_n, but it would be fairly easy to
> add.
Hi David,
Sorry for late response. In the attached patch, I removed obstack
building on fmt, and used
pp_format for formatting arg_positions by adding specifier %I (name
chosen arbitrarily).
Does that look OK ?
However it results in following warning during gcc build and am not
sure how to fix this:
../../gcc/gcc/c-family/c-common.c: In function ‘void
warn_for_restrict(unsigned int, vec<tree_node*, va_gc>*)’:
../../gcc/gcc/c-family/c-common.c:13132:24: warning: unknown
conversion type character ‘I’ in format [-Wformat=]
Thanks,
Prathamesh
>
> [...]
>
> [1] which seems to be locale-dependent itself:
> https://en.wikipedia.org/wiki/Serial_comma#Other_languages
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index fc25686..5081637 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -47,6 +47,7 @@ along with GCC; see the file COPYING3. If not see
#include "gimplify.h"
#include "substring-locations.h"
#include "spellcheck.h"
+#include "gcc-rich-location.h"
cpp_reader *parse_in; /* Declared in c-pragma.h. */
@@ -13084,4 +13085,51 @@ diagnose_mismatched_attributes (tree olddecl, tree
newdecl)
return warned;
}
+/* Warn if an argument at position param_pos is passed to a
+ restrict-qualified param, and it aliases with another argument. */
+
+void
+warn_for_restrict (unsigned param_pos, vec<tree, va_gc> *args)
+{
+ tree arg = (*args)[param_pos];
+ if (TREE_VISITED (arg) || operand_equal_p (arg, null_pointer_node, 0))
+ return;
+
+ location_t loc = EXPR_LOC_OR_LOC (arg, input_location);
+ gcc_rich_location richloc (loc);
+
+ unsigned i;
+ tree current_arg;
+ auto_vec<int> arg_positions;
+
+ FOR_EACH_VEC_ELT (*args, i, current_arg)
+ {
+ if (i == param_pos)
+ continue;
+
+ tree current_arg = (*args)[i];
+ if (operand_equal_p (arg, current_arg, 0))
+ {
+ TREE_VISITED (current_arg) = 1;
+ arg_positions.safe_push (i + 1);
+ }
+ }
+
+ if (arg_positions.is_empty ())
+ return;
+
+ int pos;
+ FOR_EACH_VEC_ELT (arg_positions, i, pos)
+ {
+ tree arg = (*args)[pos - 1];
+ if (EXPR_HAS_LOCATION (arg))
+ richloc.add_range (EXPR_LOCATION (arg), false);
+ }
+
+ warning_at_rich_loc (&richloc, OPT_Wrestrict, "passing argument %i to"
+ " restrict-qualified parameter aliases with argument%s
%I",
+ param_pos + 1, (arg_positions.length() > 1) ? "s" : "",
+ &arg_positions);
+}
+
#include "gt-c-family-c-common.h"
diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h
index 5bbf951..f29ea1f 100644
--- a/gcc/c-family/c-common.h
+++ b/gcc/c-family/c-common.h
@@ -922,6 +922,7 @@ extern void c_parse_final_cleanups (void);
extern void warn_for_omitted_condop (location_t, tree);
extern void warn_for_memset (location_t, tree, tree, int);
+extern void warn_for_restrict (unsigned, vec<tree, va_gc> *);
/* These macros provide convenient access to the various _STMT nodes. */
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index c55c7c3..08edea0 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -1032,6 +1032,11 @@ Wduplicate-decl-specifier
C ObjC Var(warn_duplicate_decl_specifier) Warning LangEnabledBy(C ObjC,Wall)
Warn when a declaration has duplicate const, volatile, restrict or _Atomic
specifier.
+Wrestrict
+C ObjC C++ ObjC++ Var(warn_restrict) Warning LangEnabledBy(C ObjC C++
ObjC++,Wall)
+Warn when an argument passed to a restrict-qualified parameter aliases with
+another argument.
+
ansi
C ObjC C++ ObjC++
A synonym for -std=c89 (for C) or -std=c++98 (for C++).
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 58424a9..f9043ba 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -8368,6 +8368,25 @@ c_parser_postfix_expression_after_primary (c_parser
*parser,
warn_for_memset (expr_loc, arg0, arg2, literal_zero_mask);
}
+ if (TREE_CODE (expr.value) == FUNCTION_DECL && warn_restrict)
+ {
+ unsigned i;
+ tree arg;
+ FOR_EACH_VEC_SAFE_ELT (exprlist, i, arg)
+ TREE_VISITED (arg) = 0;
+
+ unsigned param_pos = 0;
+ function_args_iterator iter;
+ tree t;
+ FOREACH_FUNCTION_ARGS (TREE_TYPE (expr.value), t, iter)
+ {
+ if (POINTER_TYPE_P (t) && TYPE_RESTRICT (t)
+ && !TYPE_READONLY (TREE_TYPE (t)))
+ warn_for_restrict (param_pos, exprlist);
+ param_pos++;
+ }
+ }
+
start = expr.get_start ();
finish = parser->tokens_buf[0].get_finish ();
expr.value
diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index fb88021..87d9ef6 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -6878,6 +6878,26 @@ cp_parser_postfix_expression (cp_parser *parser, bool
address_p, bool cast_p,
warn_for_memset (input_location, arg0, arg2, literal_mask);
}
+ if (TREE_CODE (postfix_expression) == FUNCTION_DECL
+ && warn_restrict)
+ {
+ unsigned i;
+ tree arg;
+ FOR_EACH_VEC_SAFE_ELT (args, i, arg)
+ TREE_VISITED (arg) = 0;
+
+ unsigned param_pos = 0;
+ for (tree decl = DECL_ARGUMENTS (postfix_expression);
+ decl != NULL_TREE;
+ decl = DECL_CHAIN (decl), param_pos++)
+ {
+ tree type = TREE_TYPE (decl);
+ if (POINTER_TYPE_P (type) && TYPE_RESTRICT (type)
+ && !TYPE_READONLY (TREE_TYPE (type)))
+ warn_for_restrict (param_pos, args);
+ }
+ }
+
if (TREE_CODE (postfix_expression) == COMPONENT_REF)
{
tree instance = TREE_OPERAND (postfix_expression, 0);
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 8eb5eff..d24eb6f 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -288,7 +288,7 @@ Objective-C and Objective-C++ Dialects}.
-Wparentheses -Wno-pedantic-ms-format @gol
-Wplacement-new -Wplacement-new=@var{n} @gol
-Wpointer-arith -Wno-pointer-to-int-cast @gol
--Wno-pragmas -Wredundant-decls -Wno-return-local-addr @gol
+-Wno-pragmas -Wredundant-decls -Wrestrict -Wno-return-local-addr @gol
-Wreturn-type -Wsequence-point -Wshadow -Wno-shadow-ivar @gol
-Wshift-overflow -Wshift-overflow=@var{n} @gol
-Wshift-count-negative -Wshift-count-overflow -Wshift-negative-value @gol
@@ -5348,6 +5348,12 @@ compilations.
Warn when deleting a pointer to incomplete type, which may cause
undefined behavior at runtime. This warning is enabled by default.
+@item -Wrestrict
+@opindex Wrestrict
+@opindex Wno-restrict
+Warn when an argument passed to a restrict-qualified parameter
+aliases with another argument
+
@item -Wuseless-cast @r{(C++ and Objective-C++ only)}
@opindex Wuseless-cast
@opindex Wno-useless-cast
diff --git a/gcc/pretty-print.c b/gcc/pretty-print.c
index a39815e..e8bd1ef 100644
--- a/gcc/pretty-print.c
+++ b/gcc/pretty-print.c
@@ -610,6 +610,23 @@ pp_format (pretty_printer *pp, text_info *text)
(pp, *text->args_ptr, precision, unsigned, "u");
break;
+ case 'I':
+ {
+ vec<int> *v = va_arg (*text->args_ptr, vec<int> *);
+ unsigned i;
+ int pos;
+
+ FOR_EACH_VEC_ELT ((*v), i, pos)
+ {
+ pp_scalar (pp, "%i", pos);
+ if (v->length () > 1 && i < (v->length () - 1))
+ {
+ pp_comma (pp);
+ pp_space (pp);
+ }
+ }
+ break;
+ }
case 'x':
if (wide)
pp_scalar (pp, HOST_WIDE_INT_PRINT_HEX,
diff --git a/gcc/testsuite/c-c++-common/pr35503-1.c
b/gcc/testsuite/c-c++-common/pr35503-1.c
new file mode 100644
index 0000000..25e3721
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pr35503-1.c
@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-Wrestrict" } */
+
+int foo (char *__restrict buf, const char *__restrict fmt, ...);
+
+void f(void)
+{
+ char buf[100] = "hello";
+ foo (buf, "%s-%s", buf, "world"); /* { dg-warning "passing argument 1 to
restrict-qualified parameter aliases with argument 3" } */
+}
diff --git a/gcc/testsuite/c-c++-common/pr35503-2.c
b/gcc/testsuite/c-c++-common/pr35503-2.c
new file mode 100644
index 0000000..bfcd944
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pr35503-2.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-fdiagnostics-show-caret -Wrestrict" } */
+
+void f(int *__restrict x, int *y, int *__restrict z, int *w);
+
+void foo(int alpha, int beta)
+{
+ f (&alpha, &beta, &alpha, &alpha); /* { dg-warning "passing argument 1 to
restrict-qualified parameter aliases with arguments 3, 4" } */
+
+/* { dg-begin-multiline-output "" }
+ f (&alpha, &beta, &alpha, &alpha);
+ ^~~~~~ ~~~~~~ ~~~~~~
+ { dg-end-multiline-output "" } */
+}
diff --git a/gcc/testsuite/c-c++-common/pr35503-3.c
b/gcc/testsuite/c-c++-common/pr35503-3.c
new file mode 100644
index 0000000..8cbacab
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pr35503-3.c
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-Wrestrict" } */
+
+void f(int *x, int *__restrict y);
+
+void foo(int a)
+{
+ f (&a, &a); /* { dg-warning "passing argument 2 to restrict-qualified
parameter aliases with argument 1" } */
+}
