From: Yao Qi <qiyaoltc@gmail.com>
To: Pedro Alves <palves@redhat.com>
Cc: Yao Qi <qiyaoltc@gmail.com>, gdb-patches@sourceware.org
Subject: Re: [PATCH/7.10 2/2] gdbserver: Fix non-stop / fork / step-over issues
Date: Wed, 05 Aug 2015 11:41:00 -0000 [thread overview]
Message-ID: <86k2tac68i.fsf@gmail.com> (raw)
In-Reply-To: <55C0EB12.9050209@redhat.com> (Pedro Alves's message of "Tue, 04 Aug 2015 17:40:50 +0100")
Pedro Alves <palves@redhat.com> writes:
> I've now extended the test to run in a few different modes, along
> a couple different axis. One axis is with/without conditional
> breakpoints on the target enabled. That exposes the same fails you
> saw on arm, on x86 gdbserver as well. And then the other axis is
> with/without joining _all_ threads before exiting. If we gracefully
> terminate the breakpoint thread (new mode), then the test should be
> passing everywhere, because what fails is gdb's handling of the inferior
> disappearing while a thread is stopped (and being inspected).
> Therefore that new mode is not kfailed.
>
> For testing convenience, I've pushed this along with the previous patch
> to the users/palves/gdbserver-fork-issues branch on sourceware.org.
> Let me know if this works for you.
Thanks, Pedro. There are no fails on arm-linux with GDBserver. Some
comments on your patch below,
> @@ -3089,8 +3142,25 @@ linux_wait_1 (ptid_t ptid,
> info_p = &info;
> else
> info_p = NULL;
> - linux_resume_one_lwp (event_child, event_child->stepping,
> - WSTOPSIG (w), info_p);
> +
> + if (step_over_finished)
> + {
> + /* We cancelled this thread's step-over above. We still
> + need to unsuspend all other LWPs, and set then back
s/set then/set them/?
> + running again while the signal handler runs. */
> + unsuspend_all_lwps (event_child);
> +
> + /* Enqueue the pending signal info so that proceed_all_lwps
> + doesn't lose it. */
> + enqueue_pending_signal (event_child, WSTOPSIG (w), info_p);
> +
> + proceed_all_lwps ();
> + }
> + else
> + {
> + linux_resume_one_lwp (event_child, event_child->stepping,
> + WSTOPSIG (w), info_p);
> + }
> return ignore_event (ourstatus);
> }
>
> @@ -3111,8 +3181,15 @@ linux_wait_1 (ptid_t ptid,
> || (current_thread->last_resume_kind == resume_step
> && !in_step_range)
> || event_child->stop_reason == TARGET_STOPPED_BY_WATCHPOINT
> - || (!step_over_finished && !in_step_range
> - && !bp_explains_trap && !trace_event)
> + || (!in_step_range
> + && !bp_explains_trap
> + && !trace_event
> + /* A step-over was finished just now? */
> + && !step_over_finished
> + /* A step-over had been finished previously,
> + and the single-step was left pending? */
> + && !(current_thread->last_resume_kind == resume_continue
> + && event_child->stop_reason == TARGET_STOPPED_BY_SINGLE_STEP))
> || (gdb_breakpoint_here (event_child->stop_pc)
I don't fully understand this, what is a case that "step-over had been
finished previously, but the single-step was left pending"?
> (linux_wait_1): If passing a signal to the inferior after
> finishing a step-over, unsuspend and re-resume all lwps. If we
> see a single-step event but the thread should be continuing, don't
> pass the trap to gdb.
however, the explanations in ChangeLog look more reasonable.
> && gdb_condition_true_at_breakpoint (event_child->stop_pc)
> && gdb_no_commands_at_breakpoint (event_child->stop_pc))
> @@ -4189,16 +4298,36 @@ static int
> start_step_over (struct lwp_info *lwp)
> {
> struct thread_info *thread = get_lwp_thread (lwp);
> + ptid_t thread_ptid;
> struct thread_info *saved_thread;
> CORE_ADDR pc;
> int step;
>
> + thread_ptid = ptid_of (thread);
> +
> if (debug_threads)
> debug_printf ("Starting step-over on LWP %ld. Stopping all threads\n",
> lwpid_of (thread));
>
> stop_all_lwps (1, lwp);
> - gdb_assert (lwp->suspended == 0);
> +
> + /* Re-find the LWP as it may have exited. */
> + lwp = find_lwp_pid (thread_ptid);
> + if (lwp == NULL || lwp_is_marked_dead (lwp))
> + {
> + if (debug_threads)
> + debug_printf ("Step-over thread died "
> + "(another thread exited the process?).\n");
> + unstop_all_lwps (1, lwp);
> + return 0;
> + }
> +
> + if (lwp->suspended != 0)
> + {
> + internal_error (__FILE__, __LINE__,
> + "LWP %ld suspended=%d\n", lwpid_of (thread),
> + lwp->suspended);
> + }
>
> if (debug_threads)
> debug_printf ("Done stopping all threads for step-over.\n");
> @@ -4229,7 +4358,19 @@ start_step_over (struct lwp_info *lwp)
>
> current_thread = saved_thread;
>
> - linux_resume_one_lwp (lwp, step, 0, NULL);
> + TRY
> + {
> + linux_resume_one_lwp_throw (lwp, step, 0, NULL);
> + }
IIUC, we do TRY/CATCH here because thread may have exited, but we've
done that in this function (start_step_over), do we still need to worry
about these exited threads?
> + CATCH (ex, RETURN_MASK_ERROR)
> + {
> + unstop_all_lwps (1, lwp);
> +
> + if (!check_ptrace_stopped_lwp_gone (lwp))
I am thinking about the order of these two function calls. Does the
order matter here? Probably we need to check_ptrace_stopped_lwp_gone
first before unstop_all_lwps.
> + throw_exception (ex);
> + return 0;
> + }
> + END_CATCH
>
> /* Require next event from this LWP. */
> step_over_bkpt = thread->entry.id;
> @@ -4270,6 +4411,39 @@ finish_step_over (struct lwp_info *lwp)
> return 0;
> }
>
> +/* If there's a step over in progress, wait until all threads stop
> + (that is, until the stepping thread finishes its step), and
> + unsuspend all lwps. The stepping thread ends with its status
> + pending, which is processed later when we get back to processing
> + events. */
> +
> +static void
> +complete_ongoing_step_over (void)
> +{
> + if (!ptid_equal (step_over_bkpt, null_ptid))
> + {
> + struct lwp_info *lwp;
> + int wstat;
> + int ret;
> +
> + if (debug_threads)
> + debug_printf ("detach: step over in progress, finish it first\n");
"detach:" in the debug output looks obsolete.
--
Yao (齐尧)
next prev parent reply other threads:[~2015-08-05 11:41 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-31 17:03 [PATCH/7.10 0/2] gdbserver: Fix several fork support (& co) issues Pedro Alves
2015-07-31 17:04 ` [PATCH/7.10 1/2] Linux gdbserver confused when event randomization returns a process exit event Pedro Alves
2015-08-03 10:59 ` Yao Qi
2015-08-06 9:34 ` [pushed][PATCH 1/3] Linux gdbserver fork event debug output (Re: [PATCH/7.10 1/2] Linux gdbserver confused when event randomization returns a process exit event) Pedro Alves
2015-08-06 9:43 ` [pushed]Re: [PATCH/7.10 1/2] Linux gdbserver confused when event randomization returns a process exit event Pedro Alves
2015-07-31 17:04 ` [PATCH/7.10 2/2] gdbserver: Fix non-stop / fork / step-over issues Pedro Alves
2015-07-31 18:04 ` Don Breazeal
2015-07-31 19:02 ` Pedro Alves
2015-08-05 22:19 ` Don Breazeal
2015-08-06 10:09 ` Pedro Alves
2015-08-03 15:14 ` Yao Qi
2015-08-03 16:20 ` Pedro Alves
2015-08-04 16:40 ` Pedro Alves
2015-08-05 11:41 ` Yao Qi [this message]
2015-08-05 15:10 ` Pedro Alves
2015-08-06 9:44 ` [pushed] " Pedro Alves
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=86k2tac68i.fsf@gmail.com \
--to=qiyaoltc@gmail.com \
--cc=gdb-patches@sourceware.org \
--cc=palves@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).