From 8d40cbfde5988ac6d7dad2fc2796d13c9ddb210b Mon Sep 17 00:00:00 2001 From: Tom de Vries Date: Sat, 27 Jul 2024 10:05:20 +0200 Subject: [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 Tested-By: Luis Machado --- gdb/arm-linux-nat.c | 98 +++++++++++++++++++++++++++++++++++---- gdb/testsuite/gdb.base/hbreak.exp | 40 +++------------- 2 files changed, 94 insertions(+), 44 deletions(-) (limited to 'gdb') 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" -- cgit v1.1