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

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

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