diff mbox

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

Message ID CAJXstsBiQa15H63ADXRuRaVTD-R_vyJcEXzxtSOtZ7YKE_O9ww@mail.gmail.com
State New
Headers show

Commit Message

Prathamesh Kulkarni April 27, 2014, 5:09 p.m. UTC 
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.

[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
>> >

Comments

Andrew Pinski April 27, 2014, 5:52 p.m. UTC | #1 
> 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)?

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/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 is_array_parm;
 };
 
 struct GTY(()) tree_decl_with_vis {
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,7 @@  extern void decl_value_expr_insert (tree
 #define DECL_INCOMING_RTL(NODE) \
   (PARM_DECL_CHECK (NODE)->parm_decl.incoming_rtl)
 
+
 /* Nonzero for a given ..._DECL node means that no warnings should be
    generated just because this node is unused.  */
 #define DECL_IN_SYSTEM_HEADER(NODE) \
@@ -4486,6 +4488,18 @@  opts_for_fn (const_tree fndecl)
   return TREE_OPTIMIZATION (fn_opts);
 }
 
+static inline void
+set_parm_decl_is_array (tree node, bool val)
+{
+  PARM_DECL_CHECK (node)->parm_decl.is_array_parm = val;
+}
+
+static inline bool
+parm_decl_array_p (const_tree node)
+{
+  return PARM_DECL_CHECK (node)->parm_decl.is_array_parm;
+}
+
 /* opt flag for function FNDECL, e.g. opts_for_fn (fndecl, optimize) is
    the optimization level of function fndecl.  */
 #define opt_for_fn(fndecl, opt) (opts_for_fn (fndecl)->x_##opt)
Index: gcc/c/c-decl.c
===================================================================
--- gcc/c/c-decl.c	(revision 209800)
+++ gcc/c/c-decl.c	(working copy)
@@ -4630,9 +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);
-
+  set_parm_decl_is_array (decl, parm->declarator->kind == cdk_array); 
   decl = pushdecl (decl);
-
   finish_decl (decl, input_location, NULL_TREE, NULL_TREE, NULL_TREE);
 }
 
Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 209800)
+++ gcc/c/c-typeck.c	(working copy)
@@ -2730,6 +2730,12 @@  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 && parm_decl_array_p (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);
+}