Patchwork softfloat breaks cocoa.m

login
register
mail settings
Submitter Andreas Färber
Date Aug. 28, 2011, 12:09 p.m.
Message ID <EE3117B0-EED9-47CE-BF19-77988DEE8830@web.de>
Download mbox | patch
Permalink /patch/111901/
State New
Headers show

Comments

Andreas Färber - Aug. 28, 2011, 12:09 p.m.
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
Alexander Graf - Aug. 28, 2011, 1:02 p.m.
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
Peter Maydell - Aug. 28, 2011, 1:32 p.m.
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
Andreas Färber - Aug. 28, 2011, 1:34 p.m.
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
Alexander Graf - Aug. 28, 2011, 1:47 p.m.
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
Peter Maydell - Aug. 28, 2011, 2:08 p.m.
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
Alexander Graf - Aug. 28, 2011, 4:10 p.m.
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

>
Peter Maydell - Sept. 2, 2011, 4:38 p.m.
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
Andreas Färber - Sept. 2, 2011, 6:18 p.m.
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
Peter Maydell - Sept. 3, 2011, 2:49 p.m.
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

Patch

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"