diff mbox

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

Message ID CAJXstsDhLd-ObOHdKnDuoR87v3Ah_QsVcQP-g3jH14m7BeLD4w@mail.gmail.com
State New
Headers show

Commit Message

Prathamesh Kulkarni April 27, 2014, 9:36 p.m. UTC 
On Mon, Apr 28, 2014 at 2:13 AM, Andrew Pinski <pinskia@gmail.com> wrote:
> 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?
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
>>>>>> <sizeof-array-argument.patch>
diff mbox

Patch

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);
+}