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