[7/8] Resume the inferior with signal rather than stepping over

Message ID 1457088276-1170-8-git-send-email-yao.qi@linaro.org
State New, archived
Headers

Commit Message

Yao Qi March 4, 2016, 10:44 a.m. UTC
  When GDBserver steps over a breakpoint using software single step, it
enqueues the signal, single step and deliver the signal in the next
resume if step over is not needed.  In this way, the program won't
receive the signal if the conditional breakpoint is set a branch to
self instruction, because the step over is always needed.

This patch removes the restriction that don't deliver the signal to
the inferior if we are trying to reinsert a breakpoint for software
single step and change the decision on resume vs. step-over when the
LWP has pending signals to deliver.

gdb/gdbserver:

2016-03-04  Yao Qi  <yao.qi@linaro.org>

	* linux-low.c (LWP_SIGNAL_CAN_BE_DELIVERED): Adjust.
	(need_step_over_p): Return zero if the LWP has pending signals
	can be delivered on software single step target.
---
 gdb/gdbserver/linux-low.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)
  

Comments

Pedro Alves March 11, 2016, 12:04 p.m. UTC | #1
On 03/04/2016 10:44 AM, Yao Qi wrote:
> When GDBserver steps over a breakpoint using software single step, it
> enqueues the signal, single step and deliver the signal in the next
> resume if step over is not needed.  In this way, the program won't
> receive the signal if the conditional breakpoint is set a branch to
> self instruction, because the step over is always needed.
> 
> This patch removes the restriction that don't deliver the signal to
> the inferior if we are trying to reinsert a breakpoint for software
> single step and change the decision on resume vs. step-over when the
> LWP has pending signals to deliver.

Once the handler returns, it'll retrap the same breakpoint we already stopped
for, and thus I think we'll count a spurious tracepoint/breakpoint hit.

Sounds like we'll need to do like GDB does and remember that we are advancing
past a signal handler, and need to get back to stepping over the breakpoint
if/when the handler returns successfully?

> 
> gdb/gdbserver:
> 
> 2016-03-04  Yao Qi  <yao.qi@linaro.org>
> 
> 	* linux-low.c (LWP_SIGNAL_CAN_BE_DELIVERED): Adjust.
> 	(need_step_over_p): Return zero if the LWP has pending signals
> 	can be delivered on software single step target.
> ---
>   gdb/gdbserver/linux-low.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 2330e67..9bae787 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -4119,12 +4119,10 @@ single_step (struct lwp_info* lwp)
>   }
>   
>   /* The signal can not be delivered to the inferior if we are trying to
> -   reinsert a breakpoint for software single step or we're trying to
>      finish a fast tracepoint collect.  */
>   
>   #define LWP_SIGNAL_CAN_BE_DELIVERED(LWP)			\
> -  !(((LWP)->bp_reinsert != 0 && can_software_single_step ())	\
> -    || (LWP)->collecting_fast_tracepoint)
> +  !((LWP)->collecting_fast_tracepoint)
>   
>   /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
>      SIGNAL is nonzero, give it that signal.  */
> @@ -4572,6 +4570,20 @@ need_step_over_p (struct inferior_list_entry *entry, void *dummy)
>         return 0;
>       }
>   
> +  /* On software single step target, resume the inferior with signal
> +     rather than stepping over.  */
> +  if (can_software_single_step ()
> +      && lwp->pending_signals != NULL
> +      && LWP_SIGNAL_CAN_BE_DELIVERED (lwp))
> +    {
> +      if (debug_threads)
> +	debug_printf ("Need step over [LWP %ld]? Ignoring, has pending"
> +		      " signals.\n",
> +		      lwpid_of (thread));
> +
> +      return 0;

I notice this missed restoring the current thread (below).

> +    }
> +
>     saved_thread = current_thread;
>     current_thread = thread;
>   
>
  
Yao Qi March 21, 2016, 9:39 a.m. UTC | #2
Pedro Alves <palves@redhat.com> writes:

> Once the handler returns, it'll retrap the same breakpoint we already stopped
> for, and thus I think we'll count a spurious tracepoint/breakpoint hit.
>
> Sounds like we'll need to do like GDB does and remember that we are advancing
> past a signal handler, and need to get back to stepping over the breakpoint
> if/when the handler returns successfully?

I think it is no longer an issue as we live with the spurious double
trap, right?

>>   
>> +  /* On software single step target, resume the inferior with signal
>> +     rather than stepping over.  */
>> +  if (can_software_single_step ()
>> +      && lwp->pending_signals != NULL
>> +      && LWP_SIGNAL_CAN_BE_DELIVERED (lwp))
>> +    {
>> +      if (debug_threads)
>> +	debug_printf ("Need step over [LWP %ld]? Ignoring, has pending"
>> +		      " signals.\n",
>> +		      lwpid_of (thread));
>> +
>> +      return 0;
>
> I notice this missed restoring the current thread (below).
>

The code below is to switch current_thread to thread, not restoring.
Since we don't switch current_thread here, we don't have to do anything.

>> +    }
>> +
>>     saved_thread = current_thread;
>>     current_thread = thread;
  

Patch

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index 2330e67..9bae787 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -4119,12 +4119,10 @@  single_step (struct lwp_info* lwp)
 }
 
 /* The signal can not be delivered to the inferior if we are trying to
-   reinsert a breakpoint for software single step or we're trying to
    finish a fast tracepoint collect.  */
 
 #define LWP_SIGNAL_CAN_BE_DELIVERED(LWP)			\
-  !(((LWP)->bp_reinsert != 0 && can_software_single_step ())	\
-    || (LWP)->collecting_fast_tracepoint)
+  !((LWP)->collecting_fast_tracepoint)
 
 /* Resume execution of LWP.  If STEP is nonzero, single-step it.  If
    SIGNAL is nonzero, give it that signal.  */
@@ -4572,6 +4570,20 @@  need_step_over_p (struct inferior_list_entry *entry, void *dummy)
       return 0;
     }
 
+  /* On software single step target, resume the inferior with signal
+     rather than stepping over.  */
+  if (can_software_single_step ()
+      && lwp->pending_signals != NULL
+      && LWP_SIGNAL_CAN_BE_DELIVERED (lwp))
+    {
+      if (debug_threads)
+	debug_printf ("Need step over [LWP %ld]? Ignoring, has pending"
+		      " signals.\n",
+		      lwpid_of (thread));
+
+      return 0;
+    }
+
   saved_thread = current_thread;
   current_thread = thread;