Patchwork [v3,07/17] Misc switch_back_to_stepped_thread cleanups

login
register
mail settings
Submitter Pedro Alves
Date April 17, 2015, 10:45 a.m.
Message ID <1429267521-21047-8-git-send-email-palves@redhat.com>
Download mbox | patch
Permalink /patch/6289/
State New
Headers show

Comments

Pedro Alves - April 17, 2015, 10:45 a.m.
Several misc cleanups that prepare the tail end of this function, the
part that actually re-resumes the stepped thread.

The most non-obvious would be the currently_stepping change, I guess.
That's because it isn't ever correct to pass step=1 to target_resume
on software single-step targets, and currently_stepping works at a
conceptual higher level, it returns step=true even on software step
targets.  It doesn't really matter on hardware step targets, as the
breakpoint will be hit immediately, but it's just wrong on software
step targets.  I tested it against my x86 software single-step branch,
and it indeed fixes failed assertions (that catch spurious
PTRACE_SINGLESTEP requests) there.

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

	* infrun.c (switch_back_to_stepped_thread): Use ecs->ptid instead
	pf inferior_ptid.  If the stepped thread vanished, return 0
	instead of resuming here.  Use reset_ecs.  Print the prev_pc and
	the current stop_pc in log message.  Clear trap_expected if the
	thread advanced.  Don't pass currently_stepping to
	do_target_resume.
---
 gdb/infrun.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)
Yao Qi - April 21, 2015, 9:50 a.m.
Pedro Alves <palves@redhat.com> writes:

> Several misc cleanups that prepare the tail end of this function, the
> part that actually re-resumes the stepped thread.
>
> The most non-obvious would be the currently_stepping change, I guess.
> That's because it isn't ever correct to pass step=1 to target_resume
> on software single-step targets, and currently_stepping works at a
> conceptual higher level, it returns step=true even on software step
> targets.  It doesn't really matter on hardware step targets, as the
> breakpoint will be hit immediately, but it's just wrong on software
> step targets.  I tested it against my x86 software single-step branch,
> and it indeed fixes failed assertions (that catch spurious
> PTRACE_SINGLESTEP requests) there.
>
> gdb/ChangeLog:
> 2015-04-17  Pedro Alves  <palves@redhat.com>
>
> 	* infrun.c (switch_back_to_stepped_thread): Use ecs->ptid instead
> 	pf inferior_ptid.  If the stepped thread vanished, return 0
        ^^ of

> 	instead of resuming here.  Use reset_ecs.  Print the prev_pc and
> 	the current stop_pc in log message.  Clear trap_expected if the
> 	thread advanced.  Don't pass currently_stepping to
> 	do_target_resume.
Doug Evans - April 22, 2015, 5:21 a.m.
Pedro Alves writes:
 > [...] it isn't ever correct to pass step=1 to target_resume
 > on software single-step targets [...]

Sounds like a good thing to document in a comment or assert.
Can do?

 > , and currently_stepping works at a
 > conceptual higher level, it returns step=true even on software step
 > targets.  It doesn't really matter on hardware step targets, as the
 > breakpoint will be hit immediately, but it's just wrong on software
 > step targets.  I tested it against my x86 software single-step branch,
 > and it indeed fixes failed assertions (that catch spurious
 > PTRACE_SINGLESTEP requests) there.
 > 
 > gdb/ChangeLog:
 > 2015-04-17  Pedro Alves  <palves@redhat.com>
 > 
 > 	* infrun.c (switch_back_to_stepped_thread): Use ecs->ptid instead
 > 	pf inferior_ptid.  If the stepped thread vanished, return 0
 > 	instead of resuming here.  Use reset_ecs.  Print the prev_pc and
 > 	the current stop_pc in log message.  Clear trap_expected if the
 > 	thread advanced.  Don't pass currently_stepping to
 > 	do_target_resume.
 >...
Pedro Alves - April 22, 2015, 8:04 p.m.
On 04/21/2015 10:50 AM, Yao Qi wrote:

>> gdb/ChangeLog:
>> 2015-04-17  Pedro Alves  <palves@redhat.com>
>>
>> 	* infrun.c (switch_back_to_stepped_thread): Use ecs->ptid instead
>> 	pf inferior_ptid.  If the stepped thread vanished, return 0
>         ^^ of

Thanks, fixed.

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index beb9ea2..ce2be31 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -5702,7 +5702,7 @@  switch_back_to_stepped_thread (struct execution_control_state *ecs)
         {
 	  /* Ignore threads of processes we're not resuming.  */
 	  if (!sched_multi
-	      && ptid_get_pid (tp->ptid) != ptid_get_pid (inferior_ptid))
+	      && ptid_get_pid (tp->ptid) != ptid_get_pid (ecs->ptid))
 	    continue;
 
 	  /* When stepping over a breakpoint, we lock all threads
@@ -5766,19 +5766,17 @@  switch_back_to_stepped_thread (struct execution_control_state *ecs)
 				    "stepped thread, it has vanished\n");
 
 	      delete_thread (tp->ptid);
-	      keep_going (ecs);
-	      return 1;
+	      return 0;
 	    }
 
 	  if (debug_infrun)
 	    fprintf_unfiltered (gdb_stdlog,
 				"infrun: switching back to stepped thread\n");
 
-	  ecs->event_thread = tp;
-	  ecs->ptid = tp->ptid;
-	  context_switch (ecs->ptid);
+	  reset_ecs (ecs, tp);
+	  switch_to_thread (tp->ptid);
 
-	  stop_pc = regcache_read_pc (get_thread_regcache (ecs->ptid));
+	  stop_pc = regcache_read_pc (get_thread_regcache (tp->ptid));
 	  frame = get_current_frame ();
 	  gdbarch = get_frame_arch (frame);
 
@@ -5802,23 +5800,28 @@  switch_back_to_stepped_thread (struct execution_control_state *ecs)
 
 	      if (debug_infrun)
 		fprintf_unfiltered (gdb_stdlog,
-				    "infrun: expected thread advanced also\n");
+				    "infrun: expected thread advanced also "
+				    "(%s -> %s)\n",
+				    paddress (target_gdbarch (), tp->prev_pc),
+				    paddress (target_gdbarch (), stop_pc));
 
 	      /* Clear the info of the previous step-over, as it's no
-		 longer valid.  It's what keep_going would do too, if
-		 we called it.  Must do this before trying to insert
-		 the sss breakpoint, otherwise if we were previously
-		 trying to step over this exact address in another
-		 thread, the breakpoint ends up not installed.  */
+		 longer valid (if the thread was trying to step over a
+		 breakpoint, it has already succeeded).  It's what
+		 keep_going would do too, if we called it.  Do this
+		 before trying to insert the sss breakpoint, otherwise
+		 if we were previously trying to step over this exact
+		 address in another thread, the breakpoint is
+		 skipped.  */
 	      clear_step_over_info ();
+	      tp->control.trap_expected = 0;
 
 	      insert_single_step_breakpoint (get_frame_arch (frame),
 					     get_frame_address_space (frame),
 					     stop_pc);
 
 	      resume_ptid = user_visible_resume_ptid (tp->control.stepping_command);
-	      do_target_resume (resume_ptid,
-				currently_stepping (tp), GDB_SIGNAL_0);
+	      do_target_resume (resume_ptid, 0, GDB_SIGNAL_0);
 	      prepare_to_wait (ecs);
 	    }
 	  else