diff mbox series

[v2] gdb/arm: Remove tpidruro register from non-FreeBSD target descriptions

Message ID 20240228031042.375726-1-thiago.bauermann@linaro.org
State New
Headers
Series [v2] gdb/arm: Remove tpidruro register from non-FreeBSD target descriptions |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Testing passed

Commit Message

Thiago Jung Bauermann Feb. 28, 2024, 3:10 a.m. UTC 
  Commit 92d48a1e4eac ("Add an arm-tls feature which includes the tpidruro
register from CP15.") introduced the org.gnu.gdb.arm.tls feature, which
adds the tpidruro register, and unconditionally enabled it in
aarch32_create_target_description.

In Linux, the tpidruro register isn't available via ptrace in the 32-bit
kernel but it is available for an aarch32 program running under an arm64
kernel via the ptrace compat interface.  This isn't currently implemented
however, which causes GDB on arm-linux with 64-bit kernel to list the
register but show it as unavailable, as reported by Tom de Vries:

  $ gdb -q -batch a.out -ex start -ex 'p $tpidruro'
  Temporary breakpoint 1 at 0x512

  Temporary breakpoint 1, 0xaaaaa512 in main ()
  $1 = <unavailable>

Simon Marchi then clarified:

> The only time we should be seeing some "unavailable" registers or memory
> is in the context of tracepoints, for things that are not collected.
> Seeing an unavailable register here is a sign that something is not
> right.

Therefore, disable the TLS feature in aarch32 target descriptions for Linux
and NetBSD targets (the latter also doesn't seem to support accessing
tpidruro either, based on a quick look at arm-netbsd-nat.c).

This patch fixes the following tests:

Running gdb.base/inline-frame-cycle-unwind.exp ...
FAIL: gdb.base/inline-frame-cycle-unwind.exp: cycle at level 3: backtrace when the unwind is broken at frame 3
FAIL: gdb.base/inline-frame-cycle-unwind.exp: cycle at level 5: backtrace when the unwind is broken at frame 5
FAIL: gdb.base/inline-frame-cycle-unwind.exp: cycle at level 1: backtrace when the unwind is broken at frame 1

Tested with Ubuntu 22.04.3 on armv8l-linux-gnueabihf in native,
native-gdbserver and native-extended-gdbserver targets with no regressions.

PR tdep/31418
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31418
---

Hello,

As often happens, after I sent v1 of this patch I noticed a problem with it:
It introduces two variants of aarch32 target description (one with TLS
support and one without), but only creates and returns the first one that was
requested.  This version fixes the problem.

The only change compared to v1 is in gdb/aarch32-tdep.c.

Changes in v2:
- Cache two versions of tdesc_aarch32 in aarch32_read_description, one with
TLS support and one without.t

 gdb/aarch32-tdep.c               | 15 ++++++++++-----
 gdb/aarch32-tdep.h               |  2 +-
 gdb/aarch64-linux-nat.c          |  2 +-
 gdb/arch/aarch32.c               |  5 +++--
 gdb/arch/aarch32.h               |  2 +-
 gdb/arm-fbsd-tdep.c              |  2 +-
 gdb/arm-linux-nat.c              |  2 +-
 gdb/arm-linux-tdep.c             |  2 +-
 gdb/arm-netbsd-nat.c             |  2 +-
 gdbserver/linux-aarch32-tdesc.cc |  2 +-
 10 files changed, 21 insertions(+), 15 deletions(-)


base-commit: 407ca654547b100903f7eab44d078a2440736f13

Comments

John Baldwin Feb. 28, 2024, 7:10 p.m. UTC | #1 
On 2/27/24 7:10 PM, Thiago Jung Bauermann wrote:
> Commit 92d48a1e4eac ("Add an arm-tls feature which includes the tpidruro
> register from CP15.") introduced the org.gnu.gdb.arm.tls feature, which
> adds the tpidruro register, and unconditionally enabled it in
> aarch32_create_target_description.
> 
> In Linux, the tpidruro register isn't available via ptrace in the 32-bit
> kernel but it is available for an aarch32 program running under an arm64
> kernel via the ptrace compat interface.  This isn't currently implemented
> however, which causes GDB on arm-linux with 64-bit kernel to list the
> register but show it as unavailable, as reported by Tom de Vries:
> 
>    $ gdb -q -batch a.out -ex start -ex 'p $tpidruro'
>    Temporary breakpoint 1 at 0x512
> 
>    Temporary breakpoint 1, 0xaaaaa512 in main ()
>    $1 = <unavailable>
> 
> Simon Marchi then clarified:
> 
>> The only time we should be seeing some "unavailable" registers or memory
>> is in the context of tracepoints, for things that are not collected.
>> Seeing an unavailable register here is a sign that something is not
>> right.
> 
> Therefore, disable the TLS feature in aarch32 target descriptions for Linux
> and NetBSD targets (the latter also doesn't seem to support accessing
> tpidruro either, based on a quick look at arm-netbsd-nat.c).
> 
> This patch fixes the following tests:
> 
> Running gdb.base/inline-frame-cycle-unwind.exp ...
> FAIL: gdb.base/inline-frame-cycle-unwind.exp: cycle at level 3: backtrace when the unwind is broken at frame 3
> FAIL: gdb.base/inline-frame-cycle-unwind.exp: cycle at level 5: backtrace when the unwind is broken at frame 5
> FAIL: gdb.base/inline-frame-cycle-unwind.exp: cycle at level 1: backtrace when the unwind is broken at frame 1
> 
> Tested with Ubuntu 22.04.3 on armv8l-linux-gnueabihf in native,
> native-gdbserver and native-extended-gdbserver targets with no regressions.
> 
> PR tdep/31418
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=31418
> ---
> 
> Hello,
> 
> As often happens, after I sent v1 of this patch I noticed a problem with it:
> It introduces two variants of aarch32 target description (one with TLS
> support and one without), but only creates and returns the first one that was
> requested.  This version fixes the problem.
> 
> The only change compared to v1 is in gdb/aarch32-tdep.c.
> 
> Changes in v2:
> - Cache two versions of tdesc_aarch32 in aarch32_read_description, one with
> TLS support and one without.t

Good catch, V2 looks ok to me as well.

Approved-By: John Baldwin <jhb@FreeBSD.org>
Thiago Jung Bauermann Feb. 29, 2024, 3:34 p.m. UTC | #2 
John Baldwin <jhb@FreeBSD.org> writes:

> On 2/27/24 7:10 PM, Thiago Jung Bauermann wrote:
>> Hello,
>> As often happens, after I sent v1 of this patch I noticed a problem with it:
>> It introduces two variants of aarch32 target description (one with TLS
>> support and one without), but only creates and returns the first one that was
>> requested.  This version fixes the problem.
>> The only change compared to v1 is in gdb/aarch32-tdep.c.
>> Changes in v2:
>> - Cache two versions of tdesc_aarch32 in aarch32_read_description, one with
>> TLS support and one without.t
>
> Good catch, V2 looks ok to me as well.
>
> Approved-By: John Baldwin <jhb@FreeBSD.org>

Thank you again! Just pushed as commit bbb12eb9c84a.
diff mbox series

Patch

diff --git a/gdb/aarch32-tdep.c b/gdb/aarch32-tdep.c
index 9177b47d1481..0b7783c3e15d 100644
--- a/gdb/aarch32-tdep.c
+++ b/gdb/aarch32-tdep.c
@@ -22,15 +22,20 @@ 
 #include "gdbsupport/common-regcache.h"
 #include "arch/aarch32.h"
 
-static struct target_desc *tdesc_aarch32;
+static struct target_desc *tdesc_aarch32_list[2];
 
 /* See aarch32-tdep.h.  */
 
 const target_desc *
-aarch32_read_description ()
+aarch32_read_description (bool tls)
 {
-  if (tdesc_aarch32 == nullptr)
-    tdesc_aarch32 = aarch32_create_target_description ();
+  struct target_desc *tdesc = tdesc_aarch32_list[tls];
 
-  return tdesc_aarch32;
+  if (tdesc == nullptr)
+    {
+      tdesc = aarch32_create_target_description (tls);
+      tdesc_aarch32_list[tls] = tdesc;
+    }
+
+  return tdesc;
 }
diff --git a/gdb/aarch32-tdep.h b/gdb/aarch32-tdep.h
index 654483438485..009ee61890e2 100644
--- a/gdb/aarch32-tdep.h
+++ b/gdb/aarch32-tdep.h
@@ -22,6 +22,6 @@  struct target_desc;
 
 /* Get the AArch32 target description.  */
 
-const target_desc *aarch32_read_description ();
+const target_desc *aarch32_read_description (bool tls);
 
 #endif /* aarch32-tdep.h.  */
diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index 11a41e1afae0..9dc45e1c1d96 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -887,7 +887,7 @@  aarch64_linux_nat_target::read_description ()
 
   ret = ptrace (PTRACE_GETREGSET, tid, NT_ARM_VFP, &iovec);
   if (ret == 0)
-    return aarch32_read_description ();
+    return aarch32_read_description (false);
 
   CORE_ADDR hwcap = linux_get_hwcap ();
   CORE_ADDR hwcap2 = linux_get_hwcap2 ();
diff --git a/gdb/arch/aarch32.c b/gdb/arch/aarch32.c
index c910e3b5a388..9f3e25088f41 100644
--- a/gdb/arch/aarch32.c
+++ b/gdb/arch/aarch32.c
@@ -25,7 +25,7 @@ 
 /* See aarch32.h.  */
 
 target_desc *
-aarch32_create_target_description ()
+aarch32_create_target_description (bool tls)
 {
   target_desc_up tdesc = allocate_target_description ();
 
@@ -39,7 +39,8 @@  aarch32_create_target_description ()
   /* Create a vfpv3 feature, then a blank NEON feature.  */
   regnum = create_feature_arm_arm_vfpv3 (tdesc.get (), regnum);
   tdesc_create_feature (tdesc.get (), "org.gnu.gdb.arm.neon");
-  regnum = create_feature_arm_arm_tls (tdesc.get (), regnum);
+  if (tls)
+    regnum = create_feature_arm_arm_tls (tdesc.get (), regnum);
 
   return tdesc.release ();
 }
diff --git a/gdb/arch/aarch32.h b/gdb/arch/aarch32.h
index f7ee6faeea3c..1811b8a7a925 100644
--- a/gdb/arch/aarch32.h
+++ b/gdb/arch/aarch32.h
@@ -22,6 +22,6 @@ 
 
 /* Create the AArch32 target description.  */
 
-target_desc *aarch32_create_target_description ();
+target_desc *aarch32_create_target_description (bool tls);
 
 #endif /* aarch32.h.  */
diff --git a/gdb/arm-fbsd-tdep.c b/gdb/arm-fbsd-tdep.c
index b485951c3764..7b82de2166b5 100644
--- a/gdb/arm-fbsd-tdep.c
+++ b/gdb/arm-fbsd-tdep.c
@@ -228,7 +228,7 @@  arm_fbsd_read_description_auxv (const std::optional<gdb::byte_vector> &auxv,
   if (arm_hwcap & HWCAP_VFP)
     {
       if (arm_hwcap & HWCAP_NEON)
-	return aarch32_read_description ();
+	return aarch32_read_description (tls);
       else if ((arm_hwcap & (HWCAP_VFPv3 | HWCAP_VFPD32))
 	       == (HWCAP_VFPv3 | HWCAP_VFPD32))
 	return arm_read_description (ARM_FP_TYPE_VFPV3, tls);
diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
index 02994732563a..75f498cd5b3f 100644
--- a/gdb/arm-linux-nat.c
+++ b/gdb/arm-linux-nat.c
@@ -568,7 +568,7 @@  arm_linux_nat_target::read_description ()
       /* NEON implies VFPv3-D32 or no-VFP unit.  Say that we only support
 	 Neon with VFPv3-D32.  */
       if (arm_hwcap & HWCAP_NEON)
-	return aarch32_read_description ();
+	return aarch32_read_description (false);
       else if ((arm_hwcap & (HWCAP_VFPv3 | HWCAP_VFPv3D16)) == HWCAP_VFPv3)
 	return arm_read_description (ARM_FP_TYPE_VFPV3, false);
 
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index cc79247aaf12..a8b27a7463a6 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -740,7 +740,7 @@  arm_linux_core_read_description (struct gdbarch *gdbarch,
       /* NEON implies VFPv3-D32 or no-VFP unit.  Say that we only support
 	 Neon with VFPv3-D32.  */
       if (arm_hwcap & HWCAP_NEON)
-	return aarch32_read_description ();
+	return aarch32_read_description (false);
       else if ((arm_hwcap & (HWCAP_VFPv3 | HWCAP_VFPv3D16)) == HWCAP_VFPv3)
 	return arm_read_description (ARM_FP_TYPE_VFPV3, false);
 
diff --git a/gdb/arm-netbsd-nat.c b/gdb/arm-netbsd-nat.c
index df8b5ec7b03c..4b9f92946412 100644
--- a/gdb/arm-netbsd-nat.c
+++ b/gdb/arm-netbsd-nat.c
@@ -350,7 +350,7 @@  arm_netbsd_nat_target::read_description ()
 
   len = sizeof(flag);
   if (sysctlbyname("machdep.neon_present", &flag, &len, NULL, 0) == 0 && flag)
-    return aarch32_read_description ();
+    return aarch32_read_description (false);
 
   return arm_read_description (ARM_FP_TYPE_VFPV3, false);
 }
diff --git a/gdbserver/linux-aarch32-tdesc.cc b/gdbserver/linux-aarch32-tdesc.cc
index a696d8946e29..54c6f62e9965 100644
--- a/gdbserver/linux-aarch32-tdesc.cc
+++ b/gdbserver/linux-aarch32-tdesc.cc
@@ -32,7 +32,7 @@  aarch32_linux_read_description ()
 {
   if (tdesc_aarch32 == nullptr)
     {
-      tdesc_aarch32 = aarch32_create_target_description ();
+      tdesc_aarch32 = aarch32_create_target_description (false);
 
       static const char *expedite_regs[] = { "r11", "sp", "pc", 0 };
       init_target_desc (tdesc_aarch32, expedite_regs);