Re: [Qemu-devel] [PATCH] Fix compilation on GCC 4.5

[Søren Sandmann]

Stefan Weil <address@hidden> writes:

> That's strange.
>
> The lines which cause compiler errors look like this:
>
>     vfio_eoi(DO_UPCAST(VFIODevice, bars[bar->nr], bar));
>
> There are more uses of DO_UPCAST without any compiler error:
>
>     VFIODevice *vdev = DO_UPCAST(VFIODevice, pdev, pdev);
>
> Neither of both lines creates a compiler error with any of my
> compilers (gcc 4.4 up to latest gcc, Linux and MinGW hosts).

Maybe the difference is that bars[bar->nr] is an expression whereas pdev
is just an identifier.

But on closer look, the macro actually looks pretty suspicious to
me. The comment above it that says it does compile time checking, is
probably not true. In the case where 'field' is bars[bar->nr] I'd be
very surprised if the compiler is smart enough to optimize the array
away entirely. It would have to reason like this: The output of
__builtin_offset is always >= 0, so the size of the array can only ever
be negative or zero, which means the array can be deleted because if the
length isn't zero, the program invokes undefined behavior.

It *could* do this, but does it, and is that something qemu should rely
on?

Even if this probably small runtime hit is deemed acceptable, it's
likely surprising to many users of the macro that the 'field' expression
is evaluated twice, so if the "compile time checking" is staying, maybe
it should be done with something like this instead:

diff --git a/osdep.h b/osdep.h
index cb213e0..51ffab0 100644
--- a/osdep.h
+++ b/osdep.h
@@ -42,10 +42,11 @@ typedef signed int              int_fast16_t;
 
 /* Convert from a base type to a parent type, with compile time
 checking.  */
 #ifdef __GNUC__
-#define DO_UPCAST(type, field, dev) ( __extension__ ( { \
-    char __attribute__((unused)) offset_must_be_zero[ \
-        -offsetof(type, field)]; \
-    container_of(dev, type, field);}))
+#define DO_UPCAST(type, field, dev) ( __extension__ ( { \
+    const typeof(((type *) 0)->field) *__mptr = (dev); \
+    ssize_t __offset = - offsetof (type, field); \
+    char __attribute__((unused)) offset_must_be_zero[ __offset ]; \
+    ((type *) ((char *) __mptr + __offset)); } ) )
 #else
 #define DO_UPCAST(type, field, dev) container_of(dev, type, field)
 #endif

This compiles with my GCC, but I haven't tested the resulting binary at
all.

> When I compile with gcc option -save-temps, I get this code
> for the first line:
>
>     vfio_eoi(( __extension__ ( { char __attribute__((unused))
> offset_must_be_zero[ -__builtin_offsetof (VFIODevice, bars[bar->nr])];
> ({ const typeof(((VFIODevice *) 0)->bars[bar->nr]) *__mptr = (bar);
> (VFIODevice *) ((char *) __mptr - __builtin_offsetof (VFIODevice,
> bars[bar->nr]));});})));

I get what appears to be exactly the same:

 vfio_eoi(( __extension__ ( { char __attribute__((unused))
 offset_must_be_zero[ -__builtin_offsetof (VFIODevice, bars[bar->nr])];
 ({ const typeof(((VFIODevice *) 0)->bars[bar->nr]) *__mptr = (bar);
 (VFIODevice *) ((char *) __mptr - __builtin_offsetof (VFIODevice,
 bars[bar->nr]));});})));

> Could you please replace the first line by that code and try your compiler?
> If it remains silent, I suspect a bad offsetof macro.

Replacing the vfio_eoi[...] lines with the expansion that you get
doesn't change anything. It still produces a warning which becomes an
error due to -Werror.

> Do you compile on Linux? Which distribution or which C library do you
> use?

Fedora 14.

dhcp-100-3-184:~% rpm -q glibc
glibc-2.13-2.x86_64
glibc-2.13-2.i686

Here is a small program that reproduces the behavior:

    dhcp-100-3-184:~% cat offset.c 

    struct foo
    {
        int bar[17];
    };
    
    int external (void);
    
    int
    main ()
    {
        struct foo baz = { { 0 } };
    
        __extension__ ( {
            char offset_must_be_zero [ - __builtin_offsetof (struct foo,
                 bar[external()])]  __attribute ((unused));
            });
    
        return baz.bar[8];
    }

    dhcp-100-3-184:~% gcc -Werror -W -Wall -Wunused offset.c 
    cc1: warnings being treated as errors
    offset.c: In function ‘main’:
    offset.c:16:25: error: value computed is not used


Søren