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

[Wei-min Pan <weimin dot pan at oracle dot 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