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

[Greg Kurz <groug@kaod.org>]

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 ?)

> > That's a bigger change reverting bits of already merged patches. I can
> > take care of it if you prefer.   
> 
> Could I keep the current patch in my series so that I can get the
> patches finally merged? You could then do any clean up that you like on
> top of it, ok?
> 
> > I use a f29 for dev. Which compiler should I install ?   
> 
> Any version of Clang with -std=gnu99 should do the job here, I think.
> 
>  Thomas