[PATCH v2 1/4] Remove stale breakpoint step-over information
Luis Machado
luis.machado@linaro.org
Wed Feb 19 11:26:00 GMT 2020
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.
More information about the Gdb-patches
mailing list