On Sun, Apr 27, 2014 at 11:22 PM, <pins...@gmail.com> wrote:
>
>
>> On Apr 27, 2014, at 10:09 AM, Prathamesh Kulkarni
>> <bilbotheelffri...@gmail.com> wrote:
>>
>>> 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.
>
> 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>
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);
+}
