sparse and anonymous unions

sparse and anonymous unions

[Hans Verkuil]

Hi!

I'm trying to cut down the list of sparse warnings and errors I get when
compiling drivers/media. Most of them are obviously our problem, but there
is one that seems to be a sparse bug:

drivers/media/v4l2-core/v4l2-dv-timings.c:30:9: error: unknown field name in initializer

This uses the v4l2_dv_timings type which is defined here:

include/uapi/linux/videodev2.h

and which has an anonymous union:

struct v4l2_dv_timings {
        __u32 type;
        union {
                struct v4l2_bt_timings  bt;
                __u32   reserved[32];
        };
} __attribute__ ((packed));

The macro used in the source above comes from this header:

include/uapi/linux/v4l2-dv-timings.h

and is defined as follows:

#if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))
/* Sadly gcc versions older than 4.6 have a bug in how they initialize
   anonymous unions where they require additional curly brackets.
   This violates the C1x standard. This workaround adds the curly brackets
   if needed. */
#define V4L2_INIT_BT_TIMINGS(_width, args...) \
        { .bt = { _width , ## args } }
#else
#define V4L2_INIT_BT_TIMINGS(_width, args...) \
        .bt = { _width , ## args }
#endif

/* CEA-861-E timings (i.e. standard HDTV timings) */
        
#define V4L2_DV_BT_CEA_640X480P59_94 { \
        .type = V4L2_DV_BT_656_1120, \
        V4L2_INIT_BT_TIMINGS(640, 480, 0, 0, \
                25175000, 16, 96, 48, 10, 2, 33, 0, 0, 0, \
                V4L2_DV_BT_STD_DMT | V4L2_DV_BT_STD_CEA861, 0) \
}

The problem is that it seems that sparse follows the old pre-4.6 rules for
initializing anonymous unions instead of what is actually in the C standard.

If I add ' || defined(__CHECKER__)' to the #if above it will pass without
generating sparse errors.

Is this something that can be fixed in sparse, or am I forced to add the
'defined(__CHECKER__)' to the #if condition?

Regards,

	Hans

Re: sparse and anonymous unions

[Hans Verkuil]

On 03/15/2014 11:26 AM, Hans Verkuil wrote:
> Hi!
> 
> I'm trying to cut down the list of sparse warnings and errors I get when
> compiling drivers/media. Most of them are obviously our problem, but there
> is one that seems to be a sparse bug:
> 
> drivers/media/v4l2-core/v4l2-dv-timings.c:30:9: error: unknown field name in initializer
> 
> This uses the v4l2_dv_timings type which is defined here:
> 
> include/uapi/linux/videodev2.h
> 
> and which has an anonymous union:
> 
> struct v4l2_dv_timings {
>         __u32 type;
>         union {
>                 struct v4l2_bt_timings  bt;
>                 __u32   reserved[32];
>         };
> } __attribute__ ((packed));
> 
> The macro used in the source above comes from this header:
> 
> include/uapi/linux/v4l2-dv-timings.h
> 
> and is defined as follows:
> 
> #if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))
> /* Sadly gcc versions older than 4.6 have a bug in how they initialize
>    anonymous unions where they require additional curly brackets.
>    This violates the C1x standard. This workaround adds the curly brackets
>    if needed. */
> #define V4L2_INIT_BT_TIMINGS(_width, args...) \
>         { .bt = { _width , ## args } }
> #else
> #define V4L2_INIT_BT_TIMINGS(_width, args...) \
>         .bt = { _width , ## args }
> #endif
> 
> /* CEA-861-E timings (i.e. standard HDTV timings) */
>         
> #define V4L2_DV_BT_CEA_640X480P59_94 { \
>         .type = V4L2_DV_BT_656_1120, \
>         V4L2_INIT_BT_TIMINGS(640, 480, 0, 0, \
>                 25175000, 16, 96, 48, 10, 2, 33, 0, 0, 0, \
>                 V4L2_DV_BT_STD_DMT | V4L2_DV_BT_STD_CEA861, 0) \
> }
> 
> The problem is that it seems that sparse follows the old pre-4.6 rules for
> initializing anonymous unions instead of what is actually in the C standard.
> 
> If I add ' || defined(__CHECKER__)' to the #if above it will pass without
> generating sparse errors.
> 
> Is this something that can be fixed in sparse, or am I forced to add the
> 'defined(__CHECKER__)' to the #if condition?

Here is a simple test case for this problem:

====== anon-union.c ======
struct s {
        union {
                int val;
        };
};

static struct s foo = { .val = 5, };
/*
 * check-name: duplicate extern array
 *
 * check-error-start
 * check-error-end
 */
====== anon-union.c ======

Running sparse gives:

anon-union.c:7:26: error: unknown field name in initializer

Regards,

	Hans

Re: sparse and anonymous unions

[Hans Verkuil]

On 03/31/2014 09:51 AM, Hans Verkuil wrote:
> On 03/15/2014 11:26 AM, Hans Verkuil wrote:
>> Hi!
>>
>> I'm trying to cut down the list of sparse warnings and errors I get when
>> compiling drivers/media. Most of them are obviously our problem, but there
>> is one that seems to be a sparse bug:
>>
>> drivers/media/v4l2-core/v4l2-dv-timings.c:30:9: error: unknown field name in initializer
>>
>> This uses the v4l2_dv_timings type which is defined here:
>>
>> include/uapi/linux/videodev2.h
>>
>> and which has an anonymous union:
>>
>> struct v4l2_dv_timings {
>>         __u32 type;
>>         union {
>>                 struct v4l2_bt_timings  bt;
>>                 __u32   reserved[32];
>>         };
>> } __attribute__ ((packed));
>>
>> The macro used in the source above comes from this header:
>>
>> include/uapi/linux/v4l2-dv-timings.h
>>
>> and is defined as follows:
>>
>> #if __GNUC__ < 4 || (__GNUC__ == 4 && (__GNUC_MINOR__ < 6))
>> /* Sadly gcc versions older than 4.6 have a bug in how they initialize
>>    anonymous unions where they require additional curly brackets.
>>    This violates the C1x standard. This workaround adds the curly brackets
>>    if needed. */
>> #define V4L2_INIT_BT_TIMINGS(_width, args...) \
>>         { .bt = { _width , ## args } }
>> #else
>> #define V4L2_INIT_BT_TIMINGS(_width, args...) \
>>         .bt = { _width , ## args }
>> #endif
>>
>> /* CEA-861-E timings (i.e. standard HDTV timings) */
>>         
>> #define V4L2_DV_BT_CEA_640X480P59_94 { \
>>         .type = V4L2_DV_BT_656_1120, \
>>         V4L2_INIT_BT_TIMINGS(640, 480, 0, 0, \
>>                 25175000, 16, 96, 48, 10, 2, 33, 0, 0, 0, \
>>                 V4L2_DV_BT_STD_DMT | V4L2_DV_BT_STD_CEA861, 0) \
>> }
>>
>> The problem is that it seems that sparse follows the old pre-4.6 rules for
>> initializing anonymous unions instead of what is actually in the C standard.
>>
>> If I add ' || defined(__CHECKER__)' to the #if above it will pass without
>> generating sparse errors.
>>
>> Is this something that can be fixed in sparse, or am I forced to add the
>> 'defined(__CHECKER__)' to the #if condition?
> 
> Here is a simple test case for this problem:
> 
> ====== anon-union.c ======
> struct s {
>         union {
>                 int val;
>         };
> };
> 
> static struct s foo = { .val = 5, };
> /*
>  * check-name: duplicate extern array

Oops: that should be:

check-name: test anonymous union initializer

>  *
>  * check-error-start
>  * check-error-end
>  */
> ====== anon-union.c ======
> 
> Running sparse gives:
> 
> anon-union.c:7:26: error: unknown field name in initializer
> 
> Regards,
> 
> 	Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: sparse and anonymous unions

[Linus Torvalds]

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

On Mon, Mar 31, 2014 at 12:51 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> Here is a simple test case for this problem:
>
> ====== anon-union.c ======
> struct s {
>         union {
>                 int val;
>         };
> };
>
> static struct s foo = { .val = 5, };

Ok, this fixes the warning, but we seem to still mess up the actual
initializer. It looks like some later phase gets the offset wrong, so
when we lay things out in memory, we'll put things at offset zero
(which is right for your test-case, but not if there was something
before that anonymous union).

Regardless, that only matters for real code generation, not for using
sparse as a semantic checker, so this patch is correct and is an
improvement.

Chris, mind applying this one too? It removes more lines than it adds
while fixing things, by removing the helper function that isn't good
at anoymous unions, and using another one that does this all right..

                  Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 1169 bytes --]

 evaluate.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/evaluate.c b/evaluate.c
index 8a53b3e884e0..5adfc1e3ff26 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -2171,17 +2171,6 @@ static int evaluate_arguments(struct symbol *f, struct symbol *fn, struct expres
 	return 1;
 }
 
-static struct symbol *find_struct_ident(struct symbol *ctype, struct ident *ident)
-{
-	struct symbol *sym;
-
-	FOR_EACH_PTR(ctype->symbol_list, sym) {
-		if (sym->ident == ident)
-			return sym;
-	} END_FOR_EACH_PTR(sym);
-	return NULL;
-}
-
 static void convert_index(struct expression *e)
 {
 	struct expression *child = e->idx_expression;
@@ -2290,11 +2279,12 @@ static struct expression *check_designators(struct expression *e,
 			}
 			e = e->idx_expression;
 		} else if (e->type == EXPR_IDENTIFIER) {
+			int offset = 0;
 			if (ctype->type != SYM_STRUCT && ctype->type != SYM_UNION) {
 				err = "field name not in struct or union";
 				break;
 			}
-			ctype = find_struct_ident(ctype, e->expr_ident);
+			ctype = find_identifier(e->expr_ident, ctype->symbol_list, &offset);
 			if (!ctype) {
 				err = "unknown field name in";
 				break;

Re: sparse and anonymous unions

[Christopher Li]

On Mon, Mar 31, 2014 at 10:17 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Chris, mind applying this one too? It removes more lines than it adds
> while fixing things, by removing the helper function that isn't good
> at anoymous unions, and using another one that does this all right..

The patch is applied. I add the signed off for you too, hope you don't mind.

Chris