[11/12] Use reinsert_breakpoint for vCont;s
Commit Message
V2: fix spaces in changelog entry,
use maybe_hw_step,
cancel step-over if signal arrives (!maybe_internal_trap),
This patch is to teach GDBserver using software single step to handle
vCont;s. Simply speaking, if the thread's resume request is resume_step,
install reinsert breakpoint at the next pcs when GDBserver is about to
resume threads. These reinsert breakpoints of a thread are removed,
when GDBserver gets an event from that thread. Note that GDBserver may
or may not report this event back to GDB.
gdb/gdbserver:
2016-05-20 Yao Qi <yao.qi@linaro.org>
* linux-low.c (resume_stopped_resumed_lwps): If resume request
is resume_step, call maybe_hw_step.
(linux_wait_1): Stop all threads, remove reinsert breakpoints,
and unstop them.
(linux_resume_one_thread): If resume request is resume_step,
call maybe_hw_step.
(linux_resume): Install reinsert breakpoints if the thread is
requested to resume_step.
(proceed_one_lwp): If resume request is resume_step, call
maybe_hw_step.
(proceed_all_lwps): Install reinsert breakpoints if the thread is
requested to resume_step.
---
gdb/gdbserver/linux-low.c | 99 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 94 insertions(+), 5 deletions(-)
Comments
On 06/02/2016 10:30 AM, Yao Qi wrote:
> @@ -4293,7 +4313,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
>
> step = maybe_hw_step (thread);
> }
> - else
> + else if (lwp->resume != NULL && lwp->resume->kind != resume_step)
> {
> /* If the thread isn't doing step-over, there shouldn't be any
> reinsert breakpoints. */
Consider (non-stop RSP):
-> vCont;s:1
<- OK
-> vCont;s:2
<- OK
The handling of the second vCont sets thread 1's lwp->resume to NULL.
The lwp->resume pointer is only meaningful within linux_resume
and its callees. (But this function is called in other contexts.)
> @@ -5009,12 +5033,52 @@ linux_resume (struct thread_resume *resume_info, size_t n)
> debug_printf ("Resuming, no pending status or step over needed\n");
> }
>
> + /* Before we resume the threads, if resume_step is requested by GDB,
> + stop all threads and install reinsert breakpoints. */
Looking again, I think the rationale for stopping threads should
be mentioned here, as it's not obvious.
> + if (!leave_all_stopped && can_software_single_step ())
> + {
> + struct inferior_list_entry *inf, *tmp;
> +
> + if (debug_threads)
> + debug_printf ("Handle resume_step.\n");
> +
> + ALL_INFERIORS (&all_threads, inf, tmp)
> + {
> + struct thread_info *thread = (struct thread_info *) inf;
> + struct lwp_info *lwp = get_thread_lwp (thread);
> +
> + if (lwp->resume != NULL && lwp->resume->kind == resume_step)
> + {
> + if (!resume_step_is_handled)
> + {
> + stop_all_lwps (0, NULL);
> +
> + if (debug_threads)
> + debug_printf ("Done stopping all threads.\n");
> +
> + resume_step_is_handled = 1;
> + }
> +
> + install_software_single_step_breakpoints (lwp);
> +
> + if (debug_threads)
> + debug_printf ("Insert breakpoint for resume_step LWP %ld\n",
> + lwpid_of (thread));
> + }
> + }
> +
> + if (debug_threads)
> + debug_printf ("Handle resume_step. Done\n");
> + }
> +
> /* Even if we're leaving threads stopped, queue all signals we'd
> otherwise deliver. */
> find_inferior (&all_threads, linux_resume_one_thread, &leave_all_stopped);
>
> if (need_step_over)
> start_step_over (get_thread_lwp (need_step_over));
> + else if (resume_step_is_handled)
> + unstop_all_lwps (0, NULL);
>
> if (debug_threads)
> {
> @@ -5110,7 +5174,8 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
> if (debug_threads)
> debug_printf (" stepping LWP %ld, client wants it stepping\n",
> lwpid_of (thread));
> - step = 1;
> +
> + step = maybe_hw_step (thread);
> }
> else if (lwp->bp_reinsert != 0)
> {
> @@ -5176,6 +5241,30 @@ proceed_all_lwps (void)
> if (debug_threads)
> debug_printf ("Proceeding, no step-over needed\n");
>
> + /* Re-install the reinsert breakpoints on software single step target
> + if the client wants it step. */
> + if (can_software_single_step ())
Not immediately obvious to why is this necessary. Where were they
removed in the first place? I'm it must be necessary, but maybe
extending the comment helps.
> + {
> + struct inferior_list_entry *inf, *tmp;
> +
> + ALL_INFERIORS (&all_threads, inf, tmp)
> + {
> + struct thread_info *thread = (struct thread_info *) inf;
> +
> + if (thread->last_resume_kind == resume_step)
> + {
> + struct lwp_info *lwp = get_thread_lwp (thread);
> +
> + if (!has_reinsert_breakpoints (thread))
> + install_software_single_step_breakpoints (lwp);
> +
> + if (debug_threads)
> + debug_printf ("Insert breakpoint for resume_step LWP %ld\n",
> + lwpid_of (thread));
> + }
> + }
> + }
> +
> find_inferior (&all_threads, proceed_one_lwp, NULL);
> }
Thanks,
Pedro Alves
Pedro Alves <palves@redhat.com> writes:
>> @@ -4293,7 +4313,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
>>
>> step = maybe_hw_step (thread);
>> }
>> - else
>> + else if (lwp->resume != NULL && lwp->resume->kind != resume_step)
>> {
>> /* If the thread isn't doing step-over, there shouldn't be any
>> reinsert breakpoints. */
>
> Consider (non-stop RSP):
>
> -> vCont;s:1
> <- OK
> -> vCont;s:2
> <- OK
>
> The handling of the second vCont sets thread 1's lwp->resume to NULL.
If so, the assert won't be called for thread 1.
> The lwp->resume pointer is only meaningful within linux_resume
> and its callees. (But this function is called in other contexts.)
>
When I wrote the patch, it took me a while to think about this condition
check. I wanted to remove this condition and assert, but finally
decided to leave it there, as it is not harmful. If lwp->resume is only
meaningful within linux_resume and its callees, how about remove the
condition check and assert here?
>> @@ -5009,12 +5033,52 @@ linux_resume (struct thread_resume *resume_info, size_t n)
>> debug_printf ("Resuming, no pending status or step over needed\n");
>> }
>>
>> + /* Before we resume the threads, if resume_step is requested by GDB,
>> + stop all threads and install reinsert breakpoints. */
>
> Looking again, I think the rationale for stopping threads should
> be mentioned here, as it's not obvious.
>
How about this,
/* Before we resume the threads, if resume_step is requested by GDB,
we need to access the inferior memory to install reinsert
breakpoints, so stop all threads. */
>> @@ -5110,7 +5174,8 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
>> if (debug_threads)
>> debug_printf (" stepping LWP %ld, client wants it stepping\n",
>> lwpid_of (thread));
>> - step = 1;
>> +
>> + step = maybe_hw_step (thread);
>> }
>> else if (lwp->bp_reinsert != 0)
>> {
>> @@ -5176,6 +5241,30 @@ proceed_all_lwps (void)
>> if (debug_threads)
>> debug_printf ("Proceeding, no step-over needed\n");
>>
>> + /* Re-install the reinsert breakpoints on software single step target
>> + if the client wants it step. */
>> + if (can_software_single_step ())
>
> Not immediately obvious to why is this necessary. Where were they
> removed in the first place? I'm it must be necessary, but maybe
> extending the comment helps.
How about this
/* On software single step target, we removed reinsert breakpoints
after we get any events from the inferior. If the client wants
thread step, re-install these reinsert breakpoints. */
On 06/14/2016 02:14 PM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
>
>>> @@ -4293,7 +4313,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
>>>
>>> step = maybe_hw_step (thread);
>>> }
>>> - else
>>> + else if (lwp->resume != NULL && lwp->resume->kind != resume_step)
>>> {
>>> /* If the thread isn't doing step-over, there shouldn't be any
>>> reinsert breakpoints. */
>>
>> Consider (non-stop RSP):
>>
>> -> vCont;s:1
>> <- OK
>> -> vCont;s:2
>> <- OK
>>
>> The handling of the second vCont sets thread 1's lwp->resume to NULL.
>
> If so, the assert won't be called for thread 1.
>
>> The lwp->resume pointer is only meaningful within linux_resume
>> and its callees. (But this function is called in other contexts.)
>>
>
> When I wrote the patch, it took me a while to think about this condition
> check. I wanted to remove this condition and assert, but finally
> decided to leave it there, as it is not harmful. If lwp->resume is only
> meaningful within linux_resume and its callees, how about remove the
> condition check and assert here?
Yes, if it's only for the assert, then let's remove it.
>
>>> @@ -5009,12 +5033,52 @@ linux_resume (struct thread_resume *resume_info, size_t n)
>>> debug_printf ("Resuming, no pending status or step over needed\n");
>>> }
>>>
>>> + /* Before we resume the threads, if resume_step is requested by GDB,
>>> + stop all threads and install reinsert breakpoints. */
>>
>> Looking again, I think the rationale for stopping threads should
>> be mentioned here, as it's not obvious.
>>
>
> How about this,
>
> /* Before we resume the threads, if resume_step is requested by GDB,
> we need to access the inferior memory to install reinsert
> breakpoints, so stop all threads. */
That doesn't tell the reader why we need to stop _all_ threads. The
threads that are about to be resumed are obviously stopped, and
thus we could already _access_ inferior memory through them.
I guess this is about flushing instruction caches?
>>> @@ -5176,6 +5241,30 @@ proceed_all_lwps (void)
>>> if (debug_threads)
>>> debug_printf ("Proceeding, no step-over needed\n");
>>>
>>> + /* Re-install the reinsert breakpoints on software single step target
>>> + if the client wants it step. */
>>> + if (can_software_single_step ())
>>
>> Not immediately obvious to why is this necessary. Where were they
>> removed in the first place? I'm it must be necessary, but maybe
>> extending the comment helps.
>
> How about this
>
> /* On software single step target, we removed reinsert breakpoints
> after we get any events from the inferior.
Is that all events, even internal events? From the patch, it seemed
like it was only before reporting an event to gdb.
> If the client wants
> thread step, re-install these reinsert breakpoints. */
>
If we only remove before reporting an event to gdb, then I don't
understand this. We already insert single-step breakpoints when
we process the resume request from gdb, no?
Thanks,
Pedro Alves
@@ -2579,7 +2579,10 @@ resume_stopped_resumed_lwps (struct inferior_list_entry *entry)
&& !lp->status_pending_p
&& thread->last_status.kind == TARGET_WAITKIND_IGNORE)
{
- int step = thread->last_resume_kind == resume_step;
+ int step = 0;
+
+ if (thread->last_resume_kind == resume_step)
+ step = maybe_hw_step (thread);
if (debug_threads)
debug_printf ("RSRL: resuming stopped-resumed LWP %s at %s: step=%d\n",
@@ -3530,6 +3533,23 @@ linux_wait_1 (ptid_t ptid,
return ignore_event (ourstatus);
}
+ /* Remove reinsert breakpoints ... */
+ if (can_software_single_step ()
+ && has_reinsert_breakpoints (current_thread)
+ /*... if GDB requests this thread doing resume_step or ...*/
+ && (current_thread->last_resume_kind == resume_step
+ /* GDBserver has already started the step-over for vCont;s,
+ but it gets some other signal, like SIGSTOP sent by
+ GDBserver for vCont;t or other signal program received. */
+ || !maybe_internal_trap))
+ {
+ stop_all_lwps (1, event_child);
+
+ delete_reinsert_breakpoints (current_ptid);
+
+ unstop_all_lwps (1, event_child);
+ }
+
/* Note that all addresses are always "out of the step range" when
there's no range to begin with. */
in_step_range = lwp_in_step_range (event_child);
@@ -4293,7 +4313,7 @@ linux_resume_one_lwp_throw (struct lwp_info *lwp,
step = maybe_hw_step (thread);
}
- else
+ else if (lwp->resume != NULL && lwp->resume->kind != resume_step)
{
/* If the thread isn't doing step-over, there shouldn't be any
reinsert breakpoints. */
@@ -4853,7 +4873,6 @@ linux_resume_one_thread (struct inferior_list_entry *entry, void *arg)
{
struct thread_info *thread = (struct thread_info *) entry;
struct lwp_info *lwp = get_thread_lwp (thread);
- int step;
int leave_all_stopped = * (int *) arg;
int leave_pending;
@@ -4922,10 +4941,14 @@ linux_resume_one_thread (struct inferior_list_entry *entry, void *arg)
if (!leave_pending)
{
+ int step = 0;
+
if (debug_threads)
debug_printf ("resuming LWP %ld\n", lwpid_of (thread));
- step = (lwp->resume->kind == resume_step);
+ if (lwp->resume->kind == resume_step)
+ step = maybe_hw_step (thread);
+
linux_resume_one_lwp (lwp, step, lwp->resume->sig, NULL);
}
else
@@ -4966,6 +4989,7 @@ linux_resume (struct thread_resume *resume_info, size_t n)
struct thread_info *need_step_over = NULL;
int any_pending;
int leave_all_stopped;
+ int resume_step_is_handled = 0;
if (debug_threads)
{
@@ -5009,12 +5033,52 @@ linux_resume (struct thread_resume *resume_info, size_t n)
debug_printf ("Resuming, no pending status or step over needed\n");
}
+ /* Before we resume the threads, if resume_step is requested by GDB,
+ stop all threads and install reinsert breakpoints. */
+ if (!leave_all_stopped && can_software_single_step ())
+ {
+ struct inferior_list_entry *inf, *tmp;
+
+ if (debug_threads)
+ debug_printf ("Handle resume_step.\n");
+
+ ALL_INFERIORS (&all_threads, inf, tmp)
+ {
+ struct thread_info *thread = (struct thread_info *) inf;
+ struct lwp_info *lwp = get_thread_lwp (thread);
+
+ if (lwp->resume != NULL && lwp->resume->kind == resume_step)
+ {
+ if (!resume_step_is_handled)
+ {
+ stop_all_lwps (0, NULL);
+
+ if (debug_threads)
+ debug_printf ("Done stopping all threads.\n");
+
+ resume_step_is_handled = 1;
+ }
+
+ install_software_single_step_breakpoints (lwp);
+
+ if (debug_threads)
+ debug_printf ("Insert breakpoint for resume_step LWP %ld\n",
+ lwpid_of (thread));
+ }
+ }
+
+ if (debug_threads)
+ debug_printf ("Handle resume_step. Done\n");
+ }
+
/* Even if we're leaving threads stopped, queue all signals we'd
otherwise deliver. */
find_inferior (&all_threads, linux_resume_one_thread, &leave_all_stopped);
if (need_step_over)
start_step_over (get_thread_lwp (need_step_over));
+ else if (resume_step_is_handled)
+ unstop_all_lwps (0, NULL);
if (debug_threads)
{
@@ -5110,7 +5174,8 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
if (debug_threads)
debug_printf (" stepping LWP %ld, client wants it stepping\n",
lwpid_of (thread));
- step = 1;
+
+ step = maybe_hw_step (thread);
}
else if (lwp->bp_reinsert != 0)
{
@@ -5176,6 +5241,30 @@ proceed_all_lwps (void)
if (debug_threads)
debug_printf ("Proceeding, no step-over needed\n");
+ /* Re-install the reinsert breakpoints on software single step target
+ if the client wants it step. */
+ if (can_software_single_step ())
+ {
+ struct inferior_list_entry *inf, *tmp;
+
+ ALL_INFERIORS (&all_threads, inf, tmp)
+ {
+ struct thread_info *thread = (struct thread_info *) inf;
+
+ if (thread->last_resume_kind == resume_step)
+ {
+ struct lwp_info *lwp = get_thread_lwp (thread);
+
+ if (!has_reinsert_breakpoints (thread))
+ install_software_single_step_breakpoints (lwp);
+
+ if (debug_threads)
+ debug_printf ("Insert breakpoint for resume_step LWP %ld\n",
+ lwpid_of (thread));
+ }
+ }
+ }
+
find_inferior (&all_threads, proceed_one_lwp, NULL);
}