Re: [C PATCH] proposal to add new warning -Wsizeof-array-argument

[Andrew Pinski <pinskia at gmail dot com>]

On Sun, Apr 27, 2014 at 1:25 PM, Prathamesh Kulkarni
<bilbotheelffriend@gmail.com> wrote:
> On Mon, Apr 28, 2014 at 1:31 AM, Andrew Pinski <pinskia@gmail.com> wrote:
>> On Sun, Apr 27, 2014 at 12:50 PM, Prathamesh Kulkarni
>> <bilbotheelffriend@gmail.com> wrote:
>>> On Sun, Apr 27, 2014 at 11:22 PM,  <pinskia@gmail.com> wrote:
>>>>
>>>>
>>>>> On Apr 27, 2014, at 10:09 AM, Prathamesh Kulkarni <bilbotheelffriend@gmail.com> wrote:
>>>>>
>>>>>> On Sun, Apr 27, 2014 at 8:48 PM, Trevor Saunders <tsaunders@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 <tsaunders@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>