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

[Ulrich Weigand <uweigand@de.ibm.com>]

On Wed, Sep 09, 2020 at 05:19:49PM -0300, Rogerio Alves wrote:
> On 7/9/20 12:48 PM, Ulrich Weigand wrote:
> > On Fri, Jul 03, 2020 at 04:03:21PM -0300, Rogerio Alves wrote:
> >> -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.

OK, thanks.

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

It's certainly fine to do Linux first.  Just something to think about.

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

OK.

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

Yes, we still need the XML files, but this rule is about the *.dat files
in regformats, and I believe the powerpc-32.dat file is actually unused.
These files are only used by gdbserver, and there is no gdbserver target
that uses powerpc-32.o as far as I can see.  I believe I just forgot to
remove that rule years ago.

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

That's actually good question, I don't think there's a simple way to
perform this check.  One option to check, as I mentioned, would be
to run the GDB test suite running a GDB built with the patch against
a gdbserver built without the patch, and vice versa, and making sure
there are no additional failures.

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

I was just confused why some targets explicitly set srv_regobj to
the empty string, while others do not set it at all.  But I guess
given that other Linux targets also do not set it at all, that
should be fine.

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

OK.

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

OK.

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

Well, I believe even with fast tracepoints the user can specify which
registers he wants to collect, so I do think the full set should be
available.  That's what other target do as well, anyway.


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

OK.

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

OK.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com