From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 73174 invoked by alias); 2 Sep 2016 17:44:21 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 73160 invoked by uid 89); 2 Sep 2016 17:44:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.4 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=va_arg, subsystems X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 02 Sep 2016 17:44:10 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D19E37DCD1; Fri, 2 Sep 2016 17:44:07 +0000 (UTC) Received: from vpn-230-246.phx2.redhat.com (vpn-230-246.phx2.redhat.com [10.3.230.246]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u82Hi6no022729; Fri, 2 Sep 2016 13:44:06 -0400 Message-ID: <1472838245.5595.183.camel@redhat.com> Subject: Re: PR35503 - warn for restrict pointer From: David Malcolm To: Prathamesh Kulkarni , Richard Biener Cc: Tom de Vries , gcc Patches , Marek Polacek , "Joseph S. Myers" , Jason Merrill Date: Fri, 02 Sep 2016 17:44:00 -0000 In-Reply-To: References: <181560f0-c738-cff9-9002-101686da4c14@mentor.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-09/txt/msg00110.txt.bz2 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 *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 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