diff mbox

[v2,06/23] Make thread_still_needs_step_over consider stepping_over_watchpoint too

Message ID 1428410990-28560-7-git-send-email-palves@redhat.com
State New, archived
Headers

Commit Message

Pedro Alves April 7, 2015, 12:49 p.m. UTC 
  I noticed that even though keep_going knows to start a step over for a
watchpoint, thread_still_needs_step_over doesn't.

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

	* infrun.c (thread_still_needs_step_over): Rename to ...
	(thread_still_needs_step_over_bp): ... this.
	(enum step_over_what): New.
	(thread_still_needs_step_over): Reimplement.
---
 gdb/infrun.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)

Comments

Yao Qi April 8, 2015, 9:28 a.m. UTC | #1 
Pedro Alves <palves@redhat.com> writes:

> +static int
> +thread_still_needs_step_over (struct thread_info *tp)
> +{
> +  struct inferior *inf = find_inferior_ptid (tp->ptid);

'inf' isn't used.

> @@ -6327,6 +6357,8 @@ keep_going (struct execution_control_state *ecs)
>        else
>  	clear_step_over_info ();
>  
> +      ecs->event_thread->control.trap_expected = (remove_bp || remove_wps);
> +
>        /* Stop stepping if inserting breakpoints fails.  */
>        TRY
>  	{
> @@ -6341,8 +6373,6 @@ keep_going (struct execution_control_state *ecs)
>  	}
>        END_CATCH
>  
> -      ecs->event_thread->control.trap_expected = (remove_bp || remove_wps);
> -

Why do we hoist this line in front of the TRY/CATCH block? because we
want to set control.trap_expected before insert_breakpoints which may
throw exception?  We need a ChangeLog entry for it.
Pedro Alves April 13, 2015, 10:47 a.m. UTC | #2 
On 04/08/2015 10:28 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:
> 
>> +static int
>> +thread_still_needs_step_over (struct thread_info *tp)
>> +{
>> +  struct inferior *inf = find_inferior_ptid (tp->ptid);
> 
> 'inf' isn't used.
> 

Whoops.  It used to in an older version.  Dropped.

>> @@ -6327,6 +6357,8 @@ keep_going (struct execution_control_state *ecs)
>>        else
>>  	clear_step_over_info ();
>>  
>> +      ecs->event_thread->control.trap_expected = (remove_bp || remove_wps);
>> +
>>        /* Stop stepping if inserting breakpoints fails.  */
>>        TRY
>>  	{
>> @@ -6341,8 +6373,6 @@ keep_going (struct execution_control_state *ecs)
>>  	}
>>        END_CATCH
>>  
>> -      ecs->event_thread->control.trap_expected = (remove_bp || remove_wps);
>> -
> 
> Why do we hoist this line in front of the TRY/CATCH block? because we
> want to set control.trap_expected before insert_breakpoints which may
> throw exception?  We need a ChangeLog entry for it.

Short version: there's really no reason for that. I've undone that last
week, and all the testing I've been doing so far never tripped on
any problem.

The longer version is that in a earlier version of this series (never posted
anywhere), I had a version of keep_going and resume that only prepared the
resume, but did not insert breakpoints or call 'target_resume', so that we could
aggregate all the target_resume calls into a single call.  That
trap_expected set back then was still done by keep_going.  That caused a lot of
pain and so in the end, I undid all that.  When I put back the insert_breakpoints
etc code I probably put it after the trap_expected line instead
of before, probably just because it's easier to read that way, as there's a
connection between that trap_expected set and the code that comes before the
breakpoints insertion.  And then after staring at these patches for
so long, my brain just learned to ignore this diff's hunk as
not important.  :-)

Thanks!
diff mbox

Patch

diff --git a/gdb/infrun.c b/gdb/infrun.c
index 1e8ee4f..c544362 100644
--- a/gdb/infrun.c
+++ b/gdb/infrun.c
@@ -2521,7 +2521,7 @@  clear_proceed_status (int step)
    meanwhile, we can skip the whole step-over dance.  */
 
 static int
-thread_still_needs_step_over (struct thread_info *tp)
+thread_still_needs_step_over_bp (struct thread_info *tp)
 {
   if (tp->stepping_over_breakpoint)
     {
@@ -2538,6 +2538,34 @@  thread_still_needs_step_over (struct thread_info *tp)
   return 0;
 }
 
+/* Bit flags indicating what the thread needs to step over.  */
+
+enum step_over_what
+  {
+    STEP_OVER_BREAKPOINT = 1,
+    STEP_OVER_WATCHPOINT = 2
+  };
+
+/* Check whether thread TP still needs to start a step-over in order
+   to make progress when resumed.  Returns a union of enum
+   step_over_what bits, indicating what needs to be stepped over.  */
+
+static int
+thread_still_needs_step_over (struct thread_info *tp)
+{
+  struct inferior *inf = find_inferior_ptid (tp->ptid);
+  int what = 0;
+
+  if (thread_still_needs_step_over_bp (tp))
+    what |= STEP_OVER_BREAKPOINT;
+
+  if (tp->stepping_over_watchpoint
+      && !target_have_steppable_watchpoint)
+    what |= STEP_OVER_WATCHPOINT;
+
+  return what;
+}
+
 /* Returns true if scheduler locking applies.  STEP indicates whether
    we're about to do a step/next-like command to a thread.  */
 
@@ -6274,6 +6302,7 @@  keep_going (struct execution_control_state *ecs)
       int remove_bp;
       int remove_wps;
       enum gdb_signal signo;
+      enum step_over_what step_what;
 
       /* Either the trap was not expected, but we are continuing
 	 anyway (if we got a signal, the user asked it be passed to
@@ -6294,11 +6323,6 @@  keep_going (struct execution_control_state *ecs)
 	 instruction, and then re-insert the breakpoint when that step
 	 is finished.  */
 
-      remove_bp = (ecs->hit_singlestep_breakpoint
-		   || thread_still_needs_step_over (ecs->event_thread));
-      remove_wps = (ecs->event_thread->stepping_over_watchpoint
-		    && !target_have_steppable_watchpoint);
-
       /* Do not deliver GDB_SIGNAL_TRAP (except when the user
 	 explicitly specifies that such a signal should be delivered
 	 to the target program).  Typically, that would occur when a
@@ -6315,6 +6339,12 @@  keep_going (struct execution_control_state *ecs)
 
       signo = ecs->event_thread->suspend.stop_signal;
 
+      step_what = thread_still_needs_step_over (ecs->event_thread);
+
+      remove_bp = (ecs->hit_singlestep_breakpoint
+		   || (step_what & STEP_OVER_BREAKPOINT));
+      remove_wps = (step_what & STEP_OVER_WATCHPOINT);
+
       if (remove_bp
 	  && !use_displaced_stepping_now_p (get_regcache_arch (regcache),
 					    signo))
@@ -6327,6 +6357,8 @@  keep_going (struct execution_control_state *ecs)
       else
 	clear_step_over_info ();
 
+      ecs->event_thread->control.trap_expected = (remove_bp || remove_wps);
+
       /* Stop stepping if inserting breakpoints fails.  */
       TRY
 	{
@@ -6341,8 +6373,6 @@  keep_going (struct execution_control_state *ecs)
 	}
       END_CATCH
 
-      ecs->event_thread->control.trap_expected = (remove_bp || remove_wps);
-
       discard_cleanups (old_cleanups);
       resume (ecs->event_thread->suspend.stop_signal);
     }