On Sun, Apr 27, 2014 at 8:48 PM, Trevor Saunders <tsaund...@mozilla.com> wrote: > On Sun, Apr 27, 2014 at 06:21:20PM +0530, Prathamesh Kulkarni wrote: >> On Sun, Apr 27, 2014 at 5:31 PM, Trevor Saunders <tsaund...@mozilla.com> >> wrote: >> > On Sun, Apr 27, 2014 at 02:31:46AM +0530, Prathamesh Kulkarni wrote: >> >> Hi, >> >> Shall it a good idea to add new warning -Wsizeof-array-argument that >> >> warns when sizeof is applied on parameter declared as an array ? >> > >> > Seems reasonable enough. >> > >> >> Similar to clang's -Wsizeof-array-argument: >> >> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110613/042812.html >> >> This was also reported as PR6940: >> >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=6940 >> >> >> >> I have attached a patch that adds the warning to C front-end. >> > >> > if we're doing this for C, we should probably do it for C++ too. >> > >> >> I implemented it by adding a new member BOOL_BITFIELD is_array_parm to >> >> tree_parm_decl. Not sure if that's a good approach. >> > >> > I'm about the last one who should comment on this, but it seems pretty >> > crazy you can't use data that's already stored. >> AFAIU, the information about declarator is stored in c_declarator. >> c_declarator->kind == cdk_array holds true >> if the declarator is an array. However in push_parm_decl, call to >> grokdeclarator returns decl of pointer_type, corresponding to array >> declarator if the array is parameter (TREE_TYPE (decl) is >> pointer_type). So I guess we lose that information there. > > I guess that sort of makes sense, so I'll shut up ;) > >> >> Index: gcc/tree-core.h >> >> =================================================================== >> >> --- gcc/tree-core.h (revision 209800) >> >> +++ gcc/tree-core.h (working copy) >> >> @@ -1411,6 +1411,7 @@ struct GTY(()) tree_const_decl { >> >> struct GTY(()) tree_parm_decl { >> >> struct tree_decl_with_rtl common; >> >> rtx incoming_rtl; >> >> + BOOL_BITFIELD is_array_parm; >> > >> > BOOL_BITFIELD only makes sense if you declare it as an actually bitfield >> > with size less than that of unisgned int, otherwise you might as well >> > use that directly. On the other hand I wonder if we can't just nuke >> > BOOL_BITFIELD, it seems to be legacy from a time of C and bool not being >> > a builtin type? >> Thanks, I wasn't aware of that. Could we now use C++ bool type instead ? > > you can certainly do |bool x;| as struct fields, that's already all > over. However its not entirely clear to me if |bool x : 1;| will work > everywhere and take the single bit you'd expect, istr there being > compilers that do stupid things if you use multiple types next to each > other in bitfields, but I'm not sure if we need to care about any of > those. Changed to bool is_array_parm; and from macros to inline functions.
[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 >> >
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); +}