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

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 1219 bytes --]

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 ?
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.
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.
Also should it be enabled by -Wall or -Wextra or only by
-Wsizeof-array-argument ?
Currently I have made it enabled by -Wall along with -Wsizeof-array-argument.

Bootstrapped on x86_64-unknown-linux-gnu.

[gcc]
* tree-core.h (tree_parm_decl): Add new member BOOL_BITFIELD is_array_parm
* tree.h (SET_PARM_DECL_IS_ARRAY): New macro.
            (PARM_DECL_ARRAY_P): New macro.

[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

[-- Attachment #2: sizeof-array-argument.patch --]
[-- Type: text/x-patch, Size: 4080 bytes --]

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;
 };
 
 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,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))
+
+#define PARM_DECL_ARRAY_P(NODE) \
+  (PARM_DECL_CHECK (NODE)->parm_decl.is_array_parm != 0)
+
 /* 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) \
Index: gcc/c/c-decl.c
===================================================================
--- gcc/c/c-decl.c	(revision 209800)
+++ gcc/c/c-decl.c	(working copy)
@@ -4626,13 +4626,14 @@ push_parm_decl (const struct c_parm *par
 {
   tree attrs = parm->attrs;
   tree decl;
+  int is_array;
 
   decl = grokdeclarator (parm->declarator, parm->specs, PARM, false, NULL,
 			 &attrs, expr, NULL, DEPRECATED_NORMAL);
   decl_attributes (&decl, attrs, 0);
-
+  is_array = parm->declarator->kind == cdk_array;
+  SET_PARM_DECL_IS_ARRAY (decl, is_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);
+}

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

[Trevor Saunders]

[-- Attachment #1: Type: text/plain, Size: 2464 bytes --]

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.

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

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

Trev


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Prathamesh Kulkarni]

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.
>
>> 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 ?
>
>> 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
>

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

[Trevor Saunders]

[-- Attachment #1: Type: text/plain, Size: 3667 bytes --]

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.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 4385 bytes --]

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

[-- Attachment #2: sizeof-array-argument.patch --]
[-- Type: text/x-patch, Size: 4323 bytes --]

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

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

[pinskia]

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

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 5551 bytes --]

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).

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

[-- Attachment #2: sizeof-array-argument.patch --]
[-- Type: text/x-patch, Size: 3303 bytes --]

Index: gcc/c/c-decl.c
===================================================================
--- gcc/c/c-decl.c	(revision 209800)
+++ gcc/c/c-decl.c	(working copy)
@@ -4626,13 +4626,19 @@ push_parm_decl (const struct c_parm *par
 {
   tree attrs = parm->attrs;
   tree decl;
+  struct lang_type *lt;
 
   decl = grokdeclarator (parm->declarator, parm->specs, PARM, false, NULL,
 			 &attrs, expr, NULL, DEPRECATED_NORMAL);
   decl_attributes (&decl, attrs, 0);
-
+  
+  lt = TYPE_LANG_SPECIFIC (TREE_TYPE (decl));
+  if (!lt) 
+    lt = XNEW (struct lang_type);
+  lt->is_array_parm = parm->declarator->kind == cdk_array;
+  TYPE_LANG_SPECIFIC (TREE_TYPE (decl)) = lt;
+  
   decl = pushdecl (decl);
-
   finish_decl (decl, input_location, NULL_TREE, NULL_TREE, NULL_TREE);
 }
 
Index: gcc/c/c-lang.h
===================================================================
--- gcc/c/c-lang.h	(revision 209800)
+++ gcc/c/c-lang.h	(working copy)
@@ -33,6 +33,7 @@ struct GTY((variable_size)) lang_type {
      as a list of adopted protocols or a pointer to a corresponding
      @interface.  See objc/objc-act.h for details.  */
   tree objc_info;
+  bool is_array_parm;
 };
 
 struct GTY((variable_size)) lang_decl {
Index: gcc/c/c-typeck.c
===================================================================
--- gcc/c/c-typeck.c	(revision 209800)
+++ gcc/c/c-typeck.c	(working copy)
@@ -2730,6 +2730,14 @@ c_expr_sizeof_expr (location_t loc, stru
   else
     {
       bool expr_const_operands = true;
+      struct lang_type *lt = TYPE_LANG_SPECIFIC (TREE_TYPE (expr.value));
+
+      if (warn_sizeof_array_argument && TREE_CODE (expr.value) == PARM_DECL && lt && lt->is_array_parm)
+	{
+	  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);
+}

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

[Andrew Pinski]

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

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 6687 bytes --]

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.

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>

[-- Attachment #2: sizeof-array-argument.patch --]
[-- Type: text/x-patch, Size: 2510 bytes --]

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);
+  
+  DECL_LANG_FLAG_2 (decl) = parm->declarator->kind == cdk_array;
 
   decl = pushdecl (decl);
 
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 && DECL_LANG_FLAG_2 (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);
+}

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

[Andrew Pinski]

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>

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 7900 bytes --]

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>

[-- Attachment #2: sizeof-array-argument.patch --]
[-- Type: text/x-patch, Size: 3103 bytes --]

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

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

[Joseph S. Myers]

The following apply to all versions of this patch:

* New options need documenting in invoke.texi.

* New options need nonempty help text in c.opt.  (It's unfortunate that 
the -Wsizeof-pointer-memaccess option immediately above got added without 
such help text.)

* Don't use %s with IDENTIFIER_POINTER in diagnostics; instead, use %qE 
with the identifier passed directly to warning_at, as that will handle 
converting extended identifiers appropriately for the user's locale.

* What happens if some declarations of the function use a pointer and some 
use an array?  You should consider this case, decide what the best 
semantics are and ensure the testsuite verifies that they are implemented.

-- 
Joseph S. Myers
joseph@codesourcery.com

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

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 1870 bytes --]

On Fri, May 2, 2014 at 5:50 AM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> The following apply to all versions of this patch:
>
> * New options need documenting in invoke.texi.
Added.
>
> * New options need nonempty help text in c.opt.  (It's unfortunate that
> the -Wsizeof-pointer-memaccess option immediately above got added without
> such help text.)
Added.
>
> * Don't use %s with IDENTIFIER_POINTER in diagnostics; instead, use %qE
> with the identifier passed directly to warning_at, as that will handle
> converting extended identifiers appropriately for the user's locale.
Thanks, modified the patch to use %qE.
>
> * What happens if some declarations of the function use a pointer and some
> use an array?  You should consider this case, decide what the best
> semantics are and ensure the testsuite verifies that they are implemented.
The warning is issued only if the parameter is declared as array in
function definition.
I hope that's reasonable, clang does the same thing.

Example:
int f1(int *p);
int f1(int p[]) { return (int) sizeof (p); } // warns

int f2(int p[])
int f2(int *p) { return (int) sizeof (p); } // does not warn

In this version of the patch, the warning is only enabled by passing
-Wsizeof-array-argument.
Should it be enabled by -Wextra or -Wall ?

Bootstrapped and tested 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.

[gcc/doc]
  * invoke.texi: Document Wsizeof-array-argument.

Thanks and Regards,
Prathamesh

>
> --
> Joseph S. Myers
> joseph@codesourcery.com

[-- Attachment #2: sizeof-array-argument.patch --]
[-- Type: text/x-patch, Size: 4351 bytes --]

Index: gcc/c/c-decl.c
===================================================================
--- gcc/c/c-decl.c	(revision 210004)
+++ gcc/c/c-decl.c	(working copy)
@@ -4650,6 +4650,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 210004)
+++ 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 210004)
+++ gcc/c/c-typeck.c	(working copy)
@@ -2732,6 +2732,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 %qE shall return size of %qT",
+		      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 210004)
+++ gcc/c-family/c.opt	(working copy)
@@ -517,6 +517,10 @@ 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
+Warn when sizeof is applied on a parameter declared as an array
+
 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/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 210004)
+++ gcc/doc/invoke.texi	(working copy)
@@ -265,6 +265,7 @@ Objective-C and Objective-C++ Dialects}.
 -Wreturn-type  -Wsequence-point  -Wshadow @gol
 -Wsign-compare  -Wsign-conversion -Wfloat-conversion @gol
 -Wsizeof-pointer-memaccess @gol
+-Wsizeof-array-argument @gol
 -Wstack-protector -Wstack-usage=@var{len} -Wstrict-aliasing @gol
 -Wstrict-aliasing=n @gol -Wstrict-overflow -Wstrict-overflow=@var{n} @gol
 -Wsuggest-attribute=@r{[}pure@r{|}const@r{|}noreturn@r{|}format@r{]} @gol
@@ -4639,6 +4640,12 @@ but a pointer, and suggests a possible f
 @code{memcpy (&foo, ptr, sizeof (&foo));}.  This warning is enabled by
 @option{-Wall}.
 
+@item -Wsizeof-array-argument
+@opindex Wsizeof-array-argument
+@opindex Wno-sizeof-array-argument
+Warn when sizeof is applied to parameter that is declared as an array
+in function definition.
+
 @item -Waddress
 @opindex Waddress
 @opindex Wno-address
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,31 @@
+/* { 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 f1(int *p)
+{
+  return (int) sizeof (p);
+}
+
+int f2(int *p);
+
+int f2(int p[])
+{
+  return (int) sizeof (p);  /* { dg-warning "sizeof on array parameter" } */
+}
+
+int f3(int x[]);
+
+int f3(int *x)
+{
+  return (int) sizeof (x);
+}

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

[Marek Polacek]

On Sat, May 03, 2014 at 04:51:53PM +0530, Prathamesh Kulkarni wrote:
>   * c-tree.h (C_ARRAY_PARM): New macro, alias for DECL_LANG_FLAG_2.

"New macro." would be enough.

> --- gcc/c/c-decl.c	(revision 210004)
> +++ gcc/c/c-decl.c	(working copy)
> @@ -4650,6 +4650,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);
> +  

Trailing whitespace (you might want to drop this newline altogether).

> --- gcc/c/c-tree.h	(revision 210004)
> +++ 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 */

"as an array.  */"

> --- gcc/c/c-typeck.c	(revision 210004)
> +++ gcc/c/c-typeck.c	(working copy)
> @@ -2732,6 +2732,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)) 

Line too long.  Please move each condition on its own line.

> +	{
> +	  warning_at (loc, 0, "sizeof on array parameter %qE shall return size of %qT",

You want OPT_Wsizeof_array_argument instead of 0 here.
Also I'd s/shall/will/.  Also line is too long + trailing whitespace.

> +Wsizeof-array-argument
> +C Var(warn_sizeof_array_argument) Warning
> +Warn when sizeof is applied on a parameter declared as an array

I wonder if we want also ObjC here.  I guess it'd be good to have this
also for C++.

It seems that this could be enabled by default.

	Marek