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

Message ID alpine.LFD.2.21.1911051531570.13542@redsun52.ssa.fujisawa.hgst.com
State New
Headers show
Series
  • GDB fixes for the remote end having gone astray
Related show

Commit Message

Maciej W. Rozycki Nov. 6, 2019, 8:51 p.m.
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

Comments

Luis Machado Feb. 19, 2020, 11:26 a.m. | #1
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.

Patch

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);