Re: [PATCH v2 7/7] target/ppc: wrapped some TCG only logic with ifdefs

[Bruno Piazera Larsen]

On 18/05/2021 23:02, David Gibson wrote:

> On Tue, May 18, 2021 at 12:05:15PM -0300, Bruno Larsen (billionai) wrote:
> > Wrapped some function calls in cpu_init.c, gdbstub.c, mmu-hash64.c and
> > excp_helper.c that were TCG only with ifdef CONFIG_TCG, to support
> > building without TCG.
> >
> > for excp_helper we also moved the function do_rfi higher in the file to
> > reduce the ifdef count.
> >       
> The description's no longer really accurate since some of the fixups
> are no longer ifdef based.
>     
    Sure, will do
> <snip>
> >                           0x00000000);
> >          }
> >      }
> > @@ -8605,11 +8603,13 @@ static void ppc_cpu_realize(DeviceState *dev, Error **errp)
> >          }
> >      }
> >  
> > +#ifdef CONFIG_TCG
> >      create_ppc_opcodes(cpu, &local_err);
> >      if (local_err != NULL) {
> >          error_propagate(errp, local_err);
> >          goto unrealize;
> >      }
> > +#endif
> >       
> In this instance, I think it would be cleaner to create a no-op stub
> for create_ppc_opcodes() and destroy_ppc_opcodes() rather than using
> ifdefs.
>     
    Ok. will add the stubs in tcg-stubs.c
> >      init_ppc_proc(cpu);
> >  
> >      ppc_gdb_init(cs, pcc);
> > @@ -8798,7 +8798,9 @@ static void ppc_cpu_unrealize(DeviceState *dev)
> >  
> >      cpu_remove_sync(CPU(cpu));
> >  
> > +#ifdef CONFIG_TCG
> >      destroy_ppc_opcodes(cpu);
> > +#endif
> >  }
> >  
> >  static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
> > @@ -9296,7 +9298,9 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
> >      cc->class_by_name = ppc_cpu_class_by_name;
> >      cc->has_work = ppc_cpu_has_work;
> >      cc->dump_state = ppc_cpu_dump_state;
> > +#ifdef CONFIG_TCG
> >      cc->dump_statistics = ppc_cpu_dump_statistics;
> > +#endif
> >      cc->set_pc = ppc_cpu_set_pc;
> >      cc->gdb_read_register = ppc_cpu_gdb_read_register;
> >      cc->gdb_write_register = ppc_cpu_gdb_write_register;
> > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> > index 80bb6e70e9..a14b529722 100644
> > --- a/target/ppc/excp_helper.c
> > +++ b/target/ppc/excp_helper.c
> > @@ -19,9 +19,13 @@
> >  #include "qemu/osdep.h"
> >  #include "qemu/main-loop.h"
> >  #include "cpu.h"
> > +#ifdef CONFIG_TCG
> >  #include "exec/helper-proto.h"
> > +#endif
> >       
> I don't like the look of ifdefs amongst the includes.  Generally the
> headers themselves should be made safe (if unnecessary) to include for
> !TCG builds.
>

The problems that happen with exec/helper-proto.h and exec/cpu_ldst.h is that they include headers themselves, trace/generated-helpers.h and tcg-target.h respectively, which are in folders that are not included as -iquote in the gcc CLI call.

So the problem is that it is trying to include headers that gcc doesn't see as part of the project anymore. The best option (I think) would be to fix the gcc command generation so headers are safe to include, but since I was very confused with all the scripts related to generating everything, I ended up going with not including the files. Should I change the configure script, so that we can include headers from tcg/ppc? I can also just separate headers that can be ifdef'd away from the rest with a blank line, so it's more visible later

> >  #include "exec/exec-all.h"
> > +#ifdef CONFIG_TCG
> >  #include "exec/cpu_ldst.h"
> > +#endif
> >  #include "internal.h"
> >  #include "helper_regs.h"
> >       
> The remaining ifdef changes look fine.  Some it would be nice to clean
> up better in future, but there's no rush.
>
>

--
Bruno Piazera Larsen
Instituto de Pesquisas ELDORADO
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer