From patchwork Sun Sep 18 17:16:53 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Prathamesh Kulkarni X-Patchwork-Id: 76485 Delivered-To: patch@linaro.org Received: by 10.140.106.72 with SMTP id d66csp598911qgf; Sun, 18 Sep 2016 10:17:25 -0700 (PDT) X-Received: by 10.98.55.1 with SMTP id e1mr39854874pfa.58.1474219045270; Sun, 18 Sep 2016 10:17:25 -0700 (PDT) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id b76si21148137pfd.120.2016.09.18.10.17.24 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 18 Sep 2016 10:17:25 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-return-436169-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org; spf=pass (google.com: domain of gcc-patches-return-436169-patch=linaro.org@gcc.gnu.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=gcc-patches-return-436169-patch=linaro.org@gcc.gnu.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; q=dns; s=default; b=BLYFGQOZs/V3DEU DOUKGwcN6pwnT3EJCtJ/hCUiDaDM8bc+yaXJP44CyRxf1JHryvbLWcD0TA6d2ylQ t7QEH5f5oV95IV/MfAHu40BXKwyDcdASZReywdOa0twyy/HRAygkbvNWJr3ZH3Ml EYPOKGRsNk3ylEwfSlVvfyf+Oqbo= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; s=default; bh=8fhI592byotKfDJ0WdWb0 n1UJ2U=; b=SpXIHc9fvkSeyj/A8M3FafmwDYMoAMz2TdoJ3IQ5YWHqGVxEuq3E6 LGT01hoeVoNdf0/Lcej9yXxqYWXZiPFbYQuy0dV7Hua6xhYIwhS3eyIP4ee4OWQC u2Nngx/Yyinz+ghNI2yCNQx//6EVTQ/OwmlxzC3rNoeqZwqpOymNBM= Received: (qmail 31723 invoked by alias); 18 Sep 2016 17:17:08 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 28944 invoked by uid 89); 18 Sep 2016 17:17:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, RCVD_IN_SORBS_SPAM, SPF_PASS autolearn=ham version=3.3.2 spammy=10326, oxford, sk:en.wiki, UD:en.wikipedia.org X-HELO: mail-io0-f177.google.com Received: from mail-io0-f177.google.com (HELO mail-io0-f177.google.com) (209.85.223.177) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 18 Sep 2016 17:16:56 +0000 Received: by mail-io0-f177.google.com with SMTP id q92so72396127ioi.1 for ; Sun, 18 Sep 2016 10:16:56 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=8LOeBRGIVS/N0Lw12XFeMNX80TtYSCOqDudV0VmGlU4=; b=iy6p+S2HxUuIJWEab2DyC7wf9FzZexSCffRQg6tIgciUn+n4WZvipOnu/kI+OcmTqV iafSCmzFEaQ2Uyw8NNaynwAnJTHzKwxRWkrqo/VrkFLyc7/u8xdjgZBUd3FXA/aaGcOQ guKt1H3XEmIoIeJ26eGuKnKr2EDuGKMNcWxRW13zQrSVN3AiDW2SjmZF85jCHnjz6I/Y brH/1uaAFv8Dvh4TRBXzrBFaZQfuTvam6yOvSJXk8anGPbVP58z8kibICfxw9i37U6RZ rMh//La1kWOqUWFEioCbfZTB1cBdpbiN+r5BAnrN924pJNILVVm+dkpGtddg+3f0L2xe VerA== X-Gm-Message-State: AE9vXwPZzvv1gOKhwNFfSncddwKWf/yaCJm9hdeAdGM7jKsCxiPduyewNfqwkVM6cDicDL4nBznEtDZpvznU8E2d X-Received: by 10.107.160.83 with SMTP id j80mr32101750ioe.27.1474219014870; Sun, 18 Sep 2016 10:16:54 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.81.85 with HTTP; Sun, 18 Sep 2016 10:16:53 -0700 (PDT) In-Reply-To: <1472838245.5595.183.camel@redhat.com> References: <181560f0-c738-cff9-9002-101686da4c14@mentor.com> <1472838245.5595.183.camel@redhat.com> From: Prathamesh Kulkarni Date: Sun, 18 Sep 2016 22:46:53 +0530 Message-ID: Subject: Re: PR35503 - warn for restrict pointer To: David Malcolm Cc: Richard Biener , Tom de Vries , gcc Patches , Marek Polacek , "Joseph S. Myers" , Jason Merrill X-IsSubscribed: yes On 2 September 2016 at 23:14, David Malcolm 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 *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. 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*)’: ../../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 *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 + 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 *); /* 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 *v = va_arg (*text->args_ptr, vec *); + 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" } */ +}