Re: [PING][PATCH v2 PR gdb/21870] aarch64: Leftover uncleared debug registers

[Wei-min Pan <weimin.pan@oracle.com>]

On 2/14/2019 5:01 AM, Pedro Alves wrote:
> On 02/13/2019 09:56 PM, Wei-min Pan wrote:
>> On 2/13/2019 3:40 AM, Pedro Alves wrote:
>>> On 02/12/2019 01:10 AM, Weimin Pan wrote:
>>>> +clean_restart $testfile
>>>> +
>>>> +set test "run to exit"
>>>> +gdb_test_multiple "run" "$test" {
>>>> +    -re "exited with code 01.*$gdb_prompt $" {
>>>> +        pass "$test"
>>>> +    }
>>>> +    -re "exited normally.*$gdb_prompt $" {
>>>> +        pass "$test"
>>>> +    }
>>>> +}
>>> A naked "run" command doesn't work when testing against
>>> gdbserver with --target_board=native-gdbserver.
>>>
>>> Is "run" important here?  Could this use runto_main + "continue" instead?
>> As long as the test run doesn't assert, we could instead use
>> "runto_main + "continue". Thanks for pointing this out.
> OOC, why does asserting make a difference?
>
>>> Also, the comment at the top of the file says:
>>>
>>>    # This test checks that GDB does not alter watchpoints set by an inferior.
>>>    # It sets a watchpoint on memory then writes to the watched memory.
>>>    # It will exit with 1 if the watchpoint is not reached.
>>>
>>> But I couldn't spot where that "exit with 1" happens in the .c file.
>> You are right, it should be "exit with 2".
>>
>>> Also, when that happens, we're issuing a pass, as seen above.
>>> Is that intended?
>> "Exit with 1" could happen if the PTRACE_SETREGSET call should
>> fail which is ok as long as it doesn't cause assertion.
> I see.  Could you add some comments, please?

Done.

> Could you send a patch?

Just pushed the updated patch.


> BTW:
>
>   - why is the SET_WATCHPOINT macro necessary?

It's not necessary. Removed.

>
>   - what does the ??? below mean?
>
>      dreg_state.dbg_regs[0].ctrl |= 2 << 1; // GDB: ???: enabled at el0

Removed the "GDB: ???:" part.

>
>   - The code has:
>
>    atexit (cleanup);
>
>    (...)
>
>    but then also calls cleanup() return returning (twice, end of main),
>    which seems unnecessary?

You're right, the second call is redundant and is removed.

>
>   - While at it, the gdb_compile + clean_restart in the .exp file
>     could be replaced with a single prepare_for_testing call.

Nice. Replaced.

Thanks.

>
>> Thanks for your comments.
>>
> Thanks,
> Pedro Alves