Re: [Qemu-devel] Re: Porting QEMU to new hosts with unusual ABI (sizeof(long) != sizeof(void *))

[Markus Armbruster]

Stefan Weil <address@hidden> writes:

> Am 07.02.2011 08:23, schrieb Markus Armbruster:
>> Stefan Weil <address@hidden> writes:
>>> Am 05.02.2011 16:35, schrieb Blue Swirl:
>> [...]
>>>> The patch changes also signed longs to uintptr_t. That could introduce
>>>> regressions, so please use signed/unsigned as original.
>>>
>>> I changed the code manually, and there was only one location where
>>> signed/unsigned made a difference. That single case was an int
>>> parameter passed in a void pointer, and I used intptr_t there.
>>>
>>> I had the impression that in the current code (long) was
>>> sometimes used because it is shorter than (unsigned long) :-)
>>>
>>> As long as changes are made manually with the necessary care,
>>> I'd recommend using uintptr_t where possible.
>>
>> I'd recommend not to mix up the intptr portability clean up with the
>> signedness cleanup. Much easier to review separately. Moreover,
>> cleaning up signedness changes generated code, while cleaning up the
>> types shouldn't (except on hosts where the code doesn't work).
>> Testable, just don't forget to strip the debug info.
>>
>> [...]
>
>
>
> Markus, your recommendation is ok for modifications which change the
> generated code or which need more context for the review.
>
> I don't think that you will have any problem with reviewing
> "signedness changes" like these ones:
>
> -#define saddr(x) (uint8_t *)(long)(x)
> -#define laddr(x) (uint8_t *)(long)(x)
> +#define saddr(x) (uint8_t *)(uintptr_t)(x)
> +#define laddr(x) (uint8_t *)(uintptr_t)(x)
>
> Neither of these changes changes the binary code for the commonly used
> hosts,
> and the patch does not need further context for the review.
>
> I intend to split my patch in three parts:
>
> * one for tcg_gen_exit_tb calls which will be modified as Blue Swirl
> has suggested
> * one for the parameter passing of a signed value via pointer
> * one for the rest which contains only a handful of trivial
> "signedness changes",
>   all following the same pattern like the example given above
>
> Is that ok?

Let's see the patches :)