Patchwork Fix circular dependency for HOST_LONG_BITS qemu-common.h <-> bswap.h

login
register
mail settings
Submitter David Gibson
Date Jan. 22, 2013, 5:33 a.m.
Message ID <1358832806-30670-1-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/214341/
State New
Headers show

Comments

David Gibson - Jan. 22, 2013, 5:33 a.m.
Commit c732a52d3e3b7ed42d7daa94ba40a83408cd6f22 from Richard Henderson
changed leul_to_cpu() in bswap.h from a macro to an inline function.  Both
versions use HOST_LONG_BITS, but as an inline, HOST_LONG_BITS now needs to
be evaluated at the point of definition rather than only when the macro is
invoked.

HOST_LONG_BITS is defined in qemu-common.h... which in turn includes
bswap.h leading to a circular dependency.  This doesn't show up on little
endian hosts like x86, because the macros used within leul_to_cpu() end
up removing the reference to HOST_LONG_BITS.  This problem, however, breaks
build on all big endian hosts such as powerpc.

This patch fixes the problem by moving the basic HOST_LONG_BITS definition
to osdep.h, which is already included before bswap.h.

Cc: Richard Henderson <rth@twiddle.net>

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 include/qemu-common.h |    9 ---------
 include/qemu/osdep.h  |   10 ++++++++++
 2 files changed, 10 insertions(+), 9 deletions(-)
David Gibson - Feb. 4, 2013, 11:30 p.m.
Anthony, Richard, anyone?

Please apply - qemu has now been build-broken on all big endian
platforms for a month.

On Tue, Jan 22, 2013 at 04:33:26PM +1100, David Gibson wrote:
> Commit c732a52d3e3b7ed42d7daa94ba40a83408cd6f22 from Richard Henderson
> changed leul_to_cpu() in bswap.h from a macro to an inline function.  Both
> versions use HOST_LONG_BITS, but as an inline, HOST_LONG_BITS now needs to
> be evaluated at the point of definition rather than only when the macro is
> invoked.
> 
> HOST_LONG_BITS is defined in qemu-common.h... which in turn includes
> bswap.h leading to a circular dependency.  This doesn't show up on little
> endian hosts like x86, because the macros used within leul_to_cpu() end
> up removing the reference to HOST_LONG_BITS.  This problem, however, breaks
> build on all big endian hosts such as powerpc.
> 
> This patch fixes the problem by moving the basic HOST_LONG_BITS definition
> to osdep.h, which is already included before bswap.h.
> 
> Cc: Richard Henderson <rth@twiddle.net>
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  include/qemu-common.h |    9 ---------
>  include/qemu/osdep.h  |   10 ++++++++++
>  2 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index ca464bb..ca7f8dc 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -72,15 +72,6 @@
>  #define TIME_MAX LONG_MAX
>  #endif
>  
> -/* HOST_LONG_BITS is the size of a native pointer in bits. */
> -#if UINTPTR_MAX == UINT32_MAX
> -# define HOST_LONG_BITS 32
> -#elif UINTPTR_MAX == UINT64_MAX
> -# define HOST_LONG_BITS 64
> -#else
> -# error Unknown pointer size
> -#endif
> -
>  #ifndef CONFIG_IOVEC
>  #define CONFIG_IOVEC
>  struct iovec {
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 87d3b9c..ebac074 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -3,6 +3,7 @@
>  
>  #include <stdarg.h>
>  #include <stddef.h>
> +#include <stdint.h>
>  #include <stdbool.h>
>  #ifdef __OpenBSD__
>  #include <sys/types.h>
> @@ -18,6 +19,15 @@ typedef unsigned int            uint_fast16_t;
>  typedef signed int              int_fast16_t;
>  #endif
>  
> +/* HOST_LONG_BITS is the size of a native pointer in bits. */
> +#if UINTPTR_MAX == UINT32_MAX
> +# define HOST_LONG_BITS 32
> +#elif UINTPTR_MAX == UINT64_MAX
> +# define HOST_LONG_BITS 64
> +#else
> +# error Unknown pointer size
> +#endif
> +
>  #ifndef glue
>  #define xglue(x, y) x ## y
>  #define glue(x, y) xglue(x, y)
Richard Henderson - Feb. 4, 2013, 11:52 p.m.
On 2013-02-04 15:30, David Gibson wrote:
> Anthony, Richard, anyone?
>
> Please apply - qemu has now been build-broken on all big endian
> platforms for a month.

I know.  See also my bswap.h patch which also fixes the width
of long vs uintptr_t.  No one seems willing to pick these up...


r~
Peter Maydell - Feb. 5, 2013, 12:07 a.m.
On 4 February 2013 23:52, Richard Henderson <rth@twiddle.net> wrote:
> On 2013-02-04 15:30, David Gibson wrote:
>> Anthony, Richard, anyone?
>>
>> Please apply - qemu has now been build-broken on all big endian
>> platforms for a month.
>
>
> I know.  See also my bswap.h patch which also fixes the width
> of long vs uintptr_t.  No one seems willing to pick these up...

In both cases, the patch:
 * was sent out after the soft freeze
 * doesn't have a "for-1.4" tag
 * doesn't have a summary line that clearly says "fixes build
   failure" either
 * hasn't got a Reviewed-by: tag from anybody

so I'm not terribly surprised they haven't got picked up.
You could start by reviewing each others' patches :-)

-- PMM
Richard Henderson - Feb. 5, 2013, 12:11 a.m.
On 2013-02-04 16:07, Peter Maydell wrote:
> On 4 February 2013 23:52, Richard Henderson <rth@twiddle.net> wrote:
>> On 2013-02-04 15:30, David Gibson wrote:
>>> Anthony, Richard, anyone?
>>>
>>> Please apply - qemu has now been build-broken on all big endian
>>> platforms for a month.
>>
>>
>> I know.  See also my bswap.h patch which also fixes the width
>> of long vs uintptr_t.  No one seems willing to pick these up...
>
> In both cases, the patch:
>   * was sent out after the soft freeze

First version was the day before.

>   * doesn't have a "for-1.4" tag

Yes, that one is my fault.

>   * doesn't have a summary line that clearly says "fixes build
>     failure" either
>   * hasn't got a Reviewed-by: tag from anybody

It's gotten Acked-by's though.


r~
Andreas Färber - Feb. 5, 2013, 10:42 a.m.
Am 05.02.2013 01:07, schrieb Peter Maydell:
> On 4 February 2013 23:52, Richard Henderson <rth@twiddle.net> wrote:
>> On 2013-02-04 15:30, David Gibson wrote:
>>> Anthony, Richard, anyone?
>>>
>>> Please apply - qemu has now been build-broken on all big endian
>>> platforms for a month.
>>
>>
>> I know.  See also my bswap.h patch which also fixes the width
>> of long vs uintptr_t.  No one seems willing to pick these up...
> 
> In both cases, the patch:
>  * was sent out after the soft freeze
>  * doesn't have a "for-1.4" tag
>  * doesn't have a summary line that clearly says "fixes build
>    failure" either
>  * hasn't got a Reviewed-by: tag from anybody

I ack'ed it, which in my terminology usually means that I reviewed and
tested it.

> 
> so I'm not terribly surprised they haven't got picked up.
> You could start by reviewing each others' patches :-)

Personally I see no reason to keep around misnamed HOST_LONG_BITS at all
when we can easily calculate its value using sizeof(uintptr_t) * 8 or
replace it by different conditions as suggested by rth. I thus prefer
his patch and have been waiting for Blue to pick it up for 1.4.

Andreas
David Gibson - Feb. 6, 2013, 5:22 a.m.
On Tue, Feb 05, 2013 at 11:42:30AM +0100, Andreas Färber wrote:
> Am 05.02.2013 01:07, schrieb Peter Maydell:
> > On 4 February 2013 23:52, Richard Henderson <rth@twiddle.net> wrote:
> >> On 2013-02-04 15:30, David Gibson wrote:
> >>> Anthony, Richard, anyone?
> >>>
> >>> Please apply - qemu has now been build-broken on all big endian
> >>> platforms for a month.
> >>
> >>
> >> I know.  See also my bswap.h patch which also fixes the width
> >> of long vs uintptr_t.  No one seems willing to pick these up...
> > 
> > In both cases, the patch:
> >  * was sent out after the soft freeze
> >  * doesn't have a "for-1.4" tag
> >  * doesn't have a summary line that clearly says "fixes build
> >    failure" either
> >  * hasn't got a Reviewed-by: tag from anybody
> 
> I ack'ed it, which in my terminology usually means that I reviewed and
> tested it.
> 
> > 
> > so I'm not terribly surprised they haven't got picked up.
> > You could start by reviewing each others' patches :-)
> 
> Personally I see no reason to keep around misnamed HOST_LONG_BITS at all
> when we can easily calculate its value using sizeof(uintptr_t) * 8 or
> replace it by different conditions as suggested by rth. I thus prefer

Ok, I missed rth's patch to do this differently.  Note that sizeof()
will not work in this case, because we need the correct value at cpp
time.

> his patch and have been waiting for Blue to pick it up for 1.4.

Sure, whatever.  Can we please just get whichever damn fix *in*.
Anthony Liguori - Feb. 13, 2013, 5:41 p.m.
David Gibson <dwg@au1.ibm.com> writes:

> On Tue, Feb 05, 2013 at 11:42:30AM +0100, Andreas Färber wrote:
>> Am 05.02.2013 01:07, schrieb Peter Maydell:
>> > On 4 February 2013 23:52, Richard Henderson <rth@twiddle.net> wrote:
>> >> On 2013-02-04 15:30, David Gibson wrote:
>> >>> Anthony, Richard, anyone?
>> >>>
>> >>> Please apply - qemu has now been build-broken on all big endian
>> >>> platforms for a month.
>> >>
>> >>
>> >> I know.  See also my bswap.h patch which also fixes the width
>> >> of long vs uintptr_t.  No one seems willing to pick these up...
>> > 
>> > In both cases, the patch:
>> >  * was sent out after the soft freeze
>> >  * doesn't have a "for-1.4" tag
>> >  * doesn't have a summary line that clearly says "fixes build
>> >    failure" either
>> >  * hasn't got a Reviewed-by: tag from anybody
>> 
>> I ack'ed it, which in my terminology usually means that I reviewed and
>> tested it.
>> 
>> > 
>> > so I'm not terribly surprised they haven't got picked up.
>> > You could start by reviewing each others' patches :-)
>> 
>> Personally I see no reason to keep around misnamed HOST_LONG_BITS at all
>> when we can easily calculate its value using sizeof(uintptr_t) * 8 or
>> replace it by different conditions as suggested by rth. I thus prefer
>
> Ok, I missed rth's patch to do this differently.  Note that sizeof()
> will not work in this case, because we need the correct value at cpp
> time.
>
>> his patch and have been waiting for Blue to pick it up for 1.4.
>
> Sure, whatever.  Can we please just get whichever damn fix *in*.

For the sake of completeness, "rth's patch" means:

commit 91107fdf4443d2171e06840e87277bb7a047343b
Author: Richard Henderson <rth@twiddle.net>
Date:   Mon Feb 4 16:21:06 2013 -0800

    bswap: Fix width of swap in leul_to_cpu

Correct?

If so, this was committed before you sent this note.  Can someone
confirm if we still have a problem on big endian hosts?

Regards,

Anthony Liguori

>
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson
Andreas Färber - Feb. 13, 2013, 5:48 p.m.
Am 13.02.2013 18:41, schrieb Anthony Liguori:
> David Gibson <dwg@au1.ibm.com> writes:
> 
>> On Tue, Feb 05, 2013 at 11:42:30AM +0100, Andreas Färber wrote:
>>> Am 05.02.2013 01:07, schrieb Peter Maydell:
>>>> On 4 February 2013 23:52, Richard Henderson <rth@twiddle.net> wrote:
>>>>> On 2013-02-04 15:30, David Gibson wrote:
>>>>>> Anthony, Richard, anyone?
>>>>>>
>>>>>> Please apply - qemu has now been build-broken on all big endian
>>>>>> platforms for a month.
>>>>>
>>>>>
>>>>> I know.  See also my bswap.h patch which also fixes the width
>>>>> of long vs uintptr_t.  No one seems willing to pick these up...
>>>>
>>>> In both cases, the patch:
>>>>  * was sent out after the soft freeze
>>>>  * doesn't have a "for-1.4" tag
>>>>  * doesn't have a summary line that clearly says "fixes build
>>>>    failure" either
>>>>  * hasn't got a Reviewed-by: tag from anybody
>>>
>>> I ack'ed it, which in my terminology usually means that I reviewed and
>>> tested it.
>>>
>>>>
>>>> so I'm not terribly surprised they haven't got picked up.
>>>> You could start by reviewing each others' patches :-)
>>>
>>> Personally I see no reason to keep around misnamed HOST_LONG_BITS at all
>>> when we can easily calculate its value using sizeof(uintptr_t) * 8 or
>>> replace it by different conditions as suggested by rth. I thus prefer
>>
>> Ok, I missed rth's patch to do this differently.  Note that sizeof()
>> will not work in this case, because we need the correct value at cpp
>> time.
>>
>>> his patch and have been waiting for Blue to pick it up for 1.4.
>>
>> Sure, whatever.  Can we please just get whichever damn fix *in*.
> 
> For the sake of completeness, "rth's patch" means:
> 
> commit 91107fdf4443d2171e06840e87277bb7a047343b
> Author: Richard Henderson <rth@twiddle.net>
> Date:   Mon Feb 4 16:21:06 2013 -0800
> 
>     bswap: Fix width of swap in leul_to_cpu
> 
> Correct?

Yes.

> If so, this was committed before you sent this note.  Can someone
> confirm if we still have a problem on big endian hosts?

Around Central European lunch time today things seemed to compile fine,
and `make check` worked, too. (Thanks!)

Regards,
Andreas

Patch

diff --git a/include/qemu-common.h b/include/qemu-common.h
index ca464bb..ca7f8dc 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -72,15 +72,6 @@ 
 #define TIME_MAX LONG_MAX
 #endif
 
-/* HOST_LONG_BITS is the size of a native pointer in bits. */
-#if UINTPTR_MAX == UINT32_MAX
-# define HOST_LONG_BITS 32
-#elif UINTPTR_MAX == UINT64_MAX
-# define HOST_LONG_BITS 64
-#else
-# error Unknown pointer size
-#endif
-
 #ifndef CONFIG_IOVEC
 #define CONFIG_IOVEC
 struct iovec {
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 87d3b9c..ebac074 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -3,6 +3,7 @@ 
 
 #include <stdarg.h>
 #include <stddef.h>
+#include <stdint.h>
 #include <stdbool.h>
 #ifdef __OpenBSD__
 #include <sys/types.h>
@@ -18,6 +19,15 @@  typedef unsigned int            uint_fast16_t;
 typedef signed int              int_fast16_t;
 #endif
 
+/* HOST_LONG_BITS is the size of a native pointer in bits. */
+#if UINTPTR_MAX == UINT32_MAX
+# define HOST_LONG_BITS 32
+#elif UINTPTR_MAX == UINT64_MAX
+# define HOST_LONG_BITS 64
+#else
+# error Unknown pointer size
+#endif
+
 #ifndef glue
 #define xglue(x, y) x ## y
 #define glue(x, y) xglue(x, y)