[PATCH v2 1/4] Remove stale breakpoint step-over information
Maciej W. Rozycki
macro@wdc.com
Wed Nov 6 20:51:00 GMT 2019
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);
More information about the Gdb-patches
mailing list