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

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

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