On Sun, 2016-09-18 at 22:46 +0530, Prathamesh Kulkarni wrote:
> On 2 September 2016 at 23:14, David Malcolm <dmalc...@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 ?
Thanks - this is a big improvement.
In pretty-print.c, please can you add a description of "%I" to the
comment at the top of pp_format. Also, please add some selftests for
%I to test_pp_format, covering, say the case of 0, 1 and 2 elements in
the vec (to ensure that the separator commas are working properly).
> 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
