| Message ID | 1547051976-13982-2-git-send-email-thuth@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Force the C standard to gnu99 | expand |
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); >
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
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.
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); >
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. >
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
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.
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
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
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
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
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
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);
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(-)