Re: [PATCH v2] Use flexible target descriptors for Linux PowerPC

[Rogerio Alves <rcardoso@linux.ibm.com>]

Sorry for take me that long to reply this message. I was busy with other 
GDB tasks and I missed that reply

On 7/9/20 12:48 PM, Ulrich Weigand wrote:
> 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've renamed this only because the others architectures uses something
like <arch>_create_target_description. But in fact this is linux specfic 
so I will change it to ppc_linux_create_target_description.

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

Not sure about the other power targets for now I only targeted linux.
If that's the case maybe I should sent a separated patch if you agree 
with that.

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

Yes. Since it's linux I should call set_tdesc_osbi here. [Not sure why 
aarch64 did not call it]

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

In that case we have a rs6000-tdep files on top folder that use this 
regformats that is why I did not remove. I've only remove the rules for 
rs6000/powerpc-*l* (linux) since my patch only target Linux for now.
In fact it's is kinda confusing to me why we have those rs6000?

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

I am checking it here. Since I am new on GDB I am not sure how to check 
this layout.

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

With the new interface we should not use srv_regobj. Only the old 
interfaces use it. Targets like aarch64 and x86_64 do not uses.

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

In fact we don't need. Not sure why I did not removed this.

>> --- 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 ...
> 

Yes. In fact I did forgot to remove those but when I've tested it got a 
build error I removed those and works but I forgot to commit this small 
change before generate the patch. It will be fixed on v3

>> @@ -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.
> 

I was talking to Pedro about it by the time I was working on this patch 
he explains that IPA only colects a few registers and we may not get 
those registers in IPA that's why I did that. Now I am testing using a 
fast tracepoint to see what registers it collects and I may include the 
ones that is missing here.

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

Yes I am ignoring the result in that case because I don't need the tdesc 
returned. read_description calls create description and save that on a 
table. The original intend was to call that function on linux-low too 
(to get the expedite regs):

ppc-linux-low.cc:

current_process ()->tdesc = tdesc;

should be

current_process ()->tdesc = ppc_linux_read_description (features);

But I made a mistake and change the parameter to is_ppc and decide to 
call create_description directly on ppc-linux-low. In fact I have to 
replace = tdesc to ppc_linux_read_description (features) and change the 
parameter from is_ppc64 to features.

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

Yes.

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

I will remove this include. I put that based on other linux-*-tdesc.cc 
files.

>> +/* 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.
>

Ok.

>> +/* 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.
> 

I kinda explain that above when I call ppc_linux_read_description and 
ignore the result. My solution will be change the parameter and call 
ppc_linux_read_description on linux-low instead call create_description 
directly. This should fix this mistake.

> Bye,
> Ulrich
> 

Regards
Rogerio