Re: [Qemu-devel] [PATCH] fix gcc4 compile warnings

[Andre Przywara]

andrzej zaborowski wrote:
> On 30/11/2007, Andre Przywara <address@hidden> wrote:
> >  > These casts are not the right way to get rid of the warnings, as are
> >  > some of the casts in other files in qemu_put_* and qemu_get_*
> >  > arguments. In this case the warnings are true positives and the bugs
> >  > causing the warnings have to be addressed instead of just the
> >  > warnings.
> >
> > Are you sure of that? Most of the fixes are like this:
> >  >> -    qemu_put_be32s(f, &s->count_shift);
> >  >> +    qemu_put_be32s(f, (uint32_t *)&s->count_shift);
> > qemu_put_be32s is (QEMUFile *f, const uint32_t *pv), but after all the
> > 2nd argument is only a _pointer_ to an unsigned variable, the size is
> > the same (thanks to the C99 explicit types).
>
> count_shift is an int so it is the machine word size, not necesarily
> 32-bit AFAIK. Otherwise I guess gcc wouldn't warn. You can possibly
> use:
>
> qemu_put_be32(f, s->count_shift);
>
> Or point qemu_put_be32_s() to a int32_t/uint32_t variable.
Oh, you are right, I missed that. Will rework the patch.

> > What solution do you prefer for the opaque types? I have used the simple:
> >  >> -    void *args[MAX_ARGS];
> >  >> +    intptr_t args[MAX_ARGS];
> > A more portable and clean solution would be this:
> > -    void *args[MAX_ARGS];
> > +    union
> > +    {
> > +        void* ptr;
> > +        int i;
> > +    } args[MAX_ARGS];
> > If you prefer this, I can change the patch accordingly.
>
> I'm not sure why you get a warning here and I'm unable to run a build
> at the moment. A void * should be able to store some (unknown size)
> integer regardless of the platform.

sizeof(void*) is 8, whereas sizeof(int) is 4 on a 64bit platform. If I 
assign a 32bit value to a 64bit (pointer) variable, GCC4 says:
warning: cast to pointer from integer of different size

You are right that a pointer _should_ be able to hold an integer, but 
AFAIK this is not guaranteed (aka written in the standard). To avoid 
those problems intptr_t was introduced.
But I think opaque types in C are broken anyway, so the union version 
makes it at least more readable, since you explicitly say integer or 
pointer at the assignment and usage. But I have no problem with using 
intptr_t, the union was just a suggestion.

What about fixing monitor.c#monitor_handle_command in general and avoid 
the opaque types at all?

But wouldn't the union version work well even on win64?


Regards,
Andre.

--
Andre Przywara
AMD-OSRC (Dresden)
Tel: x84917