Re: [PATCH v2 15/23] Implement all-stop on top of a target running non-stop mode

[Pedro Alves <palves@redhat.com>]

On 04/08/2015 12:08 PM, Pedro Alves wrote:
> 
> Hmm.  Looks like the assertion caught a pre-existing problem.
> This sets up the thread to re-hit the breakpoint at PC once the
> signal handler returns, and lets _all_ threads run.  But, what if had
> _other_ threads that needed a step-over too?  Those will run too,
> and immediately re-trap the same breakpoint, but GDB will re-report them.

I thought it'd be easy to test that with multiple-step-overs.exp test, and
indeed, bad things happened.  I filed bugs and pushed the test in upfront,
as managing all these patch dependencies is getting complicated... 

 [pushed] Add test for PR18214 and PR18216 - multiple step-overs with queued signals
 https://sourceware.org/ml/gdb-patches/2015-04/msg00293.html

> Maybe we should set a step-resume breakpoint on _all_ threads that need
> a step-over, not just the current.  I'll need to think a bit about this.

That works.  I tested it with native | remote, all-stop | non-stop,
software | hardware single-step, and all pass now.

Both PR18214 and PR18216 end up fixed with this series (and this
patch).  I don't exactly (yet) which patch fixed PR18216.

I'll try moving the start_step_over_inferior hunk to the proper patch.
That bit alone should fix the signal-sigtrap.exp assertion you tripped
on.  The rest I'm planning on leaving as a separate patch.

From b0a3b75b242714cbdad0c93ddcabf00af0663d12 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 8 Apr 2015 16:59:56 +0100
Subject: [PATCH] PR18214 - multiple step overs + deliver signal, on sss
 targets

The 'resume' code that detects that we're stepping over a breakpoint
and delivering a signal at the same time:

  /* Currently, our software single-step implementation leads to different
     results than hardware single-stepping in one situation: when stepping
     into delivering a signal which has an associated signal handler,
     hardware single-step will stop at the first instruction of the handler,
     while software single-step will simply skip execution of the handler.
...
     Fortunately, we can at least fix this particular issue.  We detect
     here the case where we are about to deliver a signal while software
     single-stepping with breakpoints removed.  In this situation, we
     revert the decisions to remove all breakpoints and insert single-
     step breakpoints, and instead we install a step-resume breakpoint
     at the current address, deliver the signal without stepping, and
     once we arrive back at the step-resume breakpoint, actually step
     over the breakpoint we originally wanted to step over.  */

doesn't handle the case of another thread also needing to step over a
breakpoint.  It just resumes all threads.  And then that other thread
immediately re-traps the same breakpoint it had stopped for.

Fix it by setting a step-resume breakpoint on _all_ threads that need
a step-over, not just the current.

gdb/ChangeLog:
2015-04-08  Pedro Alves  <palves@redhat.com>

	* infrun.c (start_step_over_inferior): Assert the thread is
	executing.  If in all-stop, also return early if the thread now
	has a step resume set (to skip past a signal handler).
	(setup_step_after_step_resume_breakpoint): New function.
	(resume): When delivering a signal with software single-step, set
	a step-resume breakpoint in all threads that need a step-over, not
	just the current thread.
---
 gdb/infrun.c | 69 ++++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 18 deletions(-)

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 6511c8b..9c85628 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2001,7 +2001,11 @@ start_step_over_inferior (struct inferior *inf)
       if (!ecs->wait_some_more)
 	error (_("Command aborted."));
 
-      if (tp->control.trap_expected)
+      gdb_assert (tp->executing);
+
+      if (tp->control.trap_expected
+	  || (tp->step_after_step_resume_breakpoint
+	      && !target_is_non_stop_p ()))
 	{
 	  if (inf->step_over_queue_head == NULL)
 	    {
@@ -2206,6 +2210,26 @@ do_target_resume (ptid_t resume_ptid, int step, enum gdb_signal sig)
   target_resume (resume_ptid, step, sig);
 }
 
+/* In order to skip a signal handler, set a high-priority step-resume
+   breakpoint the TP's current frame, and setup to step over it once
+   it is reached.  */
+
+static void
+setup_step_after_step_resume_breakpoint (struct thread_info *tp)
+{
+  /* If we have nested signals or a pending signal is delivered
+     immediately after a handler returns, we might already have a
+     step-resume breakpoint set on the earlier handler.  We cannot set
+     another step-resume breakpoint; just continue on until the
+     original breakpoint is hit.  */
+  if (tp->control.step_resume_breakpoint == NULL)
+    {
+      switch_to_thread (tp->ptid);
+      insert_hp_step_resume_breakpoint_at_frame (get_current_frame ());
+      tp->step_after_step_resume_breakpoint = 1;
+    }
+}
+
 /* Resume the inferior, but allow a QUIT.  This is useful if the user
    wants to interrupt some lengthy single-stepping operation
    (for child processes, the SIGINT goes to the inferior, and so
@@ -2470,6 +2494,11 @@ resume (enum gdb_signal sig)
   if (use_software_single_step && execution_direction == EXEC_FORWARD)
     gdb_assert (!step);
 
+  /* Decide the set of threads to ask the target to resume.  Start
+     by assuming everything will be resumed, than narrow the set
+     by applying increasingly restricting conditions.  */
+  resume_ptid = user_visible_resume_ptid (user_step);
+
   /* Currently, our software single-step implementation leads to different
      results than hardware single-stepping in one situation: when stepping
      into delivering a signal which has an associated signal handler,
@@ -2498,21 +2527,30 @@ resume (enum gdb_signal sig)
       && sig != GDB_SIGNAL_0
       && step_over_info_valid_p ())
     {
-      /* If we have nested signals or a pending signal is delivered
-	 immediately after a handler returns, might might already have
-	 a step-resume breakpoint set on the earlier handler.  We cannot
-	 set another step-resume breakpoint; just continue on until the
-	 original breakpoint is hit.  */
-      if (tp->control.step_resume_breakpoint == NULL)
-	{
-	  insert_hp_step_resume_breakpoint_at_frame (get_current_frame ());
-	  tp->step_after_step_resume_breakpoint = 1;
-	}
+      clear_step_over_info ();
+      tp->control.trap_expected = 0;
 
       delete_single_step_breakpoints (tp);
 
-      clear_step_over_info ();
-      tp->control.trap_expected = 0;
+      setup_step_after_step_resume_breakpoint (tp);
+
+      if (!target_is_non_stop_p ())
+	{
+	  struct thread_info *current = tp;
+
+	  ALL_NON_EXITED_THREADS (tp)
+	    if (ptid_match (tp->ptid, resume_ptid))
+	      {
+		if (tp->step_over_next != NULL)
+		  {
+		    thread_step_over_chain_remove (tp);
+		    setup_step_after_step_resume_breakpoint (tp);
+		  }
+	      }
+
+	  tp = current;
+	  switch_to_thread (tp->ptid);
+	}
 
       insert_breakpoints ();
     }
@@ -2523,11 +2561,6 @@ resume (enum gdb_signal sig)
   if (use_software_single_step)
     gdb_assert (!(thread_has_single_step_breakpoints_set (tp) && step));
 
-  /* Decide the set of threads to ask the target to resume.  Start
-     by assuming everything will be resumed, than narrow the set
-     by applying increasingly restricting conditions.  */
-  resume_ptid = user_visible_resume_ptid (user_step);
-
   /* Maybe resume a single thread after all.  */
   if (target_is_non_stop_p ())
     {
-- 
1.9.3