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

[Greg Kurz <groug@kaod.org>]

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