On Sun, Apr 27, 2014 at 1:25 PM, Prathamesh Kulkarni <bilbotheelffri...@gmail.com> wrote: > On Mon, Apr 28, 2014 at 1:31 AM, Andrew Pinski <pins...@gmail.com> wrote: >> On Sun, Apr 27, 2014 at 12:50 PM, Prathamesh Kulkarni >> <bilbotheelffri...@gmail.com> wrote: >>> On Sun, Apr 27, 2014 at 11:22 PM, <pins...@gmail.com> wrote: >>>> >>>> >>>>> On Apr 27, 2014, at 10:09 AM, Prathamesh Kulkarni >>>>> <bilbotheelffri...@gmail.com> wrote: >>>>> >>>>>> On Sun, Apr 27, 2014 at 8:48 PM, Trevor Saunders <tsaund...@mozilla.com> >>>>>> 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 >>>>>>>> <tsaund...@mozilla.com> 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? 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 >>>>> <sizeof-array-argument.patch>