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

["Andre Przywara" <andre.przywara@amd.com>]

andrzej zaborowski wrote:
> On 30/11/2007, Andre Przywara <andre.przywara@amd.com> 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