Patchwork bswap: Don't rely on HOST_LONG_BITS

login
register
mail settings
Submitter Richard Henderson
Date Jan. 30, 2013, 8:55 p.m.
Message ID <1359579300-27862-1-git-send-email-rth@twiddle.net>
Download mbox | patch
Permalink /patch/216958/
State New
Headers show

Comments

Richard Henderson - Jan. 30, 2013, 8:55 p.m.
This is not always defined in all places qemu/bswap.h is used.
If we include qemu-common.h to get it, we create an include loop.
This resolves a build problem on any big-endian host like ppc64.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 include/qemu/bswap.h | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
Stefan Weil - Jan. 30, 2013, 9:04 p.m.
Am 30.01.2013 21:55, schrieb Richard Henderson:
> This is not always defined in all places qemu/bswap.h is used.
> If we include qemu-common.h to get it, we create an include loop.
> This resolves a build problem on any big-endian host like ppc64.
>
> Signed-off-by: Richard Henderson<rth@twiddle.net>
> ---
>   include/qemu/bswap.h | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index e6d4798..d3af35d 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -2,8 +2,8 @@
>   #define BSWAP_H
>
>   #include "config-host.h"
> -
>   #include<inttypes.h>
> +#include<limits.h>
>   #include "fpu/softfloat.h"
>
>   #ifdef CONFIG_MACHINE_BSWAP_H
> @@ -458,7 +458,15 @@ static inline void cpu_to_32wu(uint32_t *p, uint32_t v)
>
>   static inline unsigned long leul_to_cpu(unsigned long v)
>   {
> -    return le_bswap(v, HOST_LONG_BITS);
> +    /* In order to break an include loop between here and
> +       qemu-common.h, don't rely on HOST_LONG_BITS.  */
> +#if ULONG_MAX == UINT32_MAX
> +    return le_bswap(v, 32);
> +#elif ULONG_MAX == UINT64_MAX
> +    return le_bswap(v, 64);
> +#else
> +# error Unknown sizeof long
> +#endif
>   }
>
>   #undef le_bswap

That would be wrong for 64 bit MinGW-w64 because
HOST_LONG_BITS is _not_ the bit size of a long value.

See qemu-common.h for the correct definition.

Regards
Stefan W.
Richard Henderson - Jan. 30, 2013, 9:59 p.m.
On 01/30/2013 01:04 PM, Stefan Weil wrote:
>>   static inline unsigned long leul_to_cpu(unsigned long v)
>>   {
>> -    return le_bswap(v, HOST_LONG_BITS);
>> +    /* In order to break an include loop between here and
>> +       qemu-common.h, don't rely on HOST_LONG_BITS.  */
>> +#if ULONG_MAX == UINT32_MAX
>> +    return le_bswap(v, 32);
>> +#elif ULONG_MAX == UINT64_MAX
>> +    return le_bswap(v, 64);
>> +#else
>> +# error Unknown sizeof long
>> +#endif
>>   }
>>
>>   #undef le_bswap
>
> That would be wrong for 64 bit MinGW-w64 because
> HOST_LONG_BITS is _not_ the bit size of a long value.
>
> See qemu-common.h for the correct definition.

I beg your pardon, but it *is* right.  HOST_LONG_BITS is set to the size 
of a host pointer, not a host long.  Which suggests that my patch is the 
*only* correct way to do this, at least for now.


r~
Andreas Färber - Jan. 30, 2013, 10:03 p.m.
Am 30.01.2013 22:59, schrieb Richard Henderson:
> On 01/30/2013 01:04 PM, Stefan Weil wrote:
>>>   static inline unsigned long leul_to_cpu(unsigned long v)
>>>   {
>>> -    return le_bswap(v, HOST_LONG_BITS);
>>> +    /* In order to break an include loop between here and
>>> +       qemu-common.h, don't rely on HOST_LONG_BITS.  */
>>> +#if ULONG_MAX == UINT32_MAX
>>> +    return le_bswap(v, 32);
>>> +#elif ULONG_MAX == UINT64_MAX
>>> +    return le_bswap(v, 64);
>>> +#else
>>> +# error Unknown sizeof long
>>> +#endif
>>>   }
>>>
>>>   #undef le_bswap
>>
>> That would be wrong for 64 bit MinGW-w64 because
>> HOST_LONG_BITS is _not_ the bit size of a long value.
>>
>> See qemu-common.h for the correct definition.
> 
> I beg your pardon, but it *is* right.  HOST_LONG_BITS is set to the size
> of a host pointer, not a host long.  Which suggests that my patch is the
> *only* correct way to do this, at least for now.

IIUC on w64 ULONG_MAX == UINT32_MAX but 64 is desired for pointer.
w64 is not C99-compliant.

Andreas
Andreas Färber - Jan. 30, 2013, 10:11 p.m.
Am 30.01.2013 23:03, schrieb Andreas Färber:
> Am 30.01.2013 22:59, schrieb Richard Henderson:
>> On 01/30/2013 01:04 PM, Stefan Weil wrote:
>>>>   static inline unsigned long leul_to_cpu(unsigned long v)
>>>>   {
>>>> -    return le_bswap(v, HOST_LONG_BITS);
>>>> +    /* In order to break an include loop between here and
>>>> +       qemu-common.h, don't rely on HOST_LONG_BITS.  */
>>>> +#if ULONG_MAX == UINT32_MAX
>>>> +    return le_bswap(v, 32);
>>>> +#elif ULONG_MAX == UINT64_MAX
>>>> +    return le_bswap(v, 64);
>>>> +#else
>>>> +# error Unknown sizeof long
>>>> +#endif
>>>>   }
>>>>
>>>>   #undef le_bswap
>>>
>>> That would be wrong for 64 bit MinGW-w64 because
>>> HOST_LONG_BITS is _not_ the bit size of a long value.
>>>
>>> See qemu-common.h for the correct definition.
>>
>> I beg your pardon, but it *is* right.  HOST_LONG_BITS is set to the size
>> of a host pointer, not a host long.  Which suggests that my patch is the
>> *only* correct way to do this, at least for now.
> 
> IIUC on w64 ULONG_MAX == UINT32_MAX but 64 is desired for pointer.
> w64 is not C99-compliant.

Oh I see now: function is named ul and operates on unsigned long. ;)
So your patch carries an implicit bugfix that should be mentioned in the
commit message.

Andreas

Patch

diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
index e6d4798..d3af35d 100644
--- a/include/qemu/bswap.h
+++ b/include/qemu/bswap.h
@@ -2,8 +2,8 @@ 
 #define BSWAP_H
 
 #include "config-host.h"
-
 #include <inttypes.h>
+#include <limits.h>
 #include "fpu/softfloat.h"
 
 #ifdef CONFIG_MACHINE_BSWAP_H
@@ -458,7 +458,15 @@  static inline void cpu_to_32wu(uint32_t *p, uint32_t v)
 
 static inline unsigned long leul_to_cpu(unsigned long v)
 {
-    return le_bswap(v, HOST_LONG_BITS);
+    /* In order to break an include loop between here and
+       qemu-common.h, don't rely on HOST_LONG_BITS.  */
+#if ULONG_MAX == UINT32_MAX
+    return le_bswap(v, 32);
+#elif ULONG_MAX == UINT64_MAX
+    return le_bswap(v, 64);
+#else
+# error Unknown sizeof long
+#endif
 }
 
 #undef le_bswap