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

["Maciej W. Rozycki" <macro at wdc dot com>]

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;
+
   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);
+
   bpstat_clear (&tp->control.stop_bpstat);
 
   btrace_teardown (tp);