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

[Thomas Huth <thuth@redhat.com>]

On 2019-01-16 14:23, Greg Kurz wrote:
> On Wed, 16 Jan 2019 12:47:36 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 2019-01-16 12:43, Cédric Le Goater wrote:
>>> On 1/11/19 9:17 AM, Thomas Huth 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 hw/intc/xics.c:35:
>>>> include/hw/ppc/xics.h:43:25: error: redefinition of typedef 'ICPState' is a C11 feature
>>>>       [-Werror,-Wtypedef-redefinition]
>>>> typedef struct ICPState ICPState;
>>>>                         ^
>>>> target/ppc/cpu.h:1181:25: note: previous definition is here
>>>> typedef struct ICPState ICPState;
>>>>                         ^
>>>> Work around the problems by including the proper headers instead.  
>>>
>>> Thomas,
>>>
>>>
>>> After a closer look, I think we should use 'void *' under PowerPCCPU 
>>> as it was the case before I introduced the second interrupt presenter.  
>>
>> If you don't like the #includes, why not simply do anonymous struct
>> forward declarations here? I think that would be better than "void *".
>>
> 
> That's questionable. These two fields are only used by the machine code and
> the interrupt controller code.
> 
> $ git grep -E '(icp|tctx)' target/ppc/
> target/ppc/cpu.h:    ICPState *icp;
> target/ppc/cpu.h:    XiveTCTX *tctx;
> 
> $ git grep -E 'cpu\->(icp|tctx)' 
> hw/intc/spapr_xive_kvm.c:        kvmppc_xive_cpu_set_state(cpu->tctx, &local_err);
> hw/intc/spapr_xive_kvm.c:        kvmppc_xive_cpu_connect(cpu->tctx, &local_err);
> hw/intc/xics_kvm.c:        icp_kvm_connect(cpu->icp, &local_err);
> hw/intc/xics_kvm.c:        icp_set_kvm_state(cpu->icp, 1);
> hw/intc/xics_spapr.c:    icp_set_cppr(cpu->icp, cppr);
> hw/intc/xics_spapr.c:    uint32_t xirr = icp_accept(cpu->icp);
> hw/intc/xics_spapr.c:    uint32_t xirr = icp_accept(cpu->icp);
> hw/intc/xics_spapr.c:    icp_eoi(cpu->icp, xirr);
> hw/intc/xics_spapr.c:    uint32_t xirr = icp_ipoll(cpu->icp, &mfrr);
> hw/intc/xive.c:    XiveTCTX *tctx = cpu->tctx;
> hw/intc/xive.c:    XiveTCTX *tctx = cpu->tctx;
> hw/intc/xive.c:        XiveTCTX *tctx = cpu->tctx;
> hw/ppc/pnv.c:    cpu->icp = ICP(obj);
> hw/ppc/pnv.c:    return cpu ? cpu->icp : NULL;
> hw/ppc/pnv.c:        icp_pic_print_info(cpu->icp, mon);
> hw/ppc/pnv_core.c:    object_unparent(OBJECT(cpu->icp));
> hw/ppc/spapr.c:    return cpu ? cpu->icp : NULL;
> hw/ppc/spapr_cpu_core.c:    if (cpu->icp) {
> hw/ppc/spapr_cpu_core.c:        object_unparent(OBJECT(cpu->icp));
> hw/ppc/spapr_cpu_core.c:    if (cpu->tctx) {
> hw/ppc/spapr_cpu_core.c:        object_unparent(OBJECT(cpu->tctx));
> hw/ppc/spapr_irq.c:        icp_pic_print_info(cpu->icp, mon);
> hw/ppc/spapr_irq.c:    cpu->icp = ICP(obj);
> hw/ppc/spapr_irq.c:            icp_resend(cpu->icp);
> hw/ppc/spapr_irq.c:        xive_tctx_pic_print_info(cpu->tctx, mon);
> hw/ppc/spapr_irq.c:    cpu->tctx = XIVE_TCTX(obj);
> hw/ppc/spapr_irq.c:    spapr_xive_set_tctx_os_cam(cpu->tctx);
> hw/ppc/spapr_irq.c:        spapr_xive_set_tctx_os_cam(cpu->tctx);
> hw/ppc/spapr_irq.c:        spapr_xive_set_tctx_os_cam(cpu->tctx);
> 
> It thus looks wrong to expose their type in target/ppc/cpu.h. I guess
> they should be hidden behind an opaque data pointer (maybe the existing
> void *machine_data ?)

While that's maybe cleaner at a quick glance first, you still have to
deal with the fact that it's used by two different machines (pseries and
pnv) ... not sure whether that easily distinguishable in xive.c for example?

 Thomas