From patchwork Sun Apr 27 21:36:23 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Prathamesh Kulkarni X-Patchwork-Id: 343196 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 099E31400BB for ; Mon, 28 Apr 2014 07:36:36 +1000 (EST) 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:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=DT8c5/cA2rb/y8jWXZ XIwQPGpdWJG8KRDgbe9E+ii7+p67eGej3RAqeS8ly3lUABDP0nOMm+uHJU7wjKGl XqwY/Pyxbwd9rRef+ss6prsgxt0vZrIgKin6yq2mW/QL5wM+yFoR5nbXYq4z86a+ L34so/nIaERjY6sp7pz37kTDc= 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:date:message-id:subject :from:to:cc:content-type; s=default; bh=v7wfnXbc9w+XdEmM7KceV0tJ l+o=; b=XXqU6qz8Dt9lamCxX1UQ/lTL9NwhTrBUKv9xsNw6584kc6JwO/l0rJq4 QE0co4+XiN68i3HfJuqOYHpehvWVIsk1Duga2JCCkcXAyahOdakWrIgA8wcHUpm8 ZSgvSnEXxAytahydL0oq06U7PsfLCQf9k1eKcPmYJkdQhQnMJTg= Received: (qmail 27300 invoked by alias); 27 Apr 2014 21:36:28 -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 27289 invoked by uid 89); 27 Apr 2014 21:36:28 -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, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ob0-f182.google.com Received: from mail-ob0-f182.google.com (HELO mail-ob0-f182.google.com) (209.85.214.182) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sun, 27 Apr 2014 21:36:25 +0000 Received: by mail-ob0-f182.google.com with SMTP id uy5so6506577obc.13 for ; Sun, 27 Apr 2014 14:36:24 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.60.59.196 with SMTP id b4mr3788104oer.48.1398634583961; Sun, 27 Apr 2014 14:36:23 -0700 (PDT) Received: by 10.182.105.166 with HTTP; Sun, 27 Apr 2014 14:36:23 -0700 (PDT) In-Reply-To: References: <20140427120125.GA2354@tsaunders-iceball.corp.tor1.mozilla.com> <20140427151814.GB2354@tsaunders-iceball.corp.tor1.mozilla.com> <893B8D9F-8E28-4F9E-B05C-1B8CB402C346@gmail.com> Date: Mon, 28 Apr 2014 03:06:23 +0530 Message-ID: Subject: Re: [C PATCH] proposal to add new warning -Wsizeof-array-argument From: Prathamesh Kulkarni To: Andrew Pinski Cc: Trevor Saunders , Richard Biener , "Joseph S. Myers" , Marek Polacek , "gcc-patches@gcc.gnu.org" X-IsSubscribed: yes On Mon, Apr 28, 2014 at 2:13 AM, Andrew Pinski wrote: > On Sun, Apr 27, 2014 at 1:25 PM, Prathamesh Kulkarni > wrote: >> On Mon, Apr 28, 2014 at 1:31 AM, Andrew Pinski wrote: >>> On Sun, Apr 27, 2014 at 12:50 PM, Prathamesh Kulkarni >>> wrote: >>>> On Sun, Apr 27, 2014 at 11:22 PM, wrote: >>>>> >>>>> >>>>>> On Apr 27, 2014, at 10:09 AM, Prathamesh Kulkarni wrote: >>>>>> >>>>>>> On Sun, Apr 27, 2014 at 8:48 PM, Trevor Saunders wrote: >>>>>>>> On Sun, Apr 27, 2014 at 06:21:20PM +0530, Prathamesh Kulkarni wrote: >>>>>>>>> On Sun, Apr 27, 2014 at 5:31 PM, Trevor Saunders wrote: >>>>>>>>>> On Sun, Apr 27, 2014 at 02:31:46AM +0530, Prathamesh Kulkarni wrote: >>>>>>>>>> Hi, >>>>>>>>>> Shall it a good idea to add new warning -Wsizeof-array-argument that >>>>>>>>>> warns when sizeof is applied on parameter declared as an array ? >>>>>>>>> >>>>>>>>> Seems reasonable enough. >>>>>>>>> >>>>>>>>>> Similar to clang's -Wsizeof-array-argument: >>>>>>>>>> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110613/042812.html >>>>>>>>>> This was also reported as PR6940: >>>>>>>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=6940 >>>>>>>>>> >>>>>>>>>> I have attached a patch that adds the warning to C front-end. >>>>>>>>> >>>>>>>>> if we're doing this for C, we should probably do it for C++ too. >>>>>>>>> >>>>>>>>>> I implemented it by adding a new member BOOL_BITFIELD is_array_parm to >>>>>>>>>> tree_parm_decl. Not sure if that's a good approach. >>>>>>>>> >>>>>>>>> I'm about the last one who should comment on this, but it seems pretty >>>>>>>>> crazy you can't use data that's already stored. >>>>>>>> AFAIU, the information about declarator is stored in c_declarator. >>>>>>>> c_declarator->kind == cdk_array holds true >>>>>>>> if the declarator is an array. However in push_parm_decl, call to >>>>>>>> grokdeclarator returns decl of pointer_type, corresponding to array >>>>>>>> declarator if the array is parameter (TREE_TYPE (decl) is >>>>>>>> pointer_type). So I guess we lose that information there. >>>>>>> >>>>>>> I guess that sort of makes sense, so I'll shut up ;) >>>>>>> >>>>>>>>>> Index: gcc/tree-core.h >>>>>>>>>> =================================================================== >>>>>>>>>> --- gcc/tree-core.h (revision 209800) >>>>>>>>>> +++ gcc/tree-core.h (working copy) >>>>>>>>>> @@ -1411,6 +1411,7 @@ struct GTY(()) tree_const_decl { >>>>>>>>>> struct GTY(()) tree_parm_decl { >>>>>>>>>> struct tree_decl_with_rtl common; >>>>>>>>>> rtx incoming_rtl; >>>>>>>>>> + BOOL_BITFIELD is_array_parm; >>>>>>>>> >>>>>>>>> BOOL_BITFIELD only makes sense if you declare it as an actually bitfield >>>>>>>>> with size less than that of unisgned int, otherwise you might as well >>>>>>>>> use that directly. On the other hand I wonder if we can't just nuke >>>>>>>>> BOOL_BITFIELD, it seems to be legacy from a time of C and bool not being >>>>>>>>> a builtin type? >>>>>>>> Thanks, I wasn't aware of that. Could we now use C++ bool type instead ? >>>>>>> >>>>>>> you can certainly do |bool x;| as struct fields, that's already all >>>>>>> over. However its not entirely clear to me if |bool x : 1;| will work >>>>>>> everywhere and take the single bit you'd expect, istr there being >>>>>>> compilers that do stupid things if you use multiple types next to each >>>>>>> other in bitfields, but I'm not sure if we need to care about any of >>>>>>> those. >>>>>> Changed to bool is_array_parm; >>>>>> and from macros to inline functions. >>>>> >>>>> I don't like this field being part of the generic code as it increases the size of the struct for all front-ends and even during LTO. Is there a way to do this using one of the language specific bitfields that are already there (language flags iirc)? >>>> I guess the warning would be shared by c-family languages, so I had >>>> added the field to tree_parm_decl. >>>> This patch is C-only (added the member to c-lang.h:lang_type instead). >>> >>> That was not talking about. I was talking about DECL_LANG_FLAG_* >>> which is already there for your usage. >>> You should be able to use DECL_LANG_FLAG_2 as it is unused for both C >>> and C++ for PARM_DECLs. This should also reduce the size of the patch >>> too. >> Thanks for pointing it out, I have modified the patch. >> >> [gcc/c] >> * c-decl.c (push_parm_decl): Set DECL_LANG_FLAG_2 (decl) if declarator >> is array parameter. >> * c-typeck.c (c_expr_sizeof_expr): Check for sizeof-array-argument warning. >> >> [gcc/c-family] >> * c.opt (-Wsizeof-array-argument): New option. >> >> [gcc/testsuite/gcc.dg] >> * sizeof-array-argument.c: New test-case. > > > Can you add a new macro in c-tree.h for this new usage of > DECL_LANG_FLAG_2 so it is easier to figure out what the used lang flag > bits are in use and to understand what that flag bit means? Is name C_ARRAY_PARM ok ? Bootstrapped on x86_64-unknown-linux-gnu [gcc/c] * c-tree.h (C_ARRAY_PARM): New macro, alias for DECL_LANG_FLAG_2. * c-decl.c (push_parm_decl): Set C_ARRAY_PARM (decl) if declarator is an array parameter. * c-typeck.c (c_expr_sizeof_expr): Check for sizeof-array-argument warning. [gcc/c-family] * c.opt (-Wsizeof-array-argument): New option. [gcc/testsuite/gcc.dg] * sizeof-array-argument.c: New test-case. Thanks and Regards, Prathamesh > > Thanks, > Andrew Pinski > >> >> Thanks and Regards, >> Prathamesh >>> >>> Thanks, >>> Andrew Pinski >>> >>>> >>>> [gcc/c] >>>> * c-decl.c (push_parm_decl): Check if declarator is array parameter. >>>> * c-lang.h (lang_type): Add new member is_array_parm. >>>> * c-typeck.c (c_expr_sizeof_expr): Check for sizeof-array-argument warning. >>>> >>>> [gcc/c-family] >>>> * c.opt (-Wsizeof-array-argument): New option. >>>> >>>> [gcc/testsuite/gcc.dg] >>>> * sizeof-array-argument.c: New test-case. >>>> >>>> Thanks and Regards, >>>> Prathamesh >>>>> >>>>> Thanks, >>>>> Andrew >>>>> >>>>> >>>>>> >>>>>> [gcc] >>>>>> * tree-core.h (tree_parm_decl): Add new member bool is_array_parm >>>>>> * tree.h (set_parm_decl_is_array): New function. >>>>>> (parm_decl_array_p): New function. >>>>>> >>>>>> [gcc/c] >>>>>> * c-decl.c (push_parm_decl): Call set_parm_decl_is_array. >>>>>> * c-typeck.c (c_expr_sizeof_expr): Add check for sizeof-array-argument warning. >>>>>> >>>>>> [gcc/c-family] >>>>>> * c.opt (-Wsizeof-array-argument): New option. >>>>>> >>>>>> [gcc/testsuite/gcc.dg] >>>>>> * sizeof-array-argument.c: New test-case. >>>>>> >>>>>> Thanks and Regards, >>>>>> Prathamesh >>>>>>> >>>>>>> Trev >>>>>>> >>>>>>>>> >>>>>>>>>> Index: gcc/tree.h >>>>>>>>>> =================================================================== >>>>>>>>>> --- gcc/tree.h (revision 209800) >>>>>>>>>> +++ gcc/tree.h (working copy) >>>>>>>>>> @@ -1742,6 +1742,7 @@ extern void protected_set_expr_location >>>>>>>>>> #define TYPE_LANG_SPECIFIC(NODE) \ >>>>>>>>>> (TYPE_CHECK (NODE)->type_with_lang_specific.lang_specific) >>>>>>>>>> >>>>>>>>>> + >>>>>>>>>> #define TYPE_VALUES(NODE) (ENUMERAL_TYPE_CHECK (NODE)->type_non_common.values) >>>>>>>>>> #define TYPE_DOMAIN(NODE) (ARRAY_TYPE_CHECK (NODE)->type_non_common.values) >>>>>>>>>> #define TYPE_FIELDS(NODE) \ >>>>>>>>>> @@ -2258,6 +2259,12 @@ extern void decl_value_expr_insert (tree >>>>>>>>>> #define DECL_INCOMING_RTL(NODE) \ >>>>>>>>>> (PARM_DECL_CHECK (NODE)->parm_decl.incoming_rtl) >>>>>>>>>> >>>>>>>>>> +#define SET_PARM_DECL_IS_ARRAY(NODE, val) \ >>>>>>>>>> + (PARM_DECL_CHECK (NODE)->parm_decl.is_array_parm = (val)) >>>>>>>>> >>>>>>>>> if we're adding more stuff here is there a reason it needs to be a macro >>>>>>>>> not a inline function? >>>>>>>> No, shall change that to inline function. >>>>>>>>> >>>>>>>>> Trev >>>>>> Index: gcc/c/c-decl.c =================================================================== --- gcc/c/c-decl.c (revision 209800) +++ gcc/c/c-decl.c (working copy) @@ -4630,6 +4630,8 @@ push_parm_decl (const struct c_parm *par decl = grokdeclarator (parm->declarator, parm->specs, PARM, false, NULL, &attrs, expr, NULL, DEPRECATED_NORMAL); decl_attributes (&decl, attrs, 0); + + C_ARRAY_PARM (decl) = parm->declarator->kind == cdk_array; decl = pushdecl (decl); Index: gcc/c/c-tree.h =================================================================== --- gcc/c/c-tree.h (revision 209800) +++ gcc/c/c-tree.h (working copy) @@ -66,6 +66,9 @@ along with GCC; see the file COPYING3. /* For a FUNCTION_DECL, nonzero if it was an implicit declaration. */ #define C_DECL_IMPLICIT(EXP) DECL_LANG_FLAG_2 (EXP) +/* For a PARM_DECL, nonzero if parameter was declared as array */ +#define C_ARRAY_PARM(NODE) DECL_LANG_FLAG_2 (NODE) + /* For FUNCTION_DECLs, evaluates true if the decl is built-in but has been declared. */ #define C_DECL_DECLARED_BUILTIN(EXP) \ Index: gcc/c/c-typeck.c =================================================================== --- gcc/c/c-typeck.c (revision 209800) +++ gcc/c/c-typeck.c (working copy) @@ -2730,6 +2730,13 @@ c_expr_sizeof_expr (location_t loc, stru else { bool expr_const_operands = true; + + if (warn_sizeof_array_argument && TREE_CODE (expr.value) == PARM_DECL && C_ARRAY_PARM (expr.value)) + { + warning_at (loc, 0, "sizeof on array parameter %<%s%> shall return size of %qT", + IDENTIFIER_POINTER (DECL_NAME (expr.value)), expr.original_type); + inform (DECL_SOURCE_LOCATION (expr.value), "declared here"); + } tree folded_expr = c_fully_fold (expr.value, require_constant_value, &expr_const_operands); ret.value = c_sizeof (loc, TREE_TYPE (folded_expr)); Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt (revision 209800) +++ gcc/c-family/c.opt (working copy) @@ -509,6 +509,9 @@ Warn about missing fields in struct init Wsizeof-pointer-memaccess C ObjC C++ ObjC++ Var(warn_sizeof_pointer_memaccess) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) +Wsizeof-array-argument +C Var(warn_sizeof_array_argument) Warning LangEnabledBy(C,Wall) + Wsuggest-attribute=format C ObjC C++ ObjC++ Var(warn_suggest_attribute_format) Warning Warn about functions which might be candidates for format attributes Index: gcc/testsuite/gcc.dg/sizeof-array-argument.c =================================================================== --- gcc/testsuite/gcc.dg/sizeof-array-argument.c (revision 0) +++ gcc/testsuite/gcc.dg/sizeof-array-argument.c (working copy) @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-Wsizeof-array-argument" } */ + +int foo(int a[]) +{ + return (int) sizeof (a); /* { dg-warning "sizeof on array parameter" } */ +} + +int bar(int x, int b[3]) +{ + return x + (int) sizeof (b); /* { dg-warning "sizeof on array parameter" } */ +} + +int f(int *p) +{ + return (int) sizeof (*p); +}