| Message ID | 1358832806-30670-1-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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)
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~
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
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~
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
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*.
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
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
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)
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(-)