[5/9,v7] Introduce common-regcache.h

  This introduces common-regcache.h.  This contains two functions that
allow nat/linux-btrace.c to be simplified.  A better long term
solution would be unify the regcache code, but this is sufficient for
now.

This patch differs from the version I posted on August 1
(https://sourceware.org/ml/gdb-patches/2014-08/msg00010.html) in that
some suggested documentation changes have been made and that various
updates were required to reflect regcache changes recently committed
by Andreas Arnez.

gdb/ChangeLog:

	* common/common-regcache.h: New file.
	* Makefile.in (HFILES_NO_SRCDIR): Add common/common-regcache.h.
	* regcache.h: Include common-regcache.h.
	(regcache_read_pc): Don't declare.
	* regcache.c (get_thread_regcache_for_ptid): New function.
	* nat/linux-btrace.c: Don't include regcache.h.
	Include common-regcache.h.
	(perf_event_read_bts): Use get_thread_regcache_for_ptid.

gdb/gdbserver/ChangeLog:

	* regcache.h: Include common-regcache.h.
	(regcache_read_pc): Don't declare.
	* regcache.c (get_thread_regcache_for_ptid): New function.
---
 gdb/ChangeLog                |   12 ++++++++++++
 gdb/Makefile.in              |    3 ++-
 gdb/common/common-regcache.h |   35 +++++++++++++++++++++++++++++++++++
 gdb/gdbserver/ChangeLog      |    7 +++++++
 gdb/gdbserver/regcache.c     |    8 ++++++++
 gdb/gdbserver/regcache.h     |    4 ++--
 gdb/nat/linux-btrace.c       |    8 ++------
 gdb/regcache.c               |    7 +++++++
 gdb/regcache.h               |    3 ++-
 9 files changed, 77 insertions(+), 10 deletions(-)
 create mode 100644 gdb/common/common-regcache.h
  

On 08/29/2014 02:51 PM, Gary Benson wrote:
> This introduces common-regcache.h.  This contains two functions that
> allow nat/linux-btrace.c to be simplified.  A better long term
> solution would be unify the regcache code, but this is sufficient for
> now.
> 
> This patch differs from the version I posted on August 1
> (https://sourceware.org/ml/gdb-patches/2014-08/msg00010.html) in that
> some suggested documentation changes have been made and that various
> updates were required to reflect regcache changes recently committed
> by Andreas Arnez.
> 
> gdb/ChangeLog:
> 
> 	* common/common-regcache.h: New file.
> 	* Makefile.in (HFILES_NO_SRCDIR): Add common/common-regcache.h.
> 	* regcache.h: Include common-regcache.h.
> 	(regcache_read_pc): Don't declare.
> 	* regcache.c (get_thread_regcache_for_ptid): New function.
> 	* nat/linux-btrace.c: Don't include regcache.h.
> 	Include common-regcache.h.
> 	(perf_event_read_bts): Use get_thread_regcache_for_ptid.
> 
> gdb/gdbserver/ChangeLog:
> 
> 	* regcache.h: Include common-regcache.h.
> 	(regcache_read_pc): Don't declare.
> 	* regcache.c (get_thread_regcache_for_ptid): New function.
> diff --git a/gdb/gdbserver/regcache.h b/gdb/gdbserver/regcache.h
> index 891fead..0c933f3 100644
> --- a/gdb/gdbserver/regcache.h
> +++ b/gdb/gdbserver/regcache.h
> @@ -19,6 +19,8 @@
>  #ifndef REGCACHE_H
>  #define REGCACHE_H
>  
> +#include "common-regcache.h"
> +
>  struct thread_info;
>  struct target_desc;
>  
> @@ -91,8 +93,6 @@ void registers_to_string (struct regcache *regcache, char *buf);
>  
>  void registers_from_string (struct regcache *regcache, char *buf);
>  
> -CORE_ADDR regcache_read_pc (struct regcache *regcache);
> -

Like in the target patches, please leave breadcrumbs pointing
to the shared header (here and elsewhere).

Otherwise looks good to me too.

Thanks,
Pedro Alves
  

Gary Benson writes:
 > This introduces common-regcache.h.  This contains two functions that
 > allow nat/linux-btrace.c to be simplified.  A better long term
 > solution would be unify the regcache code, but this is sufficient for
 > now.
 > 
 > This patch differs from the version I posted on August 1
 > (https://sourceware.org/ml/gdb-patches/2014-08/msg00010.html) in that
 > some suggested documentation changes have been made and that various
 > updates were required to reflect regcache changes recently committed
 > by Andreas Arnez.
 > 
 > gdb/ChangeLog:
 > 
 > 	* common/common-regcache.h: New file.
 > 	* Makefile.in (HFILES_NO_SRCDIR): Add common/common-regcache.h.
 > 	* regcache.h: Include common-regcache.h.
 > 	(regcache_read_pc): Don't declare.
 > 	* regcache.c (get_thread_regcache_for_ptid): New function.
 > 	* nat/linux-btrace.c: Don't include regcache.h.
 > 	Include common-regcache.h.
 > 	(perf_event_read_bts): Use get_thread_regcache_for_ptid.
 > 
 > gdb/gdbserver/ChangeLog:
 > 
 > 	* regcache.h: Include common-regcache.h.
 > 	(regcache_read_pc): Don't declare.
 > 	* regcache.c (get_thread_regcache_for_ptid): New function.
 >[...]
 > diff --git a/gdb/common/common-regcache.h b/gdb/common/common-regcache.h
 > new file mode 100644
 > index 0000000..aef6bfd
 > --- /dev/null
 > +++ b/gdb/common/common-regcache.h
 > @@ -0,0 +1,35 @@
 > +/* Cache and manage the values of registers
 > +
 > +   Copyright (C) 1986-2014 Free Software Foundation, Inc.
 > +
 > +   This file is part of GDB.
 > +
 > +   This program is free software; you can redistribute it and/or modify
 > +   it under the terms of the GNU General Public License as published by
 > +   the Free Software Foundation; either version 3 of the License, or
 > +   (at your option) any later version.
 > +
 > +   This program is distributed in the hope that it will be useful,
 > +   but WITHOUT ANY WARRANTY; without even the implied warranty of
 > +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 > +   GNU General Public License for more details.
 > +
 > +   You should have received a copy of the GNU General Public License
 > +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 > +
 > +#ifndef COMMON_REGCACHE_H
 > +#define COMMON_REGCACHE_H
 > +
 > +/* This header is a stopgap until we have an independent regcache.  */
 > +
 > +/* Return the register cache associated with the thread specified
 > +   by PTID.  This function must be provided by the client.  */

Hi.
Can you add a comment here explaining the ownership of the memory
object returned?
E.g., is it cached "internally" to the function so that the caller
doesn't have to free it?

Thanks!

 > +
 > +extern struct regcache *get_thread_regcache_for_ptid (ptid_t ptid);
 > +
 > +/* Read the PC register.  This function must be provided by the
 > +   client.  */
 > +
 > +extern CORE_ADDR regcache_read_pc (struct regcache *regcache);
 > +
 > +#endif /* COMMON_REGCACHE_H */
  

Doug Evans wrote:
> Gary Benson writes:
> > +/* Return the register cache associated with the thread specified
> > +   by PTID.  This function must be provided by the client.  */
> 
> Can you add a comment here explaining the ownership of the memory
> object returned?  E.g., is it cached "internally" to the function
> so that the caller doesn't have to free it?

That seems odd.  We don't document other similar functions this way:
I'm thinking GDB's get_current_arch, current_inferior, target_gdbarch,
or gdbserver's current_process, current_target_desc.  It seems the
pattern is to note if the caller must free the object and to remain
quiet otherwise.

How about I change the comment to "Return _a_pointer_to_ the register
cache..."?  (underlines for emphasis here).

Cheers,
Gary
  

On Thu, Sep 11, 2014 at 4:02 AM, Gary Benson <gbenson@redhat.com> wrote:
> Doug Evans wrote:
>> Gary Benson writes:
>> > +/* Return the register cache associated with the thread specified
>> > +   by PTID.  This function must be provided by the client.  */
>>
>> Can you add a comment here explaining the ownership of the memory
>> object returned?  E.g., is it cached "internally" to the function
>> so that the caller doesn't have to free it?
>
> That seems odd.  We don't document other similar functions this way:
> I'm thinking GDB's get_current_arch, current_inferior, target_gdbarch,
> or gdbserver's current_process, current_target_desc.  It seems the
> pattern is to note if the caller must free the object and to remain
> quiet otherwise.

Yeah, but I never liked such things being implicit.
I can't trust a missing "caller must free" as being intentional.
[One can equally argue the presence of "caller must free" (or "not
free") isn't necessarily trustable, but such things don't change that
often.]

With a name like "get_current_foo", the "current" makes things less
implicit (at least to me).

If we were using c++ then object ownership can (often, though not
always) be more clearly expressed and then such things can be more
reasonably left as being implicit in comments.
But we don't have that so if we're going to be cleaning things up, and
maybe even paying a little attention to API design, I figure IWBN to
have things be clearer than they are today.

[Plus I get bitten time and again by taking gdb's existing practice as
something we actually want to keep. :-)]

> How about I change the comment to "Return _a_pointer_to_ the register
> cache..."?  (underlines for emphasis here).

If one was going to add emphasis, I'd emphasize _the_. :-)
  

Doug Evans wrote:
> On Thu, Sep 11, 2014 at 4:02 AM, Gary Benson <gbenson@redhat.com> wrote:
> > Doug Evans wrote:
> > > Gary Benson writes:
> > > > +/* Return the register cache associated with the thread specified
> > > > +   by PTID.  This function must be provided by the client.  */
> > >
> > > Can you add a comment here explaining the ownership of the
> > > memory object returned?  E.g., is it cached "internally" to
> > > the function so that the caller doesn't have to free it?
> >
> > That seems odd.  We don't document other similar functions this
> > way: I'm thinking GDB's get_current_arch, current_inferior,
> > target_gdbarch, or gdbserver's current_process,
> > current_target_desc.  It seems the pattern is to note if the
> > caller must free the object and to remain quiet otherwise.
> 
> Yeah, but I never liked such things being implicit.  I can't trust a
> missing "caller must free" as being intentional.  [One can equally
> argue the presence of "caller must free" (or "not free") isn't
> necessarily trustable, but such things don't change that often.]
> 
> With a name like "get_current_foo", the "current" makes things less
> implicit (at least to me).
> 
> If we were using c++ then object ownership can (often, though not
> always) be more clearly expressed and then such things can be more
> reasonably left as being implicit in comments.  But we don't have
> that so if we're going to be cleaning things up, and maybe even
> paying a little attention to API design, I figure IWBN to have
> things be clearer than they are today.
> 
> [Plus I get bitten time and again by taking gdb's existing practice
> as something we actually want to keep. :-)]
> 
> > How about I change the comment to "Return _a_pointer_to_ the
> > register cache..."?  (underlines for emphasis here).
> 
> If one was going to add emphasis, I'd emphasize _the_. :-)

I tried to figure out how to succinctly write what you're asking me
to here but I'm stuck.  Is there a function documented in this way
in GDB I can use as an example?

In the interests of keeping things moving I've pushed the patch with
this comment:

   Return a pointer to the register cache associated with the
   thread specified by PTID.  This function must be provided by
   the client.

Thanks,
Gary
  

On Fri, Sep 12, 2014 at 2:45 AM, Gary Benson <gbenson@redhat.com> wrote:
> I tried to figure out how to succinctly write what you're asking me
> to here but I'm stuck.  Is there a function documented in this way
> in GDB I can use as an example?
>
> In the interests of keeping things moving I've pushed the patch with
> this comment:
>
>    Return a pointer to the register cache associated with the
>    thread specified by PTID.  This function must be provided by
>    the client.

Fine by me.