Re: [patch] `set scheduler-locking step' made compatible with PPC atomic seqs [fix]

[Jan Kratochvil <jan dot kratochvil at redhat dot com>]

Hi,

followup to my patch post (not commented/accepted):
	http://sources.redhat.com/ml/gdb-patches/2007-06/msg00455.html

Its testcase is still valid:
	gdb-6.6-ppc-atomic-sequence-test1.patch
	http://sources.redhat.com/ml/gdb-patches/2007-06/txt00003.txt

here is a fix of the patch there:
	gdb-6.6-scheduler_locking-step-sw-watchpoints4.patch
	http://sources.redhat.com/ml/gdb-patches/2007-06/txt00002.txt

Replacement of the variable ONESTEP with the tristate RESUME_STEP had a bug
reproducible only on s390 - common arches get SIGTRAP on a second
PTRACE_SINGLESTEP of a still inserted breakpoint.


Regards,
Jan


On Tue, 26 Jun 2007 09:36:40 +0200, Jan Kratochvil wrote:
> Hi,
> 
> if you set `set scheduler-locking step' it will still lock up despite the
> recently accepted atomic sequences stepper if any software watchpoint exists.
> It will start stepping and never let the other thread free the thread locks.
> Such lockup should occur only during manual `step', not due to a watchpoint.
> Red Hat is using default `set scheduler-locking step' (before it got
> implemented upstream there was such implicit mode by a custom patch).
> 
> The first patch implements the fix as a minimal changeset assuming free `int'
> <-> `enum resume_step' interchange.  The second patch implements the same
> functionality by changing all the GDB code to the `enum resume_step' style
> - provided only as a preview, patch is obsolete and a ChangeLog entry would
> need to be included, upon request.
> 
> Attached also a testcase.
> 
> 
> Regards,
> Jan

2007-12-05  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* inferior.h (enum resume_step): New definition.
	(resume): Change STEP parameter type to ENUM RESUME_STEP.
	* infrun.c (resume): Likewise.  Extend debug printing of the STEP
	parameter.  Lock the scheduler only for intentional stepping.
	(proceed): Replace the variable ONESTEP with tristate RESUME_STEP.
	Set the third RESUME_STEP state according to BPSTAT_SHOULD_STEP.
	(currently_stepping): Change the return type to ENUM RESUME_STEP.
	Return RESUME_STEP_NEEDED if it is just due to BPSTAT_SHOULD_STEP.
	* linux-nat.c (select_singlestep_lwp_callback): Do not focus on
	the software watchpoint events.
	* linux-nat.h (struct lwp_info): Redeclare STEP as ENUM RESUME_STEP.

--- ./gdb/inferior.h	23 Aug 2007 18:08:35 -0000	1.85
+++ ./gdb/inferior.h	5 Dec 2007 22:19:59 -0000
@@ -179,7 +179,15 @@ extern void reopen_exec_file (void);
 /* The `resume' routine should only be called in special circumstances.
    Normally, use `proceed', which handles a lot of bookkeeping.  */
 
-extern void resume (int, enum target_signal);
+enum resume_step
+  {
+    /* currently_stepping () should return non-zero for non-continue.  */
+    RESUME_STEP_CONTINUE = 0,
+    RESUME_STEP_USER,		/* Stepping is intentional by the user.  */
+    RESUME_STEP_NEEDED		/* Stepping only for software watchpoints.  */
+  };
+
+extern void resume (enum resume_step, enum target_signal);
 
 /* From misc files */
 
--- ./gdb/infrun.c	30 Nov 2007 10:03:16 -0000	1.255
+++ ./gdb/infrun.c	5 Dec 2007 22:19:59 -0000
@@ -74,7 +74,8 @@ static void set_schedlock_func (char *ar
 
 struct execution_control_state;
 
-static int currently_stepping (struct execution_control_state *ecs);
+static enum resume_step currently_stepping (struct execution_control_state
+									  *ecs);
 
 static void xdb_handle_command (char *args, int from_tty);
 
@@ -494,15 +495,18 @@ set_schedlock_func (char *args, int from
    STEP nonzero if we should step (zero to continue instead).
    SIG is the signal to give the inferior (zero for none).  */
 void
-resume (int step, enum target_signal sig)
+resume (enum resume_step step, enum target_signal sig)
 {
   int should_resume = 1;
   struct cleanup *old_cleanups = make_cleanup (resume_cleanups, 0);
   QUIT;
 
   if (debug_infrun)
-    fprintf_unfiltered (gdb_stdlog, "infrun: resume (step=%d, signal=%d)\n",
-			step, sig);
+    fprintf_unfiltered (gdb_stdlog, "infrun: resume (step=%s, signal=%d)\n",
+			(step == RESUME_STEP_CONTINUE ? "RESUME_STEP_CONTINUE"
+			: (step == RESUME_STEP_USER ? "RESUME_STEP_USER"
+			                            : "RESUME_STEP_NEEDED")),
+			sig);
 
   /* FIXME: calling breakpoint_here_p (read_pc ()) three times! */
 
@@ -618,9 +622,10 @@ a command like `return' or `jump' to con
 	  resume_ptid = inferior_ptid;
 	}
 
-      if ((scheduler_mode == schedlock_on)
+      if (scheduler_mode == schedlock_on
 	  || (scheduler_mode == schedlock_step
-	      && (step || singlestep_breakpoints_inserted_p)))
+	      && (step == RESUME_STEP_USER
+		  || singlestep_breakpoints_inserted_p)))
 	{
 	  /* User-settable 'scheduler' mode requires solo thread resume. */
 	  resume_ptid = inferior_ptid;
@@ -729,7 +734,7 @@ static CORE_ADDR prev_pc;
 void
 proceed (CORE_ADDR addr, enum target_signal siggnal, int step)
 {
-  int oneproc = 0;
+  enum resume_step resume_step = RESUME_STEP_CONTINUE;
 
   if (step > 0)
     step_start_function = find_pc_function (read_pc ());
@@ -743,13 +748,13 @@ proceed (CORE_ADDR addr, enum target_sig
 	   step one instruction before inserting breakpoints so that
 	   we do not stop right away (and report a second hit at this
 	   breakpoint).  */
-	oneproc = 1;
+	resume_step = RESUME_STEP_USER;
       else if (gdbarch_single_step_through_delay_p (current_gdbarch)
               && gdbarch_single_step_through_delay (current_gdbarch,
                                                     get_current_frame ()))
 	/* We stepped onto an instruction that needs to be stepped
 	   again before re-inserting the breakpoint, do so.  */
-	oneproc = 1;
+	resume_step = RESUME_STEP_USER;
     }
   else
     {
@@ -773,9 +778,9 @@ proceed (CORE_ADDR addr, enum target_sig
      that reported the most recent event.  If a step-over is required
      it returns TRUE and sets the current thread to the old thread. */
   if (prepare_to_proceed (step))
-    oneproc = 1;
-
-  if (oneproc)
+    resume_step = RESUME_STEP_USER;
+  
+  if (resume_step == RESUME_STEP_USER)
     /* We will get a trace trap after one instruction.
        Continue it automatically and insert breakpoints then.  */
     trap_expected = 1;
@@ -819,8 +824,13 @@ proceed (CORE_ADDR addr, enum target_sig
      updated correctly when the inferior is stopped.  */
   prev_pc = read_pc ();
 
+  if (step)
+    resume_step = RESUME_STEP_USER;
+  if (resume_step == RESUME_STEP_CONTINUE && bpstat_should_step ())
+    resume_step = RESUME_STEP_NEEDED;
+
   /* Resume inferior.  */
-  resume (oneproc || step || bpstat_should_step (), stop_signal);
+  resume (resume_step, stop_signal);
 
   /* Wait for it to stop (if not standalone)
      and in any case decode why it stopped, and act accordingly.  */
@@ -2729,14 +2739,20 @@ process_event_stop_test:
 
 /* Are we in the middle of stepping?  */
 
-static int
+static enum resume_step
 currently_stepping (struct execution_control_state *ecs)
 {
-  return ((!ecs->handling_longjmp
-	   && ((step_range_end && step_resume_breakpoint == NULL)
-	       || trap_expected))
-	  || ecs->stepping_through_solib_after_catch
-	  || bpstat_should_step ());
+  if (!ecs->handling_longjmp
+      && ((step_range_end && step_resume_breakpoint == NULL) || trap_expected))
+    return RESUME_STEP_USER;
+
+  if (ecs->stepping_through_solib_after_catch)
+    return RESUME_STEP_USER;
+
+  if (bpstat_should_step ())
+    return RESUME_STEP_NEEDED;
+
+  return RESUME_STEP_CONTINUE;
 }
 
 /* Subroutine call with source code we should not step over.  Do step
--- ./gdb/linux-nat.c	11 Oct 2007 19:35:29 -0000	1.70
+++ ./gdb/linux-nat.c	5 Dec 2007 22:19:59 -0000
@@ -1753,7 +1753,10 @@ count_events_callback (struct lwp_info *
 static int
 select_singlestep_lwp_callback (struct lwp_info *lp, void *data)
 {
-  if (lp->step && lp->status != 0)
+  /* We do not focus on software watchpoints as we would not catch
+     STEPPING_PAST_SINGLESTEP_BREAKPOINT breakpoints in some other thread
+     as they would remain pending due to `Push back breakpoint for %s'.  */
+  if (lp->step == RESUME_STEP_USER && lp->status != 0)
     return 1;
   else
     return 0;
--- ./gdb/linux-nat.h	1 Oct 2007 00:22:50 -0000	1.20
+++ ./gdb/linux-nat.h	5 Dec 2007 22:19:59 -0000
@@ -55,8 +55,8 @@ struct lwp_info
   /* If non-zero, a pending wait status.  */
   int status;
 
-  /* Non-zero if we were stepping this LWP.  */
-  int step;
+  /* The kind of stepping of this LWP.  */
+  enum resume_step step;
 
   /* Non-zero si_signo if this LWP stopped with a trap.  si_addr may
      be the address of a hardware watchpoint.  */