[PATCH v2] Use flexible target descriptors for Linux PowerPC

Ulrich Weigand uweigand@de.ibm.com
Thu Jul 9 15:48:02 GMT 2020


On Fri, Jul 03, 2020 at 04:03:21PM -0300, Rogerio Alves wrote:
> Use flexible target descriptors for Linux PowerPC. This allows a fine-tune
> selection of avalible target CPU features por POWER.

Thanks for working on this!

> diff --git a/gdb/arch/ppc-linux-common.c b/gdb/arch/ppc-linux-common.c
> index 183fc03395..36e2319a7f 100644
> --- a/gdb/arch/ppc-linux-common.c
> +++ b/gdb/arch/ppc-linux-common.c
> -const struct target_desc *
> -ppc_linux_match_description (struct ppc_linux_features features)
> +const target_desc *
> +ppc_create_target_description (struct ppc_linux_features features)

So you've renamed it from a name including "linux" to one without,
while the function is still actually Linux-specific.  Is this a
good idea?

[ I guess a separate question is whether we shouldn't also switch
all other powerpc targets to the new method ... ]

>  {
> -  struct target_desc *tdesc = NULL;
> +  target_desc *tdesc = allocate_target_description ();
> +  long regnum = 0;
>  
>    if (features.wordsize == 8)
>      {
> -      if (features.vsx)
> -	tdesc = (features.htm? tdesc_powerpc_isa207_htm_vsx64l
> -		 : features.isa207? tdesc_powerpc_isa207_vsx64l
> -		 : features.ppr_dscr? tdesc_powerpc_isa205_ppr_dscr_vsx64l
> -		 : features.isa205? tdesc_powerpc_isa205_vsx64l
> -		 : tdesc_powerpc_vsx64l);
> -      else if (features.altivec)
> -	tdesc = (features.isa205? tdesc_powerpc_isa205_altivec64l
> -		 : tdesc_powerpc_altivec64l);
> -      else
> -	tdesc = (features.isa205? tdesc_powerpc_isa205_64l
> -		 : tdesc_powerpc_64l);
> +      #ifndef IN_PROCESS_AGENT
> +	set_tdesc_architecture (tdesc, "power:common64");

Do we need to call set_tdesc_osabi here like is done for other targets?

> index cc65baa6ed..3140eeb00c 100644
> --- a/gdb/features/Makefile
> +++ b/gdb/features/Makefile
> @@ -51,17 +51,6 @@ WHICH = arm/arm-with-iwmmxt arm/arm-with-vfpv2 arm/arm-with-vfpv3 \
>  	mips64-linux mips64-dsp-linux \
>  	nios2-linux \
>  	rs6000/powerpc-32 \

It seems to me that this file is actually unused as well, so we can really
remove all the rs6000 regformats files then.  (Including the special Makefile
rule associated with them.)

Talking about removed regformats files: did you verify that the new code still
uses the same layout of the register packets, e.g. by running a new gdb against
an old gdbserver and vice versa?

> diff --git a/gdbserver/configure.srv b/gdbserver/configure.srv
> index 9a027e44af..0ff094d32c 100644
> --- a/gdbserver/configure.srv
> +++ b/gdbserver/configure.srv

> -  powerpc*-*-linux*)	srv_regobj="powerpc-32l.o"

Should we now use
  srv_regobj=""
like the other targets do?

>  			ipa_obj="${ipa_ppc_linux_regobj} linux-ppc-ipa.o"
> +			ipa_obj="${ipa_obj} linux-ppc-tdesc-ipa.o"
> +			ipa_obj="${ipa_obj} arch/ppc-linux-common-ipa.o"

I believe we now no longer need ${ipa_ppc_linux_regobj}, right?

> --- a/gdbserver/linux-ppc-ipa.cc
> +++ b/gdbserver/linux-ppc-ipa.cc
> @@ -22,7 +22,6 @@
>  #include <sys/mman.h>
>  #include "tracepoint.h"
>  #include "arch/ppc-linux-tdesc.h"

Wasn't that file deleted?  How does this still build?  Or am I
missing something here ...

> @@ -174,58 +173,12 @@ alloc_jump_pad_buffer (size_t size)
>  const struct target_desc *
>  get_ipa_tdesc (int idx)
>  {
> -  switch (idx)
> -    {
> -#ifdef __powerpc64__
> -    case PPC_TDESC_BASE:
> -      return tdesc_powerpc_64l;
> -    case PPC_TDESC_ALTIVEC:
> -      return tdesc_powerpc_altivec64l;
> -    case PPC_TDESC_VSX:
> -      return tdesc_powerpc_vsx64l;
> -    case PPC_TDESC_ISA205:
> -      return tdesc_powerpc_isa205_64l;
> -    case PPC_TDESC_ISA205_ALTIVEC:
> -      return tdesc_powerpc_isa205_altivec64l;
> -    case PPC_TDESC_ISA205_VSX:
> -      return tdesc_powerpc_isa205_vsx64l;
> -    case PPC_TDESC_ISA205_PPR_DSCR_VSX:
> -      return tdesc_powerpc_isa205_ppr_dscr_vsx64l;
> -    case PPC_TDESC_ISA207_VSX:
> -      return tdesc_powerpc_isa207_vsx64l;
> -    case PPC_TDESC_ISA207_HTM_VSX:
> -      return tdesc_powerpc_isa207_htm_vsx64l;
> -#else
> -    case PPC_TDESC_BASE:
> -      return tdesc_powerpc_32l;
> -    case PPC_TDESC_ALTIVEC:
> -      return tdesc_powerpc_altivec32l;
> -    case PPC_TDESC_VSX:
> -      return tdesc_powerpc_vsx32l;
> -    case PPC_TDESC_ISA205:
> -      return tdesc_powerpc_isa205_32l;
> -    case PPC_TDESC_ISA205_ALTIVEC:
> -      return tdesc_powerpc_isa205_altivec32l;
> -    case PPC_TDESC_ISA205_VSX:
> -      return tdesc_powerpc_isa205_vsx32l;
> -    case PPC_TDESC_ISA205_PPR_DSCR_VSX:
> -      return tdesc_powerpc_isa205_ppr_dscr_vsx32l;
> -    case PPC_TDESC_ISA207_VSX:
> -      return tdesc_powerpc_isa207_vsx32l;
> -    case PPC_TDESC_ISA207_HTM_VSX:
> -      return tdesc_powerpc_isa207_htm_vsx32l;
> -    case PPC_TDESC_E500:
> -      return tdesc_powerpc_e500l;
> -#endif
> -    default:
> -      internal_error (__FILE__, __LINE__,
> -                     "unknown ipa tdesc index: %d", idx);
> -#ifdef __powerpc64__
> -      return tdesc_powerpc_64l;
> -#else
> -      return tdesc_powerpc_32l;
> -#endif
> -    }
> +  bool is_ppc64 = false;
> +  #ifdef __powerpc64__
> +    is_ppc64 = true;
> +  #endif
> +
> +  return ppc_linux_read_description (is_ppc64);

Something seems wrong here.  If I'm reading this correctly,
that description will only have the bare minimum register set,
with all optional features switched off.  The original code
used the full register set available on the current machine.

> @@ -234,26 +187,11 @@ get_ipa_tdesc (int idx)
>  void
>  initialize_low_tracepoint (void)
>  {
> -#ifdef __powerpc64__
> -  init_registers_powerpc_64l ();
> -  init_registers_powerpc_altivec64l ();
> -  init_registers_powerpc_vsx64l ();
> -  init_registers_powerpc_isa205_64l ();
> -  init_registers_powerpc_isa205_altivec64l ();
> -  init_registers_powerpc_isa205_vsx64l ();
> -  init_registers_powerpc_isa205_ppr_dscr_vsx64l ();
> -  init_registers_powerpc_isa207_vsx64l ();
> -  init_registers_powerpc_isa207_htm_vsx64l ();
> -#else
> -  init_registers_powerpc_32l ();
> -  init_registers_powerpc_altivec32l ();
> -  init_registers_powerpc_vsx32l ();
> -  init_registers_powerpc_isa205_32l ();
> -  init_registers_powerpc_isa205_altivec32l ();
> -  init_registers_powerpc_isa205_vsx32l ();
> -  init_registers_powerpc_isa205_ppr_dscr_vsx32l ();
> -  init_registers_powerpc_isa207_vsx32l ();
> -  init_registers_powerpc_isa207_htm_vsx32l ();
> -  init_registers_powerpc_e500l ();
> -#endif
> +
> +  bool is_ppc64 = false;
> +  #ifdef __powerpc64__
> +    is_ppc64 = true;
> +  #endif
> +
> +  ppc_linux_read_description (is_ppc64)

Why do you call that routine and ignore it's result?
Isn't that a no-op here?

> diff --git a/gdbserver/linux-ppc-low.cc b/gdbserver/linux-ppc-low.cc
> index 337d555aee..b982be3e1b 100644
> --- a/gdbserver/linux-ppc-low.cc
> +++ b/gdbserver/linux-ppc-low.cc
> @@ -29,7 +29,6 @@
>  #include "arch/ppc-linux-tdesc.h"

Same question as above: wasn't that file deleted?

> diff --git a/gdbserver/linux-ppc-tdesc.cc b/gdbserver/linux-ppc-tdesc.cc
> new file mode 100644
> index 0000000000..3bf97d20fc

> +#include <inttypes.h>

What is this needed for?

> +/* All possible ppc target descriptors.  */
> +struct target_desc *tdesc_ppc_list[2];

See above, I don't believe it is correct to only
support two variants here.

> +/* Create the Power target description.  */
> +const target_desc *
> +ppc_linux_read_description (bool is_ppc64)
> +{
> +  struct target_desc *tdesc = tdesc_ppc_list[is_ppc64];
> +
> +  if (tdesc == NULL)
> +   {
> +      struct ppc_linux_features features;
> +      features.wordsize = (is_ppc64) ? 8 : 4;
> +      *tdesc = ppc_create_target_description (features);
> +
> +      static const char* expedite_regs[] = { "r1", "pc" };
> +      init_target_desc (tdesc, expedite_regs);
> +
> +      tdesc_ppc_list[is_pp64] = tdesc;
> +   }
> +
> +  return tdesc;
> +}

You're using this only in the -ipa code.  I don't think
it makes sense to have a separate file just for that.

On some other platforms that do have this file, they're
using it both from the linux-low code and the -ipa code,
but you're directly calling ppc_create_target_description
there.

However, it does look like you're missing the call to
init_target_desc in your linux-low code, so we don't
actually get expedite_regs set, which is a regression.

Bye,
Ulrich


More information about the Gdb-patches mailing list