Re: [PATCH 05/11 v5] Add target/target.h

[Doug Evans <dje@google.com>]

Gary Benson writes:
 > This adds target/target.h.  This file declares some functions that
 > the shared code can use and that the clients must implement.  It
 > also changes some shared code to use these functions.
 > 
 > gdb/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* target/target.h: New file.
 > 	* Makefile.in (HFILES_NO_SRCDIR): Add target/target.h.
 > 	* target.h: Include target/target.h.
 > 	(target_resume, target_wait, target_stop, target_read_memory)
 > 	(target_write_memory, non_stop): Don't declare.
 > 	* target.c (target_read_uint32): New function.
 > 	* common/agent.c: Include target/target.h.
 > 	[!GDBSERVER]: Don't include target.h or infrun.h.
 > 	(agent_get_helper_thread_id): Use target_read_uint32.
 > 	(agent_run_command): Always use target_resume, target_stop,
 > 	target_wait, target_write_memory, and target_read_memory.
 > 	(agent_capability_check): Use target_read_uint32.
 > 
 > gdb/gdbserver/
 > 2014-08-01  Tom Tromey  <tromey@redhat.com>
 > 	    Gary Benson  <gbenson@redhat.com>
 > 
 > 	* target.c (target_wait, target_stop, target_resume)
 > 	(target_read_memory, target_read_uint32)
 > 	(target_write_memory): New functions.
 > ---
 >  gdb/ChangeLog           |   16 ++++++++
 >  gdb/Makefile.in         |    2 +-
 >  gdb/common/agent.c      |   76 +++---------------------------------
 >  gdb/gdbserver/ChangeLog |    7 +++
 >  gdb/gdbserver/target.c  |   58 ++++++++++++++++++++++++++++
 >  gdb/target.c            |   16 ++++++++
 >  gdb/target.h            |   38 +------------------
 >  gdb/target/target.h     |   98 +++++++++++++++++++++++++++++++++++++++++++++++
 >  8 files changed, 204 insertions(+), 107 deletions(-)
 >  create mode 100644 gdb/target/target.h
 > 
 > diff --git a/gdb/Makefile.in b/gdb/Makefile.in
 > index 8429ebc..090c912 100644
 > --- a/gdb/Makefile.in
 > +++ b/gdb/Makefile.in
 > @@ -936,7 +936,7 @@ ctf.h nat/i386-cpuid.h nat/i386-gcc-cpuid.h target/resume.h \
 >  target/wait.h target/waitstatus.h nat/linux-nat.h nat/linux-waitpid.h \
 >  common/print-utils.h common/rsp-low.h nat/i386-dregs.h x86-linux-nat.h \
 >  i386-linux-nat.h common/common-defs.h common/errors.h common/common-types.h \
 > -common/common-debug.h
 > +common/common-debug.h target/target.h
 >  
 >  # Header files that already have srcdir in them, or which are in objdir.
 >  
 > diff --git a/gdb/common/agent.c b/gdb/common/agent.c
 > index 7071ce6..5c19454 100644
 > --- a/gdb/common/agent.c
 > +++ b/gdb/common/agent.c
 > @@ -21,10 +21,9 @@
 >  #include "server.h"
 >  #else
 >  #include "defs.h"
 > -#include "target.h"
 > -#include "infrun.h"
 >  #include "objfiles.h"
 >  #endif
 > +#include "target/target.h"
 >  #include <unistd.h>
 >  #include "agent.h"
 >  #include "filestuff.h"
 > @@ -126,23 +125,9 @@ agent_get_helper_thread_id (void)
 >  {
 >    if  (helper_thread_id == 0)
 >      {
 > -#ifdef GDBSERVER
 > -      if (read_inferior_memory (ipa_sym_addrs.addr_helper_thread_id,
 > -				(unsigned char *) &helper_thread_id,
 > -				sizeof helper_thread_id))
 > -#else
 > -      enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());
 > -      gdb_byte buf[4];
 > -
 > -      if (target_read_memory (ipa_sym_addrs.addr_helper_thread_id,
 > -			      buf, sizeof buf) == 0)
 > -	helper_thread_id = extract_unsigned_integer (buf, sizeof buf,
 > -						     byte_order);
 > -      else
 > -#endif
 > -	{
 > -	  warning (_("Error reading helper thread's id in lib"));
 > -	}
 > +      if (target_read_uint32 (ipa_sym_addrs.addr_helper_thread_id,
 > +			      &helper_thread_id))
 > +	warning (_("Error reading helper thread's id in lib"));
 >      }
 >  
 >    return helper_thread_id;
 > @@ -220,13 +205,8 @@ agent_run_command (int pid, const char *cmd, int len)
 >    int tid = agent_get_helper_thread_id ();
 >    ptid_t ptid = ptid_build (pid, tid, 0);
 >  
 > -#ifdef GDBSERVER
 > -  int ret = write_inferior_memory (ipa_sym_addrs.addr_cmd_buf,
 > -				   (const unsigned char *) cmd, len);
 > -#else
 >    int ret = target_write_memory (ipa_sym_addrs.addr_cmd_buf,
 >  				 (gdb_byte *) cmd, len);
 > -#endif
 >  
 >    if (ret != 0)
 >      {
 > @@ -237,18 +217,7 @@ agent_run_command (int pid, const char *cmd, int len)
 >    DEBUG_AGENT ("agent: resumed helper thread\n");
 >  
 >    /* Resume helper thread.  */
 > -#ifdef GDBSERVER
 > -{
 > -  struct thread_resume resume_info;
 > -
 > -  resume_info.thread = ptid;
 > -  resume_info.kind = resume_continue;
 > -  resume_info.sig = GDB_SIGNAL_0;
 > -  (*the_target->resume) (&resume_info, 1);
 > -}
 > -#else
 > - target_resume (ptid, 0, GDB_SIGNAL_0);
 > -#endif
 > +  target_resume (ptid, 0, GDB_SIGNAL_0);
 >  
 >    fd = gdb_connect_sync_socket (pid);
 >    if (fd >= 0)
 > @@ -284,37 +253,18 @@ agent_run_command (int pid, const char *cmd, int len)
 >        int was_non_stop = non_stop;
 >        /* Stop thread PTID.  */
 >        DEBUG_AGENT ("agent: stop helper thread\n");
 > -#ifdef GDBSERVER
 > -      {
 > -	struct thread_resume resume_info;
 > -
 > -	resume_info.thread = ptid;
 > -	resume_info.kind = resume_stop;
 > -	resume_info.sig = GDB_SIGNAL_0;
 > -	(*the_target->resume) (&resume_info, 1);

I certainly welcome replacing that with target_stop(). :-)

 > -      }
 > -
 > -      non_stop = 1;
 > -      mywait (ptid, &status, 0, 0);

The old gdbserver code set non_stop = 1 *after* asking
the target to stop, whereas now it'll be done before (right?).
Just checking that that's ok.
E.g., I see a test for non_stop in linux_resume (which feels weird to be
using in this context given that we're talking about target_stop :-)).

 > -#else
 >        non_stop = 1;
 >        target_stop (ptid);
 >  
 >        memset (&status, 0, sizeof (status));
 >        target_wait (ptid, &status, 0);
 > -#endif
 >        non_stop = was_non_stop;
 >      }
 >  
 >    if (fd >= 0)
 >      {
 > -#ifdef GDBSERVER
 > -      if (read_inferior_memory (ipa_sym_addrs.addr_cmd_buf,
 > -				(unsigned char *) cmd, IPA_CMD_BUF_SIZE))
 > -#else
 >        if (target_read_memory (ipa_sym_addrs.addr_cmd_buf, (gdb_byte *) cmd,
 >  			      IPA_CMD_BUF_SIZE))
 > -#endif
 >  	{
 >  	  warning (_("Error reading command response"));
 >  	  return -1;
 > @@ -334,20 +284,8 @@ agent_capability_check (enum agent_capa agent_capa)
 >  {
 >    if (agent_capability == 0)
 >      {
 > -#ifdef GDBSERVER
 > -      if (read_inferior_memory (ipa_sym_addrs.addr_capability,
 > -				(unsigned char *) &agent_capability,
 > -				sizeof agent_capability))
 > -#else
 > -      enum bfd_endian byte_order = gdbarch_byte_order (target_gdbarch ());
 > -      gdb_byte buf[4];
 > -
 > -      if (target_read_memory (ipa_sym_addrs.addr_capability,
 > -			      buf, sizeof buf) == 0)
 > -	agent_capability = extract_unsigned_integer (buf, sizeof buf,
 > -						     byte_order);
 > -      else
 > -#endif
 > +      if (target_read_uint32 (ipa_sym_addrs.addr_capability,
 > +			      &agent_capability))
 >  	warning (_("Error reading capability of agent"));
 >      }
 >    return agent_capability & agent_capa;
 > [...]

LGTM with the above question resolved.