aarch64 multi-arch part 6: HW breakpoint on unaligned address
Commit Message
Nowadays, both aarch64 GDB and linux kernel assumes that address for
setting breakpoint should be 4-byte aligned. However that is not true
after we support multi-arch, because thumb instruction can be at 2-byte
aligned address. Patch http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/375141.html
to linux kernel is to teach kernel to handle 2-byte aligned address for
HW breakpoint, while this patch is to teach aarch64 GDB handle 2-byte
aligned address.
First of all, we call gdbarch_breakpoint_from_pc to get the instruction
length rather than using hard-coded 4. Secondly, in GDBserver, we set
length back to 2 if it is 3, because GDB encode 3 in it to indicate it
is a 32-bit thumb breakpoint. Then we relax the address alignment
check from 4-byte aligned to 2-byte aligned.
This patch enables some tests (such as gdb.base/break-idempotent.exp,
gdb.base/cond-eval-mode.exp, gdb.base/watchpoint-reuse-slot.exp,) and
fixes many fails (such as gdb.base/hbreak2.exp) when the program is
compiled in thumb mode on aarch64.
Regression tested on aarch64-linux, both native and gdbserver. This
is the last patch of multi-arch work.
gdb:
2015-10-13 Yao Qi <yao.qi@linaro.org>
* aarch64-linux-nat.c (aarch64_linux_insert_hw_breakpoint):
Call gdbarch_breakpoint_from_pc to instruction length.
(aarch64_linux_remove_hw_breakpoint): Likewise.
* nat/aarch64-linux-hw-point.c (aarch64_point_is_aligned):
Set alignment to 2 for breakpoint.
(aarch64_handle_breakpoint): Update comments.
gdb/gdbserver:
2015-10-13 Yao Qi <yao.qi@linaro.org>
* linux-aarch64-low.c (aarch64_insert_point): Set len to 2
if it is 3.
(aarch64_remove_point): Likewise.
---
gdb/aarch64-linux-nat.c | 8 ++++++--
gdb/gdbserver/linux-aarch64-low.c | 18 ++++++++++++------
gdb/nat/aarch64-linux-hw-point.c | 15 ++++++++++++---
3 files changed, 30 insertions(+), 11 deletions(-)
Comments
On 10/13/2015 11:11 AM, Yao Qi wrote:
> --- a/gdb/gdbserver/linux-aarch64-low.c
> +++ b/gdb/gdbserver/linux-aarch64-low.c
> @@ -315,9 +315,12 @@ aarch64_insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
> ret = -1;
> }
> else
> - ret =
> - aarch64_handle_breakpoint (targ_type, addr, len, 1 /* is_insert */,
> - state);
> + {
> + if (len == 3)
> + len = 2;
I think this warrants a comment. E.g., someone reading
arm-linux-low.c:arm_linux_hw_point_initialize quite easily grasps
what 3 means.
> + ret = aarch64_handle_breakpoint (targ_type, addr, len,
> + 1 /* is_insert */, state);
> + }
>
> if (show_debug_regs)
> aarch64_show_debug_reg_state (state, "insert_point", addr, len,
> @@ -353,9 +356,12 @@ aarch64_remove_point (enum raw_bkpt_type type, CORE_ADDR addr,
> aarch64_handle_watchpoint (targ_type, addr, len, 0 /* is_insert */,
> state);
> else
> - ret =
> - aarch64_handle_breakpoint (targ_type, addr, len, 0 /* is_insert */,
> - state);
> + {
> + if (len == 3)
> + len = 2;
> + ret = aarch64_handle_breakpoint (targ_type, addr, len,
> + 0 /* is_insert */, state);
> + }
>
> if (show_debug_regs)
> aarch64_show_debug_reg_state (state, "remove_point", addr, len,
> diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
> index bca6ec1..d15e518 100644
> --- a/gdb/nat/aarch64-linux-hw-point.c
> +++ b/gdb/nat/aarch64-linux-hw-point.c
> @@ -112,8 +112,17 @@ aarch64_point_encode_ctrl_reg (enum target_hw_bp_type type, int len)
> static int
> aarch64_point_is_aligned (int is_watchpoint, CORE_ADDR addr, int len)
> {
> - unsigned int alignment = is_watchpoint ? AARCH64_HWP_ALIGNMENT
> - : AARCH64_HBP_ALIGNMENT;
> + unsigned int alignment = 0;
> +
> + if (is_watchpoint)
> + alignment = AARCH64_HWP_ALIGNMENT;
> + else
> + {
> + /* Set alignment to 2 only if the current process is 32-bit,
> + since thumb instruction can be 2-byte aligned. Otherwise, set
> + alignment to AARCH64_HBP_ALIGNMENT. */
> + alignment = 2;
Is some other code doing what the comment says? I'm not seeing
any obvious 32-bit check.
> + }
>
> if (addr & (alignment - 1))
> return 0;
> @@ -445,7 +454,7 @@ aarch64_handle_breakpoint (enum target_hw_bp_type type, CORE_ADDR addr,
> struct aarch64_debug_reg_state *state)
> {
> /* The hardware breakpoint on AArch64 should always be 4-byte
> - aligned. */
> + aligned, but on AArch32, it can be 2-byte aligned. */
> if (!aarch64_point_is_aligned (0 /* is_watchpoint */ , addr, len))
> return -1;
>
> -- 1.9.1
>
Thanks,
Pedro Alves
Pedro Alves <palves@redhat.com> writes:
>> + {
>> + if (len == 3)
>> + len = 2;
>
> I think this warrants a comment. E.g., someone reading
> arm-linux-low.c:arm_linux_hw_point_initialize quite easily grasps
> what 3 means.
>
How about the comment like this?
if (len == 3)
{
/* LEN is 3 means the breakpoint is set on a 32-bit thumb
instruction. Set it to 2 to correctly encode length bit
mask in hardware/watchpoint control register. */
len = 2;
}
>> diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
>> index bca6ec1..d15e518 100644
>> --- a/gdb/nat/aarch64-linux-hw-point.c
>> +++ b/gdb/nat/aarch64-linux-hw-point.c
>> @@ -112,8 +112,17 @@ aarch64_point_encode_ctrl_reg (enum target_hw_bp_type type, int len)
>> static int
>> aarch64_point_is_aligned (int is_watchpoint, CORE_ADDR addr, int len)
>> {
>> - unsigned int alignment = is_watchpoint ? AARCH64_HWP_ALIGNMENT
>> - : AARCH64_HBP_ALIGNMENT;
>> + unsigned int alignment = 0;
>> +
>> + if (is_watchpoint)
>> + alignment = AARCH64_HWP_ALIGNMENT;
>> + else
>> + {
>> + /* Set alignment to 2 only if the current process is 32-bit,
>> + since thumb instruction can be 2-byte aligned. Otherwise, set
>> + alignment to AARCH64_HBP_ALIGNMENT. */
>> + alignment = 2;
>
> Is some other code doing what the comment says? I'm not seeing
> any obvious 32-bit check.
No, I don't do the 32-bit check here. Ideally, we should set alignment
to 2 only when the process is 32-bit, and still use 4 as alignment
otherwise. However, I don't find an easy way to do the 32-bit check
here, because this code is used by both GDB and GDBserver. We can do
the 32-bit check in GDB and GDBserver respectively, and pass the result
to nat/aarch64-linux-hw-point.c, but I don't like putting information down
multiple levels like this.
On Tue, Oct 13, 2015 at 8:26 AM, Yao Qi <qiyaoltc@gmail.com> wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>>> + {
>>> + if (len == 3)
>>> + len = 2;
>>
>> I think this warrants a comment. E.g., someone reading
>> arm-linux-low.c:arm_linux_hw_point_initialize quite easily grasps
>> what 3 means.
>>
>
> How about the comment like this?
>
> if (len == 3)
> {
> /* LEN is 3 means the breakpoint is set on a 32-bit thumb
> instruction. Set it to 2 to correctly encode length bit
> mask in hardware/watchpoint control register. */
> len = 2;
> }
>
>>> diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
>>> index bca6ec1..d15e518 100644
>>> --- a/gdb/nat/aarch64-linux-hw-point.c
>>> +++ b/gdb/nat/aarch64-linux-hw-point.c
>>> @@ -112,8 +112,17 @@ aarch64_point_encode_ctrl_reg (enum target_hw_bp_type type, int len)
>>> static int
>>> aarch64_point_is_aligned (int is_watchpoint, CORE_ADDR addr, int len)
>>> {
>>> - unsigned int alignment = is_watchpoint ? AARCH64_HWP_ALIGNMENT
>>> - : AARCH64_HBP_ALIGNMENT;
>>> + unsigned int alignment = 0;
>>> +
>>> + if (is_watchpoint)
>>> + alignment = AARCH64_HWP_ALIGNMENT;
>>> + else
>>> + {
>>> + /* Set alignment to 2 only if the current process is 32-bit,
>>> + since thumb instruction can be 2-byte aligned. Otherwise, set
>>> + alignment to AARCH64_HBP_ALIGNMENT. */
>>> + alignment = 2;
>>
>> Is some other code doing what the comment says? I'm not seeing
>> any obvious 32-bit check.
>
> No, I don't do the 32-bit check here. Ideally, we should set alignment
> to 2 only when the process is 32-bit, and still use 4 as alignment
> otherwise. However, I don't find an easy way to do the 32-bit check
> here, because this code is used by both GDB and GDBserver. We can do
> the 32-bit check in GDB and GDBserver respectively, and pass the result
> to nat/aarch64-linux-hw-point.c, but I don't like putting information down
> multiple levels like this.
Also it is not just about 32bit vs 64bit either. It is about aarch32
vs aarch64. I think we should push that information down as far as we
can.
Thanks,
Andrew
>
> --
> Yao (齐尧)
On 10/13/2015 04:26 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>>> + {
>>> + if (len == 3)
>>> + len = 2;
>>
>> I think this warrants a comment. E.g., someone reading
>> arm-linux-low.c:arm_linux_hw_point_initialize quite easily grasps
>> what 3 means.
>>
>
> How about the comment like this?
>
> if (len == 3)
> {
> /* LEN is 3 means the breakpoint is set on a 32-bit thumb
> instruction. Set it to 2 to correctly encode length bit
> mask in hardware/watchpoint control register. */
> len = 2;
> }
Sounds fine.
>
>>> diff --git a/gdb/nat/aarch64-linux-hw-point.c b/gdb/nat/aarch64-linux-hw-point.c
>>> index bca6ec1..d15e518 100644
>>> --- a/gdb/nat/aarch64-linux-hw-point.c
>>> +++ b/gdb/nat/aarch64-linux-hw-point.c
>>> @@ -112,8 +112,17 @@ aarch64_point_encode_ctrl_reg (enum target_hw_bp_type type, int len)
>>> static int
>>> aarch64_point_is_aligned (int is_watchpoint, CORE_ADDR addr, int len)
>>> {
>>> - unsigned int alignment = is_watchpoint ? AARCH64_HWP_ALIGNMENT
>>> - : AARCH64_HBP_ALIGNMENT;
>>> + unsigned int alignment = 0;
>>> +
>>> + if (is_watchpoint)
>>> + alignment = AARCH64_HWP_ALIGNMENT;
>>> + else
>>> + {
>>> + /* Set alignment to 2 only if the current process is 32-bit,
>>> + since thumb instruction can be 2-byte aligned. Otherwise, set
>>> + alignment to AARCH64_HBP_ALIGNMENT. */
>>> + alignment = 2;
>>
>> Is some other code doing what the comment says? I'm not seeing
>> any obvious 32-bit check.
>
> No, I don't do the 32-bit check here. Ideally, we should set alignment
> to 2 only when the process is 32-bit, and still use 4 as alignment
> otherwise. However, I don't find an easy way to do the 32-bit check
> here, because this code is used by both GDB and GDBserver. We can do
> the 32-bit check in GDB and GDBserver respectively, and pass the result
> to nat/aarch64-linux-hw-point.c, but I don't like putting information down
> multiple levels like this.
At least the comment should be updated. It's quite misleading as is.
Thanks,
Pedro Alves
@@ -608,11 +608,13 @@ aarch64_linux_insert_hw_breakpoint (struct target_ops *self,
{
int ret;
CORE_ADDR addr = bp_tgt->placed_address = bp_tgt->reqstd_address;
- const int len = 4;
+ int len;
const enum target_hw_bp_type type = hw_execute;
struct aarch64_debug_reg_state *state
= aarch64_get_debug_reg_state (ptid_get_pid (inferior_ptid));
+ gdbarch_breakpoint_from_pc (gdbarch, &addr, &len);
+
if (show_debug_regs)
fprintf_unfiltered
(gdb_stdlog,
@@ -640,11 +642,13 @@ aarch64_linux_remove_hw_breakpoint (struct target_ops *self,
{
int ret;
CORE_ADDR addr = bp_tgt->placed_address;
- const int len = 4;
+ int len = 4;
const enum target_hw_bp_type type = hw_execute;
struct aarch64_debug_reg_state *state
= aarch64_get_debug_reg_state (ptid_get_pid (inferior_ptid));
+ gdbarch_breakpoint_from_pc (gdbarch, &addr, &len);
+
if (show_debug_regs)
fprintf_unfiltered
(gdb_stdlog, "remove_hw_breakpoint on entry (addr=0x%08lx, len=%d))\n",
@@ -315,9 +315,12 @@ aarch64_insert_point (enum raw_bkpt_type type, CORE_ADDR addr,
ret = -1;
}
else
- ret =
- aarch64_handle_breakpoint (targ_type, addr, len, 1 /* is_insert */,
- state);
+ {
+ if (len == 3)
+ len = 2;
+ ret = aarch64_handle_breakpoint (targ_type, addr, len,
+ 1 /* is_insert */, state);
+ }
if (show_debug_regs)
aarch64_show_debug_reg_state (state, "insert_point", addr, len,
@@ -353,9 +356,12 @@ aarch64_remove_point (enum raw_bkpt_type type, CORE_ADDR addr,
aarch64_handle_watchpoint (targ_type, addr, len, 0 /* is_insert */,
state);
else
- ret =
- aarch64_handle_breakpoint (targ_type, addr, len, 0 /* is_insert */,
- state);
+ {
+ if (len == 3)
+ len = 2;
+ ret = aarch64_handle_breakpoint (targ_type, addr, len,
+ 0 /* is_insert */, state);
+ }
if (show_debug_regs)
aarch64_show_debug_reg_state (state, "remove_point", addr, len,
@@ -112,8 +112,17 @@ aarch64_point_encode_ctrl_reg (enum target_hw_bp_type type, int len)
static int
aarch64_point_is_aligned (int is_watchpoint, CORE_ADDR addr, int len)
{
- unsigned int alignment = is_watchpoint ? AARCH64_HWP_ALIGNMENT
- : AARCH64_HBP_ALIGNMENT;
+ unsigned int alignment = 0;
+
+ if (is_watchpoint)
+ alignment = AARCH64_HWP_ALIGNMENT;
+ else
+ {
+ /* Set alignment to 2 only if the current process is 32-bit,
+ since thumb instruction can be 2-byte aligned. Otherwise, set
+ alignment to AARCH64_HBP_ALIGNMENT. */
+ alignment = 2;
+ }
if (addr & (alignment - 1))
return 0;
@@ -445,7 +454,7 @@ aarch64_handle_breakpoint (enum target_hw_bp_type type, CORE_ADDR addr,
struct aarch64_debug_reg_state *state)
{
/* The hardware breakpoint on AArch64 should always be 4-byte
- aligned. */
+ aligned, but on AArch32, it can be 2-byte aligned. */
if (!aarch64_point_is_aligned (0 /* is_watchpoint */ , addr, len))
return -1;