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

[Simon Marchi via Gdb-patches <gdb-patches@sourceware.org>]

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?

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?

Simon