From: Antoine Tremblay <antoine.tremblay@ericsson.com>
To: Yao Qi <qiyaoltc@gmail.com>
Cc: <gdb-patches@sourceware.org>
Subject: Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping
Date: Mon, 16 Jan 2017 17:28:00 -0000 [thread overview]
Message-ID: <wwokpojmvrty.fsf@ericsson.com> (raw)
In-Reply-To: <20161129120702.9490-1-antoine.tremblay@ericsson.com>
Ping.
Antoine Tremblay writes:
> ** Note these patches depend on:
> https://sourceware.org/ml/gdb-patches/2016-11/msg00914.html
>
> Before, installing single-step breakpoints was done in proceed_one_lwp as
> each thread was started.
>
> This caused a problem on ARM, which is the only architecture using
> software single-step on which it is not safe to modify an instruction
> to insert a breakpoint in one thread while the other threads are running.
> See the architecture manual section "A3.5.4 Concurrent modification and
> execution of instructions":
>
> "The ARMv7 architecture limits the set of instructions that can be
> executed by one thread of execution as they are being modified by another
> thread of execution without requiring explicit synchronization. Except
> for the instructions identified in this section, the effect of the
> concurrent modification and execution of an instruction is UNPREDICTABLE
> ."
>
> Since we want the single-step logic to work for any instruction GDBServer
> needs to stop all the threads to install a single-step breakpoint.
>
> Note that we could introduce arch specific code to check if we can do it
> for each particular instruction but I think that introducing 2 run control
> paths for single stepping would just add too much complexity to the code
> for little benefit.
>
> This patch fixes the intermittent FAILs for gdb.threads/schedlock.exp on
> ARM like discussed here:
> https://sourceware.org/ml/gdb-patches/2016-11/msg00132.html
> Tested with RACY_ITER=100 on two different boards, -marm/-thumb.
>
> Note that the FAILs in non-stop-fair-events.exp discussed in that thread
> will be fixed in an upcoming patch. They are not caused by what this patch
> fixes.
>
> Here's a few implementation details notes:
>
> Before in all-stop mode, GDBServer deleted all single-step breakpoints
> when reporting an event since it assumed that this canceled all previous
> resume requests.
>
> However, this is not true as GDBServer may hit a breakpoint instruction
> written directly in memory so not a GDB breakpoint nor a GDBServer
> breakpoint like is the case with displaced-stepping on.
>
> Considering this, this patch only deletes the current thread single-step
> breakpoint even in all-stop mode.
>
> Also with this patch, single-step breakpoints inserted in proceed_all_lwp
> are inserted before GDBServer checks if it needs to step-over.
>
> This is done in preparation for a patch that allows GDBServer to delay a
> step-over. In which case single-step breakpoints need to be inserted
> before trying to step-over.
>
> It may also be more clear that GDBServer always insert single-step
> breakpoints when calling proceed_all_lwps rather than delaying this
> insertion until a step-over is done.
>
> No regressions, tested on ubuntu 14.04 ARMv7.
> With gdbserver-native/-m{arm,thumb}
>
> gdb/gdbserver/ChangeLog:
>
> * linux-low.c (linux_wait_1): Don't remove all single-step
> breakpoints in all-stop.
> (install_all_software_single_step_breakpoints): New function.
> (linux_resume): Install software single-step breakpoints if needed.
> (proceed_one_lwp): Don't install software single-step here.
> (proceed_all_lwps): Install software single-step breakpoints if needed.
> ---
> gdb/gdbserver/linux-low.c | 145 ++++++++++++++++++++++++++--------------------
> 1 file changed, 81 insertions(+), 64 deletions(-)
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index b441ebc..15fb726 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -3747,64 +3747,14 @@ linux_wait_1 (ptid_t ptid,
>
> /* Alright, we're going to report a stop. */
>
> - /* Remove single-step breakpoints. */
> - if (can_software_single_step ())
> + if (can_software_single_step ()
> + && has_single_step_breakpoints (current_thread))
> {
> - /* Remove single-step breakpoints or not. It it is true, stop all
> - lwps, so that other threads won't hit the breakpoint in the
> - staled memory. */
> - int remove_single_step_breakpoints_p = 0;
> -
> - if (non_stop)
> - {
> - remove_single_step_breakpoints_p
> - = has_single_step_breakpoints (current_thread);
> - }
> - else
> - {
> - /* In all-stop, a stop reply cancels all previous resume
> - requests. Delete all single-step breakpoints. */
> - struct inferior_list_entry *inf, *tmp;
> -
> - ALL_INFERIORS (&all_threads, inf, tmp)
> - {
> - struct thread_info *thread = (struct thread_info *) inf;
> -
> - if (has_single_step_breakpoints (thread))
> - {
> - remove_single_step_breakpoints_p = 1;
> - break;
> - }
> - }
> - }
> -
> - if (remove_single_step_breakpoints_p)
> - {
> - /* If we remove single-step breakpoints from memory, stop all lwps,
> - so that other threads won't hit the breakpoint in the staled
> - memory. */
> - stop_all_lwps (0, event_child);
> -
> - if (non_stop)
> - {
> - gdb_assert (has_single_step_breakpoints (current_thread));
> - delete_single_step_breakpoints (current_thread);
> - }
> - else
> - {
> - struct inferior_list_entry *inf, *tmp;
> -
> - ALL_INFERIORS (&all_threads, inf, tmp)
> - {
> - struct thread_info *thread = (struct thread_info *) inf;
> -
> - if (has_single_step_breakpoints (thread))
> - delete_single_step_breakpoints (thread);
> - }
> - }
> -
> - unstop_all_lwps (0, event_child);
> - }
> + /* If we remove single-step breakpoints from memory, stop all lwps,
> + so that other threads won't hit invalid memory. */
> + stop_all_lwps (0, event_child);
> + delete_single_step_breakpoints (current_thread);
> + unstop_all_lwps (0, event_child);
> }
>
> if (!stabilizing_threads)
> @@ -4336,6 +4286,52 @@ install_software_single_step_breakpoints (struct lwp_info *lwp)
> do_cleanups (old_chain);
> }
>
> +/* Install breakpoints for software single stepping on all threads.
> + Install the breakpoints on a thread only if COND returns true.
> + Return true if we stopped the threads while doing so and they need to be
> + restarted.
> +*/
> +
> +static bool
> +install_all_software_single_step_breakpoints (
> + bool (*cond) (struct lwp_info *lwp))
> +{
> + struct inferior_list_entry *inf, *tmp;
> + bool stopped_threads = false;
> +
> + ALL_INFERIORS (&all_threads, inf, tmp)
> + {
> + struct thread_info *thread = (struct thread_info *) inf;
> + struct lwp_info *lwp = get_thread_lwp (thread);
> +
> + if (cond (lwp))
> + {
> + if (!stopped_threads)
> + {
> + /* We need to stop all threads to modify the inferior
> + instructions safely. */
> + stop_all_lwps (0, NULL);
> +
> + if (debug_threads)
> + debug_printf ("Done stopping all threads.\n");
> +
> + stopped_threads = true;
> + }
> +
> + if (!has_single_step_breakpoints (thread))
> + {
> + install_software_single_step_breakpoints (lwp);
> +
> + if (debug_threads)
> + debug_printf ("Insert breakpoint for resume_step LWP %ld\n",
> + lwpid_of (thread));
> + }
> + }
> + }
> +
> + return stopped_threads;
> +}
> +
> /* Single step via hardware or software single step.
> Return 1 if hardware single stepping, 0 if software single stepping
> or can't single step. */
> @@ -5178,6 +5174,7 @@ linux_resume (struct thread_resume *resume_info, size_t n)
> struct thread_info *need_step_over = NULL;
> int any_pending;
> int leave_all_stopped;
> + bool stopped_threads = false;
>
> if (debug_threads)
> {
> @@ -5221,12 +5218,29 @@ linux_resume (struct thread_resume *resume_info, size_t n)
> debug_printf ("Resuming, no pending status or step over needed\n");
> }
>
> + /* If resume_step is requested by GDB, install reinsert breakpoints
> + when the thread is about to be actually resumed. IOW, we don't
> + insert reinsert breakpoints if any thread has pending status. */
> + if (!leave_all_stopped && can_software_single_step ())
> + {
> + stopped_threads = install_all_software_single_step_breakpoints
> + ([] (struct lwp_info *lwp)
> + {
> + return (lwp->resume != NULL && lwp->resume->kind == resume_step);
> + });
> +
> + if (debug_threads)
> + debug_printf ("Handle resume_step. Done\n");
> + }
> +
> /* Even if we're leaving threads stopped, queue all signals we'd
> otherwise deliver. */
> find_inferior (&all_threads, linux_resume_one_thread, &leave_all_stopped);
>
> if (need_step_over)
> start_step_over (get_thread_lwp (need_step_over));
> + else if (stopped_threads)
> + unstop_all_lwps (0, NULL);
>
> if (debug_threads)
> {
> @@ -5323,13 +5337,6 @@ proceed_one_lwp (struct inferior_list_entry *entry, void *except)
> debug_printf (" stepping LWP %ld, client wants it stepping\n",
> lwpid_of (thread));
>
> - /* If resume_step is requested by GDB, install single-step
> - breakpoints when the thread is about to be actually resumed if
> - the single-step breakpoints weren't removed. */
> - if (can_software_single_step ()
> - && !has_single_step_breakpoints (thread))
> - install_software_single_step_breakpoints (lwp);
> -
> step = maybe_hw_step (thread);
> }
> else if (lwp->bp_reinsert != 0)
> @@ -5370,6 +5377,16 @@ proceed_all_lwps (void)
> {
> struct thread_info *need_step_over;
>
> + /* Always install software single step breakpoints if any. */
> + if (can_software_single_step ())
> + {
> + install_all_software_single_step_breakpoints
> + ([] (struct lwp_info *lwp)
> + {
> + return (get_lwp_thread (lwp)->last_resume_kind == resume_step);
> + });
> + }
> +
> /* If there is a thread which would otherwise be resumed, which is
> stopped at a breakpoint that needs stepping over, then don't
> resume any threads - have it step over the breakpoint with all
next prev parent reply other threads:[~2017-01-16 17:28 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-29 12:07 Antoine Tremblay
2016-11-29 12:07 ` [PATCH 2/2] Avoid step-over infinite loop in GDBServer Antoine Tremblay
2017-01-16 17:27 ` Antoine Tremblay
2017-01-18 16:31 ` Antoine Tremblay
2017-02-03 16:21 ` Pedro Alves
2017-02-17 3:39 ` Antoine Tremblay
2017-02-22 10:15 ` Yao Qi
2017-03-27 13:28 ` Antoine Tremblay
2016-11-29 12:12 ` [PATCH 1/2] This patch fixes GDBServer's run control for single stepping Antoine Tremblay
2017-01-16 17:28 ` Antoine Tremblay [this message]
2017-01-27 15:01 ` Yao Qi
2017-01-27 16:07 ` Antoine Tremblay
2017-01-27 17:01 ` Yao Qi
2017-01-27 18:24 ` Antoine Tremblay
2017-01-29 21:41 ` Yao Qi
2017-01-30 13:29 ` Antoine Tremblay
2017-02-03 16:13 ` Pedro Alves
2017-02-17 1:42 ` Antoine Tremblay
2017-02-17 2:05 ` Pedro Alves
2017-02-17 3:06 ` Antoine Tremblay
2017-02-17 22:19 ` Yao Qi
2017-02-18 0:19 ` Antoine Tremblay
2017-02-18 22:49 ` Yao Qi
2017-02-19 19:40 ` Antoine Tremblay
2017-02-19 20:31 ` Antoine Tremblay
2017-03-29 12:41 ` Antoine Tremblay
2017-03-29 14:11 ` Antoine Tremblay
2017-03-29 17:54 ` Antoine Tremblay
2017-03-30 16:06 ` Yao Qi
2017-03-30 18:31 ` Antoine Tremblay
2017-03-31 16:31 ` Yao Qi
2017-03-31 18:22 ` Antoine Tremblay
2017-04-03 12:41 ` Yao Qi
2017-04-03 13:18 ` Antoine Tremblay
2017-04-03 15:18 ` Yao Qi
2017-04-03 16:57 ` Antoine Tremblay
2017-02-16 22:32 ` Yao Qi
2017-02-17 2:17 ` Antoine Tremblay
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=wwokpojmvrty.fsf@ericsson.com \
--to=antoine.tremblay@ericsson.com \
--cc=gdb-patches@sourceware.org \
--cc=qiyaoltc@gmail.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).