Comments

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

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