This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] aarch64 multi-arch part 6: HW breakpoint on unaligned address
- From: Pedro Alves <palves at redhat dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 13 Oct 2015 18:31:52 +0100
- Subject: Re: [PATCH] aarch64 multi-arch part 6: HW breakpoint on unaligned address
- Authentication-results: sourceware.org; auth=none
- References: <1444731060-16237-1-git-send-email-yao dot qi at linaro dot org> <561CE5D2 dot 8030505 at redhat dot com> <861tcy6b84 dot fsf at gmail dot com>
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