[Qemu-devel] [PATCH v3 0/2] Force the C standard to gnu99

[Qemu-devel] [PATCH v3 0/2] Force the C standard to gnu99

[Thomas Huth]

Different versions of GCC and Clang use different versions of the C
standard by default. To avoid compilation problems with different
compilers in the future, we should enforce one language level
for all compilers. Since "gnu99" is the only usable option right now
(later versions are still marked as experimental in GCC v4.8),
we enforce compiling with -std=gnu99 now (in the second patch).
This caused some new warnings/erros to appear with clang, which are
fixed by the first patch now.

Thomas Huth (2):
  include/hw/ppc: Fix compilation with clang -std=gnu99
  configure: Force the C standard to gnu99

 configure                   | 5 ++++-
 include/hw/ppc/spapr.h      | 4 ++--
 include/hw/ppc/spapr_xive.h | 2 +-
 include/hw/ppc/xics.h       | 4 ++--
 4 files changed, 9 insertions(+), 6 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] include/hw/ppc: Fix compilation with clang -std=gnu99

[Thomas Huth]

When compiling with Clang and -std=gnu99, I get the following errors:

  CC      ppc64-softmmu/hw/intc/xics_spapr.o
In file included from hw/intc/xics_spapr.c:34:
include/hw/ppc/xics.h:46:25: error: redefinition of typedef 'ICSState' is a C11 feature
      [-Werror,-Wtypedef-redefinition]
typedef struct ICSState ICSState;
                        ^
include/hw/ppc/spapr.h:18:25: note: previous definition is here
typedef struct ICSState ICSState;
                        ^
In file included from hw/intc/xics_spapr.c:34:
include/hw/ppc/xics.h:203:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
      [-Werror,-Wtypedef-redefinition]
typedef struct sPAPRMachineState sPAPRMachineState;
                                 ^
include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
typedef struct sPAPRMachineState sPAPRMachineState;
                                 ^
  CC      ppc64-softmmu/hw/intc/spapr_xive.o
In file included from hw/intc/spapr_xive.c:19:
include/hw/ppc/spapr_xive.h:38:3: error: redefinition of typedef 'sPAPRXive' is a C11 feature
      [-Werror,-Wtypedef-redefinition]
} sPAPRXive;
  ^
include/hw/ppc/spapr.h:20:26: note: previous definition is here
typedef struct sPAPRXive sPAPRXive;
                         ^
In file included from hw/intc/spapr_xive.c:19:
include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
      [-Werror,-Wtypedef-redefinition]
typedef struct sPAPRMachineState sPAPRMachineState;
                                 ^
include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
typedef struct sPAPRMachineState sPAPRMachineState;
                                 ^
  CC      ppc64-softmmu/hw/char/spapr_vty.o
In file included from hw/char/spapr_vty.c:8:
In file included from include/hw/ppc/spapr.h:12:
include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
      [-Werror,-Wtypedef-redefinition]
typedef struct sPAPRMachineState sPAPRMachineState;
                                 ^
include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
typedef struct sPAPRMachineState sPAPRMachineState;
                                 ^

Since we're going to make -std=gnu99 mandatory, fix these issues
by including the right header files indead of typedeffing stuff twice.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/ppc/spapr.h      | 4 ++--
 include/hw/ppc/spapr_xive.h | 2 +-
 include/hw/ppc/xics.h       | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 2c77a8b..6a5ae4f 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -8,6 +8,8 @@
 #include "hw/mem/pc-dimm.h"
 #include "hw/ppc/spapr_ovec.h"
 #include "hw/ppc/spapr_irq.h"
+#include "hw/ppc/xics.h"
+#include "hw/ppc/spapr_xive.h"
 
 struct VIOsPAPRBus;
 struct sPAPRPHBState;
@@ -15,8 +17,6 @@ struct sPAPRNVRAM;
 typedef struct sPAPREventLogEntry sPAPREventLogEntry;
 typedef struct sPAPREventSource sPAPREventSource;
 typedef struct sPAPRPendingHPT sPAPRPendingHPT;
-typedef struct ICSState ICSState;
-typedef struct sPAPRXive sPAPRXive;
 
 #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
 #define SPAPR_ENTRY_POINT       0x100
diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index 728735d..aff4366 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -42,7 +42,7 @@ bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
 void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
 qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn);
 
-typedef struct sPAPRMachineState sPAPRMachineState;
+#include "hw/ppc/spapr.h"  /* for sPAPRMachineState */
 
 void spapr_xive_hcall_init(sPAPRMachineState *spapr);
 void spapr_dt_xive(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 14afda1..a7f49a4 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -47,6 +47,8 @@ typedef struct ICSState ICSState;
 typedef struct ICSIRQState ICSIRQState;
 typedef struct XICSFabric XICSFabric;
 
+#include "hw/ppc/spapr.h"
+
 #define TYPE_ICP "icp"
 #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
 
@@ -200,8 +202,6 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon);
 void ics_resend(ICSState *ics);
 void icp_resend(ICPState *ss);
 
-typedef struct sPAPRMachineState sPAPRMachineState;
-
 void spapr_dt_xics(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
                    uint32_t phandle);
 int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
-- 
1.8.3.1

[Qemu-devel] [PATCH v3 2/2] configure: Force the C standard to gnu99

[Thomas Huth]

Different versions of GCC and Clang use different versions of the C standard.
This repeatedly caused problems already, e.g. with duplicated typedefs:

 https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg05829.html

or with for-loop variable initializers:

 https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg00237.html

To avoid these problems, we should enforce the C language version to the
same level for all compilers. Since our minimum compiler versions is
GCC v4.8, our best option is "gnu99" for C code right now ("gnu17" is not
available there yet, and "gnu11" is marked as "experimental"), and "gnu++98"
for the few C++ code that we have in the repository.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 v3: Compile C++ code with -std=gnu++98

 configure | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index b9f34af..448dbc8 100755
--- a/configure
+++ b/configure
@@ -107,6 +107,9 @@ update_cxxflags() {
             -Wstrict-prototypes|-Wmissing-prototypes|-Wnested-externs|\
             -Wold-style-declaration|-Wold-style-definition|-Wredundant-decls)
                 ;;
+            -std=gnu99)
+                QEMU_CXXFLAGS=${QEMU_CXXFLAGS:+$QEMU_CXXFLAGS }"-std=gnu++98"
+                ;;
             *)
                 QEMU_CXXFLAGS=${QEMU_CXXFLAGS:+$QEMU_CXXFLAGS }$arg
                 ;;
@@ -585,7 +588,7 @@ ARFLAGS="${ARFLAGS-rv}"
 # left shift of signed integers is well defined and has the expected
 # 2s-complement style results. (Both clang and gcc agree that it
 # provides these semantics.)
-QEMU_CFLAGS="-fno-strict-aliasing -fno-common -fwrapv $QEMU_CFLAGS"
+QEMU_CFLAGS="-fno-strict-aliasing -fno-common -fwrapv -std=gnu99 $QEMU_CFLAGS"
 QEMU_CFLAGS="-Wall -Wundef -Wwrite-strings -Wmissing-prototypes $QEMU_CFLAGS"
 QEMU_CFLAGS="-Wstrict-prototypes -Wredundant-decls $QEMU_CFLAGS"
 QEMU_CFLAGS="-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE $QEMU_CFLAGS"
-- 
1.8.3.1

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] include/hw/ppc: Fix compilation with clang -std=gnu99

[Cédric Le Goater]

On 1/9/19 5:39 PM, Thomas Huth wrote:
> When compiling with Clang and -std=gnu99, I get the following errors:
> 
>   CC      ppc64-softmmu/hw/intc/xics_spapr.o
> In file included from hw/intc/xics_spapr.c:34:
> include/hw/ppc/xics.h:46:25: error: redefinition of typedef 'ICSState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct ICSState ICSState;
>                         ^
> include/hw/ppc/spapr.h:18:25: note: previous definition is here
> typedef struct ICSState ICSState;
>                         ^
> In file included from hw/intc/xics_spapr.c:34:
> include/hw/ppc/xics.h:203:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
>   CC      ppc64-softmmu/hw/intc/spapr_xive.o
> In file included from hw/intc/spapr_xive.c:19:
> include/hw/ppc/spapr_xive.h:38:3: error: redefinition of typedef 'sPAPRXive' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> } sPAPRXive;
>   ^
> include/hw/ppc/spapr.h:20:26: note: previous definition is here
> typedef struct sPAPRXive sPAPRXive;
>                          ^
> In file included from hw/intc/spapr_xive.c:19:
> include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
>   CC      ppc64-softmmu/hw/char/spapr_vty.o
> In file included from hw/char/spapr_vty.c:8:
> In file included from include/hw/ppc/spapr.h:12:
> include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
> 
> Since we're going to make -std=gnu99 mandatory, fix these issues
> by including the right header files indead of typedeffing stuff twice.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/hw/ppc/spapr.h      | 4 ++--
>  include/hw/ppc/spapr_xive.h | 2 +-
>  include/hw/ppc/xics.h       | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2c77a8b..6a5ae4f 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -8,6 +8,8 @@
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/ppc/spapr_ovec.h"
>  #include "hw/ppc/spapr_irq.h"
> +#include "hw/ppc/xics.h"
> +#include "hw/ppc/spapr_xive.h"
>  
>  struct VIOsPAPRBus;
>  struct sPAPRPHBState;
> @@ -15,8 +17,6 @@ struct sPAPRNVRAM;
>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
>  typedef struct sPAPREventSource sPAPREventSource;
>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> -typedef struct ICSState ICSState;
> -typedef struct sPAPRXive sPAPRXive;
>  
>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
>  #define SPAPR_ENTRY_POINT       0x100
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 728735d..aff4366 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -42,7 +42,7 @@ bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
>  void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
>  qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn);
>  
> -typedef struct sPAPRMachineState sPAPRMachineState;
> +#include "hw/ppc/spapr.h"  /* for sPAPRMachineState */

so both files include each other, how nice ...

>  void spapr_xive_hcall_init(sPAPRMachineState *spapr);
>  void spapr_dt_xive(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 14afda1..a7f49a4 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -47,6 +47,8 @@ typedef struct ICSState ICSState;
>  typedef struct ICSIRQState ICSIRQState;
>  typedef struct XICSFabric XICSFabric;
>  
> +#include "hw/ppc/spapr.h"
> +

and again ... I *really really* prefer forward declarations.



Reviewed-by: Cédric Le Goater <clg@kaod.org>

Grumbles,

C.


>  #define TYPE_ICP "icp"
>  #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
>  
> @@ -200,8 +202,6 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon);
>  void ics_resend(ICSState *ics);
>  void icp_resend(ICPState *ss);
>  
> -typedef struct sPAPRMachineState sPAPRMachineState;
> -
>  void spapr_dt_xics(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
>                     uint32_t phandle);
>  int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] include/hw/ppc: Fix compilation with clang -std=gnu99

[Daniel P. Berrangé]

On Wed, Jan 09, 2019 at 06:13:01PM +0100, Cédric Le Goater wrote:
> On 1/9/19 5:39 PM, Thomas Huth wrote:
> > When compiling with Clang and -std=gnu99, I get the following errors:
> > 
> >   CC      ppc64-softmmu/hw/intc/xics_spapr.o
> > In file included from hw/intc/xics_spapr.c:34:
> > include/hw/ppc/xics.h:46:25: error: redefinition of typedef 'ICSState' is a C11 feature
> >       [-Werror,-Wtypedef-redefinition]
> > typedef struct ICSState ICSState;
> >                         ^
> > include/hw/ppc/spapr.h:18:25: note: previous definition is here
> > typedef struct ICSState ICSState;
> >                         ^
> > In file included from hw/intc/xics_spapr.c:34:
> > include/hw/ppc/xics.h:203:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
> >       [-Werror,-Wtypedef-redefinition]
> > typedef struct sPAPRMachineState sPAPRMachineState;
> >                                  ^
> > include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> > typedef struct sPAPRMachineState sPAPRMachineState;
> >                                  ^
> >   CC      ppc64-softmmu/hw/intc/spapr_xive.o
> > In file included from hw/intc/spapr_xive.c:19:
> > include/hw/ppc/spapr_xive.h:38:3: error: redefinition of typedef 'sPAPRXive' is a C11 feature
> >       [-Werror,-Wtypedef-redefinition]
> > } sPAPRXive;
> >   ^
> > include/hw/ppc/spapr.h:20:26: note: previous definition is here
> > typedef struct sPAPRXive sPAPRXive;
> >                          ^
> > In file included from hw/intc/spapr_xive.c:19:
> > include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
> >       [-Werror,-Wtypedef-redefinition]
> > typedef struct sPAPRMachineState sPAPRMachineState;
> >                                  ^
> > include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> > typedef struct sPAPRMachineState sPAPRMachineState;
> >                                  ^
> >   CC      ppc64-softmmu/hw/char/spapr_vty.o
> > In file included from hw/char/spapr_vty.c:8:
> > In file included from include/hw/ppc/spapr.h:12:
> > include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
> >       [-Werror,-Wtypedef-redefinition]
> > typedef struct sPAPRMachineState sPAPRMachineState;
> >                                  ^
> > include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> > typedef struct sPAPRMachineState sPAPRMachineState;
> >                                  ^
> > 
> > Since we're going to make -std=gnu99 mandatory, fix these issues
> > by including the right header files indead of typedeffing stuff twice.
> >
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  include/hw/ppc/spapr.h      | 4 ++--
> >  include/hw/ppc/spapr_xive.h | 2 +-
> >  include/hw/ppc/xics.h       | 4 ++--
> >  3 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > index 2c77a8b..6a5ae4f 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -8,6 +8,8 @@
> >  #include "hw/mem/pc-dimm.h"
> >  #include "hw/ppc/spapr_ovec.h"
> >  #include "hw/ppc/spapr_irq.h"
> > +#include "hw/ppc/xics.h"
> > +#include "hw/ppc/spapr_xive.h"
> >  
> >  struct VIOsPAPRBus;
> >  struct sPAPRPHBState;
> > @@ -15,8 +17,6 @@ struct sPAPRNVRAM;
> >  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
> >  typedef struct sPAPREventSource sPAPREventSource;
> >  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> > -typedef struct ICSState ICSState;
> > -typedef struct sPAPRXive sPAPRXive;
> >  
> >  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
> >  #define SPAPR_ENTRY_POINT       0x100
> > diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> > index 728735d..aff4366 100644
> > --- a/include/hw/ppc/spapr_xive.h
> > +++ b/include/hw/ppc/spapr_xive.h
> > @@ -42,7 +42,7 @@ bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
> >  void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
> >  qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn);
> >  
> > -typedef struct sPAPRMachineState sPAPRMachineState;
> > +#include "hw/ppc/spapr.h"  /* for sPAPRMachineState */
> 
> so both files include each other, how nice ...

If the header files are mutually dependent it makes me wonder what the
point of having them split up is ?

Feels like either they need to be merged, or they need to be split up
and refactored even more to remove the mutual dependancy.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] include/hw/ppc: Fix compilation with clang -std=gnu99

[Cédric Le Goater]

On 1/9/19 6:28 PM, Daniel P. Berrangé wrote:
> On Wed, Jan 09, 2019 at 06:13:01PM +0100, Cédric Le Goater wrote:
>> On 1/9/19 5:39 PM, Thomas Huth wrote:
>>> When compiling with Clang and -std=gnu99, I get the following errors:
>>>
>>>   CC      ppc64-softmmu/hw/intc/xics_spapr.o
>>> In file included from hw/intc/xics_spapr.c:34:
>>> include/hw/ppc/xics.h:46:25: error: redefinition of typedef 'ICSState' is a C11 feature
>>>       [-Werror,-Wtypedef-redefinition]
>>> typedef struct ICSState ICSState;
>>>                         ^
>>> include/hw/ppc/spapr.h:18:25: note: previous definition is here
>>> typedef struct ICSState ICSState;
>>>                         ^
>>> In file included from hw/intc/xics_spapr.c:34:
>>> include/hw/ppc/xics.h:203:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>>>       [-Werror,-Wtypedef-redefinition]
>>> typedef struct sPAPRMachineState sPAPRMachineState;
>>>                                  ^
>>> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
>>> typedef struct sPAPRMachineState sPAPRMachineState;
>>>                                  ^
>>>   CC      ppc64-softmmu/hw/intc/spapr_xive.o
>>> In file included from hw/intc/spapr_xive.c:19:
>>> include/hw/ppc/spapr_xive.h:38:3: error: redefinition of typedef 'sPAPRXive' is a C11 feature
>>>       [-Werror,-Wtypedef-redefinition]
>>> } sPAPRXive;
>>>   ^
>>> include/hw/ppc/spapr.h:20:26: note: previous definition is here
>>> typedef struct sPAPRXive sPAPRXive;
>>>                          ^
>>> In file included from hw/intc/spapr_xive.c:19:
>>> include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>>>       [-Werror,-Wtypedef-redefinition]
>>> typedef struct sPAPRMachineState sPAPRMachineState;
>>>                                  ^
>>> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
>>> typedef struct sPAPRMachineState sPAPRMachineState;
>>>                                  ^
>>>   CC      ppc64-softmmu/hw/char/spapr_vty.o
>>> In file included from hw/char/spapr_vty.c:8:
>>> In file included from include/hw/ppc/spapr.h:12:
>>> include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>>>       [-Werror,-Wtypedef-redefinition]
>>> typedef struct sPAPRMachineState sPAPRMachineState;
>>>                                  ^
>>> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
>>> typedef struct sPAPRMachineState sPAPRMachineState;
>>>                                  ^
>>>
>>> Since we're going to make -std=gnu99 mandatory, fix these issues
>>> by including the right header files indead of typedeffing stuff twice.
>>>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  include/hw/ppc/spapr.h      | 4 ++--
>>>  include/hw/ppc/spapr_xive.h | 2 +-
>>>  include/hw/ppc/xics.h       | 4 ++--
>>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>> index 2c77a8b..6a5ae4f 100644
>>> --- a/include/hw/ppc/spapr.h
>>> +++ b/include/hw/ppc/spapr.h
>>> @@ -8,6 +8,8 @@
>>>  #include "hw/mem/pc-dimm.h"
>>>  #include "hw/ppc/spapr_ovec.h"
>>>  #include "hw/ppc/spapr_irq.h"
>>> +#include "hw/ppc/xics.h"
>>> +#include "hw/ppc/spapr_xive.h"
>>>  
>>>  struct VIOsPAPRBus;
>>>  struct sPAPRPHBState;
>>> @@ -15,8 +17,6 @@ struct sPAPRNVRAM;
>>>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
>>>  typedef struct sPAPREventSource sPAPREventSource;
>>>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
>>> -typedef struct ICSState ICSState;
>>> -typedef struct sPAPRXive sPAPRXive;
>>>  
>>>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
>>>  #define SPAPR_ENTRY_POINT       0x100
>>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
>>> index 728735d..aff4366 100644
>>> --- a/include/hw/ppc/spapr_xive.h
>>> +++ b/include/hw/ppc/spapr_xive.h
>>> @@ -42,7 +42,7 @@ bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
>>>  void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
>>>  qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn);
>>>  
>>> -typedef struct sPAPRMachineState sPAPRMachineState;
>>> +#include "hw/ppc/spapr.h"  /* for sPAPRMachineState */
>>
>> so both files include each other, how nice ...
> 
> If the header files are mutually dependent it makes me wonder what the
> point of having them split up is ?

The XICS interrupt controller models are used in two different machines,
sPAPR and PowerNV.

> Feels like either they need to be merged, or they need to be split up
> and refactored even more to remove the mutual dependancy.

yes or that some spapr_* definitions do not belong to the common xics.h
file.
 
Thanks,

C.

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] include/hw/ppc: Fix compilation with clang -std=gnu99

[Cédric Le Goater]

On 1/9/19 5:39 PM, Thomas Huth wrote:
> When compiling with Clang and -std=gnu99, I get the following errors:
> 
>   CC      ppc64-softmmu/hw/intc/xics_spapr.o
> In file included from hw/intc/xics_spapr.c:34:
> include/hw/ppc/xics.h:46:25: error: redefinition of typedef 'ICSState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct ICSState ICSState;
>                         ^
> include/hw/ppc/spapr.h:18:25: note: previous definition is here
> typedef struct ICSState ICSState;
>                         ^
> In file included from hw/intc/xics_spapr.c:34:
> include/hw/ppc/xics.h:203:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
>   CC      ppc64-softmmu/hw/intc/spapr_xive.o
> In file included from hw/intc/spapr_xive.c:19:
> include/hw/ppc/spapr_xive.h:38:3: error: redefinition of typedef 'sPAPRXive' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> } sPAPRXive;
>   ^
> include/hw/ppc/spapr.h:20:26: note: previous definition is here
> typedef struct sPAPRXive sPAPRXive;
>                          ^
> In file included from hw/intc/spapr_xive.c:19:
> include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
>   CC      ppc64-softmmu/hw/char/spapr_vty.o
> In file included from hw/char/spapr_vty.c:8:
> In file included from include/hw/ppc/spapr.h:12:
> include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
> 
> Since we're going to make -std=gnu99 mandatory, fix these issues
> by including the right header files indead of typedeffing stuff twice.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/hw/ppc/spapr.h      | 4 ++--
>  include/hw/ppc/spapr_xive.h | 2 +-
>  include/hw/ppc/xics.h       | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2c77a8b..6a5ae4f 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -8,6 +8,8 @@
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/ppc/spapr_ovec.h"
>  #include "hw/ppc/spapr_irq.h"
> +#include "hw/ppc/xics.h"
> +#include "hw/ppc/spapr_xive.h"
>  
>  struct VIOsPAPRBus;
>  struct sPAPRPHBState;
> @@ -15,8 +17,6 @@ struct sPAPRNVRAM;
>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
>  typedef struct sPAPREventSource sPAPREventSource;
>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> -typedef struct ICSState ICSState;
> -typedef struct sPAPRXive sPAPRXive;
>  
>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
>  #define SPAPR_ENTRY_POINT       0x100
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 728735d..aff4366 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -42,7 +42,7 @@ bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
>  void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
>  qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn);
>  
> -typedef struct sPAPRMachineState sPAPRMachineState;
> +#include "hw/ppc/spapr.h"  /* for sPAPRMachineState */
>  
>  void spapr_xive_hcall_init(sPAPRMachineState *spapr);
>  void spapr_dt_xive(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 14afda1..a7f49a4 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -47,6 +47,8 @@ typedef struct ICSState ICSState;
>  typedef struct ICSIRQState ICSIRQState;
>  typedef struct XICSFabric XICSFabric;
>  
> +#include "hw/ppc/spapr.h"
> +
>  #define TYPE_ICP "icp"
>  #define ICP(obj) OBJECT_CHECK(ICPState, (obj), TYPE_ICP)
>  
> @@ -200,8 +202,6 @@ void ics_pic_print_info(ICSState *ics, Monitor *mon);
>  void ics_resend(ICSState *ics);
>  void icp_resend(ICPState *ss);

I think the definitions below belong to another header file, 
may be "xics_spapr.h". That would be cleaner.

Thanks,

C. 

> -typedef struct sPAPRMachineState sPAPRMachineState;
> -
>  void spapr_dt_xics(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
>                     uint32_t phandle);
>  int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] include/hw/ppc: Fix compilation with clang -std=gnu99

[Greg Kurz]

On Wed, 9 Jan 2019 18:40:48 +0100
Cédric Le Goater <clg@kaod.org> wrote:

> On 1/9/19 6:28 PM, Daniel P. Berrangé wrote:
> > On Wed, Jan 09, 2019 at 06:13:01PM +0100, Cédric Le Goater wrote:  
> >> On 1/9/19 5:39 PM, Thomas Huth wrote:  
> >>> When compiling with Clang and -std=gnu99, I get the following errors:
> >>>
> >>>   CC      ppc64-softmmu/hw/intc/xics_spapr.o
> >>> In file included from hw/intc/xics_spapr.c:34:
> >>> include/hw/ppc/xics.h:46:25: error: redefinition of typedef 'ICSState' is a C11 feature
> >>>       [-Werror,-Wtypedef-redefinition]
> >>> typedef struct ICSState ICSState;
> >>>                         ^
> >>> include/hw/ppc/spapr.h:18:25: note: previous definition is here
> >>> typedef struct ICSState ICSState;
> >>>                         ^
> >>> In file included from hw/intc/xics_spapr.c:34:
> >>> include/hw/ppc/xics.h:203:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
> >>>       [-Werror,-Wtypedef-redefinition]
> >>> typedef struct sPAPRMachineState sPAPRMachineState;
> >>>                                  ^
> >>> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> >>> typedef struct sPAPRMachineState sPAPRMachineState;
> >>>                                  ^
> >>>   CC      ppc64-softmmu/hw/intc/spapr_xive.o
> >>> In file included from hw/intc/spapr_xive.c:19:
> >>> include/hw/ppc/spapr_xive.h:38:3: error: redefinition of typedef 'sPAPRXive' is a C11 feature
> >>>       [-Werror,-Wtypedef-redefinition]
> >>> } sPAPRXive;
> >>>   ^
> >>> include/hw/ppc/spapr.h:20:26: note: previous definition is here
> >>> typedef struct sPAPRXive sPAPRXive;
> >>>                          ^
> >>> In file included from hw/intc/spapr_xive.c:19:
> >>> include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
> >>>       [-Werror,-Wtypedef-redefinition]
> >>> typedef struct sPAPRMachineState sPAPRMachineState;
> >>>                                  ^
> >>> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> >>> typedef struct sPAPRMachineState sPAPRMachineState;
> >>>                                  ^
> >>>   CC      ppc64-softmmu/hw/char/spapr_vty.o
> >>> In file included from hw/char/spapr_vty.c:8:
> >>> In file included from include/hw/ppc/spapr.h:12:
> >>> include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
> >>>       [-Werror,-Wtypedef-redefinition]
> >>> typedef struct sPAPRMachineState sPAPRMachineState;
> >>>                                  ^
> >>> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> >>> typedef struct sPAPRMachineState sPAPRMachineState;
> >>>                                  ^
> >>>
> >>> Since we're going to make -std=gnu99 mandatory, fix these issues
> >>> by including the right header files indead of typedeffing stuff twice.
> >>>
> >>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>> ---
> >>>  include/hw/ppc/spapr.h      | 4 ++--
> >>>  include/hw/ppc/spapr_xive.h | 2 +-
> >>>  include/hw/ppc/xics.h       | 4 ++--
> >>>  3 files changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >>> index 2c77a8b..6a5ae4f 100644
> >>> --- a/include/hw/ppc/spapr.h
> >>> +++ b/include/hw/ppc/spapr.h
> >>> @@ -8,6 +8,8 @@
> >>>  #include "hw/mem/pc-dimm.h"
> >>>  #include "hw/ppc/spapr_ovec.h"
> >>>  #include "hw/ppc/spapr_irq.h"
> >>> +#include "hw/ppc/xics.h"
> >>> +#include "hw/ppc/spapr_xive.h"
> >>>  
> >>>  struct VIOsPAPRBus;
> >>>  struct sPAPRPHBState;
> >>> @@ -15,8 +17,6 @@ struct sPAPRNVRAM;
> >>>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
> >>>  typedef struct sPAPREventSource sPAPREventSource;
> >>>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> >>> -typedef struct ICSState ICSState;
> >>> -typedef struct sPAPRXive sPAPRXive;
> >>>  
> >>>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
> >>>  #define SPAPR_ENTRY_POINT       0x100
> >>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> >>> index 728735d..aff4366 100644
> >>> --- a/include/hw/ppc/spapr_xive.h
> >>> +++ b/include/hw/ppc/spapr_xive.h
> >>> @@ -42,7 +42,7 @@ bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
> >>>  void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
> >>>  qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn);
> >>>  
> >>> -typedef struct sPAPRMachineState sPAPRMachineState;
> >>> +#include "hw/ppc/spapr.h"  /* for sPAPRMachineState */  
> >>
> >> so both files include each other, how nice ...  
> > 
> > If the header files are mutually dependent it makes me wonder what the
> > point of having them split up is ?  
> 
> The XICS interrupt controller models are used in two different machines,
> sPAPR and PowerNV.
> 
> > Feels like either they need to be merged, or they need to be split up
> > and refactored even more to remove the mutual dependancy.  
> 
> yes or that some spapr_* definitions do not belong to the common xics.h
> file.

Yeah,

These should move to a new xics_spapr.h or maybe even spapr.h for simplicity:

void spapr_dt_xics(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
                   uint32_t phandle);
int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
void xics_spapr_init(sPAPRMachineState *spapr);

>  
> Thanks,
> 
> C.
> 

Re: [Qemu-devel] [PATCH v3 2/2] configure: Force the C standard to gnu99

[Philippe Mathieu-Daudé]

On 1/9/19 5:39 PM, Thomas Huth wrote:
> Different versions of GCC and Clang use different versions of the C standard.
> This repeatedly caused problems already, e.g. with duplicated typedefs:
> 
>  https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg05829.html
> 
> or with for-loop variable initializers:
> 
>  https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg00237.html
> 
> To avoid these problems, we should enforce the C language version to the
> same level for all compilers. Since our minimum compiler versions is
> GCC v4.8, our best option is "gnu99" for C code right now ("gnu17" is not
> available there yet, and "gnu11" is marked as "experimental"), and "gnu++98"
> for the few C++ code that we have in the repository.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Raising from a 30 years old stantard to a 20 years old one is a great
improvement (I used to be a defender of the C89, but I evolved and now
admit C99 has helpful features).

Using '-std=gnu++98' for g++ v4.8 looks like a good compromise to the
issues Daniel mentioned (still 'experimental').

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  v3: Compile C++ code with -std=gnu++98
> 
>  configure | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/configure b/configure
> index b9f34af..448dbc8 100755
> --- a/configure
> +++ b/configure
> @@ -107,6 +107,9 @@ update_cxxflags() {
>              -Wstrict-prototypes|-Wmissing-prototypes|-Wnested-externs|\
>              -Wold-style-declaration|-Wold-style-definition|-Wredundant-decls)
>                  ;;
> +            -std=gnu99)
> +                QEMU_CXXFLAGS=${QEMU_CXXFLAGS:+$QEMU_CXXFLAGS }"-std=gnu++98"
> +                ;;
>              *)
>                  QEMU_CXXFLAGS=${QEMU_CXXFLAGS:+$QEMU_CXXFLAGS }$arg
>                  ;;
> @@ -585,7 +588,7 @@ ARFLAGS="${ARFLAGS-rv}"
>  # left shift of signed integers is well defined and has the expected
>  # 2s-complement style results. (Both clang and gcc agree that it
>  # provides these semantics.)
> -QEMU_CFLAGS="-fno-strict-aliasing -fno-common -fwrapv $QEMU_CFLAGS"
> +QEMU_CFLAGS="-fno-strict-aliasing -fno-common -fwrapv -std=gnu99 $QEMU_CFLAGS"
>  QEMU_CFLAGS="-Wall -Wundef -Wwrite-strings -Wmissing-prototypes $QEMU_CFLAGS"
>  QEMU_CFLAGS="-Wstrict-prototypes -Wredundant-decls $QEMU_CFLAGS"
>  QEMU_CFLAGS="-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE $QEMU_CFLAGS"
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] include/hw/ppc: Fix compilation with clang -std=gnu99

[Paolo Bonzini]

On 09/01/19 18:28, Daniel P. Berrangé wrote:
>> so both files include each other, how nice ...
> If the header files are mutually dependent it makes me wonder what the
> point of having them split up is ?
> 
> Feels like either they need to be merged, or they need to be split up
> and refactored even more to remove the mutual dependancy.

If they include each other only for the typedefs, then prehaps the
solution is to change the coding style and allow using struct in
function prototypes.  I'm pretty sure there are several examples of this
already.

Paolo

Re: [Qemu-devel] [PATCH v3 2/2] configure: Force the C standard to gnu99

[Richard Henderson]

On 1/10/19 3:39 AM, Thomas Huth wrote:
> Different versions of GCC and Clang use different versions of the C standard.
> This repeatedly caused problems already, e.g. with duplicated typedefs:
> 
>  https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg05829.html
> 
> or with for-loop variable initializers:
> 
>  https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg00237.html
> 
> To avoid these problems, we should enforce the C language version to the
> same level for all compilers. Since our minimum compiler versions is
> GCC v4.8, our best option is "gnu99" for C code right now ("gnu17" is not
> available there yet, and "gnu11" is marked as "experimental"), and "gnu++98"
> for the few C++ code that we have in the repository.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  v3: Compile C++ code with -std=gnu++98

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] include/hw/ppc: Fix compilation with clang -std=gnu99

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

On 1/9/19 3:20 PM, Paolo Bonzini wrote:
> On 09/01/19 18:28, Daniel P. Berrangé wrote:
>>> so both files include each other, how nice ...
>> If the header files are mutually dependent it makes me wonder what the
>> point of having them split up is ?
>>
>> Feels like either they need to be merged, or they need to be split up
>> and refactored even more to remove the mutual dependancy.
> 
> If they include each other only for the typedefs, then prehaps the
> solution is to change the coding style and allow using struct in
> function prototypes.  I'm pretty sure there are several examples of this
> already.

Or stick the typedef in <typedefs.h>, instead of trying to find (or
create) some other common header.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH v3 2/2] configure: Force the C standard to gnu99

[Paolo Bonzini]

On 09/01/19 19:10, Philippe Mathieu-Daudé wrote:
> Using '-std=gnu++98' for g++ v4.8 looks like a good compromise to the
> issues Daniel mentioned (still 'experimental').
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

C++11 has many new features that almost make it an entirely different
language (the main being rvalue references, and type inference with auto
and decltype).  If it works for GCC 4.8, I would prefer using that
instead of g++98.

Paolo

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] include/hw/ppc: Fix compilation with clang -std=gnu99

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]

On 09/01/19 22:25, Eric Blake wrote:
> On 1/9/19 3:20 PM, Paolo Bonzini wrote:
>> On 09/01/19 18:28, Daniel P. Berrangé wrote:
>>>> so both files include each other, how nice ...
>>> If the header files are mutually dependent it makes me wonder what the
>>> point of having them split up is ?
>>>
>>> Feels like either they need to be merged, or they need to be split up
>>> and refactored even more to remove the mutual dependancy.
>>
>> If they include each other only for the typedefs, then prehaps the
>> solution is to change the coding style and allow using struct in
>> function prototypes.  I'm pretty sure there are several examples of this
>> already.
> 
> Or stick the typedef in <typedefs.h>, instead of trying to find (or
> create) some other common header.

Putting typedefs for a very specific machine or device in typedefs.h
doesn't seem too clean.  Usually typedefs.h is used when things are
shared by different subsystems (e.g. accelerator and device emulation,
or frontend and backend), and I'm pretty sure that it could be made even
smaller if someone was inclined to try.

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] include/hw/ppc: Fix compilation with clang -std=gnu99

[Thomas Huth]

On 2019-01-09 22:20, Paolo Bonzini wrote:
> On 09/01/19 18:28, Daniel P. Berrangé wrote:
>>> so both files include each other, how nice ...
>> If the header files are mutually dependent it makes me wonder what the
>> point of having them split up is ?
>>
>> Feels like either they need to be merged, or they need to be split up
>> and refactored even more to remove the mutual dependancy.
> 
> If they include each other only for the typedefs, then prehaps the
> solution is to change the coding style and allow using struct in
> function prototypes.  I'm pretty sure there are several examples of this
> already.

I'm all in favor for this!

 Thomas

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] include/hw/ppc: Fix compilation with clang -std=gnu99

[Thomas Huth]

On 2019-01-09 18:47, Greg Kurz wrote:
> On Wed, 9 Jan 2019 18:40:48 +0100
> Cédric Le Goater <clg@kaod.org> wrote:
> 
>> On 1/9/19 6:28 PM, Daniel P. Berrangé wrote:
>>> On Wed, Jan 09, 2019 at 06:13:01PM +0100, Cédric Le Goater wrote:  
>>>> On 1/9/19 5:39 PM, Thomas Huth wrote:  
>>>>> When compiling with Clang and -std=gnu99, I get the following errors:
>>>>>
>>>>>   CC      ppc64-softmmu/hw/intc/xics_spapr.o
>>>>> In file included from hw/intc/xics_spapr.c:34:
>>>>> include/hw/ppc/xics.h:46:25: error: redefinition of typedef 'ICSState' is a C11 feature
>>>>>       [-Werror,-Wtypedef-redefinition]
>>>>> typedef struct ICSState ICSState;
>>>>>                         ^
>>>>> include/hw/ppc/spapr.h:18:25: note: previous definition is here
>>>>> typedef struct ICSState ICSState;
>>>>>                         ^
>>>>> In file included from hw/intc/xics_spapr.c:34:
>>>>> include/hw/ppc/xics.h:203:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>>>>>       [-Werror,-Wtypedef-redefinition]
>>>>> typedef struct sPAPRMachineState sPAPRMachineState;
>>>>>                                  ^
>>>>> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
>>>>> typedef struct sPAPRMachineState sPAPRMachineState;
>>>>>                                  ^
>>>>>   CC      ppc64-softmmu/hw/intc/spapr_xive.o
>>>>> In file included from hw/intc/spapr_xive.c:19:
>>>>> include/hw/ppc/spapr_xive.h:38:3: error: redefinition of typedef 'sPAPRXive' is a C11 feature
>>>>>       [-Werror,-Wtypedef-redefinition]
>>>>> } sPAPRXive;
>>>>>   ^
>>>>> include/hw/ppc/spapr.h:20:26: note: previous definition is here
>>>>> typedef struct sPAPRXive sPAPRXive;
>>>>>                          ^
>>>>> In file included from hw/intc/spapr_xive.c:19:
>>>>> include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>>>>>       [-Werror,-Wtypedef-redefinition]
>>>>> typedef struct sPAPRMachineState sPAPRMachineState;
>>>>>                                  ^
>>>>> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
>>>>> typedef struct sPAPRMachineState sPAPRMachineState;
>>>>>                                  ^
>>>>>   CC      ppc64-softmmu/hw/char/spapr_vty.o
>>>>> In file included from hw/char/spapr_vty.c:8:
>>>>> In file included from include/hw/ppc/spapr.h:12:
>>>>> include/hw/ppc/spapr_xive.h:45:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>>>>>       [-Werror,-Wtypedef-redefinition]
>>>>> typedef struct sPAPRMachineState sPAPRMachineState;
>>>>>                                  ^
>>>>> include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
>>>>> typedef struct sPAPRMachineState sPAPRMachineState;
>>>>>                                  ^
>>>>>
>>>>> Since we're going to make -std=gnu99 mandatory, fix these issues
>>>>> by including the right header files indead of typedeffing stuff twice.
>>>>>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>  include/hw/ppc/spapr.h      | 4 ++--
>>>>>  include/hw/ppc/spapr_xive.h | 2 +-
>>>>>  include/hw/ppc/xics.h       | 4 ++--
>>>>>  3 files changed, 5 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>>> index 2c77a8b..6a5ae4f 100644
>>>>> --- a/include/hw/ppc/spapr.h
>>>>> +++ b/include/hw/ppc/spapr.h
>>>>> @@ -8,6 +8,8 @@
>>>>>  #include "hw/mem/pc-dimm.h"
>>>>>  #include "hw/ppc/spapr_ovec.h"
>>>>>  #include "hw/ppc/spapr_irq.h"
>>>>> +#include "hw/ppc/xics.h"
>>>>> +#include "hw/ppc/spapr_xive.h"
>>>>>  
>>>>>  struct VIOsPAPRBus;
>>>>>  struct sPAPRPHBState;
>>>>> @@ -15,8 +17,6 @@ struct sPAPRNVRAM;
>>>>>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
>>>>>  typedef struct sPAPREventSource sPAPREventSource;
>>>>>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
>>>>> -typedef struct ICSState ICSState;
>>>>> -typedef struct sPAPRXive sPAPRXive;
>>>>>  
>>>>>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
>>>>>  #define SPAPR_ENTRY_POINT       0x100
>>>>> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
>>>>> index 728735d..aff4366 100644
>>>>> --- a/include/hw/ppc/spapr_xive.h
>>>>> +++ b/include/hw/ppc/spapr_xive.h
>>>>> @@ -42,7 +42,7 @@ bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
>>>>>  void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
>>>>>  qemu_irq spapr_xive_qirq(sPAPRXive *xive, uint32_t lisn);
>>>>>  
>>>>> -typedef struct sPAPRMachineState sPAPRMachineState;
>>>>> +#include "hw/ppc/spapr.h"  /* for sPAPRMachineState */  
>>>>
>>>> so both files include each other, how nice ...  
>>>
>>> If the header files are mutually dependent it makes me wonder what the
>>> point of having them split up is ?  
>>
>> The XICS interrupt controller models are used in two different machines,
>> sPAPR and PowerNV.
>>
>>> Feels like either they need to be merged, or they need to be split up
>>> and refactored even more to remove the mutual dependancy.  
>>
>> yes or that some spapr_* definitions do not belong to the common xics.h
>> file.
> 
> Yeah,
> 
> These should move to a new xics_spapr.h or maybe even spapr.h for simplicity:
> 
> void spapr_dt_xics(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
>                    uint32_t phandle);
> int xics_kvm_init(sPAPRMachineState *spapr, Error **errp);
> void xics_spapr_init(sPAPRMachineState *spapr);

Ok, I'll move them to xics_spapr.h.

 Thomas

Re: [Qemu-devel] [PATCH v3 2/2] configure: Force the C standard to gnu99

[Thomas Huth]

On 2019-01-09 22:26, Paolo Bonzini wrote:
> On 09/01/19 19:10, Philippe Mathieu-Daudé wrote:
>> Using '-std=gnu++98' for g++ v4.8 looks like a good compromise to the
>> issues Daniel mentioned (still 'experimental').
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
> C++11 has many new features that almost make it an entirely different
> language (the main being rvalue references, and type inference with auto
> and decltype).  If it works for GCC 4.8, I would prefer using that
> instead of g++98.

I think we can revisit that setting when we really want to introduce
code that needs it. Currently it does not seem to be necessary, so that
does not justify to use an "experimental" feature of GCC 4.8, I guess.

 Thomas

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] include/hw/ppc: Fix compilation with clang -std=gnu99

[Daniel P. Berrangé]

On Wed, Jan 09, 2019 at 03:25:53PM -0600, Eric Blake wrote:
> On 1/9/19 3:20 PM, Paolo Bonzini wrote:
> > On 09/01/19 18:28, Daniel P. Berrangé wrote:
> >>> so both files include each other, how nice ...
> >> If the header files are mutually dependent it makes me wonder what the
> >> point of having them split up is ?
> >>
> >> Feels like either they need to be merged, or they need to be split up
> >> and refactored even more to remove the mutual dependancy.
> > 
> > If they include each other only for the typedefs, then prehaps the
> > solution is to change the coding style and allow using struct in
> > function prototypes.  I'm pretty sure there are several examples of this
> > already.
> 
> Or stick the typedef in <typedefs.h>, instead of trying to find (or
> create) some other common header.

Probably better to just have a local  spapr_types.h instead of
polluting the global namespace.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/2] include/hw/ppc: Fix compilation with clang -std=gnu99

[Greg Kurz]

On Thu, 10 Jan 2019 10:12:43 +0000
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Wed, Jan 09, 2019 at 03:25:53PM -0600, Eric Blake wrote:
> > On 1/9/19 3:20 PM, Paolo Bonzini wrote:  
> > > On 09/01/19 18:28, Daniel P. Berrangé wrote:  
> > >>> so both files include each other, how nice ...  
> > >> If the header files are mutually dependent it makes me wonder what the
> > >> point of having them split up is ?
> > >>
> > >> Feels like either they need to be merged, or they need to be split up
> > >> and refactored even more to remove the mutual dependancy.  
> > > 
> > > If they include each other only for the typedefs, then prehaps the
> > > solution is to change the coding style and allow using struct in
> > > function prototypes.  I'm pretty sure there are several examples of this
> > > already.  
> > 
> > Or stick the typedef in <typedefs.h>, instead of trying to find (or
> > create) some other common header.  
> 
> Probably better to just have a local  spapr_types.h instead of
> polluting the global namespace.
> 

I personally like this approach because it would allow to use the
typedefs everywhere, for the sake of consistency.

> 
> Regards,
> Daniel