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

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

[Andrew Burgess]

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
+}
+
+# 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"
+    }
+}
-- 
2.25.4

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

[Palmer Dabbelt]

On Thu, 04 Feb 2021 12:44:26 PST (-0800), andrew.burgess@embecosm.com 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

I don't think it's that contrived, it used to happen to me every day ;).  I
just didn't know how to fix it.

Thanks!

> 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
> +}
> +
> +# 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"
> +    }
> +}

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

[Simon Marchi]

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

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

[Andrew Burgess]

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

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

[Andrew Burgess]

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