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

[Thomas Huth <thuth@redhat.com>]

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