This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 05/11 v5] Add target/target.h
- From: Doug Evans <dje at google dot com>
- To: Gary Benson <gbenson at redhat dot com>
- Cc: gdb-patches at sourceware dot org, Pedro Alves <palves at redhat dot com>, Tom Tromey <tromey at redhat dot com>
- Date: Wed, 6 Aug 2014 10:49:08 -0700
- Subject: Re: [PATCH 05/11 v5] Add target/target.h
- Authentication-results: sourceware.org; auth=none
- References: <1406888377-25795-1-git-send-email-gbenson at redhat dot com> <1406888377-25795-6-git-send-email-gbenson at redhat dot 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.