[RFC] 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-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(-)
  

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.  */
  

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