[Qemu-devel] [PATCH v4 0/3] Force the C standard to gnu99

[Qemu-devel] [PATCH v4 0/3] 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 third patch).
This caused some new warnings/erros to appear with clang, which are
fixed by the first two patches now.

v4:
 - Avoid circular inclusion of headers and introduce xics_spapr.h

v3:
 - Compile C++ code with -std=gnu++98

v2:
 - Use gnu99 instead of gnu11

Thomas Huth (3):
  ppc: Move spapr-related prototypes from xics.h into a seperate header
    file
  ppc: Drop duplicated typedefs to be able to compile with Clang in
    gnu99 mode
  configure: Force the C standard to gnu99

 configure                   |  5 ++++-
 hw/intc/xics_kvm.c          |  1 +
 hw/intc/xics_spapr.c        |  1 +
 hw/ppc/spapr_irq.c          |  1 +
 include/hw/ppc/spapr.h      |  9 +++++----
 include/hw/ppc/spapr_xive.h |  3 +--
 include/hw/ppc/xics.h       |  7 -------
 include/hw/ppc/xics_spapr.h | 37 +++++++++++++++++++++++++++++++++++++
 target/ppc/cpu.h            |  9 +++++----
 9 files changed, 55 insertions(+), 18 deletions(-)
 create mode 100644 include/hw/ppc/xics_spapr.h

-- 
1.8.3.1

[Qemu-devel] [PATCH v4 1/3] ppc: Move spapr-related prototypes from xics.h into a seperate header file

[Thomas Huth]

When compiling with Clang in -std=gnu99 mode, there is a warning/error:

  CC      ppc64-softmmu/hw/intc/xics_spapr.o
In file included from /home/thuth/devel/qemu/hw/intc/xics_spapr.c:34:
/home/thuth/devel/qemu/include/hw/ppc/xics.h:203:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
      [-Werror,-Wtypedef-redefinition]
typedef struct sPAPRMachineState sPAPRMachineState;
                                 ^
/home/thuth/devel/qemu/include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
typedef struct sPAPRMachineState sPAPRMachineState;
                                 ^

We have to remove the duplicated typedef here and include "spapr.h" instead.
But "spapr.h" should not be included for the pnv machine files. So move
the spapr-related prototypes into a new file called "xics_spapr.h" instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/intc/xics_kvm.c          |  1 +
 hw/intc/xics_spapr.c        |  1 +
 hw/ppc/spapr_irq.c          |  1 +
 include/hw/ppc/xics.h       |  7 -------
 include/hw/ppc/xics_spapr.h | 37 +++++++++++++++++++++++++++++++++++++
 5 files changed, 40 insertions(+), 7 deletions(-)
 create mode 100644 include/hw/ppc/xics_spapr.h

diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
index ac94594..dff1330 100644
--- a/hw/intc/xics_kvm.c
+++ b/hw/intc/xics_kvm.c
@@ -34,6 +34,7 @@
 #include "sysemu/kvm.h"
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/xics.h"
+#include "hw/ppc/xics_spapr.h"
 #include "kvm_ppc.h"
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
index 9c1a90d..de6cc15 100644
--- a/hw/intc/xics_spapr.c
+++ b/hw/intc/xics_spapr.c
@@ -32,6 +32,7 @@
 #include "qemu/timer.h"
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/xics.h"
+#include "hw/ppc/xics_spapr.h"
 #include "hw/ppc/fdt.h"
 #include "qapi/visitor.h"
 
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 5fce72f..1da7a32 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -14,6 +14,7 @@
 #include "hw/ppc/spapr.h"
 #include "hw/ppc/spapr_xive.h"
 #include "hw/ppc/xics.h"
+#include "hw/ppc/xics_spapr.h"
 #include "sysemu/kvm.h"
 
 #include "trace.h"
diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
index 07508cb..fad786e 100644
--- a/include/hw/ppc/xics.h
+++ b/include/hw/ppc/xics.h
@@ -200,13 +200,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);
-void xics_spapr_init(sPAPRMachineState *spapr);
-
 Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
                    Error **errp);
 
diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
new file mode 100644
index 0000000..b1ab27d
--- /dev/null
+++ b/include/hw/ppc/xics_spapr.h
@@ -0,0 +1,37 @@
+/*
+ * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
+ *
+ * PAPR Virtualized Interrupt System, aka ICS/ICP aka xics
+ *
+ * Copyright (c) 2010, 2011 David Gibson, IBM Corporation.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#ifndef XICS_SPAPR_H
+#define XICS_SPAPR_H
+
+#include "hw/ppc/spapr.h"
+
+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);
+
+#endif /* XICS_SPAPR_H */
-- 
1.8.3.1

[Qemu-devel] [PATCH v4 2/3] ppc: Drop duplicated typedefs to be able to compile with Clang in gnu99 mode

[Thomas Huth]

When compiling the ppc code with clang and -std=gnu99, there are a
couple of warnings/errors like this one:

  CC      ppc64-softmmu/hw/intc/xics.o
In file included from /home/thuth/devel/qemu/hw/intc/xics.c:35:
/home/thuth/devel/qemu/include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
      [-Werror,-Wtypedef-redefinition]
typedef struct ICPState ICPState;
                        ^
/home/thuth/devel/qemu/target/ppc/cpu.h:1181:25: note: previous definition is here
typedef struct ICPState ICPState;
                        ^

Drop the duplicated typedefs and use normal "struct" forward declarations
like we already do it at the top of spapr.h for a couple of other definitions.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/hw/ppc/spapr.h      | 9 +++++----
 include/hw/ppc/spapr_xive.h | 3 +--
 target/ppc/cpu.h            | 9 +++++----
 3 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 9e01a5a..10d069e 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -12,11 +12,12 @@
 struct VIOsPAPRBus;
 struct sPAPRPHBState;
 struct sPAPRNVRAM;
+struct ICSState;
+struct sPAPRXive;
+
 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
@@ -127,7 +128,7 @@ struct sPAPRMachineState {
     struct VIOsPAPRBus *vio_bus;
     QLIST_HEAD(, sPAPRPHBState) phbs;
     struct sPAPRNVRAM *nvram;
-    ICSState *ics;
+    struct ICSState *ics;
     sPAPRRTCState rtc;
 
     sPAPRResizeHPT resize_hpt;
@@ -180,7 +181,7 @@ struct sPAPRMachineState {
     const char *icp_type;
     int32_t irq_map_nr;
     unsigned long *irq_map;
-    sPAPRXive  *xive;
+    struct sPAPRXive  *xive;
     sPAPRIrq *irq;
     qemu_irq *qirqs;
 
diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
index 7fdc250..556b124 100644
--- a/include/hw/ppc/spapr_xive.h
+++ b/include/hw/ppc/spapr_xive.h
@@ -11,6 +11,7 @@
 #define PPC_SPAPR_XIVE_H
 
 #include "hw/ppc/xive.h"
+#include "hw/ppc/spapr.h"
 
 #define TYPE_SPAPR_XIVE "spapr-xive"
 #define SPAPR_XIVE(obj) OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE)
@@ -41,8 +42,6 @@ bool spapr_xive_irq_claim(sPAPRXive *xive, uint32_t lisn, bool lsi);
 bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
 void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
 
-typedef struct sPAPRMachineState sPAPRMachineState;
-
 void spapr_xive_hcall_init(sPAPRMachineState *spapr);
 void spapr_dt_xive(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
                    uint32_t phandle);
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 486abaf..a62ff60 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1177,8 +1177,9 @@ do {                                            \
 
 typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
 typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
-typedef struct XiveTCTX XiveTCTX;
-typedef struct ICPState ICPState;
+
+struct XiveTCTX;
+struct ICPState;
 
 /**
  * PowerPCCPU:
@@ -1197,8 +1198,8 @@ struct PowerPCCPU {
     int vcpu_id;
     uint32_t compat_pvr;
     PPCVirtualHypervisor *vhyp;
-    ICPState *icp;
-    XiveTCTX *tctx;
+    struct ICPState *icp;
+    struct XiveTCTX *tctx;
     void *machine_data;
     int32_t node_id; /* NUMA node this CPU belongs to */
     PPCHash64Options *hash64_opts;
-- 
1.8.3.1

[Qemu-devel] [PATCH v4 3/3] 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.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 configure | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 8049b71..f4078f6 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] [PATCH v4 1/3] ppc: Move spapr-related prototypes from xics.h into a seperate header file

[Greg Kurz]

On Thu, 10 Jan 2019 10:15:34 +0100
Thomas Huth <thuth@redhat.com> wrote:

> When compiling with Clang in -std=gnu99 mode, there is a warning/error:
> 
>   CC      ppc64-softmmu/hw/intc/xics_spapr.o
> In file included from /home/thuth/devel/qemu/hw/intc/xics_spapr.c:34:
> /home/thuth/devel/qemu/include/hw/ppc/xics.h:203:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
> /home/thuth/devel/qemu/include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
> 
> We have to remove the duplicated typedef here and include "spapr.h" instead.
> But "spapr.h" should not be included for the pnv machine files. So move
> the spapr-related prototypes into a new file called "xics_spapr.h" instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  hw/intc/xics_kvm.c          |  1 +
>  hw/intc/xics_spapr.c        |  1 +
>  hw/ppc/spapr_irq.c          |  1 +
>  include/hw/ppc/xics.h       |  7 -------
>  include/hw/ppc/xics_spapr.h | 37 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 40 insertions(+), 7 deletions(-)
>  create mode 100644 include/hw/ppc/xics_spapr.h
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index ac94594..dff1330 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -34,6 +34,7 @@
>  #include "sysemu/kvm.h"
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/xics.h"
> +#include "hw/ppc/xics_spapr.h"
>  #include "kvm_ppc.h"
>  #include "qemu/config-file.h"
>  #include "qemu/error-report.h"
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 9c1a90d..de6cc15 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -32,6 +32,7 @@
>  #include "qemu/timer.h"
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/xics.h"
> +#include "hw/ppc/xics_spapr.h"
>  #include "hw/ppc/fdt.h"
>  #include "qapi/visitor.h"
>  
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 5fce72f..1da7a32 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -14,6 +14,7 @@
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/spapr_xive.h"
>  #include "hw/ppc/xics.h"
> +#include "hw/ppc/xics_spapr.h"
>  #include "sysemu/kvm.h"
>  
>  #include "trace.h"
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 07508cb..fad786e 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -200,13 +200,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);
> -void xics_spapr_init(sPAPRMachineState *spapr);
> -
>  Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
>                     Error **errp);
>  
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> new file mode 100644
> index 0000000..b1ab27d
> --- /dev/null
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -0,0 +1,37 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
> + *
> + * PAPR Virtualized Interrupt System, aka ICS/ICP aka xics
> + *
> + * Copyright (c) 2010, 2011 David Gibson, IBM Corporation.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef XICS_SPAPR_H
> +#define XICS_SPAPR_H
> +
> +#include "hw/ppc/spapr.h"
> +
> +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);
> +
> +#endif /* XICS_SPAPR_H */

Re: [Qemu-devel] [PATCH v4 1/3] ppc: Move spapr-related prototypes from xics.h into a seperate header file

[Cédric Le Goater]

On 1/10/19 10:15 AM, Thomas Huth wrote:
> When compiling with Clang in -std=gnu99 mode, there is a warning/error:
> 
>   CC      ppc64-softmmu/hw/intc/xics_spapr.o
> In file included from /home/thuth/devel/qemu/hw/intc/xics_spapr.c:34:
> /home/thuth/devel/qemu/include/hw/ppc/xics.h:203:34: error: redefinition of typedef 'sPAPRMachineState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
> /home/thuth/devel/qemu/include/hw/ppc/spapr_irq.h:25:34: note: previous definition is here
> typedef struct sPAPRMachineState sPAPRMachineState;
>                                  ^
> 
> We have to remove the duplicated typedef here and include "spapr.h" instead.
> But "spapr.h" should not be included for the pnv machine files. So move
> the spapr-related prototypes into a new file called "xics_spapr.h" instead.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>


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

Thanks,

C.

> ---
>  hw/intc/xics_kvm.c          |  1 +
>  hw/intc/xics_spapr.c        |  1 +
>  hw/ppc/spapr_irq.c          |  1 +
>  include/hw/ppc/xics.h       |  7 -------
>  include/hw/ppc/xics_spapr.h | 37 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 40 insertions(+), 7 deletions(-)
>  create mode 100644 include/hw/ppc/xics_spapr.h
> 
> diff --git a/hw/intc/xics_kvm.c b/hw/intc/xics_kvm.c
> index ac94594..dff1330 100644
> --- a/hw/intc/xics_kvm.c
> +++ b/hw/intc/xics_kvm.c
> @@ -34,6 +34,7 @@
>  #include "sysemu/kvm.h"
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/xics.h"
> +#include "hw/ppc/xics_spapr.h"
>  #include "kvm_ppc.h"
>  #include "qemu/config-file.h"
>  #include "qemu/error-report.h"
> diff --git a/hw/intc/xics_spapr.c b/hw/intc/xics_spapr.c
> index 9c1a90d..de6cc15 100644
> --- a/hw/intc/xics_spapr.c
> +++ b/hw/intc/xics_spapr.c
> @@ -32,6 +32,7 @@
>  #include "qemu/timer.h"
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/xics.h"
> +#include "hw/ppc/xics_spapr.h"
>  #include "hw/ppc/fdt.h"
>  #include "qapi/visitor.h"
>  
> diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
> index 5fce72f..1da7a32 100644
> --- a/hw/ppc/spapr_irq.c
> +++ b/hw/ppc/spapr_irq.c
> @@ -14,6 +14,7 @@
>  #include "hw/ppc/spapr.h"
>  #include "hw/ppc/spapr_xive.h"
>  #include "hw/ppc/xics.h"
> +#include "hw/ppc/xics_spapr.h"
>  #include "sysemu/kvm.h"
>  
>  #include "trace.h"
> diff --git a/include/hw/ppc/xics.h b/include/hw/ppc/xics.h
> index 07508cb..fad786e 100644
> --- a/include/hw/ppc/xics.h
> +++ b/include/hw/ppc/xics.h
> @@ -200,13 +200,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);
> -void xics_spapr_init(sPAPRMachineState *spapr);
> -
>  Object *icp_create(Object *cpu, const char *type, XICSFabric *xi,
>                     Error **errp);
>  
> diff --git a/include/hw/ppc/xics_spapr.h b/include/hw/ppc/xics_spapr.h
> new file mode 100644
> index 0000000..b1ab27d
> --- /dev/null
> +++ b/include/hw/ppc/xics_spapr.h
> @@ -0,0 +1,37 @@
> +/*
> + * QEMU PowerPC pSeries Logical Partition (aka sPAPR) hardware System Emulator
> + *
> + * PAPR Virtualized Interrupt System, aka ICS/ICP aka xics
> + *
> + * Copyright (c) 2010, 2011 David Gibson, IBM Corporation.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef XICS_SPAPR_H
> +#define XICS_SPAPR_H
> +
> +#include "hw/ppc/spapr.h"
> +
> +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);
> +
> +#endif /* XICS_SPAPR_H */
> 

Re: [Qemu-devel] [PATCH v4 2/3] ppc: Drop duplicated typedefs to be able to compile with Clang in gnu99 mode

[Greg Kurz]

On Thu, 10 Jan 2019 10:15:35 +0100
Thomas Huth <thuth@redhat.com> wrote:

> When compiling the ppc code with clang and -std=gnu99, there are a
> couple of warnings/errors like this one:
> 
>   CC      ppc64-softmmu/hw/intc/xics.o
> In file included from /home/thuth/devel/qemu/hw/intc/xics.c:35:
> /home/thuth/devel/qemu/include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
>       [-Werror,-Wtypedef-redefinition]
> typedef struct ICPState ICPState;
>                         ^
> /home/thuth/devel/qemu/target/ppc/cpu.h:1181:25: note: previous definition is here
> typedef struct ICPState ICPState;
>                         ^
> 
> Drop the duplicated typedefs and use normal "struct" forward declarations
> like we already do it at the top of spapr.h for a couple of other definitions.
> 

Hmm... so the choice here is to simply ignore the official coding
style ?

It is a bit confusing to end up with even more struct/non-struct
inconsistency. It would be good at least to update HACKING so that
people know when they can legitimately do that... or we simply don't
care anymore for the typedef rule ?

All these forward declarations could be typedefs in a "hw/ppc/spapr_types.h"
header as well, as suggested elsewhere by Daniel.

> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  include/hw/ppc/spapr.h      | 9 +++++----
>  include/hw/ppc/spapr_xive.h | 3 +--
>  target/ppc/cpu.h            | 9 +++++----
>  3 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 9e01a5a..10d069e 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -12,11 +12,12 @@
>  struct VIOsPAPRBus;
>  struct sPAPRPHBState;
>  struct sPAPRNVRAM;
> +struct ICSState;
> +struct sPAPRXive;
> +
>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
>  typedef struct sPAPREventSource sPAPREventSource;
>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> -typedef struct ICSState ICSState;

Thanks to the previous patch, I guess the ICSState type could be
obtained by including "hw/ppc/xics.h".

> -typedef struct sPAPRXive sPAPRXive;
>  
>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
>  #define SPAPR_ENTRY_POINT       0x100
> @@ -127,7 +128,7 @@ struct sPAPRMachineState {
>      struct VIOsPAPRBus *vio_bus;
>      QLIST_HEAD(, sPAPRPHBState) phbs;
>      struct sPAPRNVRAM *nvram;
> -    ICSState *ics;
> +    struct ICSState *ics;
>      sPAPRRTCState rtc;
>  
>      sPAPRResizeHPT resize_hpt;
> @@ -180,7 +181,7 @@ struct sPAPRMachineState {
>      const char *icp_type;
>      int32_t irq_map_nr;
>      unsigned long *irq_map;
> -    sPAPRXive  *xive;
> +    struct sPAPRXive  *xive;
>      sPAPRIrq *irq;
>      qemu_irq *qirqs;
>  
> diff --git a/include/hw/ppc/spapr_xive.h b/include/hw/ppc/spapr_xive.h
> index 7fdc250..556b124 100644
> --- a/include/hw/ppc/spapr_xive.h
> +++ b/include/hw/ppc/spapr_xive.h
> @@ -11,6 +11,7 @@
>  #define PPC_SPAPR_XIVE_H
>  
>  #include "hw/ppc/xive.h"
> +#include "hw/ppc/spapr.h"
>  

The sPAPRMachineState typedef statement lies in "hw/ppc/spapr_irq.h"
actually, which is a bit weird... Probably better to move the typedef
to "hw/ppc/spapr_types.h" and include it here.

>  #define TYPE_SPAPR_XIVE "spapr-xive"
>  #define SPAPR_XIVE(obj) OBJECT_CHECK(sPAPRXive, (obj), TYPE_SPAPR_XIVE)
> @@ -41,8 +42,6 @@ bool spapr_xive_irq_claim(sPAPRXive *xive, uint32_t lisn, bool lsi);
>  bool spapr_xive_irq_free(sPAPRXive *xive, uint32_t lisn);
>  void spapr_xive_pic_print_info(sPAPRXive *xive, Monitor *mon);
>  
> -typedef struct sPAPRMachineState sPAPRMachineState;
> -
>  void spapr_xive_hcall_init(sPAPRMachineState *spapr);
>  void spapr_dt_xive(sPAPRMachineState *spapr, uint32_t nr_servers, void *fdt,
>                     uint32_t phandle);
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 486abaf..a62ff60 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1177,8 +1177,9 @@ do {                                            \
>  
>  typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
>  typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
> -typedef struct XiveTCTX XiveTCTX;
> -typedef struct ICPState ICPState;
> +
> +struct XiveTCTX;
> +struct ICPState;

These could be made available from the XICS/XIVE header files.

#ifndef CONFIG_USER_ONLY
#include "hw/ppc/xive.h" /* for XiveTCTX */
#include "hw/ppc/xics.h" /* for ICPState */
#endif

>  
>  /**
>   * PowerPCCPU:
> @@ -1197,8 +1198,8 @@ struct PowerPCCPU {
>      int vcpu_id;
>      uint32_t compat_pvr;
>      PPCVirtualHypervisor *vhyp;
> -    ICPState *icp;
> -    XiveTCTX *tctx;
> +    struct ICPState *icp;
> +    struct XiveTCTX *tctx;

+#ifndef CONFIG_USER_ONLY
     ICPState *icp;
     XiveTCTX *tctx;
+#endif

These aren't needed for user anyway.

>      void *machine_data;
>      int32_t node_id; /* NUMA node this CPU belongs to */
>      PPCHash64Options *hash64_opts;

Re: [Qemu-devel] [PATCH v4 2/3] ppc: Drop duplicated typedefs to be able to compile with Clang in gnu99 mode

[Thomas Huth]

On 2019-01-10 14:15, Greg Kurz wrote:
> On Thu, 10 Jan 2019 10:15:35 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> When compiling the ppc code with clang and -std=gnu99, there are a
>> couple of warnings/errors like this one:
>>
>>   CC      ppc64-softmmu/hw/intc/xics.o
>> In file included from /home/thuth/devel/qemu/hw/intc/xics.c:35:
>> /home/thuth/devel/qemu/include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
>>       [-Werror,-Wtypedef-redefinition]
>> typedef struct ICPState ICPState;
>>                         ^
>> /home/thuth/devel/qemu/target/ppc/cpu.h:1181:25: note: previous definition is here
>> typedef struct ICPState ICPState;
>>                         ^
>>
>> Drop the duplicated typedefs and use normal "struct" forward declarations
>> like we already do it at the top of spapr.h for a couple of other definitions.
>>
> 
> Hmm... so the choice here is to simply ignore the official coding
> style ?

Are typedefs really our "official coding style"? It's mentioned in
HACKING, not in CODING_STYLE, so I rather see this as a recommendation
only. (Otherwise, all the forward struct definitions at the beginning of
spapr.h are a plain violation of the coding style, too...)

IMHO we should rather adopt the coding style of the kernel which rather
tries to avoid to typedef each and every struct.

> It is a bit confusing to end up with even more struct/non-struct
> inconsistency. It would be good at least to update HACKING so that
> people know when they can legitimately do that... or we simply don't
> care anymore for the typedef rule ?

We should maybe limit the recommendation for the typedefs to things that
we mainly need in common code and that also fit into
include/qemu/typedefs.h nicely. If we agree on that, I could send an
update for the HACKING file.

> All these forward declarations could be typedefs in a "hw/ppc/spapr_types.h"
> header as well, as suggested elsewhere by Daniel.

I'd prefer to rather get rid of the typedefs in this case instead of
introducing spapr_types.h ... but if other ppc folks are also keen on
that file (David?), I can rework my patch to introduce it.

>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 9e01a5a..10d069e 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -12,11 +12,12 @@
>>  struct VIOsPAPRBus;
>>  struct sPAPRPHBState;
>>  struct sPAPRNVRAM;
>> +struct ICSState;
>> +struct sPAPRXive;
>> +
>>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
>>  typedef struct sPAPREventSource sPAPREventSource;
>>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
>> -typedef struct ICSState ICSState;
> 
> Thanks to the previous patch, I guess the ICSState type could be
> obtained by including "hw/ppc/xics.h".

There are already plenty of other struct forward declarations without
typedefs here, so I assume my changes are ok here. David?

>> -typedef struct sPAPRXive sPAPRXive;
>>  
>>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
>>  #define SPAPR_ENTRY_POINT       0x100
>> @@ -127,7 +128,7 @@ struct sPAPRMachineState {
>>      struct VIOsPAPRBus *vio_bus;
>>      QLIST_HEAD(, sPAPRPHBState) phbs;
>>      struct sPAPRNVRAM *nvram;
>> -    ICSState *ics;
>> +    struct ICSState *ics;
>>      sPAPRRTCState rtc;
>>  
>>      sPAPRResizeHPT resize_hpt;
>> @@ -180,7 +181,7 @@ struct sPAPRMachineState {
>>      const char *icp_type;
>>      int32_t irq_map_nr;
>>      unsigned long *irq_map;
>> -    sPAPRXive  *xive;
>> +    struct sPAPRXive  *xive;
>>      sPAPRIrq *irq;
>>      qemu_irq *qirqs;
[...]
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index 486abaf..a62ff60 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -1177,8 +1177,9 @@ do {                                            \
>>  
>>  typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
>>  typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
>> -typedef struct XiveTCTX XiveTCTX;
>> -typedef struct ICPState ICPState;
>> +
>> +struct XiveTCTX;
>> +struct ICPState;
> 
> These could be made available from the XICS/XIVE header files.
> 
> #ifndef CONFIG_USER_ONLY
> #include "hw/ppc/xive.h" /* for XiveTCTX */
> #include "hw/ppc/xics.h" /* for ICPState */
> #endif

Ok, I can change it if we agree that normal struct forward declarations
are a no-go. Otherwise, I'd prefer the non-typedeffed struct forward
declarations here, I think.

 Thomas

Re: [Qemu-devel] [PATCH v4 2/3] ppc: Drop duplicated typedefs to be able to compile with Clang in gnu99 mode

[Greg Kurz]

On Thu, 10 Jan 2019 15:07:59 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 2019-01-10 14:15, Greg Kurz wrote:
> > On Thu, 10 Jan 2019 10:15:35 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> When compiling the ppc code with clang and -std=gnu99, there are a
> >> couple of warnings/errors like this one:
> >>
> >>   CC      ppc64-softmmu/hw/intc/xics.o
> >> In file included from /home/thuth/devel/qemu/hw/intc/xics.c:35:
> >> /home/thuth/devel/qemu/include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
> >>       [-Werror,-Wtypedef-redefinition]
> >> typedef struct ICPState ICPState;
> >>                         ^
> >> /home/thuth/devel/qemu/target/ppc/cpu.h:1181:25: note: previous definition is here
> >> typedef struct ICPState ICPState;
> >>                         ^
> >>
> >> Drop the duplicated typedefs and use normal "struct" forward declarations
> >> like we already do it at the top of spapr.h for a couple of other definitions.
> >>  
> > 
> > Hmm... so the choice here is to simply ignore the official coding
> > style ?  
> 
> Are typedefs really our "official coding style"? It's mentioned in
> HACKING, not in CODING_STYLE, so I rather see this as a recommendation

Indeed.

> only. (Otherwise, all the forward struct definitions at the beginning of
> spapr.h are a plain violation of the coding style, too...)
> 

Yeah.

> IMHO we should rather adopt the coding style of the kernel which rather
> tries to avoid to typedef each and every struct.
> 

From https://www.kernel.org/doc/html/latest/process/coding-style.html#typedefs :

"In general, a pointer, or a struct that has elements that can reasonably be
 directly accessed should never be a typedef."

So if we were to adopt the coding style of the kernel, we'd have to face a
lot more violations with the current QEMU code base :)

> > It is a bit confusing to end up with even more struct/non-struct
> > inconsistency. It would be good at least to update HACKING so that
> > people know when they can legitimately do that... or we simply don't
> > care anymore for the typedef rule ?  
> 
> We should maybe limit the recommendation for the typedefs to things that
> we mainly need in common code and that also fit into
> include/qemu/typedefs.h nicely. If we agree on that, I could send an
> update for the HACKING file.
> 

Makes sense.

> > All these forward declarations could be typedefs in a "hw/ppc/spapr_types.h"
> > header as well, as suggested elsewhere by Daniel.  
> 
> I'd prefer to rather get rid of the typedefs in this case instead of
> introducing spapr_types.h ... but if other ppc folks are also keen on
> that file (David?), I can rework my patch to introduce it.
> 
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index 9e01a5a..10d069e 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -12,11 +12,12 @@
> >>  struct VIOsPAPRBus;
> >>  struct sPAPRPHBState;
> >>  struct sPAPRNVRAM;
> >> +struct ICSState;
> >> +struct sPAPRXive;
> >> +
> >>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
> >>  typedef struct sPAPREventSource sPAPREventSource;
> >>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> >> -typedef struct ICSState ICSState;  
> > 
> > Thanks to the previous patch, I guess the ICSState type could be
> > obtained by including "hw/ppc/xics.h".  
> 
> There are already plenty of other struct forward declarations without
> typedefs here, so I assume my changes are ok here. David?
> 
> >> -typedef struct sPAPRXive sPAPRXive;
> >>  
> >>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
> >>  #define SPAPR_ENTRY_POINT       0x100
> >> @@ -127,7 +128,7 @@ struct sPAPRMachineState {
> >>      struct VIOsPAPRBus *vio_bus;
> >>      QLIST_HEAD(, sPAPRPHBState) phbs;
> >>      struct sPAPRNVRAM *nvram;
> >> -    ICSState *ics;
> >> +    struct ICSState *ics;
> >>      sPAPRRTCState rtc;
> >>  
> >>      sPAPRResizeHPT resize_hpt;
> >> @@ -180,7 +181,7 @@ struct sPAPRMachineState {
> >>      const char *icp_type;
> >>      int32_t irq_map_nr;
> >>      unsigned long *irq_map;
> >> -    sPAPRXive  *xive;
> >> +    struct sPAPRXive  *xive;
> >>      sPAPRIrq *irq;
> >>      qemu_irq *qirqs;  
> [...]
> >> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> >> index 486abaf..a62ff60 100644
> >> --- a/target/ppc/cpu.h
> >> +++ b/target/ppc/cpu.h
> >> @@ -1177,8 +1177,9 @@ do {                                            \
> >>  
> >>  typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
> >>  typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
> >> -typedef struct XiveTCTX XiveTCTX;
> >> -typedef struct ICPState ICPState;
> >> +
> >> +struct XiveTCTX;
> >> +struct ICPState;  
> > 
> > These could be made available from the XICS/XIVE header files.
> > 
> > #ifndef CONFIG_USER_ONLY
> > #include "hw/ppc/xive.h" /* for XiveTCTX */
> > #include "hw/ppc/xics.h" /* for ICPState */
> > #endif  
> 
> Ok, I can change it if we agree that normal struct forward declarations
> are a no-go. Otherwise, I'd prefer the non-typedeffed struct forward
> declarations here, I think.
> 

Sure.

>  Thomas

Re: [Qemu-devel] [PATCH v4 2/3] ppc: Drop duplicated typedefs to be able to compile with Clang in gnu99 mode

[Paolo Bonzini]

On 10/01/19 17:00, Greg Kurz wrote:
>>> Hmm... so the choice here is to simply ignore the official coding
>>> style ?  
>>
>> Are typedefs really our "official coding style"? It's mentioned in
>> HACKING, not in CODING_STYLE, so I rather see this as a recommendation
> 
> Indeed.
> 
>> only. (Otherwise, all the forward struct definitions at the beginning of
>> spapr.h are a plain violation of the coding style, too...)
>>
> 
> Yeah.
> 
>> IMHO we should rather adopt the coding style of the kernel which rather
>> tries to avoid to typedef each and every struct.
>>
> 
> From https://www.kernel.org/doc/html/latest/process/coding-style.html#typedefs :
> 
> "In general, a pointer, or a struct that has elements that can reasonably be
>  directly accessed should never be a typedef."
> 
> So if we were to adopt the coding style of the kernel, we'd have to face a
> lot more violations with the current QEMU code base :)

Yes, that's just not going to happen.  QEMU has a different coding
style; it's used consistently(*) and we'll live with it. I do like the
kernel style too as well, but it's just too different for a transition
to make sense and it may make even less sense if QEMU ever becomes
multi-language.  To put it clearly, this is not multiline comments.

It's a different thing to allow struct in prototypes, that's a special
case that already exists and can be adopted only if necessary.

Paolo

    (*) camel-case type identifiers are also shared with GLib and many
        other libraries we use, and they are pretty much a universal
        convention in every language but C and C++.

Re: [Qemu-devel] [PATCH v4 2/3] ppc: Drop duplicated typedefs to be able to compile with Clang in gnu99 mode

[David Gibson]

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

On Thu, Jan 10, 2019 at 03:07:59PM +0100, Thomas Huth wrote:
> On 2019-01-10 14:15, Greg Kurz wrote:
> > On Thu, 10 Jan 2019 10:15:35 +0100
> > Thomas Huth <thuth@redhat.com> wrote:
> > 
> >> When compiling the ppc code with clang and -std=gnu99, there are a
> >> couple of warnings/errors like this one:
> >>
> >>   CC      ppc64-softmmu/hw/intc/xics.o
> >> In file included from /home/thuth/devel/qemu/hw/intc/xics.c:35:
> >> /home/thuth/devel/qemu/include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
> >>       [-Werror,-Wtypedef-redefinition]
> >> typedef struct ICPState ICPState;
> >>                         ^
> >> /home/thuth/devel/qemu/target/ppc/cpu.h:1181:25: note: previous definition is here
> >> typedef struct ICPState ICPState;
> >>                         ^
> >>
> >> Drop the duplicated typedefs and use normal "struct" forward declarations
> >> like we already do it at the top of spapr.h for a couple of other definitions.
> >>
> > 
> > Hmm... so the choice here is to simply ignore the official coding
> > style ?
> 
> Are typedefs really our "official coding style"? It's mentioned in
> HACKING, not in CODING_STYLE, so I rather see this as a recommendation
> only. (Otherwise, all the forward struct definitions at the beginning of
> spapr.h are a plain violation of the coding style, too...)

I'd say it's definitely qemu coding style in practice, whatever you
argue about the wording in the docs.

> IMHO we should rather adopt the coding style of the kernel which rather
> tries to avoid to typedef each and every struct.
> 
> > It is a bit confusing to end up with even more struct/non-struct
> > inconsistency. It would be good at least to update HACKING so that
> > people know when they can legitimately do that... or we simply don't
> > care anymore for the typedef rule ?
> 
> We should maybe limit the recommendation for the typedefs to things that
> we mainly need in common code and that also fit into
> include/qemu/typedefs.h nicely. If we agree on that, I could send an
> update for the HACKING file.
> 
> > All these forward declarations could be typedefs in a "hw/ppc/spapr_types.h"
> > header as well, as suggested elsewhere by Daniel.
> 
> I'd prefer to rather get rid of the typedefs in this case instead of
> introducing spapr_types.h ... but if other ppc folks are also keen on
> that file (David?), I can rework my patch to introduce it.
> 
> >> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> >> index 9e01a5a..10d069e 100644
> >> --- a/include/hw/ppc/spapr.h
> >> +++ b/include/hw/ppc/spapr.h
> >> @@ -12,11 +12,12 @@
> >>  struct VIOsPAPRBus;
> >>  struct sPAPRPHBState;
> >>  struct sPAPRNVRAM;
> >> +struct ICSState;
> >> +struct sPAPRXive;
> >> +
> >>  typedef struct sPAPREventLogEntry sPAPREventLogEntry;
> >>  typedef struct sPAPREventSource sPAPREventSource;
> >>  typedef struct sPAPRPendingHPT sPAPRPendingHPT;
> >> -typedef struct ICSState ICSState;
> > 
> > Thanks to the previous patch, I guess the ICSState type could be
> > obtained by including "hw/ppc/xics.h".
> 
> There are already plenty of other struct forward declarations without
> typedefs here, so I assume my changes are ok here. David?
> 
> >> -typedef struct sPAPRXive sPAPRXive;
> >>  
> >>  #define HPTE64_V_HPTE_DIRTY     0x0000000000000040ULL
> >>  #define SPAPR_ENTRY_POINT       0x100
> >> @@ -127,7 +128,7 @@ struct sPAPRMachineState {
> >>      struct VIOsPAPRBus *vio_bus;
> >>      QLIST_HEAD(, sPAPRPHBState) phbs;
> >>      struct sPAPRNVRAM *nvram;
> >> -    ICSState *ics;
> >> +    struct ICSState *ics;
> >>      sPAPRRTCState rtc;
> >>  
> >>      sPAPRResizeHPT resize_hpt;
> >> @@ -180,7 +181,7 @@ struct sPAPRMachineState {
> >>      const char *icp_type;
> >>      int32_t irq_map_nr;
> >>      unsigned long *irq_map;
> >> -    sPAPRXive  *xive;
> >> +    struct sPAPRXive  *xive;
> >>      sPAPRIrq *irq;
> >>      qemu_irq *qirqs;
> [...]
> >> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> >> index 486abaf..a62ff60 100644
> >> --- a/target/ppc/cpu.h
> >> +++ b/target/ppc/cpu.h
> >> @@ -1177,8 +1177,9 @@ do {                                            \
> >>  
> >>  typedef struct PPCVirtualHypervisor PPCVirtualHypervisor;
> >>  typedef struct PPCVirtualHypervisorClass PPCVirtualHypervisorClass;
> >> -typedef struct XiveTCTX XiveTCTX;
> >> -typedef struct ICPState ICPState;
> >> +
> >> +struct XiveTCTX;
> >> +struct ICPState;
> > 
> > These could be made available from the XICS/XIVE header files.
> > 
> > #ifndef CONFIG_USER_ONLY
> > #include "hw/ppc/xive.h" /* for XiveTCTX */
> > #include "hw/ppc/xics.h" /* for ICPState */
> > #endif
> 
> Ok, I can change it if we agree that normal struct forward declarations
> are a no-go. Otherwise, I'd prefer the non-typedeffed struct forward
> declarations here, I think.
> 
>  Thomas
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v4 2/3] ppc: Drop duplicated typedefs to be able to compile with Clang in gnu99 mode

[Thomas Huth]

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

On 2019-01-11 01:17, David Gibson wrote:
> On Thu, Jan 10, 2019 at 03:07:59PM +0100, Thomas Huth wrote:
>> On 2019-01-10 14:15, Greg Kurz wrote:
>>> On Thu, 10 Jan 2019 10:15:35 +0100
>>> Thomas Huth <thuth@redhat.com> wrote:
>>>
>>>> When compiling the ppc code with clang and -std=gnu99, there are a
>>>> couple of warnings/errors like this one:
>>>>
>>>>   CC      ppc64-softmmu/hw/intc/xics.o
>>>> In file included from /home/thuth/devel/qemu/hw/intc/xics.c:35:
>>>> /home/thuth/devel/qemu/include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
>>>>       [-Werror,-Wtypedef-redefinition]
>>>> typedef struct ICPState ICPState;
>>>>                         ^
>>>> /home/thuth/devel/qemu/target/ppc/cpu.h:1181:25: note: previous definition is here
>>>> typedef struct ICPState ICPState;
>>>>                         ^
>>>>
>>>> Drop the duplicated typedefs and use normal "struct" forward declarations
>>>> like we already do it at the top of spapr.h for a couple of other definitions.
>>>>
>>>
>>> Hmm... so the choice here is to simply ignore the official coding
>>> style ?
>>
>> Are typedefs really our "official coding style"? It's mentioned in
>> HACKING, not in CODING_STYLE, so I rather see this as a recommendation
>> only. (Otherwise, all the forward struct definitions at the beginning of
>> spapr.h are a plain violation of the coding style, too...)
> 
> I'd say it's definitely qemu coding style in practice, whatever you
> argue about the wording in the docs.

So do I get you right that you rather prefer some additional #include
statements in the headers to pull in the typedef definitions from other
headers (like Greg suggested) than to do some struct forward
declarations without typedefs?
Ok, if that's what most ppc folks prefer, I'll rework this patch
accordingly...

 Thomas


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