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

[Pedro Alves <palves at redhat dot com>]

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?

Could you send a patch?

BTW:

 - why is the SET_WATCHPOINT macro necessary?

 - what does the ??? below mean?

    dreg_state.dbg_regs[0].ctrl |= 2 << 1; // GDB: ???: enabled at el0

 - The code has:

  atexit (cleanup);

  (...)

  but then also calls cleanup() return returning (twice, end of main),
  which seems unnecessary?

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

> 
> Thanks for your comments.
> 

Thanks,
Pedro Alves