[RFA,7/8] Avoid crashes when stepping through ravenscar context-switching
Commit Message
When stepping through the ravenscar context-switching code, gdb will
try to single-step the current thread, while leaving the other threads
stopped. This results in an assertion failure in finish_step_over,
because the event thread is not the request thread.
This patch introduces a new target method so that infrun can ask the
target whether this can happen; and then changes infrun to handle this
case. When the target can have an unexpected thread switch,
single-stepping may also switch threads.
gdb/ChangeLog
2019-03-26 Tom Tromey <tromey@adacore.com>
* infrun.c (finish_step_over): Check
target_can_randomly_thread_switch.
(handle_signal_stop): Handle random thread switches.
* ravenscar-thread.c (struct ravenscar_thread_target)
<can_randomly_thread_switch>: New method.
* target-delegates.c: Rebuild.
* target.h (struct target_ops) <can_randomly_thread_switch>:
New method.
(target_can_randomly_thread_switch): New define.
---
gdb/ChangeLog | 12 ++++++++++++
gdb/infrun.c | 39 ++++++++++++++++++++++++++++++++-------
gdb/ravenscar-thread.c | 5 +++++
gdb/target-delegates.c | 27 +++++++++++++++++++++++++++
gdb/target.h | 12 ++++++++++++
5 files changed, 88 insertions(+), 7 deletions(-)
Comments
On 3/26/19 7:44 AM, Tom Tromey wrote:
> When stepping through the ravenscar context-switching code, gdb will
> try to single-step the current thread, while leaving the other threads
> stopped. This results in an assertion failure in finish_step_over,
> because the event thread is not the request thread.
>
> This patch introduces a new target method so that infrun can ask the
> target whether this can happen; and then changes infrun to handle this
> case. When the target can have an unexpected thread switch,
> single-stepping may also switch threads.
I think single-stepping kernels via debug server stubs in things like
qemu can also do this, so it will be useful in those contexts as well.
I'm trying to think if the target is the right place to do this rather
than gdbarch, but I think you are right to do it in the target. We
might want a way to forward this request to remote targets (FreeBSD's
gdb server for the kernel is a remote target that would probably want
to say that this is true).
>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:
Tom> When stepping through the ravenscar context-switching code, gdb will
Tom> try to single-step the current thread, while leaving the other threads
Tom> stopped. This results in an assertion failure in finish_step_over,
Tom> because the event thread is not the request thread.
Tom> This patch introduces a new target method so that infrun can ask the
Tom> target whether this can happen; and then changes infrun to handle this
Tom> case. When the target can have an unexpected thread switch,
Tom> single-stepping may also switch threads.
It's been a while since I wrote this. I still haven't put it in,
because it touches a tricky area.
I found out today that it can help with a different bug:
https://sourceware.org/bugzilla/show_bug.cgi?id=22992
That bug has a test case that causes a native Windows gdb to crash.
Debugging it, I found that (I think) there's an oversight in
windows-nat.c -- namely, stepping doesn't suspend other threads.
However, this oversight doesn't exist in gdbserver, and the bug still
exists when using that.
I am not 100% sure, but I think this bug shows a Windows kernel problem
or the like, as even with thread suspension hacked in, gdb can get a
debug event on an ostensibly suspended thread.
Changing windows-nat.c to report that it is a "random thread switch"
target (which is introduced by the patch I'm replying to) at least
causes the crash to go away... so I think that's an improvement, even
though it isn't fantastic.
So, if you have some time and understand infrun, I'd appreciate it if
you could take a look at this patch; or if you understand Windows
debugging, if you could take a look at 22992 and see if I've missed
something.
thanks,
Tom
@@ -5328,7 +5328,8 @@ finish_step_over (struct execution_control_state *ecs)
/* If we're stepping over a breakpoint with all threads locked,
then only the thread that was stepped should be reporting
back an event. */
- gdb_assert (ecs->event_thread->control.trap_expected);
+ gdb_assert (ecs->event_thread->control.trap_expected
+ || target_can_randomly_thread_switch ());
clear_step_over_info ();
}
@@ -5567,14 +5568,38 @@ handle_signal_stop (struct execution_control_state *ecs)
{
if (single_step_breakpoint_inserted_here_p (aspace, pc))
{
- if (debug_infrun)
+ if (target_can_randomly_thread_switch ())
{
- fprintf_unfiltered (gdb_stdlog,
- "infrun: [%s] hit another thread's "
- "single-step breakpoint\n",
- target_pid_to_str (ecs->ptid).c_str ());
+ if (debug_infrun)
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: [%s] random thread switch\n",
+ (target_pid_to_str
+ (ecs->ptid).c_str ()));
+
+ /* Here the step was intended for some other thread;
+ but instead we saw a context switch. This can
+ happen with "green" threads in Ravenscar, and the
+ recourse is to pretend we wanted to step the new
+ thread and got an ordinary stop there. */
+ for (thread_info *th : all_threads ())
+ if (th->control.trap_expected)
+ {
+ std::swap (th->control, ecs->event_thread->control);
+ break;
+ }
+ }
+ else
+ {
+ if (debug_infrun)
+ {
+ fprintf_unfiltered (gdb_stdlog,
+ "infrun: [%s] hit another thread's "
+ "single-step breakpoint\n",
+ (target_pid_to_str
+ (ecs->ptid).c_str ()));
+ }
+ ecs->hit_singlestep_breakpoint = 1;
}
- ecs->hit_singlestep_breakpoint = 1;
}
}
else
@@ -89,6 +89,11 @@ struct ravenscar_thread_target final : public target_ops
ptid_t wait (ptid_t, struct target_waitstatus *, int) override;
void resume (ptid_t, int, enum gdb_signal) override;
+ bool can_randomly_thread_switch () override
+ {
+ return true;
+ }
+
void fetch_registers (struct regcache *, int) override;
void store_registers (struct regcache *, int) override;
@@ -19,6 +19,7 @@ struct dummy_target : public target_ops
void fetch_registers (struct regcache *arg0, int arg1) override;
void store_registers (struct regcache *arg0, int arg1) override;
void prepare_to_store (struct regcache *arg0) override;
+ bool can_randomly_thread_switch () override;
void files_info () override;
int insert_breakpoint (struct gdbarch *arg0, struct bp_target_info *arg1) override;
int remove_breakpoint (struct gdbarch *arg0, struct bp_target_info *arg1, enum remove_bp_reason arg2) override;
@@ -186,6 +187,7 @@ struct debug_target : public target_ops
void fetch_registers (struct regcache *arg0, int arg1) override;
void store_registers (struct regcache *arg0, int arg1) override;
void prepare_to_store (struct regcache *arg0) override;
+ bool can_randomly_thread_switch () override;
void files_info () override;
int insert_breakpoint (struct gdbarch *arg0, struct bp_target_info *arg1) override;
int remove_breakpoint (struct gdbarch *arg0, struct bp_target_info *arg1, enum remove_bp_reason arg2) override;
@@ -551,6 +553,31 @@ debug_target::prepare_to_store (struct regcache *arg0)
fputs_unfiltered (")\n", gdb_stdlog);
}
+bool
+target_ops::can_randomly_thread_switch ()
+{
+ return this->beneath ()->can_randomly_thread_switch ();
+}
+
+bool
+dummy_target::can_randomly_thread_switch ()
+{
+ return false;
+}
+
+bool
+debug_target::can_randomly_thread_switch ()
+{
+ bool result;
+ fprintf_unfiltered (gdb_stdlog, "-> %s->can_randomly_thread_switch (...)\n", this->beneath ()->shortname ());
+ result = this->beneath ()->can_randomly_thread_switch ();
+ fprintf_unfiltered (gdb_stdlog, "<- %s->can_randomly_thread_switch (", this->beneath ()->shortname ());
+ fputs_unfiltered (") = ", gdb_stdlog);
+ target_debug_print_bool (result);
+ fputs_unfiltered ("\n", gdb_stdlog);
+ return result;
+}
+
void
target_ops::files_info ()
{
@@ -491,6 +491,13 @@ struct target_ops
virtual void prepare_to_store (struct regcache *)
TARGET_DEFAULT_NORETURN (noprocess ());
+ /* True if single-stepping on this target can cause an
+ un-asked-for thread switch. This is normal on "green" thread
+ targets, like Ravenscar, when single-stepping through the
+ context-switching code. */
+ virtual bool can_randomly_thread_switch ()
+ TARGET_DEFAULT_RETURN (false);
+
virtual void files_info ()
TARGET_DEFAULT_IGNORE ();
virtual int insert_breakpoint (struct gdbarch *,
@@ -1402,6 +1409,11 @@ extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal);
coalesce multiple resumption requests in a single vCont packet. */
extern void target_commit_resume ();
+/* A wrapper for target_ops::can_randomly_thread_switch. */
+
+#define target_can_randomly_thread_switch() \
+ (current_top_target ()->can_randomly_thread_switch) ()
+
/* Setup to defer target_commit_resume calls, and reactivate
target_commit_resume on destruction, if it was previously
active. */