Message ID | alpine.LFD.2.21.1911051531570.13542@redsun52.ssa.fujisawa.hgst.com |
---|---|
State | New |
Headers | show |
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.
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);