[RFC] Remove need_step_over from struct lwp_info

[RFC] Remove need_step_over from struct lwp_info

[Yao Qi]

Hi,
I happen to see that field need_step_over in struct lwp_info is only
used to print a debug info.  need_step_over is set in linux_wait_1
when breakpoint_here is true, however, we check breakpoint_here too in
need_step_over_p and do the step over.  I think we don't need field
need_step_over, and check breakpoint_here directly in need_step_over_p.

This field was added in this patch
https://sourceware.org/ml/gdb-patches/2010-03/msg00605.html and the code
wasn't changed much since then.

This patch is to remove it.

gdb/gdbserver:

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

	* linux-low.h (struct lwp_info) <need_step_over>: Remove.
	* linux-low.c (linux_wait_1): Update.
	(need_step_over_p): Likewise.
---
 gdb/gdbserver/linux-low.c | 13 -------------
 gdb/gdbserver/linux-low.h |  4 ----
 2 files changed, 17 deletions(-)

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index ca6c4f6..559dea2 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -3276,9 +3276,6 @@ linux_wait_1 (ptid_t ptid,
 	     PC), we should step over it.  */
 	  if (debug_threads)
 	    debug_printf ("Hit a gdbserver breakpoint.\n");
-
-	  if (breakpoint_here (event_child->stop_pc))
-	    event_child->need_step_over = 1;
 	}
     }
   else
@@ -4544,12 +4541,6 @@ need_step_over_p (struct inferior_list_entry *entry, void *dummy)
       return 0;
     }
 
-  if (!lwp->need_step_over)
-    {
-      if (debug_threads)
-	debug_printf ("Need step over [LWP %ld]? No\n", lwpid_of (thread));
-    }
-
   if (lwp->status_pending_p)
     {
       if (debug_threads)
@@ -4575,8 +4566,6 @@ need_step_over_p (struct inferior_list_entry *entry, void *dummy)
 		      "Old stop_pc was 0x%s, PC is now 0x%s\n",
 		      lwpid_of (thread),
 		      paddress (lwp->stop_pc), paddress (pc));
-
-      lwp->need_step_over = 0;
       return 0;
     }
 
@@ -4626,8 +4615,6 @@ need_step_over_p (struct inferior_list_entry *entry, void *dummy)
 	     that find_inferior stops looking.  */
 	  current_thread = saved_thread;
 
-	  /* If the step over is cancelled, this is set again.  */
-	  lwp->need_step_over = 0;
 	  return 1;
 	}
     }
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index d4946c1..6e7ddbd 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -365,10 +365,6 @@ struct lwp_info
      a exit-jump-pad-quickly breakpoint.  This is it.  */
   struct breakpoint *exit_jump_pad_bkpt;
 
-  /* True if the LWP was seen stop at an internal breakpoint and needs
-     stepping over later when it is resumed.  */
-  int need_step_over;
-
 #ifdef USE_THREAD_DB
   int thread_known;
   /* The thread handle, used for e.g. TLS access.  Only valid if
-- 
1.9.1

Re: [RFC] Remove need_step_over from struct lwp_info

[Yao Qi]

Yao Qi <qiyaoltc@gmail.com> writes:

> @@ -3276,9 +3276,6 @@ linux_wait_1 (ptid_t ptid,
>  	     PC), we should step over it.  */
>  	  if (debug_threads)
>  	    debug_printf ("Hit a gdbserver breakpoint.\n");
> -
> -	  if (breakpoint_here (event_child->stop_pc))
> -	    event_child->need_step_over = 1;

Looks the comments above should be removed as well,

	  /* If we stepped or ran into an internal breakpoint, we've
	     already handled it.  So next time we resume (from this
	     PC), we should step over it.  */

-- 
Yao (齐尧)

Re: [RFC] Remove need_step_over from struct lwp_info

[Pedro Alves]

On 04/26/2016 08:58 AM, Yao Qi wrote:
> Hi,
> I happen to see that field need_step_over in struct lwp_info is only
> used to print a debug info.  need_step_over is set in linux_wait_1
> when breakpoint_here is true, however, we check breakpoint_here too in
> need_step_over_p and do the step over.  I think we don't need field
> need_step_over, and check breakpoint_here directly in need_step_over_p.
> 
> This field was added in this patch
> https://sourceware.org/ml/gdb-patches/2010-03/msg00605.html and the code
> wasn't changed much since then.
> 
> This patch is to remove it.
> 

The intention was for this:

  if (!lwp->need_step_over)
    {
      if (debug_threads)
	debug_printf ("Need step over [LWP %ld]? No\n", lwpid_of (thread));
    }

to not just be a debug print, but also a return.  Looks like
that might have been only on an earlier prototype, or I messed
it up and removed the return by accident before posting or
some such.


This is why we have comments like:

      if (bp_explains_trap)
	{
	  /* If we stepped or ran into an internal breakpoint, we've
	     already handled it.  So next time we resume (from this
	     PC), we should step over it.  */
	  if (debug_threads)
	    debug_printf ("Hit a gdbserver breakpoint.\n");

	  if (breakpoint_here (event_child->stop_pc))
	    event_child->need_step_over = 1;


The idea was that if the thread happens to be stopped
at a breakpoint, but the breakpoint wasn't actually hit yet, you'd
want to let it be hit, rather than step over it.  E.g., say you have
2 threads, and thread 1 stops for a breakpoint at PC1.  Since we're
in all-stop mode, gdbserver pauses thread 2 as well.  Thread 2 pauses
at PC2.  Now the user sets a breakpoint at PC2.  User continues.
gdbserver at that time would step over the breakpoint at PC2, which
is not what GDB expects.  Likewise, if the user starts a trace session
with a tracepoint at PC2, while thread 2 is stopped at PC2, gdbserver
would skip the tracepoint rather than collect it immediately.

However, since:

 [x86-linux Z0 support, and support multiple breakpoints at the same address]
 https://sourceware.org/ml/gdb-patches/2010-03/msg00917.html

need_step_over_p checks gdb_breakpoint_here, so unless the target doesn't
support Z0 breakpoints, the gdb breakpoint case ends up handled that way.
And the tracepoint case is probably not a big deal, and we can live with
it.

In any case, looks like this never worked upstream, so I'm super
fine with removing the field and cleaning this all up.

Thanks,
Pedro Alves

Re: [RFC] Remove need_step_over from struct lwp_info

[Yao Qi]

Pedro Alves <palves@redhat.com> writes:

> In any case, looks like this never worked upstream, so I'm super
> fine with removing the field and cleaning this all up.

OK, patch is pushed in.

-- 
Yao (齐尧)

From f166f943f30a91792e8754cbca9d7652fc400aae Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Thu, 28 Apr 2016 11:52:23 +0100
Subject: [PATCH] Remove need_step_over from struct lwp_info

Hi,
I happen to see that field need_step_over in struct lwp_info is only
used to print a debug info.  need_step_over is set in linux_wait_1
when breakpoint_here is true, however, we check breakpoint_here too in
need_step_over_p and do the step over.  I think we don't need field
need_step_over, and check breakpoint_here directly in need_step_over_p.

This field was added in this patch
https://sourceware.org/ml/gdb-patches/2010-03/msg00605.html and the code
wasn't changed much since then.

This patch is to remove it.

gdb/gdbserver:

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

	* linux-low.h (struct lwp_info) <need_step_over>: Remove.
	* linux-low.c (linux_wait_1): Update.
	(need_step_over_p): Likewise.

diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index efa0774..8e1e2fc 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -3267,14 +3267,8 @@ linux_wait_1 (ptid_t ptid,
 
       if (bp_explains_trap)
 	{
-	  /* If we stepped or ran into an internal breakpoint, we've
-	     already handled it.  So next time we resume (from this
-	     PC), we should step over it.  */
 	  if (debug_threads)
 	    debug_printf ("Hit a gdbserver breakpoint.\n");
-
-	  if (breakpoint_here (event_child->stop_pc))
-	    event_child->need_step_over = 1;
 	}
     }
   else
@@ -4540,12 +4534,6 @@ need_step_over_p (struct inferior_list_entry *entry, void *dummy)
       return 0;
     }
 
-  if (!lwp->need_step_over)
-    {
-      if (debug_threads)
-	debug_printf ("Need step over [LWP %ld]? No\n", lwpid_of (thread));
-    }
-
   if (lwp->status_pending_p)
     {
       if (debug_threads)
@@ -4571,8 +4559,6 @@ need_step_over_p (struct inferior_list_entry *entry, void *dummy)
 		      "Old stop_pc was 0x%s, PC is now 0x%s\n",
 		      lwpid_of (thread),
 		      paddress (lwp->stop_pc), paddress (pc));
-
-      lwp->need_step_over = 0;
       return 0;
     }
 
@@ -4622,8 +4608,6 @@ need_step_over_p (struct inferior_list_entry *entry, void *dummy)
 	     that find_inferior stops looking.  */
 	  current_thread = saved_thread;
 
-	  /* If the step over is cancelled, this is set again.  */
-	  lwp->need_step_over = 0;
 	  return 1;
 	}
     }
diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index d4946c1..6e7ddbd 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -365,10 +365,6 @@ struct lwp_info
      a exit-jump-pad-quickly breakpoint.  This is it.  */
   struct breakpoint *exit_jump_pad_bkpt;
 
-  /* True if the LWP was seen stop at an internal breakpoint and needs
-     stepping over later when it is resumed.  */
-  int need_step_over;
-
 #ifdef USE_THREAD_DB
   int thread_known;
   /* The thread handle, used for e.g. TLS access.  Only valid if