[05/11,v5] Add target/target.h
Commit Message
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
Comments
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.
Doug Evans wrote:
> Gary Benson writes:
> > @@ -284,37 +253,18 @@ agent_run_command (int pid, const char *cmd,
> > 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);
> > - }
> > -
> > - non_stop = 1;
> > - mywait (ptid, &status, 0, 0);
> > -#else
> > non_stop = 1;
> > target_stop (ptid);
> >
> > memset (&status, 0, sizeof (status));
> > target_wait (ptid, &status, 0);
> > -#endif
> > non_stop = was_non_stop;
>
> 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 :-)).
Good catch! I did not notice that change. I also don't know if it's
ok.
In the gdbserver case forcing non_stop to 1 causes need_step_over
in linux_resume to become maybe set. If non_stop had been 0
need_step_over would definitely be NULL. So forcing non_stop to 1
beforehand like this patch does means a step over might take place
that would otherwise not have.
In the GDB case forcing non_stop to 1 before target_stop forces GDB
to send a SIGSTOP to each LWP. If non_stop had been 0 linux_nat_stop
would have fallen back to inf_ptrace_stop which sends one SIGINT to
the process group.
I don't know enough about this to know which is the best solution.
Pedro would know, but he's away for the next two weeks. If what is
happening currently is correct in both cases then maybe a new target
method "target_stop_all" is required to encompass the whole block of
code.
In the interests of keeping things moving would you be ok for me to
commit the following until Pedro is back and able to advise?
/* FIXME: This conditional code needs removing, either by
setting non_stop in the same place for both cases or by
implementing a new client method for this whole block
(less the printing code) from "int was_non_stop = non_stop;"
to "non_stop = was_non_stop;". */
#ifdef GDBSERVER
target_stop (ptid);
non_stop = 1;
#else
non_stop = 1;
target_stop (ptid);
#endif
Thanks,
Gary
(Reviewing in pieces)
On 08/01/2014 11:19 AM, Gary Benson wrote:
> +/* See target/target.h. */
> +
> +int
> +target_read_uint32 (CORE_ADDR memaddr, unsigned int *result)
> +{
The type of 'result' should be uint32_t then.
> +
> +/* Resume execution of the target process PTID (or a group of
> + threads). STEP says whether to single-step or to run free; SIGGNAL
> + is the signal to be given to the target, or GDB_SIGNAL_0 for no
> + signal. The caller may not pass GDB_SIGNAL_DEFAULT. A specific
> + PTID means `step/resume only this process id'. A wildcard PTID
> + (all threads, or all threads of process) means `step/resume
> + INFERIOR_PTID, and let other threads (for which the wildcard PTID
> + matches) resume with their 'thread->suspend.stop_signal' signal
> + (usually GDB_SIGNAL_0) if it is in "pass" state, or with no signal
> + if in "no pass" state. */
Most of this comment doesn't make any sense for gdbserver, and I don't
ever will. (GDBserver's resume interface is more flexible than gdb's.)
There's no inferior_ptid in gdbserver. There's no thread->suspend.stop_signal.
In the gdbserver implemention this adds:
> + resume_info.thread = ptid;
> + resume_info.kind = step ? resume_step : resume_continue;
> + resume_info.sig = signal;
> + (*the_target->resume) (&resume_info, 1);
... when STEP is true and PTID is a wildcard, what this actually does
is tell the target to step each thread in the wildcard, all in parallel.
I think we should instead add a new 'target_continue_ptid (ptid_t ptid)'
method, that then consumes/calls gdb's target_resume in gdb's implementation,
and gdbserver's '(*the_target->resume) (&resume_info, 1);' in the
gdbserver implementation?
> +
> +extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
Thanks,
Pedro Alves
On 08/01/2014 11:19 AM, Gary Benson wrote:
> diff --git a/gdb/target.h b/gdb/target.h
> index 4d91b6b..1a10744 100644
> --- a/gdb/target.h
> +++ b/gdb/target.h
> @@ -58,6 +58,7 @@ struct dcache_struct;
> it goes into the file stratum, which is always below the process
> stratum. */
>
> +#include "target/target.h"
> #include "target/resume.h"
> #include "target/wait.h"
> #include "target/waitstatus.h"
> @@ -1206,31 +1207,6 @@ extern void target_detach (const char *, int);
>
> extern void target_disconnect (const char *, int);
>
> -/* Resume execution of the target process PTID (or a group of
> - threads). STEP says whether to single-step or to run free; SIGGNAL
> - is the signal to be given to the target, or GDB_SIGNAL_0 for no
> - signal. The caller may not pass GDB_SIGNAL_DEFAULT. A specific
> - PTID means `step/resume only this process id'. A wildcard PTID
> - (all threads, or all threads of process) means `step/resume
> - INFERIOR_PTID, and let other threads (for which the wildcard PTID
> - matches) resume with their 'thread->suspend.stop_signal' signal
> - (usually GDB_SIGNAL_0) if it is in "pass" state, or with no signal
> - if in "no pass" state. */
> -
> -extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
> -
> -/* Wait for process pid to do something. PTID = -1 to wait for any
> - pid to do something. Return pid of child, or -1 in case of error;
> - store status through argument pointer STATUS. Note that it is
> - _NOT_ OK to throw_exception() out of target_wait() without popping
> - the debugging target from the stack; GDB isn't prepared to get back
> - to the prompt with a debugging target but without the frame cache,
> - stop_pc, etc., set up. OPTIONS is a bitwise OR of TARGET_W*
> - options. */
> -
> -extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
> - int options);
> -
> /* Fetch at least register REGNO, or all regs if regno == -1. No result. */
>
> extern void target_fetch_registers (struct regcache *regcache, int regno);
> @@ -1294,9 +1270,6 @@ int target_supports_disable_randomization (void);
>
> extern int target_read_string (CORE_ADDR, char **, int, int *);
>
> -extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
> - ssize_t len);
> -
> extern int target_read_raw_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
> ssize_t len);
>
> @@ -1304,9 +1277,6 @@ extern int target_read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);
>
> extern int target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);
>
> -extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
> - ssize_t len);
> -
> extern int target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
> ssize_t len);
>
> @@ -1583,12 +1553,6 @@ extern int target_thread_alive (ptid_t ptid);
>
> extern void target_find_new_threads (void);
>
> -/* Make target stop in a continuable fashion. (For instance, under
> - Unix, this should act like SIGSTOP). This function is normally
> - used by GUIs to implement a stop button. */
> -
> -extern void target_stop (ptid_t ptid);
> -
> /* Send the specified COMMAND to the target's monitor
> (shell,interpreter) for execution. The result of the query is
> placed in OUTBUF. */
I worry about the fragmentation that moving pieces of target
interfaces to target/target.h generates ends up confusing
people. Did you consider leaving a marker in place, like:
-extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
- int options);
+ /* For target_wait see target/target.h. */
-extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
- ssize_t len);
+ /* For target_write_memory see target/target.h. */
?
Thanks,
Pedro Alves
Pedro Alves wrote:
> I worry about the fragmentation that moving pieces of target
> interfaces to target/target.h generates ends up confusing
> people. Did you consider leaving a marker in place, like:
>
> -extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
> - int options);
> + /* For target_wait see target/target.h. */
>
> -extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
> - ssize_t len);
> + /* For target_write_memory see target/target.h. */
>
> ?
I can certainly do that. Will include it in the next revision.
Cheers,
Gary
On 08/07/2014 02:48 PM, Gary Benson wrote:
> Doug Evans wrote:
>> Gary Benson writes:
>>> @@ -284,37 +253,18 @@ agent_run_command (int pid, const char *cmd,
>>> 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);
>>> - }
>>> -
>>> - non_stop = 1;
>>> - mywait (ptid, &status, 0, 0);
>>> -#else
>>> non_stop = 1;
>>> target_stop (ptid);
>>>
>>> memset (&status, 0, sizeof (status));
>>> target_wait (ptid, &status, 0);
>>> -#endif
>>> non_stop = was_non_stop;
>>
>> 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 :-)).
>
> Good catch! I did not notice that change. I also don't know if it's
> ok.
>
> In the gdbserver case forcing non_stop to 1 causes need_step_over
> in linux_resume to become maybe set.
> If non_stop had been 0
> need_step_over would definitely be NULL.
That isn't really true, see:
any_pending = 0;
if (!non_stop)
find_inferior (&all_threads, resume_status_pending_p, &any_pending); #1
...
if (!any_pending && supports_breakpoints ())
need_step_over
= (struct thread_info *) find_inferior (&all_threads,
need_step_over_p, NULL);
If non_stop is 0, then we execute #1 above, true. But, that may well
return with ANY_PENDING still clear/0, and so 'need_step_over' may end
up set anyway.
So looks fine to me.
> So forcing non_stop to 1
> beforehand like this patch does means a step over might take place
> that would otherwise not have.
See above.
>
> In the GDB case forcing non_stop to 1 before target_stop forces GDB
> to send a SIGSTOP to each LWP.
Note we're just really just stopping one LWP here, the agent helper
thread, specified in PTID, not all threads.
> If non_stop had been 0 linux_nat_stop
> would have fallen back to inf_ptrace_stop which sends one SIGINT to
> the process group.
>
Yeah, we definitely want SIGSTOP, not SIGINT here. Really, GDB_SIGNAL_0:
SIGSTOP is how we implement "quiesce with no signal" on Linux -- the
SIGSTOP is not visible to the target_wait caller. Unfortunately we have
a mixup of "interrupt/ctrl-c" vs "quiesce" in the interface.
> I don't know enough about this to know which is the best solution.
> Pedro would know, but he's away for the next two weeks. If what is
> happening currently is correct in both cases then maybe a new target
> method "target_stop_all" is required to encompass the whole block of
> code.
>
> In the interests of keeping things moving would you be ok for me to
> commit the following until Pedro is back and able to advise?
>
> /* FIXME: This conditional code needs removing, either by
> setting non_stop in the same place for both cases or by
> implementing a new client method for this whole block
> (less the printing code) from "int was_non_stop = non_stop;"
> to "non_stop = was_non_stop;". */
> #ifdef GDBSERVER
> target_stop (ptid);
> non_stop = 1;
> #else
> non_stop = 1;
> target_stop (ptid);
> #endif
Thanks,
Pedro Alves
Pedro Alves wrote:
> On 08/07/2014 02:48 PM, Gary Benson wrote:
> > Doug Evans wrote:
> > > Gary Benson writes:
> > > > @@ -284,37 +253,18 @@ agent_run_command (int pid, const char *cmd,
> > > > 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);
> > > > - }
> > > > -
> > > > - non_stop = 1;
> > > > - mywait (ptid, &status, 0, 0);
> > > > -#else
> > > > non_stop = 1;
> > > > target_stop (ptid);
> > > >
> > > > memset (&status, 0, sizeof (status));
> > > > target_wait (ptid, &status, 0);
> > > > -#endif
> > > > non_stop = was_non_stop;
> > >
> > > 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 :-)).
> >
> > Good catch! I did not notice that change. I also don't know if it's
> > ok.
> >
> > In the gdbserver case forcing non_stop to 1 causes need_step_over
> > in linux_resume to become maybe set.
>
> > If non_stop had been 0
> > need_step_over would definitely be NULL.
>
> That isn't really true, see:
>
> any_pending = 0;
> if (!non_stop)
> find_inferior (&all_threads, resume_status_pending_p, &any_pending); #1
> ...
> if (!any_pending && supports_breakpoints ())
> need_step_over
> = (struct thread_info *) find_inferior (&all_threads,
> need_step_over_p, NULL);
>
> If non_stop is 0, then we execute #1 above, true. But, that may well
> return with ANY_PENDING still clear/0, and so 'need_step_over' may end
> up set anyway.
>
> So looks fine to me.
>
> > So forcing non_stop to 1
> > beforehand like this patch does means a step over might take place
> > that would otherwise not have.
>
> See above.
>
> > In the GDB case forcing non_stop to 1 before target_stop forces GDB
> > to send a SIGSTOP to each LWP.
>
> Note we're just really just stopping one LWP here, the agent helper
> thread, specified in PTID, not all threads.
>
> > If non_stop had been 0 linux_nat_stop
> > would have fallen back to inf_ptrace_stop which sends one SIGINT to
> > the process group.
>
> Yeah, we definitely want SIGSTOP, not SIGINT here. Really, GDB_SIGNAL_0:
> SIGSTOP is how we implement "quiesce with no signal" on Linux -- the
> SIGSTOP is not visible to the target_wait caller. Unfortunately we have
> a mixup of "interrupt/ctrl-c" vs "quiesce" in the interface.
Pedro, to mirror your new 'target_continue_ptid (ptid_t ptid)' function
suggestion, I thought I might make a new 'target_stop_ptid (ptid_t ptid)'
that would handle the stop/wait combination and the non_stop fiddling.
That way everything will stay exactly as it is. Does that sound ok to
you?
Thanks,
Gary
Hi Gary,
On 08/20/2014 04:01 PM, Gary Benson wrote:
> Pedro, to mirror your new 'target_continue_ptid (ptid_t ptid)' function
> suggestion, I thought I might make a new 'target_stop_ptid (ptid_t ptid)'
> that would handle the stop/wait combination and the non_stop fiddling.
> That way everything will stay exactly as it is. Does that sound ok to
> you?
That sounds OK to me.
Thanks,
Pedro Alves
@@ -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.
@@ -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);
- }
-
- non_stop = 1;
- mywait (ptid, &status, 0, 0);
-#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;
@@ -48,6 +48,22 @@ read_inferior_memory (CORE_ADDR memaddr, unsigned char *myaddr, int len)
return res;
}
+/* See target/target.h. */
+
+int
+target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, int len)
+{
+ return read_inferior_memory (memaddr, myaddr, len);
+}
+
+/* See target/target.h. */
+
+int
+target_read_uint32 (CORE_ADDR memaddr, unsigned int *result)
+{
+ return read_inferior_memory (memaddr, (gdb_byte *) result, sizeof (*result));
+}
+
int
write_inferior_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
int len)
@@ -71,6 +87,14 @@ write_inferior_memory (CORE_ADDR memaddr, const unsigned char *myaddr,
return res;
}
+/* See target/target.h. */
+
+int
+target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr, ssize_t len)
+{
+ return write_inferior_memory (memaddr, myaddr, len);
+}
+
ptid_t
mywait (ptid_t ptid, struct target_waitstatus *ourstatus, int options,
int connected_wait)
@@ -110,6 +134,40 @@ mywait (ptid_t ptid, struct target_waitstatus *ourstatus, int options,
return ret;
}
+/* See target/target.h. */
+
+ptid_t
+target_wait (ptid_t ptid, struct target_waitstatus *status, int options)
+{
+ return mywait (ptid, status, options, 0);
+}
+
+/* See target/target.h. */
+
+void
+target_stop (ptid_t ptid)
+{
+ 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);
+}
+
+/* See target/target.h. */
+
+void
+target_resume (ptid_t ptid, int step, enum gdb_signal signal)
+{
+ struct thread_resume resume_info;
+
+ resume_info.thread = ptid;
+ resume_info.kind = step ? resume_step : resume_continue;
+ resume_info.sig = signal;
+ (*the_target->resume) (&resume_info, 1);
+}
+
int
start_non_stop (int nonstop)
{
@@ -1276,6 +1276,22 @@ target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len)
return TARGET_XFER_E_IO;
}
+/* See target/target.h. */
+
+int
+target_read_uint32 (CORE_ADDR memaddr, unsigned int *result)
+{
+ gdb_byte buf[4];
+ int r;
+
+ r = target_read_memory (memaddr, buf, sizeof buf);
+ if (r != 0)
+ return r;
+ *result = extract_unsigned_integer (buf, sizeof buf,
+ gdbarch_byte_order (target_gdbarch ()));
+ return 0;
+}
+
/* Like target_read_memory, but specify explicitly that this is a read
from the target's raw memory. That is, this read bypasses the
dcache, breakpoint shadowing, etc. */
@@ -58,6 +58,7 @@ struct dcache_struct;
it goes into the file stratum, which is always below the process
stratum. */
+#include "target/target.h"
#include "target/resume.h"
#include "target/wait.h"
#include "target/waitstatus.h"
@@ -1206,31 +1207,6 @@ extern void target_detach (const char *, int);
extern void target_disconnect (const char *, int);
-/* Resume execution of the target process PTID (or a group of
- threads). STEP says whether to single-step or to run free; SIGGNAL
- is the signal to be given to the target, or GDB_SIGNAL_0 for no
- signal. The caller may not pass GDB_SIGNAL_DEFAULT. A specific
- PTID means `step/resume only this process id'. A wildcard PTID
- (all threads, or all threads of process) means `step/resume
- INFERIOR_PTID, and let other threads (for which the wildcard PTID
- matches) resume with their 'thread->suspend.stop_signal' signal
- (usually GDB_SIGNAL_0) if it is in "pass" state, or with no signal
- if in "no pass" state. */
-
-extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
-
-/* Wait for process pid to do something. PTID = -1 to wait for any
- pid to do something. Return pid of child, or -1 in case of error;
- store status through argument pointer STATUS. Note that it is
- _NOT_ OK to throw_exception() out of target_wait() without popping
- the debugging target from the stack; GDB isn't prepared to get back
- to the prompt with a debugging target but without the frame cache,
- stop_pc, etc., set up. OPTIONS is a bitwise OR of TARGET_W*
- options. */
-
-extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
- int options);
-
/* Fetch at least register REGNO, or all regs if regno == -1. No result. */
extern void target_fetch_registers (struct regcache *regcache, int regno);
@@ -1294,9 +1270,6 @@ int target_supports_disable_randomization (void);
extern int target_read_string (CORE_ADDR, char **, int, int *);
-extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
- ssize_t len);
-
extern int target_read_raw_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
ssize_t len);
@@ -1304,9 +1277,6 @@ extern int target_read_stack (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);
extern int target_read_code (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);
-extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
- ssize_t len);
-
extern int target_write_raw_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
ssize_t len);
@@ -1583,12 +1553,6 @@ extern int target_thread_alive (ptid_t ptid);
extern void target_find_new_threads (void);
-/* Make target stop in a continuable fashion. (For instance, under
- Unix, this should act like SIGSTOP). This function is normally
- used by GUIs to implement a stop button. */
-
-extern void target_stop (ptid_t ptid);
-
/* Send the specified COMMAND to the target's monitor
(shell,interpreter) for execution. The result of the query is
placed in OUTBUF. */
new file mode 100644
@@ -0,0 +1,98 @@
+/* Declarations for common target functions.
+
+ 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 TARGET_COMMON_H
+#define TARGET_COMMON_H
+
+#include "target/waitstatus.h"
+
+/* This header is a stopgap until more code is shared. */
+
+/* Resume execution of the target process PTID (or a group of
+ threads). STEP says whether to single-step or to run free; SIGGNAL
+ is the signal to be given to the target, or GDB_SIGNAL_0 for no
+ signal. The caller may not pass GDB_SIGNAL_DEFAULT. A specific
+ PTID means `step/resume only this process id'. A wildcard PTID
+ (all threads, or all threads of process) means `step/resume
+ INFERIOR_PTID, and let other threads (for which the wildcard PTID
+ matches) resume with their 'thread->suspend.stop_signal' signal
+ (usually GDB_SIGNAL_0) if it is in "pass" state, or with no signal
+ if in "no pass" state. */
+
+extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
+
+/* Wait for process pid to do something. PTID = -1 to wait for any
+ pid to do something. Return pid of child, or -1 in case of error;
+ store status through argument pointer STATUS. Note that it is
+ _NOT_ OK to throw_exception() out of target_wait() without popping
+ the debugging target from the stack; GDB isn't prepared to get back
+ to the prompt with a debugging target but without the frame cache,
+ stop_pc, etc., set up. OPTIONS is a bitwise OR of TARGET_W*
+ options. */
+
+extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status,
+ int options);
+
+/* Make target stop in a continuable fashion. (For instance, under
+ Unix, this should act like SIGSTOP). This function is normally
+ used by GUIs to implement a stop button. */
+
+extern void target_stop (ptid_t ptid);
+
+/* Read LEN bytes of target memory at address MEMADDR, placing the
+ results in GDB's memory at MYADDR. Return zero for success,
+ nonzero if any error occurs. Implementations of this function may
+ define and use their own error codes, but functions in the common,
+ nat and target directories must treat the return code as opaque.
+ No guarantee is made about the contents of the data at MYADDR if
+ any error occurs. */
+
+extern int target_read_memory (CORE_ADDR memaddr, gdb_byte *myaddr,
+ ssize_t len);
+
+/* Read an unsigned 32-bit integer in the target's format from target
+ memory at address MEMADDR, storing the result in GDB's format in
+ GDB's memory at RESULT. Return zero for success, nonzero if any
+ error occurs. Implementations of this function may define and use
+ their own error codes, but functions in the common, nat and target
+ directories must treat the return code as opaque. No guarantee is
+ made about the contents of the data at RESULT if any error
+ occurs. */
+
+extern int target_read_uint32 (CORE_ADDR memaddr, unsigned int *result);
+
+/* Write LEN bytes from MYADDR to target memory at address MEMADDR.
+ Return zero for success, nonzero if any error occurs.
+ Implementations of this function may define and use their own error
+ codes, but functions in the common, nat and target directories must
+ treat the return code as opaque. No guarantee is made about the
+ contents of the data at MEMADDR if any error occurs. */
+
+extern int target_write_memory (CORE_ADDR memaddr, const gdb_byte *myaddr,
+ ssize_t len);
+
+/* If set, the inferior should be controlled in non-stop mode. In
+ this mode, each thread is controlled independently. Execution
+ commands apply only to the selected thread by default, and stop
+ events stop only the thread that had the event -- the other threads
+ are kept running freely. */
+
+extern int non_stop;
+
+#endif /* TARGET_COMMON_H */