diff options
author | Tom de Vries <tdevries@suse.de> | 2024-07-27 10:05:20 +0200 |
---|---|---|
committer | Tom de Vries <tdevries@suse.de> | 2024-07-27 10:05:20 +0200 |
commit | 8d40cbfde5988ac6d7dad2fc2796d13c9ddb210b (patch) | |
tree | ec1a5fd58c7c77b86e753867d360dc463be60523 | |
parent | ced7ecee432b1406d198b50f1d9980451d25012a (diff) | |
download | binutils-8d40cbfde5988ac6d7dad2fc2796d13c9ddb210b.zip binutils-8d40cbfde5988ac6d7dad2fc2796d13c9ddb210b.tar.gz binutils-8d40cbfde5988ac6d7dad2fc2796d13c9ddb210b.tar.bz2 |
[gdb/tdep] Fix arm thumb2 hw breakpoint
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.
Work around this by first using an inoffensive address bpts[i].address & ~0x7.
Likewise in arm_target::low_prepare_to_resume, which fixes the same fail on
target board native-gdbserver/mthumb.
While we're at it:
- use arm_hwbp_control_is_initialized in
arm_linux_nat_target::low_prepare_to_resume,
- handle the !arm_hwbp_control_is_initialized case explicitly,
- add missing '_()' in arm_target::low_prepare_to_resume,
- make error messages identical between arm_target::low_prepare_to_resume and
arm_linux_nat_target::low_prepare_to_resume,
- factor out sethbpregs_hwbp_address and sethbpregs_hwbp_control to
make the implementation more readable.
Remove the tentative xfail added in d0af16d5a10 ("[gdb/testsuite] Add xfail in
gdb.base/hbreak.exp") by simply reverting the commit.
Tested on arm-linux.
Approved-By: Luis Machado <luis.machado@arm.com>
Tested-By: Luis Machado <luis.machado@arm.com>
-rw-r--r-- | gdb/arm-linux-nat.c | 98 | ||||
-rw-r--r-- | gdb/testsuite/gdb.base/hbreak.exp | 40 | ||||
-rw-r--r-- | gdbserver/linux-arm-low.cc | 65 |
3 files changed, 147 insertions, 56 deletions
diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c index ac53bed..7b4faac 100644 --- a/gdb/arm-linux-nat.c +++ b/gdb/arm-linux-nat.c @@ -876,6 +876,14 @@ arm_hwbp_control_is_enabled (arm_hwbp_control_t control) return control & 0x1; } +/* Is the breakpoint control value CONTROL initialized? */ + +static int +arm_hwbp_control_is_initialized (arm_hwbp_control_t control) +{ + return control != 0; +} + /* Change a breakpoint control word so that it is in the disabled state. */ static arm_hwbp_control_t arm_hwbp_control_disable (arm_hwbp_control_t control) @@ -1234,6 +1242,34 @@ arm_linux_nat_target::low_delete_thread (struct arch_lwp_info *arch_lwp) xfree (arch_lwp); } +/* For PID, set the address register of hardware breakpoint pair I to + ADDRESS. */ + +static void +sethbpregs_hwbp_address (int pid, int i, unsigned int address) +{ + PTRACE_TYPE_ARG3 address_reg = (PTRACE_TYPE_ARG3) ((i << 1) + 1); + + errno = 0; + + if (ptrace (PTRACE_SETHBPREGS, pid, address_reg, &address) < 0) + perror_with_name (_("Unexpected error updating breakpoint address")); +} + +/* For PID, set the control register of hardware breakpoint pair I to + CONTROL. */ + +static void +sethbpregs_hwbp_control (int pid, int i, arm_hwbp_control_t control) +{ + PTRACE_TYPE_ARG3 control_reg = (PTRACE_TYPE_ARG3) ((i << 1) + 2); + + errno = 0; + + if (ptrace (PTRACE_SETHBPREGS, pid, control_reg, &control) < 0) + perror_with_name (_("Unexpected error setting breakpoint control")); +} + /* Called when resuming a thread. The hardware debug registers are updated when there is any change. */ @@ -1257,16 +1293,58 @@ 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]) { - errno = 0; - 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 (bpts[i].control != 0) - if (ptrace (PTRACE_SETHBPREGS, pid, - (PTRACE_TYPE_ARG3) ((i << 1) + 2), &bpts[i].control) < 0) - perror_with_name (_("Unexpected error setting breakpoint")); + unsigned int address = bpts[i].address; + arm_hwbp_control_t control = bpts[i].control; + + if (!arm_hwbp_control_is_initialized (control)) + { + /* Nothing to do. */ + } + else if (!arm_hwbp_control_is_enabled (control)) + { + /* Disable hardware breakpoint, just write the control + register. */ + sethbpregs_hwbp_control (pid, i, control); + } + else + { + /* We used to do here simply: + 1. address_reg = address + 2. control_reg = 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 breakpoint + 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. A possible fix for + this would be to only check this aspect when writing the + control reg. + + Work around this by first using an inoffensive address, which is + guaranteed to hit the offset == 0 case in + hw_breakpoint_arch_parse. */ + unsigned int aligned_address = address & ~0x7U; + if (aligned_address != address) + { + sethbpregs_hwbp_address (pid, i, aligned_address); + sethbpregs_hwbp_control (pid, i, control); + } + sethbpregs_hwbp_address (pid, i, address); + sethbpregs_hwbp_control (pid, i, control); + } arm_lwp_info->bpts_changed[i] = 0; } diff --git a/gdb/testsuite/gdb.base/hbreak.exp b/gdb/testsuite/gdb.base/hbreak.exp index b140257..73a3e2a 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 eec4649..ee89949 100644 --- a/gdbserver/linux-arm-low.cc +++ b/gdbserver/linux-arm-low.cc @@ -819,6 +819,34 @@ arm_target::low_new_fork (process_info *parent, process_info *child) child_lwp_info->wpts_changed[i] = 1; } +/* For PID, set the address register of hardware breakpoint pair I to + ADDRESS. */ + +static void +sethbpregs_hwbp_address (int pid, int i, unsigned int address) +{ + PTRACE_TYPE_ARG3 address_reg = (PTRACE_TYPE_ARG3) ((i << 1) + 1); + + errno = 0; + + if (ptrace (PTRACE_SETHBPREGS, pid, address_reg, &address) < 0) + perror_with_name (_("Unexpected error updating breakpoint address")); +} + +/* For PID, set the control register of hardware breakpoint pair I to + CONTROL. */ + +static void +sethbpregs_hwbp_control (int pid, int i, arm_hwbp_control_t control) +{ + PTRACE_TYPE_ARG3 control_reg = (PTRACE_TYPE_ARG3) ((i << 1) + 2); + + errno = 0; + + if (ptrace (PTRACE_SETHBPREGS, pid, control_reg, &control) < 0) + perror_with_name (_("Unexpected error setting breakpoint control")); +} + /* Called when resuming a thread. If the debug regs have changed, update the thread's copies. */ void @@ -834,19 +862,32 @@ 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]) { - errno = 0; + unsigned int address = proc_info->bpts[i].address; + arm_hwbp_control_t control = proc_info->bpts[i].control; - 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 (arm_hwbp_control_is_initialized (proc_info->bpts[i].control)) - if (ptrace (PTRACE_SETHBPREGS, pid, - (PTRACE_TYPE_ARG3) ((i << 1) + 2), - &proc_info->bpts[i].control) < 0) - perror_with_name ("Unexpected error setting breakpoint"); + if (!arm_hwbp_control_is_initialized (control)) + { + /* Nothing to do. */ + } + else if (!arm_hwbp_control_is_enabled (control)) + { + /* Disable hardware breakpoint, just write the control + register. */ + sethbpregs_hwbp_control (pid, i, control); + } + else + { + /* See arm_linux_nat_target::low_prepare_to_resume for detailed + comment. */ + unsigned int aligned_address = address & ~0x7U; + if (aligned_address != address) + { + sethbpregs_hwbp_address (pid, i, aligned_address); + sethbpregs_hwbp_control (pid, i, control); + } + sethbpregs_hwbp_address (pid, i, address); + sethbpregs_hwbp_control (pid, i, control); + } lwp_info->bpts_changed[i] = 0; } |