[v2,1/4] Remove stale breakpoint step-over information

  Fix an issue with the `step_over_info' structure introduced with commit 
31e77af205cf ("PR breakpoints/7143 - Watchpoint does not trigger when 
first set"), where information stored in there is not invalidated in a 
remote debug scenario where software stepping in the all-stop mode is 
forcibly interrupted and the target connection then closed.  As a result 
a subsequent target connection triggers an assertion failure on 
`ecs->event_thread->control.trap_expected' and cannot be successfully 
established, making the particular instance of GDB unusable.

This was observed with a `riscv64-linux-gnu' cross-GDB and RISC-V/Linux 
`gdbserver' being developed and an attempt to step over the `ret' aka 
`c.jr ra' instruction where the value held in the `ra' aka `x1' register 
and therefore the address of a software-stepping breakpoint to insert is 
0, as follows:

(gdb) target remote 1.2.3.4:56789
Remote debugging using 1.2.3.4:56789
warning: while parsing target description (at line 4): Target description specified unknown architecture "riscv:rv64id"
warning: Could not load XML target description; ignoring
Reading symbols from .../sysroot/lib/ld-linux-riscv64-lp64d.so.1...
0x0000001555556ef0 in _start ()
   from .../sysroot/lib/ld-linux-riscv64-lp64d.so.1
(gdb) break main
Breakpoint 1 at 0x1643c
(gdb) continue
Continuing.
Cannot access memory at address 0x0
(gdb) x /i $pc
=> 0x15555607b8 <__GI__dl_debug_state>: ret
(gdb) print /x $ra
$1 = 0x0
(gdb) stepi
^C^CInterrupted while waiting for the program.
Give up waiting? (y or n) y
Quit
(gdb) kill
Kill the program being debugged? (y or n) y
[Inferior 1 (process 8964) killed]
(gdb) target remote 1.2.3.4:56789
Remote debugging using 1.2.3.4:56789
warning: while parsing target description (at line 4): Target description specified unknown architecture "riscv:rv64id"
warning: Could not load XML target description; ignoring
.../gdb/infrun.c:5299: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n) y

This is a bug, please report it.  For instructions, see:
<http://www.gnu.org/software/gdb/bugs/>.

.../gdb/infrun.c:5299: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Create a core file of GDB? (y or n) n
Command aborted.
(gdb) 

Correct the issue by making a call to clear global breakpoint step-over 
information from `clear_thread_inferior_resources'.  To do so add an 
argument to `clear_step_over_info' giving the global thread number to 
check against, complementing one given to `set_step_over_info' when the 
information concerned is recorded, so that information is not removed 
for a thread that has stayed alive in a multi-target scenario.

Adjust all the existing `clear_step_over_info' call sites accordingly 
where a step over has completed successfully and the thread that has 
hit the breakpoint is expected to be one for which the breakpoint has 
been inserted.

	gdb/
	* infrun.h (clear_step_over_info): New prototype.
	* infrun.c (thread_is_stepping_over_breakpoint): Move earlier 
	on.
	(clear_step_over_info): Use it.  Make external.
	(resume_1, finish_step_over, handle_signal_stop)
	(keep_going_stepped_thread, keep_going_pass_signal): Adjust 
	accordingly.
	* thread.c (clear_thread_inferior_resources): Call 
	`clear_step_over_info'.
---
Hi Pedro,

On Thu, 31 Oct 2019, Pedro Alves wrote:

> > Correct the issue by making a call to clear global breakpoint step-over 
> > information from `exit_inferior_1', which is where we already do all 
> > kinds of similar clean-ups, e.g. `delete_thread' called from there 
> > clears per-thread step-over information.
> 
> This looks like a fragile place to clear this, considering
> multiprocess.  I.e., what if we're calling exit_inferior for some
> inferior other than the one that was stepping.

 Good point.

> The step over information is associated with a thread.
> I think it'd be better to clear the step over information
> when the corresponding thread is deleted.
> 
> So something like adding a thread_info parameter to
> clear_step_over_info, and then calling clear_step_over_info
> from clear_thread_inferior_resources.  clear_step_over_info
> would only clear the info if the thread matched, or if NULL
> is passed.  Would that work?

 Thanks for your suggestion.  That indeed works except that I have figured 
out we actually always have a thread to match available when making a call 
to `clear_step_over_info', so I have not implemented a special NULL case, 
and I'm not convinced matching thread numbers ahead of the call is worth 
an assertion either.

 OK to apply?

  Maciej

Changes from v1:

- Add and check against thread number argument to `clear_step_over_info'.

- Call the function from `clear_thread_inferior_resources' rather than 
  `exit_inferior_1'.

- Use the thread number argument for `clear_step_over_info' throughout.
---
 gdb/infrun.c |   40 +++++++++++++++++++++-------------------
 gdb/infrun.h |    4 ++++
 gdb/thread.c |    3 +++
 3 files changed, 28 insertions(+), 19 deletions(-)

gdb-clear-step-over-info.diff
  

On 2019-11-06 3:51 p.m., Maciej W. Rozycki wrote:
> Fix an issue with the `step_over_info' structure introduced with commit 
> 31e77af205cf ("PR breakpoints/7143 - Watchpoint does not trigger when 
> first set"), where information stored in there is not invalidated in a 
> remote debug scenario where software stepping in the all-stop mode is 
> forcibly interrupted and the target connection then closed.  As a result 
> a subsequent target connection triggers an assertion failure on 
> `ecs->event_thread->control.trap_expected' and cannot be successfully 
> established, making the particular instance of GDB unusable.
> 
> This was observed with a `riscv64-linux-gnu' cross-GDB and RISC-V/Linux 
> `gdbserver' being developed and an attempt to step over the `ret' aka 
> `c.jr ra' instruction where the value held in the `ra' aka `x1' register 
> and therefore the address of a software-stepping breakpoint to insert is 
> 0, as follows:
> 
> (gdb) target remote 1.2.3.4:56789
> Remote debugging using 1.2.3.4:56789
> warning: while parsing target description (at line 4): Target description specified unknown architecture "riscv:rv64id"
> warning: Could not load XML target description; ignoring
> Reading symbols from .../sysroot/lib/ld-linux-riscv64-lp64d.so.1...
> 0x0000001555556ef0 in _start ()
>    from .../sysroot/lib/ld-linux-riscv64-lp64d.so.1
> (gdb) break main
> Breakpoint 1 at 0x1643c
> (gdb) continue
> Continuing.
> Cannot access memory at address 0x0
> (gdb) x /i $pc
> => 0x15555607b8 <__GI__dl_debug_state>: ret
> (gdb) print /x $ra
> $1 = 0x0
> (gdb) stepi
> ^C^CInterrupted while waiting for the program.
> Give up waiting? (y or n) y
> Quit
> (gdb) kill
> Kill the program being debugged? (y or n) y
> [Inferior 1 (process 8964) killed]
> (gdb) target remote 1.2.3.4:56789
> Remote debugging using 1.2.3.4:56789
> warning: while parsing target description (at line 4): Target description specified unknown architecture "riscv:rv64id"
> warning: Could not load XML target description; ignoring
> .../gdb/infrun.c:5299: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) y
> 
> This is a bug, please report it.  For instructions, see:
> <http://www.gnu.org/software/gdb/bugs/>.
> 
> .../gdb/infrun.c:5299: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Create a core file of GDB? (y or n) n
> Command aborted.
> (gdb) 
> 
> Correct the issue by making a call to clear global breakpoint step-over 
> information from `clear_thread_inferior_resources'.  To do so add an 
> argument to `clear_step_over_info' giving the global thread number to 
> check against, complementing one given to `set_step_over_info' when the 
> information concerned is recorded, so that information is not removed 
> for a thread that has stayed alive in a multi-target scenario.
> 
> Adjust all the existing `clear_step_over_info' call sites accordingly 
> where a step over has completed successfully and the thread that has 
> hit the breakpoint is expected to be one for which the breakpoint has 
> been inserted.
> 
> 	gdb/
> 	* infrun.h (clear_step_over_info): New prototype.
> 	* infrun.c (thread_is_stepping_over_breakpoint): Move earlier 
> 	on.
> 	(clear_step_over_info): Use it.  Make external.
> 	(resume_1, finish_step_over, handle_signal_stop)
> 	(keep_going_stepped_thread, keep_going_pass_signal): Adjust 
> 	accordingly.
> 	* thread.c (clear_thread_inferior_resources): Call 
> 	`clear_step_over_info'.

This patch looks good to me, but I'd really prefer if Pedro could look at it,
since he's much more adept in this area.

Simon
  

On 11/6/19 5:51 PM, Maciej W. Rozycki wrote:
> Fix an issue with the `step_over_info' structure introduced with commit
> 31e77af205cf ("PR breakpoints/7143 - Watchpoint does not trigger when
> first set"), where information stored in there is not invalidated in a
> remote debug scenario where software stepping in the all-stop mode is
> forcibly interrupted and the target connection then closed.  As a result
> a subsequent target connection triggers an assertion failure on
> `ecs->event_thread->control.trap_expected' and cannot be successfully
> established, making the particular instance of GDB unusable.
> 
> This was observed with a `riscv64-linux-gnu' cross-GDB and RISC-V/Linux
> `gdbserver' being developed and an attempt to step over the `ret' aka
> `c.jr ra' instruction where the value held in the `ra' aka `x1' register
> and therefore the address of a software-stepping breakpoint to insert is
> 0, as follows:
> 
> (gdb) target remote 1.2.3.4:56789
> Remote debugging using 1.2.3.4:56789
> warning: while parsing target description (at line 4): Target description specified unknown architecture "riscv:rv64id"
> warning: Could not load XML target description; ignoring
> Reading symbols from .../sysroot/lib/ld-linux-riscv64-lp64d.so.1...
> 0x0000001555556ef0 in _start ()
>     from .../sysroot/lib/ld-linux-riscv64-lp64d.so.1
> (gdb) break main
> Breakpoint 1 at 0x1643c
> (gdb) continue
> Continuing.
> Cannot access memory at address 0x0
> (gdb) x /i $pc
> => 0x15555607b8 <__GI__dl_debug_state>: ret
> (gdb) print /x $ra
> $1 = 0x0
> (gdb) stepi
> ^C^CInterrupted while waiting for the program.
> Give up waiting? (y or n) y
> Quit
> (gdb) kill
> Kill the program being debugged? (y or n) y
> [Inferior 1 (process 8964) killed]
> (gdb) target remote 1.2.3.4:56789
> Remote debugging using 1.2.3.4:56789
> warning: while parsing target description (at line 4): Target description specified unknown architecture "riscv:rv64id"
> warning: Could not load XML target description; ignoring
> .../gdb/infrun.c:5299: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Quit this debugging session? (y or n) y
> 
> This is a bug, please report it.  For instructions, see:
> <http://www.gnu.org/software/gdb/bugs/>.
> 
> .../gdb/infrun.c:5299: internal-error: int finish_step_over(execution_control_state*): Assertion `ecs->event_thread->control.trap_expected' failed.
> A problem internal to GDB has been detected,
> further debugging may prove unreliable.
> Create a core file of GDB? (y or n) n
> Command aborted.
> (gdb)
> 
> Correct the issue by making a call to clear global breakpoint step-over
> information from `clear_thread_inferior_resources'.  To do so add an
> argument to `clear_step_over_info' giving the global thread number to
> check against, complementing one given to `set_step_over_info' when the
> information concerned is recorded, so that information is not removed
> for a thread that has stayed alive in a multi-target scenario.
> 
> Adjust all the existing `clear_step_over_info' call sites accordingly
> where a step over has completed successfully and the thread that has
> hit the breakpoint is expected to be one for which the breakpoint has
> been inserted.
> 
> 	gdb/
> 	* infrun.h (clear_step_over_info): New prototype.
> 	* infrun.c (thread_is_stepping_over_breakpoint): Move earlier
> 	on.
> 	(clear_step_over_info): Use it.  Make external.
> 	(resume_1, finish_step_over, handle_signal_stop)
> 	(keep_going_stepped_thread, keep_going_pass_signal): Adjust
> 	accordingly.
> 	* thread.c (clear_thread_inferior_resources): Call
> 	`clear_step_over_info'.
> ---
> Hi Pedro,
> 
> On Thu, 31 Oct 2019, Pedro Alves wrote:
> 
>>> Correct the issue by making a call to clear global breakpoint step-over
>>> information from `exit_inferior_1', which is where we already do all
>>> kinds of similar clean-ups, e.g. `delete_thread' called from there
>>> clears per-thread step-over information.
>>
>> This looks like a fragile place to clear this, considering
>> multiprocess.  I.e., what if we're calling exit_inferior for some
>> inferior other than the one that was stepping.
> 
>   Good point.
> 
>> The step over information is associated with a thread.
>> I think it'd be better to clear the step over information
>> when the corresponding thread is deleted.
>>
>> So something like adding a thread_info parameter to
>> clear_step_over_info, and then calling clear_step_over_info
>> from clear_thread_inferior_resources.  clear_step_over_info
>> would only clear the info if the thread matched, or if NULL
>> is passed.  Would that work?
> 
>   Thanks for your suggestion.  That indeed works except that I have figured
> out we actually always have a thread to match available when making a call
> to `clear_step_over_info', so I have not implemented a special NULL case,
> and I'm not convinced matching thread numbers ahead of the call is worth
> an assertion either.
> 
>   OK to apply?
> 
>    Maciej
> 
> Changes from v1:
> 
> - Add and check against thread number argument to `clear_step_over_info'.
> 
> - Call the function from `clear_thread_inferior_resources' rather than
>    `exit_inferior_1'.
> 
> - Use the thread number argument for `clear_step_over_info' throughout.
> ---
>   gdb/infrun.c |   40 +++++++++++++++++++++-------------------
>   gdb/infrun.h |    4 ++++
>   gdb/thread.c |    3 +++
>   3 files changed, 28 insertions(+), 19 deletions(-)
> 
> gdb-clear-step-over-info.diff
> Index: binutils-gdb/gdb/infrun.c
> ===================================================================
> --- binutils-gdb.orig/gdb/infrun.c
> +++ binutils-gdb/gdb/infrun.c
> @@ -1330,12 +1330,23 @@ set_step_over_info (const address_space
>     step_over_info.thread = thread;
>   }
>   
> -/* Called when we're not longer stepping over a breakpoint / an
> -   instruction, so all breakpoints are free to be (re)inserted.  */
> +/* See infrun.h.  */
>   
> -static void
> -clear_step_over_info (void)
> +int
> +thread_is_stepping_over_breakpoint (int thread)
> +{
> +  return (step_over_info.thread != -1
> +	  && thread == step_over_info.thread);
> +}
> +
> +/* See infrun.h.  */
> +
> +void
> +clear_step_over_info (int thread)
>   {
> +  if (!thread_is_stepping_over_breakpoint (thread))
> +    return;
> +

Since the thread number is only used to check if a particular thread is 
stepping over a breakpoint, wouldn't it be better to move the check to 
thread.c as opposed to embedding it in clear_step_over_info and having 
to change all of its callers?

>     if (debug_infrun)
>       fprintf_unfiltered (gdb_stdlog,
>   			"infrun: clear_step_over_info\n");
> @@ -1360,15 +1371,6 @@ stepping_past_instruction_at (struct add
>   /* See infrun.h.  */
>   
>   int
> -thread_is_stepping_over_breakpoint (int thread)
> -{
> -  return (step_over_info.thread != -1
> -	  && thread == step_over_info.thread);
> -}
> -
> -/* See infrun.h.  */
> -
> -int
>   stepping_past_nonsteppable_watchpoint (void)
>   {
>     return step_over_info.nonsteppable_watchpoint_p;
> @@ -2339,7 +2341,7 @@ resume_1 (enum gdb_signal sig)
>   				"infrun: resume: skipping permanent breakpoint, "
>   				"deliver signal first\n");
>   
> -	  clear_step_over_info ();
> +	  clear_step_over_info (tp->global_num);
>   	  tp->control.trap_expected = 0;
>   
>   	  if (tp->control.step_resume_breakpoint == NULL)
> @@ -2496,7 +2498,7 @@ resume_1 (enum gdb_signal sig)
>   
>         delete_single_step_breakpoints (tp);
>   
> -      clear_step_over_info ();
> +      clear_step_over_info (tp->global_num);
>         tp->control.trap_expected = 0;
>   
>         insert_breakpoints ();
> @@ -5298,7 +5300,7 @@ finish_step_over (struct execution_contr
>   	 back an event.  */
>         gdb_assert (ecs->event_thread->control.trap_expected);
>   
> -      clear_step_over_info ();
> +      clear_step_over_info (ecs->event_thread->global_num);
>       }
>   
>     if (!target_is_non_stop_p ())
> @@ -5913,7 +5915,7 @@ handle_signal_stop (struct execution_con
>                                   "infrun: signal may take us out of "
>                                   "single-step range\n");
>   
> -	  clear_step_over_info ();
> +	  clear_step_over_info (ecs->event_thread->global_num);
>   	  insert_hp_step_resume_breakpoint_at_frame (frame);
>   	  ecs->event_thread->step_after_step_resume_breakpoint = 1;
>   	  /* Reset trap_expected to ensure breakpoints are re-inserted.  */
> @@ -7004,7 +7006,7 @@ keep_going_stepped_thread (struct thread
>   	 breakpoint, otherwise if we were previously trying to step
>   	 over this exact address in another thread, the breakpoint is
>   	 skipped.  */
> -      clear_step_over_info ();
> +      clear_step_over_info (tp->global_num);
>         tp->control.trap_expected = 0;
>   
>         insert_single_step_breakpoint (get_frame_arch (frame),
> @@ -7547,7 +7549,7 @@ keep_going_pass_signal (struct execution
>   	{
>   	  exception_print (gdb_stderr, e);
>   	  stop_waiting (ecs);
> -	  clear_step_over_info ();
> +	  clear_step_over_info (ecs->event_thread->global_num);
>   	  return;
>   	}
>   
> Index: binutils-gdb/gdb/infrun.h
> ===================================================================
> --- binutils-gdb.orig/gdb/infrun.h
> +++ binutils-gdb/gdb/infrun.h
> @@ -120,6 +120,10 @@ extern void insert_step_resume_breakpoin
>   						  struct symtab_and_line ,
>   						  struct frame_id);
>   
> +/* Called when we're not longer stepping over a breakpoint / an
> +   instruction, so all breakpoints are free to be (re)inserted.  */
> +void clear_step_over_info (int thread);
> +
>   /* Returns true if we're trying to step past the instruction at
>      ADDRESS in ASPACE.  */
>   extern int stepping_past_instruction_at (struct address_space *aspace,
> Index: binutils-gdb/gdb/thread.c
> ===================================================================
> --- binutils-gdb.orig/gdb/thread.c
> +++ binutils-gdb/gdb/thread.c
> @@ -191,6 +191,9 @@ clear_thread_inferior_resources (struct
>   
>     delete_longjmp_breakpoint_at_next_stop (tp->global_num);
>   
> +  /* Remove any stale breakpoint step-over information.  */
> +  clear_step_over_info (tp->global_num);
> +

So something like...

/* If this thread has a pending step-over request, clear it now.  */
if (thread_is_stepping_over_breakpoint (tp->global_num))
   clear_step_over_info ();

>     bpstat_clear (&tp->control.stop_bpstat);
>   
>     btrace_teardown (tp);
> 

Otherwise it looks OK to me, assuming the testsuite has executed 
properly against the thread tests.