RE: [PATCH v2 2/2] btrace: allow recording to be started for running threads

Metzger, Markus T
2017-01-25T15:53:26Z

Hello Pedro,

Thanks for your review.

> > +/* Validate that we can read PTID's registers. */
> > +
> > +static void
> > +validate_registers_access_ptid (ptid_t ptid)
> > +{
> > + struct cleanup *cleanup = save_inferior_ptid ();
> > +
> > + inferior_ptid = ptid;
> > + validate_registers_access ();
> > + do_cleanups (cleanup);
> > +}
>
> Do we need this, we we have the new function added
> by patch #1 ?

This is a safeguard. I don't expect that we will ever throw, here.

> > + && can_access_registers_ptid (tp->ptid))
>
> If you have this check, why do you need to the TRY/CATCH ?
>
> Or even, given the validate_registers_access_ptid check
> above, why is this check necessary?
>
> > + btrace_add_pc (tp);
> > + }
> > + CATCH (exception, RETURN_MASK_ALL)
>
> Adding a RETURN_MASK_ALL always raises alarm bells,
> because this swallows a user-typed ctrl-c, which
> is probably wrong.
>
> > + {
> > + btrace_disable (tp);
> > + }
> > + END_CATCH

The TRY/CATCH is to clean things up in case of errors or ctrl-c. The caller
will clean things up for previously enabled threads but expects each
btrace_enable to either succeed or fail and throw.

I see that I forgot to rethrow, though. This is not intended to swallow
the error - only to disable tracing again in case of errors.

The can_access_registers_ptid check is to avoid the error and thus allow
"record btrace" for running (or exited) threads where the btrace_add_pc
call can be omitted.

> > +# We need to enable non-stop mode for the remote case.
> > +gdb_test_no_output "set non-stop on"
>
> This is too late with --target_board=native-extended-gdbserver.
> Use instead:
>
> save_vars { GDBFLAGS } {
> append GDBFLAGS " -ex \"set non-stop on\""
> clean_restart $testfile
> }

This test seems to run fine with --target_board=native-extended-gdbserver.

Making the change nevertheless. There's another instance of this in
gdb.btrace/non-stop.exp. Will fix this in a separate patch.

A different test, gdb.btrace/enable.exp is failing with that target_board.

Thanks,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928