Re: [PATCH] gdb/riscv: select rv32 target by default when requested

[Andrew Burgess <andrew.burgess@embecosm.com>]

Simon,

I know you're pretty busy right now.  I'm planning to push this commit
in the next couple of days.

You queries were all about whether we could/should extend the tests to
be run in more cases.  I still don't think this is would work, but if
when you have time you can explain why I'm wrong I'll be happy to
update the test case.

Thanks,
Andrew


* Andrew Burgess <andrew.burgess@embecosm.com> [2021-02-08 10:31:16 +0000]:

> * Simon Marchi <simon.marchi@polymtl.ca> [2021-02-05 11:23:21 -0500]:
> 
> > On 2021-02-04 3:44 p.m., Andrew Burgess wrote:
> > > GDB for RISC-V always uses target descriptions.  When the target
> > > doesn't provide a target description then a default is selected.
> > > Usually this default is selected based on the properties of the
> > > executable being debugged.  However, when there is no executable being
> > > debugged we currently fallback to the riscv:rv64 target description as
> > > the default.  This leads to strange behaviour like this:
> > > 
> > >   $ gdb
> > >   (gdb) set architecture riscv:rv32
> > >   (gdb) p sizeof ($pc)
> > >   $1 = 8
> > > 
> > > Despite the users specifically setting the architecture to riscv:rv32
> > > GDB still thinks that the target has riscv:rv64 register sizes.
> > > 
> > > The above is a bit of a contrived situation.  I actually ran into this
> > > situation while trying to connect to a running riscv:rv32 target
> > > without supplying an executable (the target didn't provide a target
> > > description).  When I tried to set a register on the target I ran into
> > > errors because GDB was passing 8 bytes to the target rather than the
> > > expected 4.  Even when I manually specified the architecture (as
> > > above) I couldn't convince GDB to only send 4 bytes.
> > > 
> > > This patch fixes this issue.  Now, when we selected a default target
> > > description we will make use of the user selected architecture to
> > > guide our choice.  In the above example we now get:
> > > 
> > >   $ gdb
> > >   (gdb) set architecture riscv:rv32
> > >   (gdb) p sizeof ($pc)
> > >   $1 = 4
> > > 
> > > And my real world example of connecting to a remote without an
> > > executable works fine.
> > > 
> > > I've used the fact that we can ask GDB about $pc even when no
> > > executable is loaded as the basis for a test to cover this situation.
> > > 
> > > gdb/ChangeLog:
> > > 
> > > 	* riscv-tdep.c (riscv_features_from_gdbarch_info): Rename to...
> > > 	(riscv_features_from_bfd): ...this.  Change parameter type to
> > > 	'bfd*', and update as required.
> > > 	(riscv_find_default_target_description): Update call to
> > > 	riscv_features_from_bfd.  Select a default xlen based on
> > > 	info.bfd_arch_info.
> > > 	(riscv_gdbarch_init): Update call to riscv_features_from_bfd.
> > > 
> > > gdb/testsuite/ChangeLog:
> > > 
> > > 	* gdb.arch/riscv-default-tdesc.exp: New file.
> > > ---
> > >  gdb/ChangeLog                                 | 10 ++++
> > >  gdb/riscv-tdep.c                              | 28 ++++-----
> > >  gdb/testsuite/ChangeLog                       |  4 ++
> > >  .../gdb.arch/riscv-default-tdesc.exp          | 59 +++++++++++++++++++
> > >  4 files changed, 86 insertions(+), 15 deletions(-)
> > >  create mode 100644 gdb/testsuite/gdb.arch/riscv-default-tdesc.exp
> > > 
> > > diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> > > index 460746a9bfe..2a7b792893d 100644
> > > --- a/gdb/riscv-tdep.c
> > > +++ b/gdb/riscv-tdep.c
> > > @@ -3215,13 +3215,11 @@ static const struct frame_unwind riscv_frame_unwind =
> > >    /*.prev_arch     =*/ NULL,
> > >  };
> > >  
> > > -/* Extract a set of required target features out of INFO, specifically the
> > > -   bfd being executed is examined to see what target features it requires.
> > > -   IF there is no current bfd, or the bfd doesn't indicate any useful
> > > -   features then a RISCV_GDBARCH_FEATURES is returned in its default state.  */
> > > +/* Extract a set of required target features out of ABFD.  If ABFD is
> > > +   nullptr then a RISCV_GDBARCH_FEATURES is returned in its default state.  */
> > >  
> > >  static struct riscv_gdbarch_features
> > > -riscv_features_from_gdbarch_info (const struct gdbarch_info info)
> > > +riscv_features_from_bfd (const bfd *abfd)
> > >  {
> > >    struct riscv_gdbarch_features features;
> > >  
> > > @@ -3231,11 +3229,10 @@ riscv_features_from_gdbarch_info (const struct gdbarch_info info)
> > >       only used at all if the target hasn't given us a description, so this
> > >       is really a last ditched effort to do something sane before giving
> > >       up.  */
> > > -  if (info.abfd != NULL
> > > -      && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour)
> > > +  if (abfd != nullptr && bfd_get_flavour (abfd) == bfd_target_elf_flavour)
> > >      {
> > > -      unsigned char eclass = elf_elfheader (info.abfd)->e_ident[EI_CLASS];
> > > -      int e_flags = elf_elfheader (info.abfd)->e_flags;
> > > +      unsigned char eclass = elf_elfheader (abfd)->e_ident[EI_CLASS];
> > > +      int e_flags = elf_elfheader (abfd)->e_flags;
> > >  
> > >        if (eclass == ELFCLASS32)
> > >  	features.xlen = 4;
> > > @@ -3273,13 +3270,14 @@ riscv_find_default_target_description (const struct gdbarch_info info)
> > >  {
> > >    /* Extract desired feature set from INFO.  */
> > >    struct riscv_gdbarch_features features
> > > -    = riscv_features_from_gdbarch_info (info);
> > > +    = riscv_features_from_bfd (info.abfd);
> > >  
> > > -  /* If the XLEN field is still 0 then we got nothing useful from INFO.  In
> > > -     this case we fall back to a minimal useful target, 8-byte x-registers,
> > > -     with no floating point.  */
> > > +  /* If the XLEN field is still 0 then we got nothing useful from INFO.BFD,
> > > +     maybe there was no bfd object.  In this case we fall back to a minimal
> > > +     useful target with no floating point, the x-register size is selected
> > > +     based on the architecture from INFO.  */
> > >    if (features.xlen == 0)
> > > -    features.xlen = 8;
> > > +    features.xlen = info.bfd_arch_info->bits_per_word == 32 ? 4 : 8;
> > >  
> > >    /* Now build a target description based on the feature set.  */
> > >    return riscv_lookup_target_description (features);
> > > @@ -3497,7 +3495,7 @@ riscv_gdbarch_init (struct gdbarch_info info,
> > >       target, then check that this matches with what the target is
> > >       providing.  */
> > >    struct riscv_gdbarch_features abi_features
> > > -    = riscv_features_from_gdbarch_info (info);
> > > +    = riscv_features_from_bfd (info.abfd);
> > >  
> > >    /* If the ABI_FEATURES xlen is 0 then this indicates we got no useful abi
> > >       features from the INFO object.  In this case we just treat the
> > > diff --git a/gdb/testsuite/gdb.arch/riscv-default-tdesc.exp b/gdb/testsuite/gdb.arch/riscv-default-tdesc.exp
> > > new file mode 100644
> > > index 00000000000..2b7e8fa63e8
> > > --- /dev/null
> > > +++ b/gdb/testsuite/gdb.arch/riscv-default-tdesc.exp
> > > @@ -0,0 +1,59 @@
> > > +# Copyright 2021 Free Software Foundation, Inc.
> > > +#
> > > +# This program is free software; you can redistribute it and/or modify
> > > +# it under the terms of the GNU General Public License as published by
> > > +# the Free Software Foundation; either version 3 of the License, or
> > > +# (at your option) any later version.
> > > +#
> > > +# This program is distributed in the hope that it will be useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > +# GNU General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU General Public License
> > > +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > > +
> > > +# Check the size of the $pc register as the user changes the selected
> > > +# architecture.  As no executable is provided then the size of the $pc
> > > +# register is defined by the default target description selected by
> > > +# GDB.
> > > +#
> > > +# This test ensures that GDB is selecting an RV32 default if the user
> > > +# has forced the current architecture to be riscv:rv32.
> > > +#
> > > +# In all other cases the default will be RV64.
> > > +
> > > +if {![istarget "riscv*-*-*"]} {
> > > +    verbose "Skipping ${gdb_test_file_name}."
> > > +    return
> > > +}
> > 
> > From what I can see, this test does not run anything, it should be
> > possible to run it even if the target triplet isn't riscv*-*-*, but as
> > long as support for riscv targets was compiled in (for example with
> > --enable-targets=all).  If so, maybe the check could be adjusted to
> > check for that, so that the test would run when I "make check" on my
> > x86-64 all targets build?
> 
> I don't think that would work as the default architecture is unlikely
> to be RISC-V, which is what the test is really about testing.
> 
> I agree with the principle of limiting architecture specific tests,
> but I think this test truly is RISC-V only.
> 
> > 
> > Although...
> > 
> > > +# Start GDB with no executable.
> > > +gdb_start
> > > +
> > > +# The tests are defined by a list of architecture names to switch too
> > > +# and the expected size of $pc.  The first list entry is special and
> > > +# has an empty architecture string, this checks GDB's default value on
> > > +# startup.
> > > +foreach data {{"" 8} {"riscv:rv32" 4} {"riscv:rv64" 8} {"riscv" 8} \
> > > +		  {"auto" 8}} {
> > > +    set arch [lindex $data 0]
> > > +    set size [lindex $data 1]
> > > +
> > > +    # Switch architecture.
> > > +    if { $arch != "" && $arch != "auto" } {
> > > +	gdb_test "set architecture $arch" \
> > > +	    "The target architecture is set to \"$arch\"\\."
> > > +    } elseif { $arch == "auto" } {
> > > +	gdb_test "set architecture $arch" \
> > > +	    "The target architecture is set to \"auto\" \\(currently \"riscv\"\\)\\."
> > > +    } else {
> > > +	set arch "default architecture"
> > > +    }
> > > +
> > > +    # Check the size of $pc.
> > > +    with_test_prefix "$arch" {
> > > +	gdb_test "p sizeof (\$pc)" " = $size" \
> > > +	    "size of \$pc register is $size"
> > > +    }
> > > +}
> > > 
> > 
> > I don't really understand how the test is supposed to work, how can you
> > read the $pc if you don't execute any program?
> 
> We're not reading the register, just its size, which only requires GDB
> to have a valid gdbarch, which is always the case.
> 
> As part of this test is specifically about what is the default RISC-V
> architecture if I waited until I'd passed an executable before asking
> then GDB would already have adapted to take on a gdbarch matching the
> executable.
> 
> The same basically works for x86-64 and all other architectures that I
> tested, with one caveat, $pc handling for x86-64 is a little odd, I
> guess because the register isn't really called $pc.  As a result it
> seems that we can only ask about $pc on x86-64 once the target has
> started.  But you can ask about any real register before starting, so:
> 
>   $ gdb
>   (gdb) p sizeof ($eip)
>   $1 = 4
>   (gdb)
> 
> Works fine.
> 
>