[PATCH v2] [gdb/tdep] Fix arm thumb2 hw breakpoint
Luis Machado
luis.machado@arm.com
Mon Jul 29 08:51:53 GMT 2024
On 7/27/24 09:12, Tom de Vries wrote:
> On 7/26/24 17:54, Luis Machado wrote:
>> On 7/26/24 10:54, Tom de Vries wrote:
>>> 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.
>>> ---
>>> gdb/arm-linux-nat.c | 97 +++++++++++++++++++++++++++----
>>> gdb/testsuite/gdb.base/hbreak.exp | 40 ++-----------
>>> gdbserver/linux-arm-low.cc | 65 +++++++++++++++++----
>>> 3 files changed, 146 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
>>> index ac53bed72d7..f336fd5d226 100644
>>> --- a/gdb/arm-linux-nat.c
>>> +++ b/gdb/arm-linux-nat.c
>>> @@ -876,6 +876,13 @@ arm_hwbp_control_is_enabled (arm_hwbp_control_t control)
>>> return control & 0x1;
>>> }
>>> +/* Is the breakpoint control value CONTROL initialized? */
>>
>> Just a nit for a missing newline between comment and function declaration.
>>
>>> +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 +1241,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 +1292,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 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..ee89949a2a2 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;
>>> }
>>>
>>> base-commit: b8b1a4388835bf2899bb0cc148db42a2c32e2af6
>>
>> Otherwise this is OK. I'll try to run some further validation on the emulator,
>> but I've confirmed this works for both 32-bit-on-64-bit and 32-bit arm.
>>
>
> Thanks, good to have that confirmed.
>
>> I'll chase this with the kernel folks as well.
>
> Great, curious to hear about the outcome of that.
>
>> Thanks for catching this and for the fix.
>>
>
> Sure, and thanks for the reviews and testing.
>
> Committed with aforementioned nit fixed.
>
> If you think backporting to gdb-15-branch would be good, we could do that (but would have to open a token PR for that).
Thinking about it, the bug has been there for a little while. I suppose it wasn't taken care of because hardware breakpoints
are not as useful as hardware watchpoints. The software breakpoints do the job well anyway.
With the above in mind, I'm tempted to not pursue a gdb 15 backport, as I don't think it would be a critical bug.
>
> Thanks,
> - Tom
>
>> Approved-By: Luis Machado <luis.machado@arm.com>
>> Tested-By: Luis Machado <luis.machado@arm.com>
>
More information about the Gdb-patches
mailing list