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

[Thomas Huth]

On 2019-01-09 18:47, Greg Kurz wrote:
> On Wed, 9 Jan 2019 18:40:48 +0100
> Cédric Le Goater <address@hidden> 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 <address@hidden>
>>>>> ---
>>>>>  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