Re: [RFC] [gdb/testsuite] Add xfail in gdb.base/hbreak.exp

[Tom de Vries <tdevries@suse.de>]

On 7/24/24 12:45, Luis Machado wrote:
> On 7/24/24 10:28, Tom de Vries wrote:
>> On 7/24/24 08:53, Luis Machado wrote:
>>> On 7/24/24 06:25, Tom de Vries wrote:
>>>> On 7/23/24 12:02, Luis Machado wrote:
>>>>> On 7/17/24 16:14, Luis Machado wrote:
>>>>>> On 7/17/24 16:10, 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
>>>>>>
>>>>>> That is a bit odd, but it goes through the compat layer, which is not exercised
>>>>>> as often as the 32-bit code.
>>>>>>
>>>>>> Let me see if I can reproduce this one on my end.
>>>>>
>>>>> I managed to reproduce this. I checked with the kernel folks and this should
>>>>> work, but I'm not sure where the error is coming from.
>>>>>
>>>>
>>>> Hi Luis,
>>>>
>>>> thanks for looking into this, and the approval, committed.
>>>>
>>>> Are you or the kernel folks following up on this, in terms of a linux kernel PR or some such?  It would be nice to add some sort of reference to the xfail.
>>>
>>> It's in my TODO. I'm still investigating to understand where the error is coming from. Once located, I plan to check with them for their thoughts and a possible
>>> fix. I don't think the kernel folks use the PR process much. We could probably ammend this commit later on once we have more information though.
>>>
>>
>> Ok, I spent some more time debugging this issue this morning.
>>
>> After reading kernel sources for a while, I tried out reversing the order in which the Breakpoint Register Pair is written in arm_linux_nat_target::low_prepare_to_resume, and ... the test-case passes.
>>
> 
> But what would change with reversing writing to the control registers, from gdb's perspective?
> 

Well, from gdb's perspective, the only difference is that both ptrace 
calls succeed, while with the original order the first one fails (and 
consequently there's no second call).

>> My theory at this point is that the following happens in the failing case:
>> - PTRACE_SETHBPREGS with address 0x4004e2
>> - compat_arch_ptrace
>> - compat_ptrace_sethbpregs
>> - compat_ptrace_hbp_set
>> - ptrace_hbp_set_addr
>> - ptrace_hbp_get_initialised_bp
>> - ptrace_hbp_create
>> - /* Initialise fields to sane defaults
>>       (i.e. values that will pass validation).  */
>>    attr.bp_len = HW_BREAKPOINT_LEN_4;
> 
> 
> The default starts as a 4-byte breakpoint, but is supposed to be adjusted later on to 2 bytes. If this isn't happening, I think we have a bug somewhere.
> 

Agreed, you could frame that as a kernel bug.  It would be good to known 
whether the kernel developers agree with that assessment.

>> - attr.bp_addr = 0x4004e2;
>> - modify_user_hw_breakpoint
>> - modify_user_hw_breakpoint_check
>> - hw_breakpoint_parse
>> - hw_breakpoint_arch_parse
>> - case is_compat_bp(bp)
>> - offset = 2;
>> - fallthrough to default
>> - return -EINVAL
>>
>> In short, we try to validate:
>> - attr.bp_len == HW_BREAKPOINT_LEN_4 && attr.bp_addr == 0x4004e2
>> and fail.
>>
>> By reversing the order, we validate:
>> - attr.bp_len == HW_BREAKPOINT_LEN_2 && attr.bp_addr == 0, and then
>> - attr.bp_len == HW_BREAKPOINT_LEN_2 && attr.bp_addr == 0x4004e2
>> which both succeed.
> 
> Why do we have HW_BREAKPOINT_LEN_2 above while the first case has HW_BREAKPOINT_LEN_4?
> 

Well, because we reversed the order of the two ptrace calls.

So, in the original case, the first call to ptrace uses the default 
bp_len (HW_BREAKPOINT_LEN_4) and the actual address (0x4004e2), which fails.

And in the reversed order case, the first call to ptrace uses the 
default address (0x0) and the actual bp_len (HW_BREAKPOINT_LEN_2).

[ With "default" meaning, as set by ptrace_hbp_create, and "actual", as 
set by the ptrace calls. ]

>>
>> So, my questions at this point are:
>> - is this a problem limited to aarch64 32-bit mode, or does it also
>>    occur for native 32-bit arm?
> 
> I'm not sure at this point. They are two separate code bases, but it is probably reasonable to assume the compat layer of aarch64 was based on the
> original 32-bit arm code. Checking hw_breakpoint_arch_parse for arm, it does seem fairly similar.
> 

I also observed that they're very similar.

>> - is this a kernel bug?
> 
> Potentially, if it is assuming a length that is not correct.
> 
>> - if this is a kernel bug, is there a workaround we can use?
>> - if this is not a kernel bug, is this because gdb is writing the
>>    Breakpoint Register Pair in the wrong order?
> 
> I don't think we have a specific order to write things, but if it is a bug that arises from a specific order of commands, we could potentially
> work around it.
> 

OK, I'm currently testing that approach.

Thanks,
- Tom

>>
>> Thanks,
>> - Tom
>>
>>>>
>>>> Thanks,
>>>> - Tom
>>>>
>>>>>>
>>>>>>
>>>>>>> (gdb) PASS: gdb.base/hbreak.exp: hbreak
>>>>>>> continue^M
>>>>>>> Continuing.^M
>>>>>>> Unexpected error setting breakpoint: Invalid argument.^M
>>>>>>> (gdb) FAIL: 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.
>>>>>>>
>>>>>>> I'm not sure if this is simply unsupported or if there's a kernel bug of some
>>>>>>> sort, but I don't see what gdb can do about this.
>>>>>>>
>>>>>>> Tentatively mark this as xfail.
>>>>>>>
>>>>>>> Tested on aarch64-linux.
>>>>>>> ---
>>>>>>>     gdb/testsuite/gdb.base/hbreak.exp | 40 ++++++++++++++++++++++++++-----
>>>>>>>     1 file changed, 34 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>>> diff --git a/gdb/testsuite/gdb.base/hbreak.exp b/gdb/testsuite/gdb.base/hbreak.exp
>>>>>>> index 73a3e2afb67..b140257a23e 100644
>>>>>>> --- a/gdb/testsuite/gdb.base/hbreak.exp
>>>>>>> +++ b/gdb/testsuite/gdb.base/hbreak.exp
>>>>>>> @@ -27,10 +27,38 @@ if ![runto_main] {
>>>>>>>       set breakline [gdb_get_line_number "break-at-exit"]
>>>>>>>     -gdb_test "hbreak ${srcfile}:${breakline}" \
>>>>>>> -     "Hardware assisted breakpoint \[0-9\]+ at 0x\[0-9a-f\]+: file .*${srcfile}, line ${breakline}\\." \
>>>>>>> -     "hbreak"
>>>>>>> +set re_loc "file \[^\r\n\]*$srcfile, line $breakline"
>>>>>>> +set re_dot [string_to_regexp .]
>>>>>>>     -gdb_test "continue" \
>>>>>>> -     "Continuing\\.\[ \r\n\]+Breakpoint \[0-9\]+, .*break-at-exit.*" \
>>>>>>> -     "continue to break-at-exit after 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
>>>>>>> +    }
>>>>>>> +    }
>>>>>>> +}
>>>>>>>
>>>>>>> base-commit: 0ed152c5c6b3c72fc505b331ed77e08b438d643a
>>>>>>
>>>>>
>>>>> In any case, I agree gdb doesn't have a better way to deal with it.
>>>>>
>>>>> Approved-By: Luis Machado <luis.machado@arm.com>
>>>>
>>>
>>
>