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

[Andrew Burgess <aburgess@redhat.com>]

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