diff mbox series

[gdb/aarch64] Handle unknown debug architecture versions more gracefully

Message ID 20230418135313.36300-1-luis.machado@arm.com
State New
Headers
Series [gdb/aarch64] Handle unknown debug architecture versions more gracefully |

Commit Message

Luis Machado April 18, 2023, 1:53 p.m. UTC 
  In PR 30340 it was reported that a KVM-based AArch64 system's kernel was
returning a debug architecture version of 0 when fetching both the
NT_ARM_HW_WATCH and NT_ARM_HW_BREAK register sets.

Even though the debug architecture version being reported is invalid, gdb can
still make things work by ignoring that information and relying on the counts
of hardware watchpoints and hardware breakpoints.

This patch makes gdb handle this situation a bit more gracefully by causing gdb
to warn when it sees an invalid/unknown debug architecture version, but still
allowing gdb to detect the number of hardware watchpoints and hardware
breakpoints available.

PR tdep/30340
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30340
---
 gdb/nat/aarch64-linux-hw-point.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

Comments

Andrew Burgess May 15, 2023, 10:07 a.m. UTC | #1 
Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

> In PR 30340 it was reported that a KVM-based AArch64 system's kernel was
> returning a debug architecture version of 0 when fetching both the
> NT_ARM_HW_WATCH and NT_ARM_HW_BREAK register sets.
>
> Even though the debug architecture version being reported is invalid, gdb can
> still make things work by ignoring that information and relying on the counts
> of hardware watchpoints and hardware breakpoints.
>
> This patch makes gdb handle this situation a bit more gracefully by causing gdb
> to warn when it sees an invalid/unknown debug architecture version, but still
> allowing gdb to detect the number of hardware watchpoints and hardware
> breakpoints available.
>
> PR tdep/30340

Apparently we no longer need to include this cookie.  Just using the bug
url should be enough for this commit to be linked bugzilla.

> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=30340
> ---
>  gdb/nat/aarch64-linux-hw-point.c | 24 ++++++++++++++++++++----
>  1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
> index ccb47cd5aa2..b014d387e1e 100644
> --- a/gdb/nat/aarch64-linux-hw-point.c
> +++ b/gdb/nat/aarch64-linux-hw-point.c
> @@ -250,10 +250,21 @@ aarch64_linux_get_debug_reg_capacity (int tid)
>    iov.iov_base = &dreg_state;
>    iov.iov_len = sizeof (dreg_state);
>  
> +  /* It has been reported that under KVM the debug architecture version can
> +     be reported as 0, which is invalid.  But this doesn't mean gdb can't use
> +     hardware debugging resources.  Instead of bailing out, carry on fetching
> +     the hardware breakpoints/watchpoints count so we can potentially get back
> +     on track.  */

Given my recent comment on pr gdb/30340 you might want to reword the
comment a little -- we now understand why we saw 0 in this case.

> +
>    /* Get hardware watchpoint register info.  */
> -  if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_HW_WATCH, &iov) == 0
> -      && compatible_debug_arch (AARCH64_DEBUG_ARCH (dreg_state.dbg_info)))
> +  if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_HW_WATCH, &iov) == 0)
>      {
> +      if (!compatible_debug_arch (AARCH64_DEBUG_ARCH (dreg_state.dbg_info)))
> +	warning (_("Unknown/Invalid debug architecture version %d.\n"
> +		   "Attempting to fetch the number of hardware watchpoints "
> +		   "available."),
> +		   AARCH64_DEBUG_ARCH (dreg_state.dbg_info));
> +
>        aarch64_num_wp_regs = AARCH64_DEBUG_NUM_SLOTS (dreg_state.dbg_info);

I guess the risk here is that on a later architecture the format of
dreg_state might change, in which case reading the wp/bp count might not
get the correct result.

But in that case we'd print the warning, so when things go wrong later
we will have given the user a clue for what the problem might be.

So, I think this change seems a reasonable improvement.

Reviewed-By: Andrew Burgess <aburgess@redhat.com>

Thanks,
Andrew


>        if (aarch64_num_wp_regs > AARCH64_HWP_MAX_NUM)
>  	{
> @@ -271,9 +282,14 @@ aarch64_linux_get_debug_reg_capacity (int tid)
>      }
>  
>    /* Get hardware breakpoint register info.  */
> -  if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_HW_BREAK, &iov) == 0
> -      && compatible_debug_arch (AARCH64_DEBUG_ARCH (dreg_state.dbg_info)))
> +  if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_HW_BREAK, &iov) == 0)
>      {
> +      if (!compatible_debug_arch (AARCH64_DEBUG_ARCH (dreg_state.dbg_info)))
> +	warning (_("Unknown/Invalid debug architecture version %d.\n"
> +		   "Attempting to fetch the number of hardware breakpoints "
> +		   "available."),
> +		   AARCH64_DEBUG_ARCH (dreg_state.dbg_info));
> +
>        aarch64_num_bp_regs = AARCH64_DEBUG_NUM_SLOTS (dreg_state.dbg_info);
>        if (aarch64_num_bp_regs > AARCH64_HBP_MAX_NUM)
>  	{
> -- 
> 2.25.1
diff mbox series

Patch

diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
index ccb47cd5aa2..b014d387e1e 100644
--- a/gdb/nat/aarch64-linux-hw-point.c
+++ b/gdb/nat/aarch64-linux-hw-point.c
@@ -250,10 +250,21 @@  aarch64_linux_get_debug_reg_capacity (int tid)
   iov.iov_base = &dreg_state;
   iov.iov_len = sizeof (dreg_state);
 
+  /* It has been reported that under KVM the debug architecture version can
+     be reported as 0, which is invalid.  But this doesn't mean gdb can't use
+     hardware debugging resources.  Instead of bailing out, carry on fetching
+     the hardware breakpoints/watchpoints count so we can potentially get back
+     on track.  */
+
   /* Get hardware watchpoint register info.  */
-  if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_HW_WATCH, &iov) == 0
-      && compatible_debug_arch (AARCH64_DEBUG_ARCH (dreg_state.dbg_info)))
+  if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_HW_WATCH, &iov) == 0)
     {
+      if (!compatible_debug_arch (AARCH64_DEBUG_ARCH (dreg_state.dbg_info)))
+	warning (_("Unknown/Invalid debug architecture version %d.\n"
+		   "Attempting to fetch the number of hardware watchpoints "
+		   "available."),
+		   AARCH64_DEBUG_ARCH (dreg_state.dbg_info));
+
       aarch64_num_wp_regs = AARCH64_DEBUG_NUM_SLOTS (dreg_state.dbg_info);
       if (aarch64_num_wp_regs > AARCH64_HWP_MAX_NUM)
 	{
@@ -271,9 +282,14 @@  aarch64_linux_get_debug_reg_capacity (int tid)
     }
 
   /* Get hardware breakpoint register info.  */
-  if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_HW_BREAK, &iov) == 0
-      && compatible_debug_arch (AARCH64_DEBUG_ARCH (dreg_state.dbg_info)))
+  if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_HW_BREAK, &iov) == 0)
     {
+      if (!compatible_debug_arch (AARCH64_DEBUG_ARCH (dreg_state.dbg_info)))
+	warning (_("Unknown/Invalid debug architecture version %d.\n"
+		   "Attempting to fetch the number of hardware breakpoints "
+		   "available."),
+		   AARCH64_DEBUG_ARCH (dreg_state.dbg_info));
+
       aarch64_num_bp_regs = AARCH64_DEBUG_NUM_SLOTS (dreg_state.dbg_info);
       if (aarch64_num_bp_regs > AARCH64_HBP_MAX_NUM)
 	{