| Message ID | 20161129120702.9490-1-antoine.tremblay@ericsson.com |
|---|---|
| State | New |
| Headers | show |
Oops the subject got messed-up a bit subject should read: Fix GDBServer's run control for single stepping Then 1st line: This patch fixes GDBServer's run control for single stepping. Sorry about that
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
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
** 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(-)