Re: PR35503 - warn for restrict pointer

[Prathamesh Kulkarni <prathamesh dot kulkarni at linaro dot org>]

On 2 September 2016 at 23:14, David Malcolm <dmalcolm@redhat.com> 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

Attachment: pr35503-8.diff
Description: Text document