From patchwork Tue Aug 30 17:56:57 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Prathamesh Kulkarni X-Patchwork-Id: 664239 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3sNx7540HHz9s5g for ; Wed, 31 Aug 2016 03:57:20 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=XBVrqCQY; dkim-atps=neutral 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=UO1KDNIThsyKIRc xmJM+rh/Tfa55XZCcrmuZgPmUgKnoTyNuRNHcZ/oidJU8Nkyo1RtL6Ehn0kfwP/e eWO49H3ET+DA3J1HHWaTuqdiiXcrwITQYInw4LKdhxM8hsDsb9aE/ekC1OxtFJjM sRmPbMG5YOmiHEPWq0qGcMi45ehA= 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=TCN3wGdjaX1V2YRYmCIE2 /2LM18=; b=XBVrqCQYjRnUzOghPTeHhn/G43FtYXXQ2HyRFtwyBIrHxJ0F8tpX4 XV1CTGe4p23iZbiS9aO9Frc89PXXnZYUFbY/9dA5J5RIGtW/hWP9NjaF/F8a8t1P j8MPI6vviK9QE4XXkysxUC64xuSkDY1nL2wyyK8/wUwixShe2QBvDE= Received: (qmail 53341 invoked by alias); 30 Aug 2016 17:57:11 -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 53319 invoked by uid 89); 30 Aug 2016 17:57:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=deleting X-HELO: mail-it0-f53.google.com Received: from mail-it0-f53.google.com (HELO mail-it0-f53.google.com) (209.85.214.53) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 30 Aug 2016 17:57:00 +0000 Received: by mail-it0-f53.google.com with SMTP id e63so179662084ith.1 for ; Tue, 30 Aug 2016 10:56:59 -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=yAqOg7he3z7FEkBX3dCp0vmiv7jldLZiE3eKtHRTWc8=; b=J540hfduSEXTyY5eQ6JRJnJLpyxJ0oRqqSWDsBCm5+gdEsnP/7ME8zEjgTUlpwLdxw EhvXLZfjMHuehvGrYL967Vhkm5aWRENHoIyEN58aH34DRrj4w6Tknpvi9zRrZkKWnOfI y6eEzELEB3ZS9MwdqgYmz6io4tcafY9r99CP5Wa7dlMFJM2Yf/eQ4aG3qrxKRqQUUVLv FAPRYlnDRpXE5FJ+zcw/3keY+fTrQy8voCDOm3o6+lfQ7vsXAAToLhfsQKoAyUJVnMYO LPch693rijbO3DIZ6lMd6OPSj7fQ6Edlro/d1ie9iR5p2AksHLWaQXAM10K+FVUK/Klb CMHw== X-Gm-Message-State: AE9vXwP18DIVdjWipaR6/YHDcWA9vLSJwpTHUmngjvLy0Eh26xmIYBXaQsNZO2sGLWUMuIrMK10g4n6rHXYCajDY X-Received: by 10.36.219.138 with SMTP id c132mr7565995itg.16.1472579818243; Tue, 30 Aug 2016 10:56:58 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.65.137 with HTTP; Tue, 30 Aug 2016 10:56:57 -0700 (PDT) In-Reply-To: <181560f0-c738-cff9-9002-101686da4c14@mentor.com> References: <181560f0-c738-cff9-9002-101686da4c14@mentor.com> From: Prathamesh Kulkarni Date: Tue, 30 Aug 2016 23:26:57 +0530 Message-ID: Subject: Re: PR35503 - warn for restrict pointer To: Tom de Vries Cc: gcc Patches , Marek Polacek , "Joseph S. Myers" , Jason Merrill , Richard Biener X-IsSubscribed: yes On 30 August 2016 at 22:58, Tom de Vries wrote: > On 30/08/16 17:34, Prathamesh Kulkarni wrote: >> >> On 30 August 2016 at 20:24, Tom de Vries wrote: >>> >>> On 26/08/16 13:39, Prathamesh Kulkarni wrote: >>>> >>>> >>>> Hi, >>>> The following patch adds option -Wrestrict that warns when an argument >>>> is passed to a restrict qualified parameter and it aliases with >>>> another argument. >>>> >>>> eg: >>>> int foo (const char *__restrict buf, const char *__restrict fmt, ...); >>>> >>>> void f(void) >>>> { >>>> char buf[100] = "hello"; >>>> foo (buf, "%s-%s", buf, "world"); >>>> } >>> >>> >>> >>> Does -Wrestrict generate a warning for this example? >>> >>> ... >>> void h(int n, int * restrict p, int * restrict q, int * restrict r) >>> { >>> int i; >>> for (i = 0; i < n; i++) >>> p[i] = q[i] + r[i]; >>> } >>> >>> h (100, a, b, b) >>> ... >>> >>> [ Note that this is valid C, and does not violate the restrict >>> definition. ] >>> >>> If -Wrestrict indeed generates a warning, then we should be explicit in >>> the >>> documentation that while the warning triggers on this type of example, >>> the >>> code is correct. >> >> I am afraid it would warn for the above case. The patch just checks if >> the parameter is qualified >> with restrict, and if the corresponding argument has aliases in the >> call (by calling operand_equal_p). > > >> Is such code common in practice ? > > > [ The example is from the definition of restrict in the c99 standard. ] > > According to the definition of restrict, only the restrict on p is required > to know that p doesn't alias with q and that p doesn't alias with r. > > AFAIK the current implementation of gcc already generates optimal code if > restrict is only on p. But earlier versions (and possibly other compilers as > well?) need the restrict on q and r as well. > > So I expect this code to occur. > >> If it is, I wonder if we should keep >> the warning in -Wall ? >> > > Hmm, I wonder if we can use the const keyword to silence the warning. > > So if we generate a warning for: > ... > void h(int n, int * restrict p, int * restrict q, int * restrict r) > { > int i; > for (i = 0; i < n; i++) > p[i] = q[i] + r[i]; > } > h (100, a, b, b) > ... > > but not for: > ... > void h(int n, int * restrict p, const int * restrict q, const int * restrict > r) > { > int i; > for (i = 0; i < n; i++) > p[i] = q[i] + r[i]; > } > h (100, a, b, b) > ... > > Then there's an easy way to rewrite the example such that the warning > doesn't trigger, without having to remove the restrict. The attached (untested) patch silences the warning if parameter is const qualified. I will give it some testing after resolving a different issue. Thanks, Prathamesh > > Thanks, > - Tom diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 3feb910..4239cef 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,86 @@ 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; + unsigned ranges_added = 1; + + FOR_EACH_VEC_ELT (arg_positions, i, pos) + { + /* FIXME: Fow now, only 3 ranges can be added. Remove this once + the restriction is lifted. */ + if (ranges_added < 3) + { + tree arg = (*args)[pos]; + if (EXPR_HAS_LOCATION (arg)) + { + richloc.add_range (EXPR_LOCATION (arg), false); + ranges_added++; + } + } + + 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); +} + #include "gt-c-family-c-common.h" diff --git a/gcc/c-family/c-common.h b/gcc/c-family/c-common.h index bc22baa..cdb762e 100644 --- a/gcc/c-family/c-common.h +++ b/gcc/c-family/c-common.h @@ -920,6 +920,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 a5358ed..a029a86 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -1012,6 +1012,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 C++ Var(warn_restrict) Warning LangEnabledBy(C C++,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 fe0c95f..05510f6 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -8369,6 +8369,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 690e928..ab73655 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 1f04501..3bd9612 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 @@ -5274,6 +5274,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 @r{(C and C++ only)} +@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/testsuite/c-c++-common/pr35503-1.c b/gcc/testsuite/c-c++-common/pr35503-1.c new file mode 100644 index 0000000..b47fdda --- /dev/null +++ b/gcc/testsuite/c-c++-common/pr35503-1.c @@ -0,0 +1,10 @@ +/* { dg-do compile } */ +/* { dg-options "-Wrestrict" } */ + +int foo (const 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..b09872a --- /dev/null +++ b/gcc/testsuite/c-c++-common/pr35503-2.c @@ -0,0 +1,15 @@ +/* { 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" "" { target *-*-* } 8 } */ + +/* { 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" } */ +}