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