Re: [Qemu-devel] Patch: dyngen-exec.h for OpenBSD

Re: [Qemu-devel] Patch: dyngen-exec.h for OpenBSD

[Juergen Keil]

> From: Thiemo Seufer <ths@networkno.de>
> Todd T. Fries wrote:
> > This is relative to the 20070319 snapshot.
> > 
> > 
> > --- dyngen-exec.h.orig	Mon Feb  5 17:01:54 2007
> > +++ dyngen-exec.h	Sat Mar 10 16:39:39 2007
...
> >  /* XXX: This may be wrong for 64-bit ILP32 hosts.  */
> >  typedef void * host_reg_t;
> > @@ -78,11 +83,15 @@ typedef void * host_reg_t;
> >  #define UINT32_MAX		(4294967295U)
> >  #define UINT64_MAX		((uint64_t)(18446744073709551615))
> >  
> > +#ifdef __OpenBSD__
> > +typedef struct __sFILE FILE;
> > +#else
> >  typedef struct FILE FILE;
> >  extern int fprintf(FILE *, const char *, ...);
> >  extern int printf(const char *, ...);
> >  #undef NULL
> >  #define NULL 0
> > +#endif
> 
> Shouldn't this cover only the FILE typedef?

Probably.

My dyngen-exec.h has a similar change, when I made some NetBSD experiments:

Index: dyngen-exec.h
===================================================================
RCS file: /cvsroot/qemu/qemu/dyngen-exec.h,v
retrieving revision 1.33
diff -u -B -r1.33 dyngen-exec.h
--- dyngen-exec.h       30 Mar 2007 16:44:53 -0000      1.33
+++ dyngen-exec.h       2 Apr 2007 09:42:03 -0000
@@ -78,7 +78,11 @@
 #define UINT32_MAX             (4294967295U)
 #define UINT64_MAX             ((uint64_t)(18446744073709551615))

+#ifdef __NetBSD__
+typedef struct __sFILE FILE;
+#else
 typedef struct FILE FILE;
+#endif
 extern int fprintf(FILE *, const char *, ...);
 extern int fputs(const char *, FILE *);
 extern int printf(const char *, ...);

Re: [Qemu-devel] Patch: dyngen-exec.h for OpenBSD

[Thiemo Seufer]

Juergen Keil wrote:
> 
> > From: Thiemo Seufer <ths@networkno.de>
> > Todd T. Fries wrote:
> > > This is relative to the 20070319 snapshot.
> > > 
> > > 
> > > --- dyngen-exec.h.orig	Mon Feb  5 17:01:54 2007
> > > +++ dyngen-exec.h	Sat Mar 10 16:39:39 2007
> ...
> > >  /* XXX: This may be wrong for 64-bit ILP32 hosts.  */
> > >  typedef void * host_reg_t;
> > > @@ -78,11 +83,15 @@ typedef void * host_reg_t;
> > >  #define UINT32_MAX		(4294967295U)
> > >  #define UINT64_MAX		((uint64_t)(18446744073709551615))
> > >  
> > > +#ifdef __OpenBSD__
> > > +typedef struct __sFILE FILE;
> > > +#else
> > >  typedef struct FILE FILE;
> > >  extern int fprintf(FILE *, const char *, ...);
> > >  extern int printf(const char *, ...);
> > >  #undef NULL
> > >  #define NULL 0
> > > +#endif
> > 
> > Shouldn't this cover only the FILE typedef?
> 
> Probably.
> 
> My dyngen-exec.h has a similar change, when I made some NetBSD experiments:
> 
> Index: dyngen-exec.h
> ===================================================================
> RCS file: /cvsroot/qemu/qemu/dyngen-exec.h,v
> retrieving revision 1.33
> diff -u -B -r1.33 dyngen-exec.h
> --- dyngen-exec.h       30 Mar 2007 16:44:53 -0000      1.33
> +++ dyngen-exec.h       2 Apr 2007 09:42:03 -0000
> @@ -78,7 +78,11 @@
>  #define UINT32_MAX             (4294967295U)
>  #define UINT64_MAX             ((uint64_t)(18446744073709551615))
> 
> +#ifdef __NetBSD__
> +typedef struct __sFILE FILE;
> +#else
>  typedef struct FILE FILE;
> +#endif

I made that "#ifdef _BSD" based on the assumption it is ok for all
BSD variants, including Darwin.


Thiemo

Re: [Qemu-devel] Patch: dyngen-exec.h for OpenBSD

[M. Warner Losh]

In message: <20070402124122.GE24846@networkno.de>
            Thiemo Seufer <ths@networkno.de> writes:
: I made that "#ifdef _BSD" based on the assumption it is ok for all
: BSD variants, including Darwin.

_BSD isn't defined on all variants of BSD.  sys/param.h defines BSD to
be 199506 on all BSD systems (at least all of them derived from 4.4BSD
lite).  sys/param.h also defines BSD4_3 and BSD4_4.  FreeBSD defines
__FreeBSD__ in the compiler, NetBSD defined __NetBSD__, OpenBSD
defines __OpenBSD__.  I'm unsure what darwin/osx define.

so unless I missed a change elsewhere in the build system to define
_BSD, this change needs some more thought.

Warner

Re: [Qemu-devel] Patch: dyngen-exec.h for OpenBSD

[Thiemo Seufer]

M. Warner Losh wrote:
> In message: <20070402124122.GE24846@networkno.de>
>             Thiemo Seufer <ths@networkno.de> writes:
> : I made that "#ifdef _BSD" based on the assumption it is ok for all
> : BSD variants, including Darwin.
> 
> _BSD isn't defined on all variants of BSD.  sys/param.h defines BSD to
> be 199506 on all BSD systems (at least all of them derived from 4.4BSD
> lite).  sys/param.h also defines BSD4_3 and BSD4_4.  FreeBSD defines
> __FreeBSD__ in the compiler, NetBSD defined __NetBSD__, OpenBSD
> defines __OpenBSD__.  I'm unsure what darwin/osx define.
> 
> so unless I missed a change elsewhere in the build system to define
> _BSD, this change needs some more thought.

It is already used in other files. The define in qemu comes from the
configure script via config.h, which might be a bug.


Thiemo

Re: [Qemu-devel] Patch: dyngen-exec.h for OpenBSD

[M. Warner Losh]

In message: <20070402160839.GJ24846@networkno.de>
            Thiemo Seufer <ths@networkno.de> writes:
: M. Warner Losh wrote:
: > In message: <20070402124122.GE24846@networkno.de>
: >             Thiemo Seufer <ths@networkno.de> writes:
: > : I made that "#ifdef _BSD" based on the assumption it is ok for all
: > : BSD variants, including Darwin.
: > 
: > _BSD isn't defined on all variants of BSD.  sys/param.h defines BSD to
: > be 199506 on all BSD systems (at least all of them derived from 4.4BSD
: > lite).  sys/param.h also defines BSD4_3 and BSD4_4.  FreeBSD defines
: > __FreeBSD__ in the compiler, NetBSD defined __NetBSD__, OpenBSD
: > defines __OpenBSD__.  I'm unsure what darwin/osx define.
: > 
: > so unless I missed a change elsewhere in the build system to define
: > _BSD, this change needs some more thought.
: 
: It is already used in other files. The define in qemu comes from the
: configure script via config.h, which might be a bug.

Somehow I missed that detail...  In that case, nevermind :-)

Warner

[Qemu-devel] Patch: dyngen-exec.h for OpenBSD

[Todd T. Fries]

This is relative to the 20070319 snapshot.


--- dyngen-exec.h.orig	Mon Feb  5 17:01:54 2007
+++ dyngen-exec.h	Sat Mar 10 16:39:39 2007
@@ -27,11 +27,15 @@
 #define _FILEDEFED
 #endif
 
+#include "config.h"
+
 /* NOTE: standard headers should be used with special care at this
    point because host CPU registers are used as global variables. Some
    host headers do not allow that. */
 #include <stddef.h>
-
+#ifdef __OpenBSD__
+#include <sys/types.h>
+#else
 typedef unsigned char uint8_t;
 typedef unsigned short uint16_t;
 typedef unsigned int uint32_t;
@@ -61,6 +65,7 @@ typedef signed long int64_t;
 typedef signed long long int64_t;
 #endif
 #endif
+#endif
 
 /* XXX: This may be wrong for 64-bit ILP32 hosts.  */
 typedef void * host_reg_t;
@@ -78,11 +83,15 @@ typedef void * host_reg_t;
 #define UINT32_MAX		(4294967295U)
 #define UINT64_MAX		((uint64_t)(18446744073709551615))
 
+#ifdef __OpenBSD__
+typedef struct __sFILE FILE;
+#else
 typedef struct FILE FILE;
 extern int fprintf(FILE *, const char *, ...);
 extern int printf(const char *, ...);
 #undef NULL
 #define NULL 0
+#endif
 
 #ifdef __i386__
 #define AREG0 "ebp"
-- 
Todd Fries .. todd@fries.net

 _____________________________________________
|                                             \  1.636.410.0632 (voice)
| Free Daemon Consulting, LLC                 \  1.405.227.9094 (voice)
| http://FreeDaemonConsulting.com             \  1.866.792.3418 (FAX)
| "..in support of free software solutions."  \          250797 (FWD)
|                                             \
 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
                                                 
              37E7 D3EB 74D0 8D66 A68D  B866 0326 204E 3F42 004A
                        http://todd.fries.net/pgp.txt

Re: [Qemu-devel] Patch: dyngen-exec.h for OpenBSD

[Thiemo Seufer]

Todd T. Fries wrote:
> This is relative to the 20070319 snapshot.
> 
> 
> --- dyngen-exec.h.orig	Mon Feb  5 17:01:54 2007
> +++ dyngen-exec.h	Sat Mar 10 16:39:39 2007
> @@ -27,11 +27,15 @@
>  #define _FILEDEFED
>  #endif
>  
> +#include "config.h"
> +

Doesn't seem to be necessary in the header.

>  /* NOTE: standard headers should be used with special care at this
>     point because host CPU registers are used as global variables. Some
>     host headers do not allow that. */
>  #include <stddef.h>
> -
> +#ifdef __OpenBSD__
> +#include <sys/types.h>
> +#else
>  typedef unsigned char uint8_t;
>  typedef unsigned short uint16_t;
>  typedef unsigned int uint32_t;
> @@ -61,6 +65,7 @@ typedef signed long int64_t;
>  typedef signed long long int64_t;
>  #endif
>  #endif
> +#endif

Is this specialcase really needed for OpenBSD?

>  /* XXX: This may be wrong for 64-bit ILP32 hosts.  */
>  typedef void * host_reg_t;
> @@ -78,11 +83,15 @@ typedef void * host_reg_t;
>  #define UINT32_MAX		(4294967295U)
>  #define UINT64_MAX		((uint64_t)(18446744073709551615))
>  
> +#ifdef __OpenBSD__
> +typedef struct __sFILE FILE;
> +#else
>  typedef struct FILE FILE;
>  extern int fprintf(FILE *, const char *, ...);
>  extern int printf(const char *, ...);
>  #undef NULL
>  #define NULL 0
> +#endif

Shouldn't this cover only the FILE typedef?


Thiemo

Re: [Qemu-devel] Patch: dyngen-exec.h for OpenBSD

[Todd T. Fries]

Penned by Thiemo Seufer on 20070402 10:54.53, we have:
| >  /* NOTE: standard headers should be used with special care at this
| >     point because host CPU registers are used as global variables. Some
| >     host headers do not allow that. */
| >  #include <stddef.h>
| > -
| > +#ifdef __OpenBSD__
| > +#include <sys/types.h>
| > +#else
| >  typedef unsigned char uint8_t;
| >  typedef unsigned short uint16_t;
| >  typedef unsigned int uint32_t;
| > @@ -61,6 +65,7 @@ typedef signed long int64_t;
| >  typedef signed long long int64_t;
| >  #endif
| >  #endif
| > +#endif
| 
| Is this specialcase really needed for OpenBSD?

Can you honestly tell me that on all platforms this is true?

Hello? Portability?  sys/types.h defines these types portably.
Doing so the way this code does it, is not portable.

I left your non portable code to you; if systems in general
define these types in sys/types.h then just remove the
typedefs, problem solved.
 
| >  /* XXX: This may be wrong for 64-bit ILP32 hosts.  */
| >  typedef void * host_reg_t;
| > @@ -78,11 +83,15 @@ typedef void * host_reg_t;
| >  #define UINT32_MAX		(4294967295U)
| >  #define UINT64_MAX		((uint64_t)(18446744073709551615))
| >  
| > +#ifdef __OpenBSD__
| > +typedef struct __sFILE FILE;
| > +#else
| >  typedef struct FILE FILE;
| >  extern int fprintf(FILE *, const char *, ...);
| >  extern int printf(const char *, ...);
| >  #undef NULL
| >  #define NULL 0
| > +#endif
| 
| Shouldn't this cover only the FILE typedef?

Why is it that qemu knows what the definition of these prototypes
are on all systems without consulting the header files.  I have a
better idea, lets let the header files define the prototypes.
Who would have though of that?

.. of course I purposefully intended to remove cruft that is
in header files and belongs in header files.

I'll look at your other comments later, but these I see later in
the discussion are nearing inclusion with your recommended tweaks.

I'll submit patches to correct things again if necessary.

Thanks,
-- 
Todd Fries .. todd@fries.net

 _____________________________________________
|                                             \  1.636.410.0632 (voice)
| Free Daemon Consulting, LLC                 \  1.405.227.9094 (voice)
| http://FreeDaemonConsulting.com             \  1.866.792.3418 (FAX)
| "..in support of free software solutions."  \          250797 (FWD)
|                                             \
 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
                                                 
              37E7 D3EB 74D0 8D66 A68D  B866 0326 204E 3F42 004A
                        http://todd.fries.net/pgp.txt

Re: [Qemu-devel] Patch: dyngen-exec.h for OpenBSD

[Thiemo Seufer]

Todd T. Fries wrote:
> 
> Penned by Thiemo Seufer on 20070402 10:54.53, we have:
> | >  /* NOTE: standard headers should be used with special care at this
> | >     point because host CPU registers are used as global variables. Some
> | >     host headers do not allow that. */
> | >  #include <stddef.h>
> | > -
> | > +#ifdef __OpenBSD__
> | > +#include <sys/types.h>
> | > +#else
> | >  typedef unsigned char uint8_t;
> | >  typedef unsigned short uint16_t;
> | >  typedef unsigned int uint32_t;
> | > @@ -61,6 +65,7 @@ typedef signed long int64_t;
> | >  typedef signed long long int64_t;
> | >  #endif
> | >  #endif
> | > +#endif
> | 
> | Is this specialcase really needed for OpenBSD?
> 
> Can you honestly tell me that on all platforms this is true?
> 
> Hello? Portability?  sys/types.h defines these types portably.
> Doing so the way this code does it, is not portable.
> 
> I left your non portable code to you; if systems in general
> define these types in sys/types.h then just remove the
> typedefs, problem solved.

Please consider the NOTE: above those includes.

> | >  /* XXX: This may be wrong for 64-bit ILP32 hosts.  */
> | >  typedef void * host_reg_t;
> | > @@ -78,11 +83,15 @@ typedef void * host_reg_t;
> | >  #define UINT32_MAX		(4294967295U)
> | >  #define UINT64_MAX		((uint64_t)(18446744073709551615))
> | >  
> | > +#ifdef __OpenBSD__
> | > +typedef struct __sFILE FILE;
> | > +#else
> | >  typedef struct FILE FILE;
> | >  extern int fprintf(FILE *, const char *, ...);
> | >  extern int printf(const char *, ...);
> | >  #undef NULL
> | >  #define NULL 0
> | > +#endif
> | 
> | Shouldn't this cover only the FILE typedef?
> 
> Why is it that qemu knows what the definition of these prototypes
> are on all systems without consulting the header files.  I have a
> better idea, lets let the header files define the prototypes.
> Who would have though of that?
> 
> .. of course I purposefully intended to remove cruft that is
> in header files and belongs in header files.

See above.


Thiemo

Re: [Qemu-devel] Patch: dyngen-exec.h for OpenBSD

[Paul Brook]

On Thursday 05 April 2007 23:12, Todd T. Fries wrote:
> Penned by Thiemo Seufer on 20070402 10:54.53, we have:
> | >  /* NOTE: standard headers should be used with special care at this
> | >     point because host CPU registers are used as global variables. Some
> | >     host headers do not allow that. */
> | >  #include <stddef.h>
> | > -
> | > +#ifdef __OpenBSD__
> | > +#include <sys/types.h>
> Hello? Portability?  sys/types.h defines these types portably.
> Doing so the way this code does it, is not portable.

If you want  portability you should be including stdint.h (or inttypes.h for 
old, broken systems).

> Why is it that qemu knows what the definition of these prototypes
> are on all systems without consulting the header files.  I have a
> better idea, lets let the header files define the prototypes.
> Who would have though of that?

See the big NOTE: comment above. dyngen is inherently unportable.

Paul

Re: [Qemu-devel] Patch: dyngen-exec.h for OpenBSD

[Anthony Liguori]

Paul Brook wrote:
> On Thursday 05 April 2007 23:12, Todd T. Fries wrote:
>   
>> Penned by Thiemo Seufer on 20070402 10:54.53, we have:
>> | >  /* NOTE: standard headers should be used with special care at this
>> | >     point because host CPU registers are used as global variables. Some
>> | >     host headers do not allow that. */
>> | >  #include <stddef.h>
>> | > -
>> | > +#ifdef __OpenBSD__
>> | > +#include <sys/types.h>
>> Hello? Portability?  sys/types.h defines these types portably.
>> Doing so the way this code does it, is not portable.
>>     
>
> If you want  portability you should be including stdint.h (or inttypes.h for 
> old, broken systems).
>   

I thought I'd add that this isn't just portability.  stdint.h is what 
C99 mandates although as Paul mentions, some older systems used inttypes.h.

Regards,

Anthony Liguori

>> Why is it that qemu knows what the definition of these prototypes
>> are on all systems without consulting the header files.  I have a
>> better idea, lets let the header files define the prototypes.
>> Who would have though of that?
>>     
>
> See the big NOTE: comment above. dyngen is inherently unportable.
>
> Paul
>
>
>
>