| Message ID | EE3117B0-EED9-47CE-BF19-77988DEE8830@web.de |
|---|---|
| State | New |
| Headers | show |
On 28.08.2011, at 07:09, Andreas Färber wrote: > Hello, > > The unresolved softfloat uint* conversion business bites us again: This time, the previously working Cocoa frontend stopped compiling: > > In file included from /Users/andreas/QEMU/qemu/bswap.h:14, > from /Users/andreas/QEMU/qemu/qemu-common.h:103, > from /Users/andreas/QEMU/qemu/ui/cocoa.m:28: > /Users/andreas/QEMU/qemu/fpu/softfloat.h:60: error: conflicting types for ‘uint16’ > /System/Library/Frameworks/Security.framework/Headers/cssmconfig.h:68: error: previous declaration of ‘uint16’ was here > make: *** [ui/cocoa.o] Error 1 > > Since commit cbbab9226da9572346837466a8770c117e7e65a2 (move unaligned memory access functions to bswap.h) softfloat.h is being #included in bswap.h, which gets pulled into cocoa.m through qemu-common.h. I thought Alex had set up a Darwin buildbot to catch such breakages... No, that was only an idea so far. I don't have a spare Mac I could use for that atm :(. > Any thoughts on how to proceed? My previous approach for Haiku, to replace non-standard uint16 with POSIX uint_fast16_t etc., was rejected to avoid system-dependent widths. I'd really like to get rid of the annoyingly conflicting naming though (int vs. long for 32, int vs. short for 16, ...). I'm not sure what you mean by system-dependent widths? This is only a naming collision issue, right? Can't we just name the types something more qemu specific? Alex
On 28 August 2011 14:02, Alexander Graf <agraf@suse.de> wrote: > On 28.08.2011, at 07:09, Andreas Färber wrote: >> Any thoughts on how to proceed? My previous approach for Haiku, >> to replace non-standard uint16 with POSIX uint_fast16_t etc., >> was rejected to avoid system-dependent widths. I'd really like >> to get rid of the annoyingly conflicting naming though (int vs. >> long for 32, int vs. short for 16, ...). > > I'm not sure what you mean by system-dependent widths? This is > only a naming collision issue, right? Can't we just name the types > something more qemu specific? If I recall the previous discussions correctly: At the moment softfloat has its own typedefs (int16 etc) which it uses to mean "an int which must be at least 16 bits wide but may be larger"; the POSIX names for these (as Andreas says) are int_fast16_t &c. (We've already converted softfloat's custom typedefs for "int which must be exactly 16 bits" &c to use the posix int16_t &c, so those are not a problem any more.) There are two ways we can fix the naming clash here: (1) change int16 -> int_fast16_t. This was argued against because it means that the type might have a different width depending on the host system, which means that where softfloat buggily makes assumptions about the typewidth this would surface only on the minority of systems where (eg) int_fast16_t wasn't 32 bits. (In theory this is just preserving current behaviour but in fact the set of typedefs in softfloat.h doesn't take advantage of the "may be bigger" leeway; for example we 'typedef int8_t int8' even though the comment above specifically suggests that int8 should be typedef'd as an int.) (2) change int16 -> int16_t. This was argued against because it would be dropping the information in the current softfloat code about where we require a 16 bit type for correctness and where we are happy for the type to be larger if it's more efficient. Unfortunately we didn't manage to come to a consensus on one of these two approaches, which is why we haven't renamed the offending types yet... [Personally I prefer (2).] -- PMM
Am 28.08.2011 um 15:02 schrieb Alexander Graf: > On 28.08.2011, at 07:09, Andreas Färber wrote: > >> Any thoughts on how to proceed? My previous approach for Haiku, to >> replace non-standard uint16 with POSIX uint_fast16_t etc., was >> rejected to avoid system-dependent widths. I'd really like to get >> rid of the annoyingly conflicting naming though (int vs. long for >> 32, int vs. short for 16, ...). > > I'm not sure what you mean by system-dependent widths? The core issue is this: uint16 in fpu/ does not mean uint16_t but "at least 16 bits wide" (int). Apart from the BeOS/Haiku i386 ABI being strange in using long for uint32, most uses of uint16 elsewhere mean "exactly 16 bits wide", leading to type conflicts. POSIX has two types for such least-width semantics: uint_least16_t and uint_fast16_t. We ran into the issue that a patch compiled on Darwin/ ppc64 but broke on Linux/x86_64 or i386, so Aurelien considered it too risky to use types whose actual width depends on the host system within my series. > This is only a naming collision issue, right? Can't we just name the > types something more qemu specific? We could, sure. I you agree on a replacement type, I'll happily supply patches to do the conversion. Part of the problem was that, e.g., uint16 and int are being used interchangably in declaration and implementation. I'll look into introducing matching uint16 etc. where necessary, so that the actual conversion can be done by regex. Andreas
On 28.08.2011, at 08:34, Andreas Färber wrote: > Am 28.08.2011 um 15:02 schrieb Alexander Graf: > >> On 28.08.2011, at 07:09, Andreas Färber wrote: >> >>> Any thoughts on how to proceed? My previous approach for Haiku, to replace non-standard uint16 with POSIX uint_fast16_t etc., was rejected to avoid system-dependent widths. I'd really like to get rid of the annoyingly conflicting naming though (int vs. long for 32, int vs. short for 16, ...). >> >> I'm not sure what you mean by system-dependent widths? > > The core issue is this: uint16 in fpu/ does not mean uint16_t but "at least 16 bits wide" (int). Apart from the BeOS/Haiku i386 ABI being strange in using long for uint32, most uses of uint16 elsewhere mean "exactly 16 bits wide", leading to type conflicts. > > POSIX has two types for such least-width semantics: uint_least16_t and uint_fast16_t. We ran into the issue that a patch compiled on Darwin/ppc64 but broke on Linux/x86_64 or i386, so Aurelien considered it too risky to use types whose actual width depends on the host system within my series. Well, if we want host machine dependent types we should use host dependent types. If not, why can't we just typedef them to uint32_t or uint64_t and call it a day? Apparently uint16 is expected to be uint32_t in the code IIUC. Alex
On 28 August 2011 14:47, Alexander Graf <agraf@suse.de> wrote: > Well, if we want host machine dependent types we should use host > dependent types. ...we couldn't decide what we wanted :-) >Apparently uint16 is expected to be uint32_t in the code IIUC. If the softfloat code relies on uint16 being exactly 32 bits (or indeed on it being exactly 16 bits) then it has a bug in it. -- PMM
On 28.08.2011, at 09:08, Peter Maydell <peter.maydell@linaro.org> wrote: > On 28 August 2011 14:47, Alexander Graf <agraf@suse.de> wrote: >> Well, if we want host machine dependent types we should use host >> dependent types. > > ...we couldn't decide what we wanted :-) > >> Apparently uint16 is expected to be uint32_t in the code IIUC. > > If the softfloat code relies on uint16 being exactly 32 bits > (or indeed on it being exactly 16 bits) then it has a bug in it. My point is that we can have it either host-dependent or not. Both apparently doesn't work :) So the agnostic deterministic way would be to always use the same static type on all hosts. Alex >
2011/8/28 Peter Maydell <peter.maydell@linaro.org>: > On 28 August 2011 14:02, Alexander Graf <agraf@suse.de> wrote: > There are two ways we can fix the naming clash here: > > (1) change int16 -> int_fast16_t. This was argued against because > it means that the type might have a different width depending on > the host system, which means that where softfloat buggily makes > assumptions about the typewidth this would surface only on the > minority of systems where (eg) int_fast16_t wasn't 32 bits. > (In theory this is just preserving current behaviour but in fact > the set of typedefs in softfloat.h doesn't take advantage of the > "may be bigger" leeway; for example we 'typedef int8_t int8' even > though the comment above specifically suggests that int8 should > be typedef'd as an int.) > > (2) change int16 -> int16_t. This was argued against because it > would be dropping the information in the current softfloat code > about where we require a 16 bit type for correctness and where > we are happy for the type to be larger if it's more efficient. I think I was the main advocate for (1) here. If somebody felt like doing a quick benchmark of int*_t vs int_fast*_t (just some simple floatingpoint benchmark on a linux-user target that uses TCG and softfloat) that might help break the deadlock one way or the other. If there's no real difference in speed then I'd be willing to see us take option (2) instead. -- PMM
Am 02.09.2011 um 18:38 schrieb Peter Maydell: > 2011/8/28 Peter Maydell <peter.maydell@linaro.org>: >> On 28 August 2011 14:02, Alexander Graf <agraf@suse.de> wrote: >> There are two ways we can fix the naming clash here: >> >> (1) change int16 -> int_fast16_t. This was argued against because >> it means that the type might have a different width depending on >> the host system, which means that where softfloat buggily makes >> assumptions about the typewidth this would surface only on the >> minority of systems where (eg) int_fast16_t wasn't 32 bits. >> (In theory this is just preserving current behaviour but in fact >> the set of typedefs in softfloat.h doesn't take advantage of the >> "may be bigger" leeway; for example we 'typedef int8_t int8' even >> though the comment above specifically suggests that int8 should >> be typedef'd as an int.) >> >> (2) change int16 -> int16_t. This was argued against because it >> would be dropping the information in the current softfloat code >> about where we require a 16 bit type for correctness and where >> we are happy for the type to be larger if it's more efficient. > > I think I was the main advocate for (1) here. If somebody felt > like doing a quick benchmark of int*_t vs int_fast*_t (just some > simple floatingpoint benchmark on a linux-user target that uses > TCG and softfloat) that might help break the deadlock one way > or the other. If there's no real difference in speed then I'd > be willing to see us take option (2) instead. What about my preparatory patches? Can you ack them? Andreas
On 2 September 2011 19:18, Andreas Färber <andreas.faerber@web.de> wrote:
> What about my preparatory patches? Can you ack them?
Sorry, yeah, I'd missed those; that cleanup is worth doing
whatever we decide to do here. Now reviewed.
-- PMM
diff --git a/bswap.h b/bswap.h index f41bebe..ddef453 100644 --- a/bswap.h +++ b/bswap.h @@ -11,7 +11,9 @@ #include <machine/bswap.h> #else +#ifndef NO_SOFTFLOAT_H #include "softfloat.h" +#endif #ifdef CONFIG_BYTESWAP_H #include <byteswap.h> @@ -239,6 +241,7 @@ static inline uint32_t qemu_bswap_len(uint32_t value, int len) return bswap32(value) >> (32 - 8 * len); } +#ifndef NO_SOFTFLOAT_H typedef union { float32 f; uint32_t l; @@ -294,6 +297,7 @@ typedef union { } ll; #endif } CPU_QuadU; +#endif /* unaligned/endian-independent pointer access */ @@ -423,6 +427,7 @@ static inline void stq_le_p(void *ptr, uint64_t v) stl_le_p(p + 4, v >> 32); } +#ifndef NO_SOFTFLOAT_H /* float access */ static inline float32 ldfl_le_p(const void *ptr) @@ -461,6 +466,8 @@ static inline void stfq_le_p(void *ptr, float64 v) stl_le_p(ptr + 4, u.l.upper); } +#endif + #else static inline int lduw_le_p(const void *ptr) @@ -498,6 +505,7 @@ static inline void stq_le_p(void *ptr, uint64_t v) *(uint64_t *)ptr = v; } +#ifndef NO_SOFTFLOAT_H /* float access */ static inline float32 ldfl_le_p(const void *ptr) @@ -520,6 +528,7 @@ static inline void stfq_le_p(void *ptr, float64 v) *(float64 *)ptr = v; } #endif +#endif #if !defined(HOST_WORDS_BIGENDIAN) || defined(WORDS_ALIGNED) @@ -612,6 +621,7 @@ static inline void stq_be_p(void *ptr, uint64_t v) stl_be_p((uint8_t *)ptr + 4, v); } +#ifndef NO_SOFTFLOAT_H /* float access */ static inline float32 ldfl_be_p(const void *ptr) @@ -650,6 +660,8 @@ static inline void stfq_be_p(void *ptr, float64 v) stl_be_p((uint8_t *)ptr + 4, u.l.lower); } +#endif + #else static inline int lduw_be_p(const void *ptr) @@ -687,6 +699,7 @@ static inline void stq_be_p(void *ptr, uint64_t v) *(uint64_t *)ptr = v; } +#ifndef NO_SOFTFLOAT_H /* float access */ static inline float32 ldfl_be_p(const void *ptr) @@ -711,4 +724,6 @@ static inline void stfq_be_p(void *ptr, float64 v) #endif +#endif + #endif /* BSWAP_H */ diff --git a/ui/cocoa.m b/ui/cocoa.m index d9e4e3d..4bd0346 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -25,6 +25,7 @@ #import <Cocoa/Cocoa.h> #include <crt_externs.h> +#define NO_SOFTFLOAT_H #include "qemu-common.h" #include "console.h" #include "sysemu.h"
Hello, The unresolved softfloat uint* conversion business bites us again: This time, the previously working Cocoa frontend stopped compiling: In file included from /Users/andreas/QEMU/qemu/bswap.h:14, from /Users/andreas/QEMU/qemu/qemu-common.h:103, from /Users/andreas/QEMU/qemu/ui/cocoa.m:28: /Users/andreas/QEMU/qemu/fpu/softfloat.h:60: error: conflicting types for ‘uint16’ /System/Library/Frameworks/Security.framework/Headers/cssmconfig.h:68: error: previous declaration of ‘uint16’ was here make: *** [ui/cocoa.o] Error 1 Since commit cbbab9226da9572346837466a8770c117e7e65a2 (move unaligned memory access functions to bswap.h) softfloat.h is being #included in bswap.h, which gets pulled into cocoa.m through qemu-common.h. I thought Alex had set up a Darwin buildbot to catch such breakages... Any thoughts on how to proceed? My previous approach for Haiku, to replace non-standard uint16 with POSIX uint_fast16_t etc., was rejected to avoid system-dependent widths. I'd really like to get rid of the annoyingly conflicting naming though (int vs. long for 32, int vs. short for 16, ...). A fragile and ugly workaround is to suppress all softfloat usage within bswap.h (below). Andreas