[gdb/tdep] Fix arm thumb2 hw breakpoint

Message ID 20240725131810.7320-1-tdevries@suse.de
State Superseded
Headers
Series [gdb/tdep] Fix arm thumb2 hw breakpoint |

Checks

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

Commit Message

Tom de Vries July 25, 2024, 1:18 p.m. UTC
  On an aarch64-linux system with 32-bit userland running in a chroot, and using
target board unix/mthumb I get:
...
(gdb) hbreak hbreak.c:27^M
Hardware assisted breakpoint 2 at 0x4004e2: file hbreak.c, line 27.^M
(gdb) PASS: gdb.base/hbreak.exp: hbreak
continue^M
Continuing.^M
Unexpected error setting breakpoint: Invalid argument.^M
(gdb) XFAIL: gdb.base/hbreak.exp: continue to break-at-exit after hbreak
...
due to this call in arm_linux_nat_target::low_prepare_to_resume:
...
          if (ptrace (PTRACE_SETHBPREGS, pid,
              (PTRACE_TYPE_ARG3) ((i << 1) + 1), &bpts[i].address) < 0)
            perror_with_name (_("Unexpected error setting breakpoint"));
...

This problem does not happen if instead we use a 4-byte aligned address.

This may or may not be a kernel bug.

The ptrace call is the first of two writing a register pair, an address and a
control register.

Fix or work around this by:
- first writing the address bpts[i].address & ~0x7,
- then writing the control register, and
- if necessary, updating the address to bpts[i].address.

Likewise in arm_target::low_prepare_to_resume, which fixes the same fail on
target board native-gdbserver/mthumb.

While we're at it, add missing '_()' and make error messages identical between
arm_target::low_prepare_to_resume and
arm_linux_nat_target::low_prepare_to_resume.

Remove the tentative xfail added in d0af16d5a10 ("[gdb/testsuite] Add xfail in
gdb.base/hbreak.exp") by simply reverting the commit.

Tested on aarch64-linux.
---
 gdb/arm-linux-nat.c               | 52 ++++++++++++++++++++++++++++---
 gdb/testsuite/gdb.base/hbreak.exp | 40 ++++--------------------
 gdbserver/linux-arm-low.cc        | 25 ++++++++++-----
 3 files changed, 71 insertions(+), 46 deletions(-)


base-commit: a93faed5d46039999cb1cd8659c82ac981485666
  

Patch

diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
index ac53bed72d7..8a00c8095de 100644
--- a/gdb/arm-linux-nat.c
+++ b/gdb/arm-linux-nat.c
@@ -1257,17 +1257,59 @@  arm_linux_nat_target::low_prepare_to_resume (struct lwp_info *lwp)
   for (i = 0; i < arm_linux_get_hw_breakpoint_count (); i++)
     if (arm_lwp_info->bpts_changed[i])
       {
+	PTRACE_TYPE_ARG3 address_reg = (PTRACE_TYPE_ARG3) ((i << 1) + 1);
+	PTRACE_TYPE_ARG3 control_reg = (PTRACE_TYPE_ARG3) ((i << 1) + 2);
+
 	errno = 0;
+
+	/* We used to do here simply:
+	     1. address_reg = bpts[i].address
+	     2. control_reg = bpts[i].control
+	   but the write to address_reg can fail for thumb2 instructions if
+	   the address is not 4-byte aligned.
+
+	   It's not clear whether this is a kernel bug or not, partly because
+	   PTRACE_SETHBPREGS is undocumented.
+
+	   The context is that we're using two ptrace calls to set the two
+	   halves of a register pair.  For each ptrace call, the kernel must
+	   check the arguments, and return -1 and set errno appropriately if
+	   something is wrong.  One of the aspects that needs validation is
+	   whether, in terms of hw_breakpoint_arch_parse, the address matches
+	   the breakpoint length.  This aspect can only be checked by looking
+	   in both registers, which only makes sense once a pair is written in
+	   full.
+
+	   The problem is that the kernel checks this aspect after each ptrace
+	   call, and consequently for the first call it may be checking this
+	   aspect using a default or previous value for the part of the pair
+	   not written by the call.
+
+	   Work around this by first writing an inoffensive address, which is
+	   guaranteed to hit the offset == 0 case in hw_breakpoint_arch_parse,
+	   then the control_reg, and then the actual address, in other words:
+	     1. address_reg = bpts[i].address & ~0x7U
+	     2. control_reg = bpts[i].control
+	     3. address_reg = bpts[i].address
+	   obviously skipping the 3rd step if it's the same address.  */
+	unsigned int aligned_address = bpts[i].address & ~0x7U;
 	if (arm_hwbp_control_is_enabled (bpts[i].control))
-	  if (ptrace (PTRACE_SETHBPREGS, pid,
-	      (PTRACE_TYPE_ARG3) ((i << 1) + 1), &bpts[i].address) < 0)
-	    perror_with_name (_("Unexpected error setting breakpoint"));
+	  if (ptrace (PTRACE_SETHBPREGS, pid, address_reg, &aligned_address)
+	      < 0)
+	    perror_with_name (_("Unexpected error setting breakpoint address"));
 
 	if (bpts[i].control != 0)
-	  if (ptrace (PTRACE_SETHBPREGS, pid,
-	      (PTRACE_TYPE_ARG3) ((i << 1) + 2), &bpts[i].control) < 0)
+	  if (ptrace (PTRACE_SETHBPREGS, pid, control_reg, &bpts[i].control)
+	      < 0)
 	    perror_with_name (_("Unexpected error setting breakpoint"));
 
+	if (arm_hwbp_control_is_enabled (bpts[i].control)
+	    && bpts[i].address != aligned_address)
+	  if (ptrace (PTRACE_SETHBPREGS, pid, address_reg, &bpts[i].address)
+	      < 0)
+	    perror_with_name
+	      (_("Unexpected error updating breakpoint address"));
+
 	arm_lwp_info->bpts_changed[i] = 0;
       }
 
diff --git a/gdb/testsuite/gdb.base/hbreak.exp b/gdb/testsuite/gdb.base/hbreak.exp
index b140257a23e..73a3e2afb67 100644
--- a/gdb/testsuite/gdb.base/hbreak.exp
+++ b/gdb/testsuite/gdb.base/hbreak.exp
@@ -27,38 +27,10 @@  if ![runto_main] {
 
 set breakline [gdb_get_line_number "break-at-exit"]
 
-set re_loc "file \[^\r\n\]*$srcfile, line $breakline"
-set re_dot [string_to_regexp .]
+gdb_test "hbreak ${srcfile}:${breakline}" \
+	 "Hardware assisted breakpoint \[0-9\]+ at 0x\[0-9a-f\]+: file .*${srcfile}, line ${breakline}\\." \
+	 "hbreak"
 
-set addr 0x0
-gdb_test_multiple "hbreak ${srcfile}:${breakline}" "hbreak" {
-    -re -wrap "Hardware assisted breakpoint $decimal at ($hex): $re_loc$re_dot" {
-	set addr $expect_out(1,string)
-	pass $gdb_test_name
-    }
-}
-
-set have_xfail 0
-if { [istarget arm*-*-*] } {
-    # When running 32-bit userland on aarch64 kernel, thumb instructions that
-    # are not 4-byte aligned may not be supported for setting a hardware
-    # breakpoint on.
-    set have_xfail [expr ($addr & 0x2) == 2]
-}
-
-set re_xfail \
-    [string_to_regexp \
-	 "Unexpected error setting breakpoint: Invalid argument."]
-
-gdb_test_multiple "continue" "continue to break-at-exit after hbreak" {
-    -re -wrap "Continuing\\.\[ \r\n\]+Breakpoint \[0-9\]+, .*break-at-exit.*" {
-	pass $gdb_test_name
-    }
-    -re -wrap $re_xfail {
-	if { $have_xfail } {
-	    xfail $gdb_test_name
-	} else {
-	    fail $gdb_test_name
-	}
-    }
-}
+gdb_test "continue" \
+	 "Continuing\\.\[ \r\n\]+Breakpoint \[0-9\]+, .*break-at-exit.*" \
+	 "continue to break-at-exit after hbreak"
diff --git a/gdbserver/linux-arm-low.cc b/gdbserver/linux-arm-low.cc
index eec4649b235..6eeb883e3b0 100644
--- a/gdbserver/linux-arm-low.cc
+++ b/gdbserver/linux-arm-low.cc
@@ -834,19 +834,30 @@  arm_target::low_prepare_to_resume (lwp_info *lwp)
   for (i = 0; i < arm_linux_get_hw_breakpoint_count (); i++)
     if (lwp_info->bpts_changed[i])
       {
+	PTRACE_TYPE_ARG3 address_reg = (PTRACE_TYPE_ARG3) ((i << 1) + 1);
+	PTRACE_TYPE_ARG3 control_reg = (PTRACE_TYPE_ARG3) ((i << 1) + 2);
+
 	errno = 0;
 
+	/* See arm_linux_nat_target::low_prepare_to_resume for detailed
+	   comment.  */
+	unsigned int aligned_address = proc_info->bpts[i].address & ~0x7U;
 	if (arm_hwbp_control_is_enabled (proc_info->bpts[i].control))
-	  if (ptrace (PTRACE_SETHBPREGS, pid,
-		      (PTRACE_TYPE_ARG3) ((i << 1) + 1),
-		      &proc_info->bpts[i].address) < 0)
-	    perror_with_name ("Unexpected error setting breakpoint address");
+	  if (ptrace (PTRACE_SETHBPREGS, pid, address_reg, &aligned_address)
+	      < 0)
+	    perror_with_name (_("Unexpected error setting breakpoint address"));
 
 	if (arm_hwbp_control_is_initialized (proc_info->bpts[i].control))
-	  if (ptrace (PTRACE_SETHBPREGS, pid,
-		      (PTRACE_TYPE_ARG3) ((i << 1) + 2),
+	  if (ptrace (PTRACE_SETHBPREGS, pid, control_reg,
 		      &proc_info->bpts[i].control) < 0)
-	    perror_with_name ("Unexpected error setting breakpoint");
+	    perror_with_name (_("Unexpected error setting breakpoint"));
+
+	if (arm_hwbp_control_is_enabled (proc_info->bpts[i].control)
+	    && proc_info->bpts[i].address != aligned_address)
+	  if (ptrace (PTRACE_SETHBPREGS, pid, address_reg,
+		      &proc_info->bpts[i].address) < 0)
+	    perror_with_name
+	      (_("Unexpected error updating breakpoint address"));
 
 	lwp_info->bpts_changed[i] = 0;
       }