RE: [PATCH v3 2/5] btrace: allow recording to be started (and stopped) for running threads

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

Hi Pedro,

Thanks for your review.


> > +static void
> > +validate_registers_access_ptid (ptid_t ptid)
> > +{
> > +  struct cleanup *cleanup = save_inferior_ptid ();
> > +
> > +  inferior_ptid = ptid;
> > +  validate_registers_access ();
> > +  do_cleanups (cleanup);
> > +}
> > +
> 
> Please let's refactor things a bit to avoid the need to frob
> inferior_ptid and restore with a cleanup.  Change the existing
> validate_registers_access like:
> 
>   static void
>  -validate_registers_access ()
>  +validate_registers_access_ptid (ptid_t ptid)
>   {
>    /* No selected thread, no registers.  */
>    -if (ptid_equal (inferior_ptid, null_ptid))
>    +if (ptid_equal (ptid, null_ptid))
>      error (_("No thread selected."));
>    [etc.]
> 
> and then reimplement it back on top of validate_registers_access_ptid:
> 
> validate_registers_access ()
> {
>   return validate_registers_access_ptid (inferior_ptid);
> }

I had that and then chose to rewrite it again because the errors that
validate_registers_access throws refer to "selected thread".

In the btrace case, the argument thread is the selected thread (I think
this is true for all flows that call btrace_fetch) but this is not necessarily
the case in general.  Do you think that might be confusing to the user?

I was also thinking of changing the error messages to refer to the argument
thread but that would make them less clear when PTID really refers to the
selected thread - which should be the case most of the time if not always.

Maybe this is not an issue in practice and the "selected thread" errors are
understandable enough.  What do you think?


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