* [PATCH 1/2] This patch fixes GDBServer's run control for single stepping @ 2016-11-29 12:07 Antoine Tremblay 2016-11-29 12:07 ` [PATCH 2/2] Avoid step-over infinite loop in GDBServer Antoine Tremblay ` (3 more replies) 0 siblings, 4 replies; 38+ messages in thread From: Antoine Tremblay @ 2016-11-29 12:07 UTC (permalink / raw) To: gdb-patches; +Cc: Antoine Tremblay ** 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 -- 2.9.2 ^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 2/2] Avoid step-over infinite loop in GDBServer 2016-11-29 12:07 [PATCH 1/2] This patch fixes GDBServer's run control for single stepping Antoine Tremblay @ 2016-11-29 12:07 ` Antoine Tremblay 2017-01-16 17:27 ` Antoine Tremblay ` (2 more replies) 2016-11-29 12:12 ` [PATCH 1/2] This patch fixes GDBServer's run control for single stepping Antoine Tremblay ` (2 subsequent siblings) 3 siblings, 3 replies; 38+ messages in thread From: Antoine Tremblay @ 2016-11-29 12:07 UTC (permalink / raw) To: gdb-patches; +Cc: Antoine Tremblay Before this patch, GDBServer always executed a step-over if it found a thread that needed one. This could be a problem in a situation exposed by non-stop-fair-events.exp where the code and the breakpoint placement is like so: instruction A : has a single-step breakpoint installed for thread 1 and 2 instruction B : has a single-step breakpoint installed for thread 3 and is a branch to A. In this particular case: - GDBServer stops on instruction A in thread 1. - Deletes thread 1 single-step breakpoint. - Starts a step-over of thread 1 to step-over the thread 2 breakpoint. - GDBServer finishes a step-over and is at instruction B. - GDBserver starts a step-over of thread 1 to step-over the thread 3 breakpoint at instruction B. - GDBServer stops on instuction A in thread 1. - GDBServer is now in an infinite loop. This patch avoids this issue by counting the number of times a thread does a step-over consecutively. If the thread reaches a threshold, which is currently 32, GDBServer will not step-over but rather restart all the threads. I chose a threshold of 32, so to trigger this there needs to be 32 consecutive instructions with breakpoints installed that one thread needs to step over. I think this should be rare enought to trigger only in this case but suggestions on the threshold value are welcome. If the threshold is hit and the step-over is delayed, when the thread that needed a step-over runs it will simply hit the breakpoint it needed to step-over and retry to start a step-over. This makes it possible for other threads to run and start a step-over dance of their own or pending events like signals to be handled. This patch fixes the intermittent FAILs for gdb.threads/non-stop-fair-events.exp on ARM like discussed here: https://sourceware.org/ml/gdb-patches/2016-11/msg00132.html No regressions, tested on ubuntu 14.04 ARMv7. With gdbserver-native/-m{arm,thumb} gdb/gdbserver/ChangeLog: * linux-low.c (class step_over_limiter): New class. (_step_over_limiter): New global variable. (linux_wait_1): Count step-overs. (proceed_all_lwps): Delay step-over if threshold is reached. --- gdb/gdbserver/linux-low.c | 76 +++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 70 insertions(+), 6 deletions(-) diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c index 15fb726..b84b62a 100644 --- a/gdb/gdbserver/linux-low.c +++ b/gdb/gdbserver/linux-low.c @@ -282,6 +282,53 @@ static int proceed_one_lwp (struct inferior_list_entry *entry, void *except); being stepped. */ ptid_t step_over_bkpt; +/* Class limiting the number of consecutive step-overs for a thread. */ + +class step_over_limiter +{ + public: + + step_over_limiter () : m_ptid (null_ptid), m_count (0), m_max_count (32) {} + + void step_over_done (struct lwp_info *lwp) + { + ptid_t ptid = lwp->thread->entry.id; + + if (!ptid_equal (ptid, m_ptid)) + { + m_ptid = ptid; + m_count = 0; + } + + m_count++; + } + + bool can_step_over (struct lwp_info *lwp) + { + if (!ptid_equal(lwp->thread->entry.id, m_ptid) + || m_count < m_max_count) + return true; + else + { + /* Reset here so that the step_over is retried. */ + m_ptid = null_ptid; + m_count = 0; + return false; + } + } + + private: + + ptid_t m_ptid; + int m_count; + + /* Maximum step overs for a thread, before all threads are run instead of + stepping over. */ + const int m_max_count; +}; + +step_over_limiter _step_over_limiter; + /* True if the low target can hardware single-step. */ static int @@ -3607,6 +3654,8 @@ linux_wait_1 (ptid_t ptid, doesn't lose it. */ enqueue_pending_signal (event_child, WSTOPSIG (w), info_p); + _step_over_limiter.step_over_done (event_child); + proceed_all_lwps (); } else @@ -3694,6 +3743,8 @@ linux_wait_1 (ptid_t ptid, We're going to keep waiting, so use proceed, which handles stepping over the next breakpoint. */ unsuspend_all_lwps (event_child); + + _step_over_limiter.step_over_done (event_child); } else { @@ -5400,13 +5451,26 @@ proceed_all_lwps (void) if (need_step_over != NULL) { - if (debug_threads) - debug_printf ("proceed_all_lwps: found " - "thread %ld needing a step-over\n", - lwpid_of (need_step_over)); + /* Don't step over if we're looping too much. */ + if (_step_over_limiter.can_step_over + (get_thread_lwp (need_step_over))) + { + if (debug_threads) + debug_printf ("proceed_all_lwps: found " + "thread %ld needing a step-over\n", + lwpid_of (need_step_over)); - start_step_over (get_thread_lwp (need_step_over)); - return; + start_step_over (get_thread_lwp (need_step_over)); + return; + } + else + { + if (debug_threads) + debug_printf ("proceed_all_lwps: found " + "thread %ld needing a step-over " + "but max consecutive step-overs reached\n", + lwpid_of (need_step_over)); + } } } -- 2.9.2 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/2] Avoid step-over infinite loop in GDBServer 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-22 10:15 ` Yao Qi 2 siblings, 1 reply; 38+ messages in thread From: Antoine Tremblay @ 2017-01-16 17:27 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches Ping. Antoine Tremblay writes: > Before this patch, GDBServer always executed a step-over if it found a > thread that needed one. > > This could be a problem in a situation exposed by non-stop-fair-events.exp > where the code and the breakpoint placement is like so: > > instruction A : has a single-step breakpoint installed for thread 1 and 2 > instruction B : has a single-step breakpoint installed for thread 3 > and is a branch to A. > > In this particular case: > > - GDBServer stops on instruction A in thread 1. > - Deletes thread 1 single-step breakpoint. > - Starts a step-over of thread 1 to step-over the thread 2 breakpoint. > - GDBServer finishes a step-over and is at instruction B. > - GDBserver starts a step-over of thread 1 to step-over the thread 3 > breakpoint at instruction B. > - GDBServer stops on instuction A in thread 1. > - GDBServer is now in an infinite loop. > > This patch avoids this issue by counting the number of times a thread does > a step-over consecutively. If the thread reaches a threshold, which is > currently 32, GDBServer will not step-over but rather restart all the > threads. > > I chose a threshold of 32, so to trigger this there needs to be 32 > consecutive instructions with breakpoints installed that one thread needs > to step over. I think this should be rare enought to trigger only in this > case but suggestions on the threshold value are welcome. > > If the threshold is hit and the step-over is delayed, when the thread > that needed a step-over runs it will simply hit the breakpoint it needed > to step-over and retry to start a step-over. > > This makes it possible for other threads to run and start a step-over > dance of their own or pending events like signals to be handled. > > This patch fixes the intermittent FAILs for > gdb.threads/non-stop-fair-events.exp on ARM like discussed here: > https://sourceware.org/ml/gdb-patches/2016-11/msg00132.html > > No regressions, tested on ubuntu 14.04 ARMv7. > With gdbserver-native/-m{arm,thumb} > > gdb/gdbserver/ChangeLog: > > * linux-low.c (class step_over_limiter): New class. > (_step_over_limiter): New global variable. > (linux_wait_1): Count step-overs. > (proceed_all_lwps): Delay step-over if threshold is reached. > --- > gdb/gdbserver/linux-low.c | 76 +++++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 70 insertions(+), 6 deletions(-) > > diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c > index 15fb726..b84b62a 100644 > --- a/gdb/gdbserver/linux-low.c > +++ b/gdb/gdbserver/linux-low.c > @@ -282,6 +282,53 @@ static int proceed_one_lwp (struct inferior_list_entry *entry, void *except); > being stepped. */ > ptid_t step_over_bkpt; > > +/* Class limiting the number of consecutive step-overs for a thread. */ > + > +class step_over_limiter > +{ > + public: > + > + step_over_limiter () : m_ptid (null_ptid), m_count (0), m_max_count (32) {} > + > + void step_over_done (struct lwp_info *lwp) > + { > + ptid_t ptid = lwp->thread->entry.id; > + > + if (!ptid_equal (ptid, m_ptid)) > + { > + m_ptid = ptid; > + m_count = 0; > + } > + > + m_count++; > + } > + > + bool can_step_over (struct lwp_info *lwp) > + { > + if (!ptid_equal(lwp->thread->entry.id, m_ptid) > + || m_count < m_max_count) > + return true; > + else > + { > + /* Reset here so that the step_over is retried. */ > + m_ptid = null_ptid; > + m_count = 0; > + return false; > + } > + } > + > + private: > + > + ptid_t m_ptid; > + int m_count; > + > + /* Maximum step overs for a thread, before all threads are run instead of > + stepping over. */ > + const int m_max_count; > +}; > + > +step_over_limiter _step_over_limiter; > + > /* True if the low target can hardware single-step. */ > > static int > @@ -3607,6 +3654,8 @@ linux_wait_1 (ptid_t ptid, > doesn't lose it. */ > enqueue_pending_signal (event_child, WSTOPSIG (w), info_p); > > + _step_over_limiter.step_over_done (event_child); > + > proceed_all_lwps (); > } > else > @@ -3694,6 +3743,8 @@ linux_wait_1 (ptid_t ptid, > We're going to keep waiting, so use proceed, which > handles stepping over the next breakpoint. */ > unsuspend_all_lwps (event_child); > + > + _step_over_limiter.step_over_done (event_child); > } > else > { > @@ -5400,13 +5451,26 @@ proceed_all_lwps (void) > > if (need_step_over != NULL) > { > - if (debug_threads) > - debug_printf ("proceed_all_lwps: found " > - "thread %ld needing a step-over\n", > - lwpid_of (need_step_over)); > + /* Don't step over if we're looping too much. */ > + if (_step_over_limiter.can_step_over > + (get_thread_lwp (need_step_over))) > + { > + if (debug_threads) > + debug_printf ("proceed_all_lwps: found " > + "thread %ld needing a step-over\n", > + lwpid_of (need_step_over)); > > - start_step_over (get_thread_lwp (need_step_over)); > - return; > + start_step_over (get_thread_lwp (need_step_over)); > + return; > + } > + else > + { > + if (debug_threads) > + debug_printf ("proceed_all_lwps: found " > + "thread %ld needing a step-over " > + "but max consecutive step-overs reached\n", > + lwpid_of (need_step_over)); > + } > } > } ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/2] Avoid step-over infinite loop in GDBServer 2017-01-16 17:27 ` Antoine Tremblay @ 2017-01-18 16:31 ` Antoine Tremblay 0 siblings, 0 replies; 38+ messages in thread From: Antoine Tremblay @ 2017-01-18 16:31 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches Antoine Tremblay writes: > Ping. > FYI, I just realised that this can break traceframes and breakpoint counts, I'll try to fix that or find a better way. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/2] Avoid step-over infinite loop in GDBServer 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-02-03 16:21 ` Pedro Alves 2017-02-17 3:39 ` Antoine Tremblay 2017-02-22 10:15 ` Yao Qi 2 siblings, 1 reply; 38+ messages in thread From: Pedro Alves @ 2017-02-03 16:21 UTC (permalink / raw) To: Antoine Tremblay, gdb-patches On 11/29/2016 12:07 PM, Antoine Tremblay wrote: > - GDBServer stops on instruction A in thread 1. > - Deletes thread 1 single-step breakpoint. > - Starts a step-over of thread 1 to step-over the thread 2 breakpoint. > - GDBServer finishes a step-over and is at instruction B. > - GDBserver starts a step-over of thread 1 to step-over the thread 3 > breakpoint at instruction B. > - GDBServer stops on instuction A in thread 1. > - GDBServer is now in an infinite loop. This sounds to me very much like a fairness issue. There were three threads stopped that needed to move past a breakpoint, but gdbserver always picks thread 1. Why? Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/2] Avoid step-over infinite loop in GDBServer 2017-02-03 16:21 ` Pedro Alves @ 2017-02-17 3:39 ` Antoine Tremblay 0 siblings, 0 replies; 38+ messages in thread From: Antoine Tremblay @ 2017-02-17 3:39 UTC (permalink / raw) To: Pedro Alves; +Cc: Antoine Tremblay, gdb-patches Pedro Alves writes: > On 11/29/2016 12:07 PM, Antoine Tremblay wrote: >> - GDBServer stops on instruction A in thread 1. >> - Deletes thread 1 single-step breakpoint. >> - Starts a step-over of thread 1 to step-over the thread 2 breakpoint. >> - GDBServer finishes a step-over and is at instruction B. >> - GDBserver starts a step-over of thread 1 to step-over the thread 3 >> breakpoint at instruction B. >> - GDBServer stops on instuction A in thread 1. >> - GDBServer is now in an infinite loop. > > This sounds to me very much like a fairness issue. There were > three threads stopped that needed to move past a breakpoint, but > gdbserver always picks thread 1. Why? It is a fairness issue but not between the stepping over threads, gdbserver could pick any thread rather than thread 1 and still get into the same loop. The problem is one of fairness between stepping over threads and non-stepping-over threads. For example if all the instructions in the thread's execution path have a breakpoint on it such that this thread always needs to step-over and that the only way to break that loop is to let a thread that doesn't need a step-over run then we're in an infinite loop since GDBServer always gives precedence to the threads needing a step-over. Thus this patch created more fairness by giving non-stepping over threads a chance to run. But it doesn't work... I'm not sure this can be fixed it might be more a problem like putting a breakpoint on a mutex and wanting a thread that depends on that mutex to continue... so more a user problem... but non-stop-fair-events is done in a way that this can happen... Ideas are welcome :) ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/2] Avoid step-over infinite loop in GDBServer 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-02-03 16:21 ` Pedro Alves @ 2017-02-22 10:15 ` Yao Qi 2017-03-27 13:28 ` Antoine Tremblay 2 siblings, 1 reply; 38+ messages in thread From: Yao Qi @ 2017-02-22 10:15 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches On Tue, Nov 29, 2016 at 12:07 PM, Antoine Tremblay <antoine.tremblay@ericsson.com> wrote: > Before this patch, GDBServer always executed a step-over if it found a > thread that needed one. > > This could be a problem in a situation exposed by non-stop-fair-events.exp > where the code and the breakpoint placement is like so: > > instruction A : has a single-step breakpoint installed for thread 1 and 2 > instruction B : has a single-step breakpoint installed for thread 3 > and is a branch to A. > Is instruction B following instruction A? Is it like .L1: nop b .L1 > In this particular case: > > - GDBServer stops on instruction A in thread 1. > - Deletes thread 1 single-step breakpoint. > - Starts a step-over of thread 1 to step-over the thread 2 breakpoint. > - GDBServer finishes a step-over and is at instruction B. > - GDBserver starts a step-over of thread 1 to step-over the thread 3 > breakpoint at instruction B. Why does GDBserver starts a step-over again? is it because need_step_over_p doing checks like this, if (breakpoint_here (pc) || fast_tracepoint_jump_here (pc)) { /* Don't step over a breakpoint that GDB expects to hit though. If the condition is being evaluated on the target's side and it evaluate to false, step over this breakpoint as well. */ if (gdb_breakpoint_here (pc) && gdb_condition_true_at_breakpoint (pc) && gdb_no_commands_at_breakpoint (pc)) { if (debug_threads) debug_printf ("Need step over [LWP %ld]? yes, but found" " GDB breakpoint at 0x%s; skipping step over\n", lwpid_of (thread), paddress (pc)); current_thread = saved_thread; return 0; } else { if (debug_threads) debug_printf ("Need step over [LWP %ld]? yes, " "found breakpoint at 0x%s\n", lwpid_of (thread), paddress (pc)); /* We've found an lwp that needs stepping over --- return 1 so that find_inferior stops looking. */ current_thread = saved_thread; return 1; } } there is a single step breakpoint on pc, and it is obviously not a gdb breakpoint, so 1 is returned. > - GDBServer stops on instuction A in thread 1. > - GDBServer is now in an infinite loop. > I am wondering can we take the information that we've already step over a breakpoint for thread A into need_step_over_p, if we see pc is on another single step breakpoint for thread B, don't do step over. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/2] Avoid step-over infinite loop in GDBServer 2017-02-22 10:15 ` Yao Qi @ 2017-03-27 13:28 ` Antoine Tremblay 0 siblings, 0 replies; 38+ messages in thread From: Antoine Tremblay @ 2017-03-27 13:28 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, gdb-patches Yao Qi writes: > On Tue, Nov 29, 2016 at 12:07 PM, Antoine Tremblay > <antoine.tremblay@ericsson.com> wrote: >> Before this patch, GDBServer always executed a step-over if it found a >> thread that needed one. >> >> This could be a problem in a situation exposed by non-stop-fair-events.exp >> where the code and the breakpoint placement is like so: >> >> instruction A : has a single-step breakpoint installed for thread 1 and 2 >> instruction B : has a single-step breakpoint installed for thread 3 >> and is a branch to A. >> > > Is instruction B following instruction A? Is it like > > .L1: > nop > b .L1 > Yes, assuming A is nop an B is b .L1. I reduced it to that from the real world case in non-stop-fair-events.c with : 0x0000000000400767 <+38>: mov 0x200963(%rip),%eax # 0x6010d0 <got_sig> 0x000000000040076d <+44>: test %eax,%eax 0x000000000040076f <+46>: je 0x400767 <child_function+38> And a single-step breakpoint on all instructions. >> In this particular case: >> >> - GDBServer stops on instruction A in thread 1. >> - Deletes thread 1 single-step breakpoint. >> - Starts a step-over of thread 1 to step-over the thread 2 breakpoint. >> - GDBServer finishes a step-over and is at instruction B. >> - GDBserver starts a step-over of thread 1 to step-over the thread 3 >> breakpoint at instruction B. > > Why does GDBserver starts a step-over again? is it because > need_step_over_p doing checks like this, > > if (breakpoint_here (pc) || fast_tracepoint_jump_here (pc)) > { > /* Don't step over a breakpoint that GDB expects to hit > though. If the condition is being evaluated on the target's side > and it evaluate to false, step over this breakpoint as well. */ > if (gdb_breakpoint_here (pc) > && gdb_condition_true_at_breakpoint (pc) > && gdb_no_commands_at_breakpoint (pc)) > { > if (debug_threads) > debug_printf ("Need step over [LWP %ld]? yes, but found" > " GDB breakpoint at 0x%s; skipping step over\n", > lwpid_of (thread), paddress (pc)); > > current_thread = saved_thread; > return 0; > } > else > { > if (debug_threads) > debug_printf ("Need step over [LWP %ld]? yes, " > "found breakpoint at 0x%s\n", > lwpid_of (thread), paddress (pc)); > > /* We've found an lwp that needs stepping over --- return 1 so > that find_inferior stops looking. */ > current_thread = saved_thread; > > return 1; > } > } > > there is a single step breakpoint on pc, and it is obviously not a > gdb breakpoint, so 1 is returned. > Yes. >> - GDBServer stops on instuction A in thread 1. >> - GDBServer is now in an infinite loop. >> > > I am wondering can we take the information that we've already step > over a breakpoint for thread A into need_step_over_p, if we see pc > is on another single step breakpoint for thread B, don't do step over. There's 3 parts to consider here: - What to restart ? My first thought with this was step-over the other threads that needed to step-over in a queue like fashion, but this may not be enough since we may depend on another thread to be able to make progress on the single stepped threads like is the case in non-stop-fair-events. So like this patch does I though it was better to restart all threads... - What does it mean to not step-over ? The don't do step over part is more tricky then it seems since let's say GDBServer: - hits a breakpoint on A with thread 1 - doesn't step-over and restart all threads - any thread hits the breakpoint on A At this point thread 1 has already hit the breakpoint on A even if we do not allow it to step-over, so it's stuck there and could hit the breakpoint a number of times, thus breaking the breakpoints/trace frames counts and maybe some other stuff... I'm not sure about the solution to that, ideas ? How to "undo" a breakpoint hit ? Or some other solution... - When not to step-over ? After some thought I think we could make it so that we expect the single-step breakpoints for PC to be hit in the order of insertion. Since the breakpoints are already a linked list we could make it so that if you hit a single step breakpoint and that this breakpoint is installed on multiple threads then single-step only if the thread stopped matches the thread of the first breakpoint for that PC. Otherwise continue all threads. I think this would be simpler than to record the last executed step-over for all threads and check/reset that flag so that all threads wait for each other, but that may be possible too. WDYT ? ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2016-11-29 12:07 [PATCH 1/2] This patch fixes GDBServer's run control for single stepping Antoine Tremblay 2016-11-29 12:07 ` [PATCH 2/2] Avoid step-over infinite loop in GDBServer Antoine Tremblay @ 2016-11-29 12:12 ` Antoine Tremblay 2017-01-16 17:28 ` Antoine Tremblay 2017-01-27 15:01 ` Yao Qi 3 siblings, 0 replies; 38+ messages in thread From: Antoine Tremblay @ 2016-11-29 12:12 UTC (permalink / raw) To: gdb-patches; +Cc: Antoine Tremblay 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2016-11-29 12:07 [PATCH 1/2] This patch fixes GDBServer's run control for single stepping Antoine Tremblay 2016-11-29 12:07 ` [PATCH 2/2] Avoid step-over infinite loop in GDBServer 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 2017-01-27 15:01 ` Yao Qi 3 siblings, 0 replies; 38+ messages in thread From: Antoine Tremblay @ 2017-01-16 17:28 UTC (permalink / raw) To: Yao Qi; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2016-11-29 12:07 [PATCH 1/2] This patch fixes GDBServer's run control for single stepping Antoine Tremblay ` (2 preceding siblings ...) 2017-01-16 17:28 ` Antoine Tremblay @ 2017-01-27 15:01 ` Yao Qi 2017-01-27 16:07 ` Antoine Tremblay 3 siblings, 1 reply; 38+ messages in thread From: Yao Qi @ 2017-01-27 15:01 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches On 16-11-29 07:07:01, Antoine Tremblay wrote: > ** 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 > ." This doesn't apply to ptrace. PTRACE_POKETEXT on a word aligned address is atomic, so threads observe the coherent result, either breakpoint instruction or the original instruction. We don't see any fails in -marm mode, do we? In -mthumb mode, breakpoint can be 16-bit or 32-bit, but GDBserver still PTRACE_POKETEXT on a word aligned address (in linux-low.c:linux_write_memory) and write a word each time. It should be atomic, however, if the 32-bit thumb-2 instruction is 2-byte aligned, we end up two PTRACE_POKETEXT, which is non-atomic. That is the root cause of the problem, AFAICS. 32-bit thumb-2 breakpoint was invented for single step 32-bit thumb-2 instruction in IT block, http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/007488.html but we can use 16-bit thumb breakpoint instruction anywhere else. If I force GDBserver to use 16-bit thumb breakpoint instruction even on 32-bit thumb-2 instruction, I can't see any fails in schedlock.exp anymore! See the patch below. Out of IT block, we can use 16-bit thumb breakpoint for any thumb code. In IT block, we can use 16-bit thumb breakpoint for 16-bit instruction, and 32-bit thumb-2 breakpoint for 4-byte aligned 32-bit thumb-2 instruction. However, we can not use either 16-bit or 32-bit breakpoint for 2-byte aligned 32-bit thumb-2 instruction in IT block. The reason we can't use 16-bit breakpoint on a 32-bit instruction in IT block is that 16-bit breakpoint instruction makes the second half of 32-bit instruction to another different 16-bit instruction, and the 16-bit breakpoint instruction may not be executed at all, so the second half of instruction may be executed, and the result is completely unknown. GDB sets two breakpoints on two branches, "if" branch and "else" branch, because GDB doesn't know how does the instruction to be executed affect the flag. /* There are conditional instructions after this one. If this instruction modifies the flags, then we can not predict what the next executed instruction will be. Fortunately, this instruction is architecturally forbidden to branch; we know it will fall through. Start by skipping past it. */ GDB/GDBserver has to emulate the instruction on how does it affect the flag, and only insert the breakpoint on the "true" branch. Since the target instruction will be definitely executed, we can safely use 16-bit breakpoint instruction. -- Yao (é½å°§) diff --git a/gdb/gdbserver/linux-aarch32-low.c b/gdb/gdbserver/linux-aarch32-low.c index 2b710ba..789e0e6 100644 --- a/gdb/gdbserver/linux-aarch32-low.c +++ b/gdb/gdbserver/linux-aarch32-low.c @@ -243,7 +243,7 @@ arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr) target_read_memory (*pcptr, (gdb_byte *) &inst1, 2); if (thumb_insn_size (inst1) == 4) - return ARM_BP_KIND_THUMB2; + return ARM_BP_KIND_THUMB; } return ARM_BP_KIND_THUMB; } ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-01-27 15:01 ` Yao Qi @ 2017-01-27 16:07 ` Antoine Tremblay 2017-01-27 17:01 ` Yao Qi 0 siblings, 1 reply; 38+ messages in thread From: Antoine Tremblay @ 2017-01-27 16:07 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, gdb-patches Yao Qi writes: > On 16-11-29 07:07:01, Antoine Tremblay wrote: >> ** 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 >> ." > > This doesn't apply to ptrace. PTRACE_POKETEXT on a word aligned address > is atomic, so threads observe the coherent result, either breakpoint > instruction or the original instruction. We don't see any fails in -marm > mode, do we? > Indeed. > In -mthumb mode, breakpoint can be 16-bit or 32-bit, but GDBserver still > PTRACE_POKETEXT on a word aligned address (in linux-low.c:linux_write_memory) > and write a word each time. It should be atomic, however, if the 32-bit > thumb-2 instruction is 2-byte aligned, we end up two PTRACE_POKETEXT, > which is non-atomic. That is the root cause of the problem, AFAICS. > Haaa makes total sense now :) thx! > 32-bit thumb-2 breakpoint was invented for single step 32-bit thumb-2 > instruction in IT block, > http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/007488.html > but we can use 16-bit thumb breakpoint instruction anywhere else. If I > force GDBserver to use 16-bit thumb breakpoint instruction even on 32-bit > thumb-2 instruction, I can't see any fails in schedlock.exp anymore! See > the patch below. > > Out of IT block, we can use 16-bit thumb breakpoint for any thumb code. In > IT block, we can use 16-bit thumb breakpoint for 16-bit instruction, and > 32-bit thumb-2 breakpoint for 4-byte aligned 32-bit thumb-2 instruction. > However, we can not use either 16-bit or 32-bit breakpoint for 2-byte > aligned 32-bit thumb-2 instruction in IT block. > > The reason we can't use 16-bit breakpoint on a 32-bit instruction in IT > block is that 16-bit breakpoint instruction makes the second half of > 32-bit instruction to another different 16-bit instruction, and the 16-bit > breakpoint instruction may not be executed at all, so the second half > of instruction may be executed, and the result is completely unknown. > GDB sets two breakpoints on two branches, "if" branch and "else" branch, > because GDB doesn't know how does the instruction to be executed affect > the flag. > > /* There are conditional instructions after this one. > If this instruction modifies the flags, then we can > not predict what the next executed instruction will > be. Fortunately, this instruction is architecturally > forbidden to branch; we know it will fall through. > Start by skipping past it. */ > > GDB/GDBserver has to emulate the instruction on how does it affect the > flag, and only insert the breakpoint on the "true" branch. Since the > target instruction will be definitely executed, we can safely use > 16-bit breakpoint instruction. Ouch, reading the kernel thread it looks like this emulation would be complex to say the least. I think it would be better to get the current single stepping working with the stop all threads logic since GDBServer was working like that when GDB was doing the single stepping anyway. This would fix the current regression. Then work could be done to improve GDBServer to be better at non-stopping. WDYT ? Thanks, Antoine ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-01-27 16:07 ` Antoine Tremblay @ 2017-01-27 17:01 ` Yao Qi 2017-01-27 18:24 ` Antoine Tremblay 0 siblings, 1 reply; 38+ messages in thread From: Yao Qi @ 2017-01-27 17:01 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches On Fri, Jan 27, 2017 at 4:06 PM, Antoine Tremblay <antoine.tremblay@ericsson.com> wrote: >> GDB/GDBserver has to emulate the instruction on how does it affect the >> flag, and only insert the breakpoint on the "true" branch. Since the >> target instruction will be definitely executed, we can safely use >> 16-bit breakpoint instruction. > > Ouch, reading the kernel thread it looks like this emulation would be > complex to say the least. > There are some other ideas discussed in the kernel threads, but I didn't go through them. They may work. If emulation is complex, probably we can partially fix this problem by "always using 16-bit thumb instruction if program is out of IT block". > I think it would be better to get the current single stepping working > with the stop all threads logic since GDBServer was working like that > when GDB was doing the single stepping anyway. This would fix the current > regression. > > Then work could be done to improve GDBServer to be better at > non-stopping. > > WDYT ? > Sounds like we are applying the ARM linux limitation to a general place. Other software single step targets may write breakpoint in atomic way, so we don't need to stop all threads. Even in -marm mode, we don't have to stop all threads on inserting breakpoints. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-01-27 17:01 ` Yao Qi @ 2017-01-27 18:24 ` Antoine Tremblay 2017-01-29 21:41 ` Yao Qi 0 siblings, 1 reply; 38+ messages in thread From: Antoine Tremblay @ 2017-01-27 18:24 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, gdb-patches Yao Qi writes: > On Fri, Jan 27, 2017 at 4:06 PM, Antoine Tremblay > <antoine.tremblay@ericsson.com> wrote: >>> GDB/GDBserver has to emulate the instruction on how does it affect the >>> flag, and only insert the breakpoint on the "true" branch. Since the >>> target instruction will be definitely executed, we can safely use >>> 16-bit breakpoint instruction. >> >> Ouch, reading the kernel thread it looks like this emulation would be >> complex to say the least. >> > > There are some other ideas discussed in the kernel threads, but I didn't > go through them. They may work. There was this one http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/007515.html But it got discarded: http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/007517.html > If emulation is complex, probably > we can partially fix this problem by "always using 16-bit thumb instruction > if program is out of IT block". > I would be against that since it would leave the feature partially working and crashing the program when it fails... It would also be a regression compared to previous GDBServer. Also, IT blocks are a common place to have a breakpoint/tracepoint. >> I think it would be better to get the current single stepping working >> with the stop all threads logic since GDBServer was working like that >> when GDB was doing the single stepping anyway. This would fix the current >> regression. >> >> Then work could be done to improve GDBServer to be better at >> non-stopping. >> >> WDYT ? >> > > Sounds like we are applying the ARM linux limitation to a general > place. > Other software single step targets may write breakpoint in atomic way, > so we don't need to stop all threads. Even in -marm mode, we don't > have to stop all threads on inserting breakpoints. Well ARM is the only software single stepping target, while I agreee that we would be affecting general code, I think that since there is no software single step breakpoint target that supports atomic breakpoint writes at the moment it's normal that the run control doesn't support that. I don't count -marm as an arch since there no way to check that all the program including shared libs etc is -marm, I don't think we could make the distinction in GDBServer AFAIK. Should an arch support this in the future we could adapt the run control to support both cases possibly via some arch specific property. I think also that given the current trends in architecture design the odds of another arch needing that in the future are also unlikely ? WDYT ? ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-01-27 18:24 ` Antoine Tremblay @ 2017-01-29 21:41 ` Yao Qi 2017-01-30 13:29 ` Antoine Tremblay 0 siblings, 1 reply; 38+ messages in thread From: Yao Qi @ 2017-01-29 21:41 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches On Fri, Jan 27, 2017 at 6:24 PM, Antoine Tremblay <antoine.tremblay@ericsson.com> wrote: > > Yao Qi writes: > >> On Fri, Jan 27, 2017 at 4:06 PM, Antoine Tremblay >> <antoine.tremblay@ericsson.com> wrote: >> If emulation is complex, probably >> we can partially fix this problem by "always using 16-bit thumb instruction >> if program is out of IT block". >> > > I would be against that since it would leave the feature partially > working and crashing the program when it fails... > > It would also be a regression compared to previous GDBServer. There won't be any regression. 16-bit thumb breakpoint works pretty well for any thumb instructions (16-bit and 32-bit) if program is out of IT block. The 32-bit thumb-2 breakpoint was added for single step IT block. > Also, IT blocks are a common place to have a breakpoint/tracepoint. > We don't change anything when setting breakpoint inside IT block. >>> I think it would be better to get the current single stepping working >>> with the stop all threads logic since GDBServer was working like that >>> when GDB was doing the single stepping anyway. This would fix the current >>> regression. >>> >>> Then work could be done to improve GDBServer to be better at >>> non-stopping. >>> >>> WDYT ? >>> >> >> Sounds like we are applying the ARM linux limitation to a general >> place. >> Other software single step targets may write breakpoint in atomic way, >> so we don't need to stop all threads. Even in -marm mode, we don't >> have to stop all threads on inserting breakpoints. > > Well ARM is the only software single stepping target, while I agreee > that we would be affecting general code, I think that since there is > no software single step breakpoint target that supports atomic > breakpoint writes at the moment it's normal that the run control > doesn't support that. No, ARM Linux ptrace POKETEXT is _atomic_ if the address is 4-byte aligned, so 32-bit arm breakpoint and 16-bit thumb breakpoint can be written atomically. 32-bit thumb-2 breakpoint can be written atomically too if the address is 4-byte aligned. The only problem we have is inserting a breakpoint on a 2-byte aligned 32-bit thumb-2 instruction inside IT block, we can not use neither 16-bit thumb breakpoint nor 32-bit thumb breakpoint. Everything works fine in other cases. > > I don't count -marm as an arch since there no way to check that all the > program including shared libs etc is -marm, I don't think we could make > the distinction in GDBServer AFAIK. We can check with -mthumb. My hack in last email fixes fails in schedlock.exp in thumb mode. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-01-29 21:41 ` Yao Qi @ 2017-01-30 13:29 ` Antoine Tremblay 2017-02-03 16:13 ` Pedro Alves 2017-02-16 22:32 ` Yao Qi 0 siblings, 2 replies; 38+ messages in thread From: Antoine Tremblay @ 2017-01-30 13:29 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, gdb-patches Yao Qi writes: > On Fri, Jan 27, 2017 at 6:24 PM, Antoine Tremblay > <antoine.tremblay@ericsson.com> wrote: >> >> Yao Qi writes: >> >>> On Fri, Jan 27, 2017 at 4:06 PM, Antoine Tremblay >>> <antoine.tremblay@ericsson.com> wrote: > >>> If emulation is complex, probably >>> we can partially fix this problem by "always using 16-bit thumb instruction >>> if program is out of IT block". >>> >> >> I would be against that since it would leave the feature partially >> working and crashing the program when it fails... >> >> It would also be a regression compared to previous GDBServer. > > There won't be any regression. 16-bit thumb breakpoint works pretty well > for any thumb instructions (16-bit and 32-bit) if program is out of IT block. > The 32-bit thumb-2 breakpoint was added for single step IT block. Yes so there will be a regression for single step inside an IT block if we keep using the 32 bit breakpoint since this one can be non atomic if it's not aligned properly. We could write a test case for it and it would fail like schedlock did. But it would not with the single stepping controlled by GDB like it was before. > >> Also, IT blocks are a common place to have a breakpoint/tracepoint. >> > > We don't change anything when setting breakpoint inside IT block. Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on 2 bytes like discussed before. > >>>> I think it would be better to get the current single stepping working >>>> with the stop all threads logic since GDBServer was working like that >>>> when GDB was doing the single stepping anyway. This would fix the current >>>> regression. >>>> >>>> Then work could be done to improve GDBServer to be better at >>>> non-stopping. >>>> >>>> WDYT ? >>>> >>> >>> Sounds like we are applying the ARM linux limitation to a general >>> place. >>> Other software single step targets may write breakpoint in atomic way, >>> so we don't need to stop all threads. Even in -marm mode, we don't >>> have to stop all threads on inserting breakpoints. >> >> Well ARM is the only software single stepping target, while I agreee >> that we would be affecting general code, I think that since there is >> no software single step breakpoint target that supports atomic >> breakpoint writes at the moment it's normal that the run control >> doesn't support that. > > No, ARM Linux ptrace POKETEXT is _atomic_ if the address is 4-byte > aligned, so 32-bit arm breakpoint and 16-bit thumb breakpoint can be > written atomically. 32-bit thumb-2 breakpoint can be written atomically > too if the address is 4-byte aligned. > > The only problem we have is inserting a breakpoint on a 2-byte aligned > 32-bit thumb-2 instruction inside IT block, we can not use neither 16-bit thumb > breakpoint nor 32-bit thumb breakpoint. Everything works fine in other > cases. > I mean software single stepping as a whole, which means considering all cases, is not atomic. I don't see how we could leave that case unaddressed ? Especially since it will crash the inferior ? >> >> I don't count -marm as an arch since there no way to check that all the >> program including shared libs etc is -marm, I don't think we could make >> the distinction in GDBServer AFAIK. > > We can check with -mthumb. My hack in last email fixes fails in > schedlock.exp in thumb mode. Yes but schedlock.exp is not made to test the 2 byte aligned thumb2 32-bit breakpoint IIRC. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-01-30 13:29 ` Antoine Tremblay @ 2017-02-03 16:13 ` Pedro Alves 2017-02-17 1:42 ` Antoine Tremblay 2017-02-16 22:32 ` Yao Qi 1 sibling, 1 reply; 38+ messages in thread From: Pedro Alves @ 2017-02-03 16:13 UTC (permalink / raw) To: Antoine Tremblay, Yao Qi; +Cc: gdb-patches On 01/30/2017 01:28 PM, Antoine Tremblay wrote: >> We don't change anything when setting breakpoint inside IT block. > > Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on > 2 bytes like discussed before. Can we restrict the stopping-all-threads to the case that needs it, only? An optimization that would avoid this would be to use a hardware watchpoint/breakpoint (if available) for single-stepping. IIRC, you can ARM a breakpoint (or was it a watchpoint) on ARM for triggering when the PC is different from the current PC (or really, some specified address). In IT blocks, this would probably make the thread stop on instructions that aren't really executed (e.g., the then/else branches when the condition is correspondingly false/true), unlike the current solution where breakpoint instructions are not executed by the CPU when it falls on an instruction that isn't executed (because the breakpoint opcode we use it just some magic invalid instruction, node the BKPT instruction), but I think that when the thread stops, and we're stepping an IT block, we could look at the status registers and decide whether to single-step again. TBC, I'm not suggesting that we need to do that right now. The emulation solution discussed on the lkml thread made me thing of displaced stepping -- the ARM implementation emulates some instructions. I wonder whether displaced stepping over IT blocks is already handled correctly. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-02-03 16:13 ` Pedro Alves @ 2017-02-17 1:42 ` Antoine Tremblay 2017-02-17 2:05 ` Pedro Alves 0 siblings, 1 reply; 38+ messages in thread From: Antoine Tremblay @ 2017-02-17 1:42 UTC (permalink / raw) To: Pedro Alves; +Cc: Antoine Tremblay, Yao Qi, gdb-patches Pedro Alves writes: > On 01/30/2017 01:28 PM, Antoine Tremblay wrote: > >>> We don't change anything when setting breakpoint inside IT block. >> >> Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on >> 2 bytes like discussed before. > Sorry for the delay I just saw your mail... > Can we restrict the stopping-all-threads to the case that > needs it, only? Possibly but I would like to avoid introducing 2 execution paths in the run control, it's already hard to follow as it is and it would introduce a lot of code in the arch independant code just for this case, that's something I would like to avoid too. > > An optimization that would avoid this would be to use a > hardware watchpoint/breakpoint (if available) for single-stepping. > IIRC, you can ARM a breakpoint (or was it a watchpoint) on ARM for > triggering when the PC is different from the current PC (or really, > some specified address). I did not know that but I'm worried about the limited number of hardware watchpoints available. IIRC on my board I had only 4, if GDBServer can't find one available would it refuse to single step ? That would be weird... ? It's an interesting approch however I'll dig about it more. > > In IT blocks, this would probably make the thread stop on > instructions that aren't really executed (e.g., the then/else > branches when the condition is correspondingly false/true), Really ? I need to find something about that in the arch man... > unlike the current solution where breakpoint instructions are > not executed by the CPU when it falls on an instruction that > isn't executed (because the breakpoint opcode we use it just > some magic invalid instruction, node the BKPT instruction), but > I think that when the thread stops, and we're stepping an IT > block, we could look at the status registers and decide whether > to single-step again. > > TBC, I'm not suggesting that we need to do that right now. > > The emulation solution discussed on the lkml thread made > me thing of displaced stepping -- the ARM implementation > emulates some instructions. I wonder whether displaced > stepping over IT blocks is already handled correctly. > It does look like displaced stepping would be preferable to trying to emulate and validate that the emulation is correct. Simon and I are looking into displaced stepping but we've yet to have this discussion with Yao... ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-02-17 1:42 ` Antoine Tremblay @ 2017-02-17 2:05 ` Pedro Alves 2017-02-17 3:06 ` Antoine Tremblay 0 siblings, 1 reply; 38+ messages in thread From: Pedro Alves @ 2017-02-17 2:05 UTC (permalink / raw) To: Antoine Tremblay; +Cc: Yao Qi, gdb-patches On 02/17/2017 01:41 AM, Antoine Tremblay wrote: > > Pedro Alves writes: > >> On 01/30/2017 01:28 PM, Antoine Tremblay wrote: >> >>>> We don't change anything when setting breakpoint inside IT block. >>> >>> Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on >>> 2 bytes like discussed before. >> > > Sorry for the delay I just saw your mail... > >> Can we restrict the stopping-all-threads to the case that >> needs it, only? > > Possibly but I would like to avoid introducing 2 execution paths in the > run control, it's already hard to follow as it is and it would introduce > a lot of code in the arch independant code just for this case, that's > something I would like to avoid too. I don't immediately see how this would imply introducing lots of code in the run control. We shouldn't be imposing this stop-all-threads on all archs, since it adds a lot of slowness (and the more threads the inferior has, the worse it gets). So if we already need the 2 execution paths, making the condition that determines whether to pause all threads consider more state doesn't seem to add that much complexity to the run control part itself. >> An optimization that would avoid this would be to use a >> hardware watchpoint/breakpoint (if available) for single-stepping. >> IIRC, you can ARM a breakpoint (or was it a watchpoint) on ARM for >> triggering when the PC is different from the current PC (or really, >> some specified address). > > I did not know that but I'm worried about the limited number of hardware > watchpoints available. IIRC on my board I had only 4, if GDBServer can't > find one available would it refuse to single step ? That would be > weird... ? My thought was that we'd give preference to user-requested watchpoints. I.e., treat this as an optimization. We always need to pause all threads in order to install watchpoints in all threads (watchpoints must be inserted with the thread paused, and we do that on thread resume). So if a request for a user-watchpoints comes in, we'd just interrupt the current single-step as we currently do, install the watchpoints, and go back to single-stepping, if it didn't manage to finish, as we currently do. At this point, we notice that we don't have free hardware watchpoints left, and fallback to do the slow software single-step as before. > > It's an interesting approch however I'll dig about it more. > >> >> In IT blocks, this would probably make the thread stop on >> instructions that aren't really executed (e.g., the then/else >> branches when the condition is correspondingly false/true), > > Really ? I need to find something about that in the arch man... AFAIK, in IT blocks, all instructions always "execute", but, when the condition is false, the instruction becomes as-if a nop. The only reason breakpoints don't stop there currently is that breakpoints are just another instruction (actually an undefined instruction the kernel is aware of, that causes an undefined instruction exception that the kernel translates to a SIGTRAP instead of a SIGILL), and when the condition is false, the breakpoint instruction becomes a nop too... If you have a hardware trap set to trap at such an address though, then it should trap, I believe, as if you had armed a hardware breakpoint to trap on the address of a real nop instruction. Thanks, Pedro Alves ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-02-17 2:05 ` Pedro Alves @ 2017-02-17 3:06 ` Antoine Tremblay 2017-02-17 22:19 ` Yao Qi 0 siblings, 1 reply; 38+ messages in thread From: Antoine Tremblay @ 2017-02-17 3:06 UTC (permalink / raw) To: Pedro Alves; +Cc: Antoine Tremblay, Yao Qi, gdb-patches Pedro Alves writes: > On 02/17/2017 01:41 AM, Antoine Tremblay wrote: >> >> Pedro Alves writes: >> >>> On 01/30/2017 01:28 PM, Antoine Tremblay wrote: >>> >>>>> We don't change anything when setting breakpoint inside IT block. >>>> >>>> Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on >>>> 2 bytes like discussed before. >>> >> >> Sorry for the delay I just saw your mail... >> >>> Can we restrict the stopping-all-threads to the case that >>> needs it, only? >> >> Possibly but I would like to avoid introducing 2 execution paths in the >> run control, it's already hard to follow as it is and it would introduce >> a lot of code in the arch independant code just for this case, that's >> something I would like to avoid too. > > I don't immediately see how this would imply introducing lots of code > in the run control. Well lots can be debatable, but at first glance you would basically need this current posted patch + what it removes and a switch between the 2. And I'm not sure how the IT block detection would be done. It just seems like much to me, I'm actually very surprised that you're suggesting having those two paths. It seems like it creates much to maintain to support this particular problem with the ARM breakpoints. Maybe it's just that I had such a hard time getting all that run control right, it's full of traps and corner cases with all-stop/non-stop, stopping, suspending, deleting the breakpoints, re-adding them at the right time etc... Adding more state is giving me quite a headache. > We shouldn't be imposing this stop-all-threads > on all archs, since it adds a lot of slowness (and > the more threads the inferior has, the worse it gets). I would be the first the agree usually that's something I would like to avoid but considering that it was crashing the inferior in the only architecture using this, not stopping seemed less important. Also I'm not arguing to keep it like this forever but until there's a better solution to the problem it seemed reasonable to me to take the performance hit. And I was pretty much certain that before another arch uses this we would have figured it out. Is there another arch on the horizon that would use this ? >So if > we already need the 2 execution paths, making the condition that > determines whether to pause all threads consider more state > doesn't seem to add that much complexity to the run control > part itself. > >>> An optimization that would avoid this would be to use a >>> hardware watchpoint/breakpoint (if available) for single-stepping. >>> IIRC, you can ARM a breakpoint (or was it a watchpoint) on ARM for >>> triggering when the PC is different from the current PC (or really, >>> some specified address). >> >> I did not know that but I'm worried about the limited number of hardware >> watchpoints available. IIRC on my board I had only 4, if GDBServer can't >> find one available would it refuse to single step ? That would be >> weird... ? > > My thought was that we'd give preference to user-requested > watchpoints. I.e., treat this as an optimization. We always need to > pause all threads in order to install watchpoints in all > threads (watchpoints must be inserted with the thread paused, and > we do that on thread resume). So if a request for a user-watchpoints > comes in, we'd just interrupt the current single-step as we currently > do, install the watchpoints, and go back to single-stepping, if it didn't > manage to finish, as we currently do. At this point, we notice that we > don't have free hardware watchpoints left, and fallback to do > the slow software single-step as before. > OK I see. Interesting. >> >> It's an interesting approch however I'll dig about it more. >> >>> >>> In IT blocks, this would probably make the thread stop on >>> instructions that aren't really executed (e.g., the then/else >>> branches when the condition is correspondingly false/true), >> >> Really ? I need to find something about that in the arch man... > > AFAIK, in IT blocks, all instructions always "execute", but, when > the condition is false, the instruction becomes as-if a nop. > The only reason breakpoints don't stop there currently is that > breakpoints are just another instruction (actually an undefined > instruction the kernel is aware of, that causes an undefined > instruction exception that the kernel translates to a SIGTRAP > instead of a SIGILL), and when the condition is false, the breakpoint > instruction becomes a nop too... If you have a hardware trap > set to trap at such an address though, then it should trap, I believe, > as if you had armed a hardware breakpoint to trap on the address > of a real nop instruction. > Seems to make sense :) I'll test it out with a hardware breakpoint. Thanks!, Antoine ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-02-17 3:06 ` Antoine Tremblay @ 2017-02-17 22:19 ` Yao Qi 2017-02-18 0:19 ` Antoine Tremblay 0 siblings, 1 reply; 38+ messages in thread From: Yao Qi @ 2017-02-17 22:19 UTC (permalink / raw) To: Antoine Tremblay; +Cc: Pedro Alves, gdb-patches On Fri, Feb 17, 2017 at 3:05 AM, Antoine Tremblay <antoine.tremblay@ericsson.com> wrote: > > And I'm not sure how the IT block detection would be done. > In ARM ARM, we have the pseudo code, boolean InITBlock() return (ITSTATE.IT<3:0> != ‘0000’); ITSTATE can be got from CPSR. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-02-17 22:19 ` Yao Qi @ 2017-02-18 0:19 ` Antoine Tremblay 2017-02-18 22:49 ` Yao Qi 0 siblings, 1 reply; 38+ messages in thread From: Antoine Tremblay @ 2017-02-18 0:19 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, Pedro Alves, gdb-patches Yao Qi writes: > On Fri, Feb 17, 2017 at 3:05 AM, Antoine Tremblay > <antoine.tremblay@ericsson.com> wrote: >> >> And I'm not sure how the IT block detection would be done. >> > > In ARM ARM, we have the pseudo code, > > boolean InITBlock() > return (ITSTATE.IT<3:0> != â0000â); > > ITSTATE can be got from CPSR. Yes that's good if you're inserting a breakpoint at current PC but otherwise you will need something else... ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-02-18 0:19 ` Antoine Tremblay @ 2017-02-18 22:49 ` Yao Qi 2017-02-19 19:40 ` Antoine Tremblay 2017-03-29 12:41 ` Antoine Tremblay 0 siblings, 2 replies; 38+ messages in thread From: Yao Qi @ 2017-02-18 22:49 UTC (permalink / raw) To: Antoine Tremblay; +Cc: Pedro Alves, gdb-patches On 17-02-17 19:17:56, Antoine Tremblay wrote: > > In ARM ARM, we have the pseudo code, > > > > boolean InITBlock() > > return (ITSTATE.IT<3:0> != ‘0000’); > > > > ITSTATE can be got from CPSR. > > Yes that's good if you're inserting a breakpoint at current PC but > otherwise you will need something else... In software single step, we calculate the next pcs, and select breakpoint kinds of them, according to current pc. If current pc is not within IT block (!InITBlock ()) or the last instruction in IT block (LastInITBlock ()), we can safely use 16-bit thumb breakpoint for any thumb instruction. That is, in gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state, we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()). Then, in some level, when installing software single step breakpoints, if one breakpoint type is ARM_BP_KIND_THUMB2 and installed address is 2-byte aligned, stop all threads. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 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 1 sibling, 1 reply; 38+ messages in thread From: Antoine Tremblay @ 2017-02-19 19:40 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, Pedro Alves, gdb-patches Yao Qi writes: > On 17-02-17 19:17:56, Antoine Tremblay wrote: >> > In ARM ARM, we have the pseudo code, >> > >> > boolean InITBlock() >> > return (ITSTATE.IT<3:0> != â0000â); >> > >> > ITSTATE can be got from CPSR. >> >> Yes that's good if you're inserting a breakpoint at current PC but >> otherwise you will need something else... > > In software single step, we calculate the next pcs, and select > breakpoint kinds of them, according to current pc. If current > pc is not within IT block (!InITBlock ()) or the last instruction > in IT block (LastInITBlock ()), we can safely use 16-bit thumb > breakpoint for any thumb instruction. That is, in > gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state, > we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()). > > Then, in some level, when installing software single step breakpoints, > if one breakpoint type is ARM_BP_KIND_THUMB2 and installed > address is 2-byte aligned, stop all threads. Yes that would make sense but I think we can be calling get_next_pc to insert a software single step breakpoint at an address different then the current PCs next address. See: https://sourceware.org/ml/gdb-patches/2016-06/msg00268.html "> If we only remove before reporting an event to gdb, then I don't > understand this. We already insert single-step breakpoints when > we process the resume request from gdb, no? We insert single-step breakpoints when we process the resume requests and threads are about to be resumed. If threads still have pending status, single-step breakpoints are not installed, so we need to install them in proceed_all_lwps." This is still true AFAIK so GDBServer may be at any PC stopped on a pending event and need to insert a single step breakpoint at an address unrelated to that event so in that case CPSR can't be used... Thanks, Antoine ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-02-19 19:40 ` Antoine Tremblay @ 2017-02-19 20:31 ` Antoine Tremblay 0 siblings, 0 replies; 38+ messages in thread From: Antoine Tremblay @ 2017-02-19 20:31 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, Pedro Alves, gdb-patches Antoine Tremblay writes: > Yao Qi writes: > >> On 17-02-17 19:17:56, Antoine Tremblay wrote: >>> > In ARM ARM, we have the pseudo code, >>> > >>> > boolean InITBlock() >>> > return (ITSTATE.IT<3:0> != â0000â); >>> > >>> > ITSTATE can be got from CPSR. >>> >>> Yes that's good if you're inserting a breakpoint at current PC but >>> otherwise you will need something else... >> >> In software single step, we calculate the next pcs, and select >> breakpoint kinds of them, according to current pc. If current >> pc is not within IT block (!InITBlock ()) or the last instruction >> in IT block (LastInITBlock ()), we can safely use 16-bit thumb >> breakpoint for any thumb instruction. That is, in >> gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state, >> we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()). >> >> Then, in some level, when installing software single step breakpoints, >> if one breakpoint type is ARM_BP_KIND_THUMB2 and installed >> address is 2-byte aligned, stop all threads. > > Yes that would make sense but I think we can be calling get_next_pc > to insert a software single step breakpoint at an address different then > the current PCs next address. > > See: https://sourceware.org/ml/gdb-patches/2016-06/msg00268.html > > "> If we only remove before reporting an event to gdb, then I don't >> understand this. We already insert single-step breakpoints when >> we process the resume request from gdb, no? > > We insert single-step breakpoints when we process the resume requests > and threads are about to be resumed. If threads still have pending > status, single-step breakpoints are not installed, so we need to install > them in proceed_all_lwps." > > This is still true AFAIK so GDBServer may be at any PC stopped on a > pending event and need to insert a single step breakpoint at an address > unrelated to that event so in that case CPSR can't be used... > > Thanks, > Antoine Oops looks like it had been a while since I checked the get_next_pc code, so we switch the current thread and use that PC so it should work. Disregard my last mail :) Looks like this solution is a good way forward. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-02-18 22:49 ` Yao Qi 2017-02-19 19:40 ` Antoine Tremblay @ 2017-03-29 12:41 ` Antoine Tremblay 2017-03-29 14:11 ` Antoine Tremblay 2017-03-30 16:06 ` Yao Qi 1 sibling, 2 replies; 38+ messages in thread From: Antoine Tremblay @ 2017-03-29 12:41 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, Pedro Alves, gdb-patches Yao Qi writes: > On 17-02-17 19:17:56, Antoine Tremblay wrote: >> > In ARM ARM, we have the pseudo code, >> > >> > boolean InITBlock() >> > return (ITSTATE.IT<3:0> != â0000â); >> > >> > ITSTATE can be got from CPSR. >> >> Yes that's good if you're inserting a breakpoint at current PC but >> otherwise you will need something else... > > In software single step, we calculate the next pcs, and select > breakpoint kinds of them, according to current pc. If current > pc is not within IT block (!InITBlock ()) or the last instruction > in IT block (LastInITBlock ()), we can safely use 16-bit thumb > breakpoint for any thumb instruction. That is, in > gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state, > we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()). This is not entirely true since we have to check if the next pcs are in an IT block rather then only the current one, so there's multiple scenarios. Consider if current PC is the IT instruction for example, then there's at least 2 next pcs inside the IT block where we will need to install an THUMB2 breakpoint and get_next_pcs will return that. Now I find myself trying to replicate the logic in thumb_get_next_pcs_raw for IT blocks in arm_breakpoint_kind_from_current_state and it doesn't feel right. It gives something like: void set_single_step_breakpoint (CORE_ADDR stop_at, ptid_t ptid) { struct single_step_breakpoint *bp; gdb_assert (ptid_get_pid (current_ptid) == ptid_get_pid (ptid)); CORE_ADDR placed_address = stop_at; int breakpoint_kind = target_breakpoint_kind_from_current_state (&placed_address); bp = (struct single_step_breakpoint *) set_breakpoint_type_at_with_kind (single_step_breakpoint, placed_address, NULL, breakpoint_kind); bp->ptid = ptid; } int arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr) { if (arm_is_thumb_mode ()) { if ( current_pcs_or_expected_next_pcs_are_in_it_block ??? /* That's not right... */ { *pcptr = UNMAKE_THUMB_ADDR (*pcptr); return ARM_BP_KIND_THUMB; } else { *pcptr = MAKE_THUMB_ADDR (*pcptr); return arm_breakpoint_kind_from_pc (pcptr); } } else { return arm_breakpoint_kind_from_pc (pcptr); } } It would be much better if get_next_pcs could select the breakpoint kind itself and hint that to set_single_step_breakpoint like : VEC (struct { next_pc, breakpoint_kind }) = (*the_low_target.get_next_pcs) (regcache); for (i = 0; VEC_iterate (struct { CORE_ADDR, kind }, next_pcs, i, pc); ++i) set_single_step_breakpoint (pc, kind, current_ptid); But of course that means changing that virtual function for all archs etc... :( I'm thinking of going with that approch but I would like to know if you have any other solutions to propose or if that sounds OK before writing all that code ? Thanks, Antoine ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 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 1 sibling, 1 reply; 38+ messages in thread From: Antoine Tremblay @ 2017-03-29 14:11 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, Pedro Alves, gdb-patches Antoine Tremblay writes: > Yao Qi writes: > >> On 17-02-17 19:17:56, Antoine Tremblay wrote: >>> > In ARM ARM, we have the pseudo code, >>> > >>> > boolean InITBlock() >>> > return (ITSTATE.IT<3:0> != â0000â); >>> > >>> > ITSTATE can be got from CPSR. >>> >>> Yes that's good if you're inserting a breakpoint at current PC but >>> otherwise you will need something else... >> >> In software single step, we calculate the next pcs, and select >> breakpoint kinds of them, according to current pc. If current >> pc is not within IT block (!InITBlock ()) or the last instruction >> in IT block (LastInITBlock ()), we can safely use 16-bit thumb >> breakpoint for any thumb instruction. That is, in >> gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state, >> we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()). > > This is not entirely true since we have to check if the next pcs are in > an IT block rather then only the current one, so there's multiple > scenarios. > > Consider if current PC is the IT instruction for example, then there's > at least 2 next pcs inside the IT block where we will need to install an THUMB2 > breakpoint and get_next_pcs will return that. > > Now I find myself trying to replicate the logic in thumb_get_next_pcs_raw > for IT blocks in arm_breakpoint_kind_from_current_state and it doesn't > feel right. > > It gives something like: > > void > set_single_step_breakpoint (CORE_ADDR stop_at, ptid_t ptid) > { > struct single_step_breakpoint *bp; > > gdb_assert (ptid_get_pid (current_ptid) == ptid_get_pid (ptid)); > > CORE_ADDR placed_address = stop_at; > int breakpoint_kind > = target_breakpoint_kind_from_current_state (&placed_address); > > bp = (struct single_step_breakpoint *) set_breakpoint_type_at_with_kind > (single_step_breakpoint, placed_address, NULL, breakpoint_kind); > bp->ptid = ptid; > } > > int > arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr) > { > if (arm_is_thumb_mode ()) > { > > if ( current_pcs_or_expected_next_pcs_are_in_it_block ??? /* That's > not right... */ > > { > *pcptr = UNMAKE_THUMB_ADDR (*pcptr); > return ARM_BP_KIND_THUMB; > } > else > { > *pcptr = MAKE_THUMB_ADDR (*pcptr); > return arm_breakpoint_kind_from_pc (pcptr); > } > } > else > { > return arm_breakpoint_kind_from_pc (pcptr); > } > } > > It would be much better if get_next_pcs could select the breakpoint kind > itself and hint that to set_single_step_breakpoint like : > > VEC (struct { next_pc, breakpoint_kind }) = (*the_low_target.get_next_pcs) (regcache); > > for (i = 0; VEC_iterate (struct { CORE_ADDR, kind }, next_pcs, i, pc); ++i) > set_single_step_breakpoint (pc, kind, current_ptid); > > But of course that means changing that virtual function for all archs > etc... :( > > I'm thinking of going with that approch but I would like to know if you > have any other solutions to propose or if that sounds OK before writing > all that code ? > > Thanks, > Antoine Just a small follow-up on this idea, I think it would simplify GDB's implementation too, it would have to change anyway since the interface is shared. See commit 833b7ab5008b769dca6db6d5ee1d21d33e730132 introduces a special case in arm_breakpoint_kind_from_current_state where it's checked if GDB is single stepping and if so redoes the arm_get_next_pc to get the execution mode of the next instruction. I could do the same kind of thing in GDBServer recall get_next_pcs and if I get > 1 PC in the vect it would mean I have an IT block, but it sounds like a hack. I also find that confusing given that the documentation for breakpoint_kind_from_current_state is: # Return the breakpoint kind for this target based on the current # processor state (e.g. the current instruction mode on ARM) and the # *PCPTR. In default, it is gdbarch->breakpoint_kind_from_pc. Finding the next PCs kind with this function seems like adding another meaning to it... ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-03-29 14:11 ` Antoine Tremblay @ 2017-03-29 17:54 ` Antoine Tremblay 0 siblings, 0 replies; 38+ messages in thread From: Antoine Tremblay @ 2017-03-29 17:54 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, Pedro Alves, gdb-patches Antoine Tremblay writes: >> Consider if current PC is the IT instruction for example, then there's >> at least 2 next pcs inside the IT block where we will need to install an THUMB2 >> breakpoint and get_next_pcs will return that. Oops please read that "Consider if the current instruction is the CMP instruction before an IT instruction...." Basically so that we get into the arm-get-next-pcs.c:351 case... in fact now that I think of it maybe that would be OK if I were to add that check in breakpoint_kind_from_current_state also but the previous comments still apply about the possibly hackish state of this.. Thanks, Antoine ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-03-29 12:41 ` Antoine Tremblay 2017-03-29 14:11 ` Antoine Tremblay @ 2017-03-30 16:06 ` Yao Qi 2017-03-30 18:31 ` Antoine Tremblay 1 sibling, 1 reply; 38+ messages in thread From: Yao Qi @ 2017-03-30 16:06 UTC (permalink / raw) To: Antoine Tremblay; +Cc: Pedro Alves, gdb-patches Antoine Tremblay <antoine.tremblay@ericsson.com> writes: >> In software single step, we calculate the next pcs, and select >> breakpoint kinds of them, according to current pc. If current >> pc is not within IT block (!InITBlock ()) or the last instruction >> in IT block (LastInITBlock ()), we can safely use 16-bit thumb >> breakpoint for any thumb instruction. That is, in >> gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state, >> we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()). > > This is not entirely true since we have to check if the next pcs are in > an IT block rather then only the current one, so there's multiple > scenarios. In fact, we need to know whether the next pcs will be conditionally executed or not, right? If they won't be conditionally executed, we can use 16-bit thumb breakpoint instruction. By checking CPSR, if current PC is not in IT block or on the last instruction in IT block, the next pcs can't be conditionally executed. I've already had a patch below to implement what I described. The problem of this patch is that we end up inserting different kinds of breakpoints on the same instruction. For a given 32-bit thumb instruction, GDB and GDBserver knows 32-bit thumb breakpoint instruction is used for GDB breakpoint, but only GDBserver knows 16-bit thumb breakpoint is used for GDBserver single-step breakpoint, so GDB will be confused on this. I stopped here, and start to do something else. > > Consider if current PC is the IT instruction for example, then there's > at least 2 next pcs inside the IT block where we will need to install an THUMB2 > breakpoint and get_next_pcs will return that. -- Yao (齐尧) From fad6162c9365535a09ca1072f15469e6007c0fd2 Mon Sep 17 00:00:00 2001 From: Yao Qi <yao.qi@linaro.org> Date: Fri, 24 Feb 2017 09:25:43 +0000 Subject: [PATCH] Only use 32-bit thumb-2 breakpoint instruction on necessary It takes two PTRACE_POKETEXT calls to write 32-bit thumb-2 breakpoint to a 2-byte aligned address, and other threads can observe the partially modified instruction in the memory between these two calls. This causes problems on single stepping multi-thread program in GDBserver. 32-bit thumb-2 breakpoint was invented for single step 32-bit thumb-2 instruction in IT block, http://lists.infradead.org/pipermail/linux-arm-kernel/2010-January/007488.html but we can use 16-bit thumb breakpoint instruction anywhere else. That is what this patch does. Change in set_breakpoint_type_at is similar to breakpoint.c:breakpoint_kind. This patch fixes fails in gdb.threads/schedlock.exp. Even with patch, 32-bit thumb-2 breakpoint is still used for 32-bit thumb-2 instructions in IT block, so the problem is still there. This patch is a partial fix to PR 21169. gdb/gdbserver: 2017-02-24 Yao Qi <yao.qi@linaro.org> PR server/21169 * linux-aarch32-low.c (arm_breakpoint_kind_from_current_state): Set kind to ARM_BP_KIND_THUMB if program is out of IT block or on the last instruction of IT block. * mem-break.c (set_breakpoint_type_at): Call target_breakpoint_kind_from_current_state to get breakpoint kind for single_step breakpoint. diff --git a/gdb/gdbserver/linux-aarch32-low.c b/gdb/gdbserver/linux-aarch32-low.c index 2b710ba..7409050 100644 --- a/gdb/gdbserver/linux-aarch32-low.c +++ b/gdb/gdbserver/linux-aarch32-low.c @@ -288,7 +288,30 @@ arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr) if (arm_is_thumb_mode ()) { *pcptr = MAKE_THUMB_ADDR (*pcptr); - return arm_breakpoint_kind_from_pc (pcptr); + int kind = arm_breakpoint_kind_from_pc (pcptr); + + if (kind == ARM_BP_KIND_THUMB2) + { + unsigned long cpsr; + struct regcache *regcache + = get_thread_regcache (current_thread, 1); + + collect_register_by_name (regcache, "cpsr", &cpsr); + /* Only use 32-bit thumb-2 breakpoint if *PCPTR is within + IT block, because it takes two PTRACE_POKETEXT calls to + write 32-bit thumb-2 breakpoint to a 2-byte aligned + address, and other threads can observe the partially + modified instruction in the memory between two + PTRACE_POKETEXT calls. + + Don't use 32-bit thumb-2 breakpoint if program is not + in IT block or on the last instruction of IT block, + (ITSTATE.IT<2:0> == 000). These bits are from CPSR bit + 10, 25, and 26. */ + if ((cpsr & 0x06000400) == 0) + kind = ARM_BP_KIND_THUMB; + } + return kind; } else { diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c index 6e6926a..f3845cf 100644 --- a/gdb/gdbserver/mem-break.c +++ b/gdb/gdbserver/mem-break.c @@ -855,7 +855,21 @@ set_breakpoint_type_at (enum bkpt_type type, CORE_ADDR where, { int err_ignored; CORE_ADDR placed_address = where; - int breakpoint_kind = target_breakpoint_kind_from_pc (&placed_address); + int breakpoint_kind; + + /* Get the kind of breakpoint to PLACED_ADDRESS except single-step + breakpoint. Get the kind of single-step breakpoint according to + the current register state. */ + if (type == single_step_breakpoint) + { + breakpoint_kind + = target_breakpoint_kind_from_current_state (&placed_address); + } + else + { + breakpoint_kind + = target_breakpoint_kind_from_pc (&placed_address); + } return set_breakpoint (type, raw_bkpt_type_sw, placed_address, breakpoint_kind, handler, ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-03-30 16:06 ` Yao Qi @ 2017-03-30 18:31 ` Antoine Tremblay 2017-03-31 16:31 ` Yao Qi 0 siblings, 1 reply; 38+ messages in thread From: Antoine Tremblay @ 2017-03-30 18:31 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, Pedro Alves, gdb-patches Yao Qi writes: > Antoine Tremblay <antoine.tremblay@ericsson.com> writes: > >>> In software single step, we calculate the next pcs, and select >>> breakpoint kinds of them, according to current pc. If current >>> pc is not within IT block (!InITBlock ()) or the last instruction >>> in IT block (LastInITBlock ()), we can safely use 16-bit thumb >>> breakpoint for any thumb instruction. That is, in >>> gdbserver/linux-aarch32-low.c:arm_breakpoint_kind_from_current_state, >>> we can return ARM_BP_KIND_THUMB if (!InITBlock () || LastInITBlock ()). >> >> This is not entirely true since we have to check if the next pcs are in >> an IT block rather then only the current one, so there's multiple >> scenarios. > > In fact, we need to know whether the next pcs will be conditionally > executed or not, right? If they won't be conditionally executed, we can > use 16-bit thumb breakpoint instruction. By checking CPSR, if > current PC is not in IT block or on the last instruction in IT block, > the next pcs can't be conditionally executed. I've already had a patch > below to implement what I described. > You have to add if the current instruction is an IT instruction in wich case the next instruction will be in an IT block. Also if you have a conditional instruction that would evalutate to true and is not the last one, get_next_pcs may return an instruction after the IT block, arm_breakpoint_kind_from_current_state will be called from the IT block with that PC and return a THUMB2_KIND when it should not. See the else case in arm-get-next-pcs.c:~351 My point was that we should use get_next_pc directly since it's the best place to detect if the next_pc is in the IT block. And the intent would be clear. It would give something like the patch below. (Note the GDB part of this is missing but it works with GDBServer) > The problem of this patch is that we end up inserting different > kinds of breakpoints on the same instruction. For a given 32-bit thumb > instruction, GDB and GDBserver knows 32-bit thumb breakpoint instruction > is used for GDB breakpoint, but only GDBserver knows 16-bit thumb > breakpoint is used for GDBserver single-step breakpoint, so GDB will be > confused on this. I stopped here, and start to do something else. Humm but how will the GDBServer 16-bit breakpoint be reported to GDB ? Won't it always be hit and handled by GDBServer ? And if you have a GDB breakpoint on an instruction and GDBServer puts a single step breakpoint on that GDB breakpoint instruction, GDBServer still knows of the GDB and GDBServer breakpoint types. So how does GDB get confused ? ---- commit a18806af57b6c8906594ded036009c6b30c5b7db Author: Antoine Tremblay <antoine.tremblay@ericsson.com> Date: Thu Mar 30 12:57:29 2017 -0400 getnextpc encodes the kind diff --git a/gdb/arch/arm-get-next-pcs.c b/gdb/arch/arm-get-next-pcs.c index 398dd59a..f9b5bf1 100644 --- a/gdb/arch/arm-get-next-pcs.c +++ b/gdb/arch/arm-get-next-pcs.c @@ -315,6 +315,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self) itstate = thumb_advance_itstate (itstate); } + pc = MAKE_THUMB2_BP_KIND (pc); VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc)); return next_pcs; } @@ -335,6 +336,7 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self) itstate = thumb_advance_itstate (itstate); } + pc = MAKE_THUMB2_BP_KIND (pc); VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc)); return next_pcs; } @@ -361,8 +363,13 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self) /* Set a breakpoint on the following instruction. */ gdb_assert ((itstate & 0x0f) != 0); + pc = MAKE_THUMB2_BP_KIND (pc); VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc)); + /* Reset the thumb2 kind bit to avoid affecting the next pc + value. */ + pc = UNMAKE_THUMB2_BP_KIND (pc); + cond_negated = (itstate >> 4) & 1; /* Skip all following instructions with the same @@ -378,6 +385,11 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self) } while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated); + /* Only set a thumb2 breakpoint kind if we are still in an + IT block. */ + if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated) + pc = MAKE_THUMB2_BP_KIND (pc); + VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc)); return next_pcs; diff --git a/gdb/arch/arm-linux.c b/gdb/arch/arm-linux.c index 943efe7..4b20f77 100644 --- a/gdb/arch/arm-linux.c +++ b/gdb/arch/arm-linux.c @@ -73,7 +73,7 @@ arm_linux_get_next_pcs_fixup (struct arm_get_next_pcs *self, step this instruction, this instruction isn't executed yet, and LR may not be updated yet. In other words, GDB can get the target address from LR if this instruction isn't BL or BLX. */ - if (nextpc > 0xffff0000) + if ((nextpc & 0xffffffff) > 0xffff0000) { int bl_blx_p = 0; CORE_ADDR pc = regcache_read_pc (self->regcache); diff --git a/gdb/arch/arm.h b/gdb/arch/arm.h index a86dd37..b83fc79 100644 --- a/gdb/arch/arm.h +++ b/gdb/arch/arm.h @@ -101,6 +101,9 @@ enum arm_breakpoint_kinds #define IS_THUMB_ADDR(addr) ((addr) & 1) #define MAKE_THUMB_ADDR(addr) ((addr) | 1) #define UNMAKE_THUMB_ADDR(addr) ((addr) & ~1) +#define IS_THUMB2_BP_KIND(addr) (((addr) & 1ULL << 32) >> 32) +#define MAKE_THUMB2_BP_KIND(addr) ((addr) | (1ULL << 32)) +#define UNMAKE_THUMB2_BP_KIND(addr) ((addr) & ~(1ULL << 32)) /* Support routines for instruction parsing. */ #define submask(x) ((1L << ((x) + 1)) - 1) diff --git a/gdb/gdbserver/linux-aarch32-low.c b/gdb/gdbserver/linux-aarch32-low.c index 2b710ba..fc66028 100644 --- a/gdb/gdbserver/linux-aarch32-low.c +++ b/gdb/gdbserver/linux-aarch32-low.c @@ -242,10 +242,16 @@ arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr) unsigned short inst1 = 0; target_read_memory (*pcptr, (gdb_byte *) &inst1, 2); - if (thumb_insn_size (inst1) == 4) - return ARM_BP_KIND_THUMB2; + if (thumb_insn_size (inst1) == 4 && IS_THUMB2_BP_KIND (*pcptr)) + { + *pcptr = UNMAKE_THUMB2_BP_KIND (*pcptr); + return ARM_BP_KIND_THUMB2; + } + + return ARM_BP_KIND_THUMB; } - return ARM_BP_KIND_THUMB; + else + return ARM_BP_KIND_THUMB; } else return ARM_BP_KIND_ARM; ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-03-30 18:31 ` Antoine Tremblay @ 2017-03-31 16:31 ` Yao Qi 2017-03-31 18:22 ` Antoine Tremblay 0 siblings, 1 reply; 38+ messages in thread From: Yao Qi @ 2017-03-31 16:31 UTC (permalink / raw) To: Antoine Tremblay; +Cc: Pedro Alves, gdb-patches Antoine Tremblay <antoine.tremblay@ericsson.com> writes: > You have to add if the current instruction is an IT instruction in wich > case the next instruction will be in an IT block. > Oh, you are right. > Also if you have a conditional instruction that would evalutate to > true and is not the last one, get_next_pcs may return an instruction > after the IT block, arm_breakpoint_kind_from_current_state will be > called from the IT block with that PC and return a THUMB2_KIND when it > should not. See the else case in arm-get-next-pcs.c:~351 With the current PC and CPSR, it is not difficult to know whether next_pc is still within IT block nor not, because all instructions in IT block should be sequentially executed or skipped. > > My point was that we should use get_next_pc directly since it's the best > place to detect if the next_pc is in the IT block. And the intent would > be clear. Yeah, we can record the information of breakpoint type in the return value of get_next_pc, ... > > It would give something like the patch below. (Note the GDB part of this > is missing but it works with GDBServer) > ... but using extra bit in CORE_ADDR is not a good idea to me. >> The problem of this patch is that we end up inserting different >> kinds of breakpoints on the same instruction. For a given 32-bit thumb >> instruction, GDB and GDBserver knows 32-bit thumb breakpoint instruction >> is used for GDB breakpoint, but only GDBserver knows 16-bit thumb >> breakpoint is used for GDBserver single-step breakpoint, so GDB will be >> confused on this. I stopped here, and start to do something else. > > Humm but how will the GDBServer 16-bit breakpoint be reported to GDB ? > Won't it always be hit and handled by GDBServer ? > > And if you have a GDB breakpoint on an instruction and GDBServer puts > a single step breakpoint on that GDB breakpoint instruction, GDBServer > still knows of the GDB and GDBServer breakpoint types. > > So how does GDB get confused ? That was my conclusion at that point. I got some regressions in gdb.threads/*.exp when I tested my patch (gdb running is on x86_64-linux), but I can't remember more details. I am also wondering that we can use some code in arm_adjust_breakpoint_address about detecting BPADDR is within IT block or not by scanning instructions backward, if none of two bytes (can be 16-bit thumb instruction or the 2nd half of 32-bit thumb instruction) matches IT instruction, the PC is not within IT block. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-03-31 16:31 ` Yao Qi @ 2017-03-31 18:22 ` Antoine Tremblay 2017-04-03 12:41 ` Yao Qi 0 siblings, 1 reply; 38+ messages in thread From: Antoine Tremblay @ 2017-03-31 18:22 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, Pedro Alves, gdb-patches Yao Qi writes: > Antoine Tremblay <antoine.tremblay@ericsson.com> writes: > >> You have to add if the current instruction is an IT instruction in wich >> case the next instruction will be in an IT block. >> > > Oh, you are right. > >> Also if you have a conditional instruction that would evalutate to >> true and is not the last one, get_next_pcs may return an instruction >> after the IT block, arm_breakpoint_kind_from_current_state will be >> called from the IT block with that PC and return a THUMB2_KIND when it >> should not. See the else case in arm-get-next-pcs.c:~351 > > With the current PC and CPSR, it is not difficult to know whether > next_pc is still within IT block nor not, because all instructions in IT > block should be sequentially executed or skipped. > I don't think you could figure out that this last instruction that get_next_pc is returning after the IT block is or not in it however without redoing much of the work. I think maybe the best solution would be to abstract only that part of get_next_pc in a function: the if block starting with if (self->has_thumb2_breakpoint) around line 301. And have only that part return the next_pc + the breakpoint kind, this would avoid breaking all the virtual get_next_pc functions just for that case and allow the same code to be used in kind_from_current_state. We'll still redo the work but at least the code will be in one place. WDYT ? >> >> My point was that we should use get_next_pc directly since it's the best >> place to detect if the next_pc is in the IT block. And the intent would >> be clear. > > Yeah, we can record the information of breakpoint type in the return > value of get_next_pc, ... > >> >> It would give something like the patch below. (Note the GDB part of this >> is missing but it works with GDBServer) >> > > ... but using extra bit in CORE_ADDR is not a good idea to me. > Yes it was quite hackish I was able to test with the CORE_ADDR patch that it somewhat works at least. Note that I say somewhat because stopping all the threads is proving more problematic then I thought, I took the inspiration from your original patch series V2 and installed all the single step breakpoints in linux_resume and proceed_all_lwp. But in linux_wait_event_filtered, linux_resume_one is also called with the possibility of a stepping thread in 2 places. And you can't call stop_all_lwps there... So I'm scratching my head on how to stop the threads there thinking about like calling sigprocmask (SIG_SETMASK, &prev_mask, NULL); in there to allow linux_wait_event_filtered to be called recursively... possibly, I just don't see a clean way. There's other issues too where I get GDB adjusting a breakpoint and the inferior crashing.... Might be other issues too :( >>> The problem of this patch is that we end up inserting different >>> kinds of breakpoints on the same instruction. For a given 32-bit thumb >>> instruction, GDB and GDBserver knows 32-bit thumb breakpoint instruction >>> is used for GDB breakpoint, but only GDBserver knows 16-bit thumb >>> breakpoint is used for GDBserver single-step breakpoint, so GDB will be >>> confused on this. I stopped here, and start to do something else. >> >> Humm but how will the GDBServer 16-bit breakpoint be reported to GDB ? >> Won't it always be hit and handled by GDBServer ? >> >> And if you have a GDB breakpoint on an instruction and GDBServer puts >> a single step breakpoint on that GDB breakpoint instruction, GDBServer >> still knows of the GDB and GDBServer breakpoint types. >> >> So how does GDB get confused ? > > That was my conclusion at that point. I got some regressions in > gdb.threads/*.exp when I tested my patch (gdb running is on > x86_64-linux), but I can't remember more details. > OK. Do you remember if it had to do with displaced stepping on ? There's a problem with that and the current code in all-stop. I had fixed that with the original patch from this thread by not removing all single step breakpoints in all-stop... > I am also wondering that we can use some code in > arm_adjust_breakpoint_address about detecting BPADDR is within IT block > or not by scanning instructions backward, if none of two bytes (can be > 16-bit thumb instruction or the 2nd half of 32-bit thumb instruction) > matches IT instruction, the PC is not within IT block. Looking at the code, I think reusing parts of get_next_pc would be simpler. Note I'm including the test I use in case it's useful to you. --- commit ad0288b35d85e96b6c676c665b0063b74a293dab Author: Antoine Tremblay <antoine.tremblay@ericsson.com> Date: Thu Mar 30 11:14:17 2017 -0400 test single step diff --git a/gdb/testsuite/gdb.arch/arm-single-step.c b/gdb/testsuite/gdb.arch/arm-single-step.c new file mode 100644 index 0000000..e18f4ed --- /dev/null +++ b/gdb/testsuite/gdb.arch/arm-single-step.c @@ -0,0 +1,110 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2017 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +#include <pthread.h> +#include <unistd.h> +#include <stdlib.h> +#include <signal.h> + +#define NUM_THREADS 2 +#define TIMEOUT 5 + +const int num_threads = NUM_THREADS; +pthread_t child_thread[NUM_THREADS]; +volatile pthread_t signal_thread; +volatile int run = 1; + +void +handler (int signo) +{ + run = 0; +} + +/* Align the instruction on a 2 byte boundary + Missalign it with a 4 byte boundary. */ +#define THUMB2_INST \ + asm (" .align 4 \n" \ + " nop\n" \ + " nop.w\n" \ + ) \ + +#define ITBLOCK \ + asm (" .align 4 \n" \ + " nop\n" \ + " cmp r0, #0\n" \ + " ite eq\n" \ + " nop.w \n" \ + " nop.w \n" \ + ) \ + +#define LOOP(macro) \ + while (run) \ + { \ + macro; \ + } \ + +void out_of_loop () +{ + return; +} + +void * +thumb2_function (void *arg) +{ + LOOP (THUMB2_INST); /* break thumb2 */ + out_of_loop (); + pthread_exit (NULL); +} + +void * +itblock_function (void *arg) +{ + LOOP (ITBLOCK); /* break itblock */ + out_of_loop (); + pthread_exit (NULL); +} + +int +main (void) +{ + int res; + int i, j; + void *(*functions[2]) (void *); + + functions[0] = thumb2_function; + functions[1] = itblock_function; + + signal (SIGALRM, handler); + + for (i = 0; i < 2; i++) + { + alarm (TIMEOUT); + + for (j = 0; j < NUM_THREADS; j++) + { + res = pthread_create (&child_thread[j], NULL, functions[i], NULL); + } + + for (j = 0; j < NUM_THREADS; j++) + { + res = pthread_join (child_thread[j], NULL); + } + + run = 1; + } + exit(EXIT_SUCCESS); +} diff --git a/gdb/testsuite/gdb.arch/arm-single-step.exp b/gdb/testsuite/gdb.arch/arm-single-step.exp new file mode 100644 index 0000000..c85f981 --- /dev/null +++ b/gdb/testsuite/gdb.arch/arm-single-step.exp @@ -0,0 +1,42 @@ +# Copyright 2017 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +standard_testfile +set executable ${testfile} + +if { [gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } { + untested "failed to compile" + return -1 +} + +clean_restart $executable + +if ![runto_main] { + return -1 +} + +gdb_test_no_output "set scheduler-locking off" + + +# Test each instruction type, thumb2 is a plain 2 byte aligned but not 4 +# byte aligned thumb2 instruction. itblock is the same but in an itblock. +foreach_with_prefix inst { "thumb2" "itblock" } { + gdb_breakpoint [gdb_get_line_number "break $inst"] + gdb_continue_to_breakpoint "break thumb2" ".* break $inst .*" + delete_breakpoints +# gdb_breakpoint "out_of_loop" + gdb_test "step" ".*out_of_loop.*" "stepping $inst" + # delete_breakpoints +} ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-03-31 18:22 ` Antoine Tremblay @ 2017-04-03 12:41 ` Yao Qi 2017-04-03 13:18 ` Antoine Tremblay 0 siblings, 1 reply; 38+ messages in thread From: Yao Qi @ 2017-04-03 12:41 UTC (permalink / raw) To: Antoine Tremblay; +Cc: Pedro Alves, gdb-patches Antoine Tremblay <antoine.tremblay@ericsson.com> writes: > I think maybe the best solution would be to abstract only that part of > get_next_pc in a function: the if block starting with if > (self->has_thumb2_breakpoint) around line 301. > > And have only that part return the next_pc + the breakpoint kind, this > would avoid breaking all the virtual get_next_pc functions just for that > case and allow the same code to be used in kind_from_current_state. > > We'll still redo the work but at least the code will be in one > place. WDYT ? That should be fine, although I am not exactly sure what are you going to do. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-04-03 12:41 ` Yao Qi @ 2017-04-03 13:18 ` Antoine Tremblay 2017-04-03 15:18 ` Yao Qi 0 siblings, 1 reply; 38+ messages in thread From: Antoine Tremblay @ 2017-04-03 13:18 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, Pedro Alves, gdb-patches Yao Qi writes: > Antoine Tremblay <antoine.tremblay@ericsson.com> writes: > >> I think maybe the best solution would be to abstract only that part of >> get_next_pc in a function: the if block starting with if >> (self->has_thumb2_breakpoint) around line 301. >> >> And have only that part return the next_pc + the breakpoint kind, this >> would avoid breaking all the virtual get_next_pc functions just for that >> case and allow the same code to be used in kind_from_current_state. >> >> We'll still redo the work but at least the code will be in one >> place. WDYT ? > > That should be fine, although I am not exactly sure what are you > going to do. It looks like this, comments ?: ---- commit 9a8b46f9038e9d3805c8e6ec1bdf0dff1c95c065 Author: Antoine Tremblay <antoine.tremblay@ericsson.com> Date: Mon Apr 3 09:13:20 2017 -0400 refactor get-next-pc --- gdb/arch/arm-get-next-pcs.c | 199 ++++++++++++++++++++++---------------- gdb/arch/arm-get-next-pcs.h | 9 ++ gdb/gdbserver/linux-aarch32-low.c | 46 ++++++++- gdb/gdbserver/linux-aarch32-low.h | 3 + gdb/gdbserver/linux-arm-low.c | 21 ---- gdb/gdbserver/mem-break.c | 16 ++- 6 files changed, 188 insertions(+), 106 deletions(-) diff --git a/gdb/arch/arm-get-next-pcs.c b/gdb/arch/arm-get-next-pcs.c index 398dd59a..d967e16 100644 --- a/gdb/arch/arm-get-next-pcs.c +++ b/gdb/arch/arm-get-next-pcs.c @@ -22,6 +22,7 @@ #include "common-regcache.h" #include "arm.h" #include "arm-get-next-pcs.h" +#include <vector> /* See arm-get-next-pcs.h. */ @@ -258,6 +259,115 @@ arm_deal_with_atomic_sequence_raw (struct arm_get_next_pcs *self) return next_pcs; } +/* See arm-get-next-pcs.h. */ + +std::vector <std::pair<CORE_ADDR, arm_breakpoint_kinds>> +thumb_get_next_pcs_raw_itblock +(CORE_ADDR pc, unsigned short inst1, + ULONGEST status, ULONGEST itstate, + ULONGEST (*read_mem_uint) (CORE_ADDR memaddr, int len, int byte_order), + int byte_order_for_code) +{ + std::vector <std::pair <CORE_ADDR, arm_breakpoint_kinds>> next_pcs; + + if ((inst1 & 0xff00) == 0xbf00 && (inst1 & 0x000f) != 0) + { + /* An IT instruction. Because this instruction does not + modify the flags, we can accurately predict the next + executed instruction. */ + itstate = inst1 & 0x00ff; + pc += thumb_insn_size (inst1); + + while (itstate != 0 && ! condition_true (itstate >> 4, status)) + { + inst1 = read_mem_uint (pc, 2,byte_order_for_code); + pc += thumb_insn_size (inst1); + itstate = thumb_advance_itstate (itstate); + } + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); + return next_pcs; + } + else if (itstate != 0) + { + /* We are in a conditional block. Check the condition. */ + if (! condition_true (itstate >> 4, status)) + { + /* Advance to the next executed instruction. */ + pc += thumb_insn_size (inst1); + itstate = thumb_advance_itstate (itstate); + + while (itstate != 0 && ! condition_true (itstate >> 4, status)) + { + inst1 = read_mem_uint (pc, 2, byte_order_for_code); + + pc += thumb_insn_size (inst1); + itstate = thumb_advance_itstate (itstate); + } + + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); + return next_pcs; + } + else if ((itstate & 0x0f) == 0x08) + { + /* This is the last instruction of the conditional + block, and it is executed. We can handle it normally + because the following instruction is not conditional, + and we must handle it normally because it is + permitted to branch. Fall through. */ + } + else + { + int cond_negated; + + /* There are conditional instructions after this one. + If this instruction modifies the flags, then we can + not predict what the next executed instruction will + be. Fortunately, this instruction is archi2tecturally + forbidden to branch; we know it will fall through. + Start by skipping past it. */ + pc += thumb_insn_size (inst1); + itstate = thumb_advance_itstate (itstate); + + /* Set a breakpoint on the following instruction. */ + gdb_assert ((itstate & 0x0f) != 0); + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); + + cond_negated = (itstate >> 4) & 1; + + /* Skip all following instructions with the same + condition. If there is a later instruction in the IT + block with the opposite condition, set the other + breakpoint there. If not, then set a breakpoint on + the instruction after the IT block. */ + do + { + inst1 = read_mem_uint (pc, 2, byte_order_for_code); + pc += thumb_insn_size (inst1); + itstate = thumb_advance_itstate (itstate); + } + while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated); + + if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated) + { + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); + } + else + { + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB)); + } + + return next_pcs; + } + } + + return next_pcs; +} + /* Find the next possible PCs for thumb mode. */ static VEC (CORE_ADDR) * @@ -300,89 +410,14 @@ thumb_get_next_pcs_raw (struct arm_get_next_pcs *self) if (self->has_thumb2_breakpoint) { - if ((inst1 & 0xff00) == 0xbf00 && (inst1 & 0x000f) != 0) - { - /* An IT instruction. Because this instruction does not - modify the flags, we can accurately predict the next - executed instruction. */ - itstate = inst1 & 0x00ff; - pc += thumb_insn_size (inst1); - - while (itstate != 0 && ! condition_true (itstate >> 4, status)) - { - inst1 = self->ops->read_mem_uint (pc, 2,byte_order_for_code); - pc += thumb_insn_size (inst1); - itstate = thumb_advance_itstate (itstate); - } - - VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc)); - return next_pcs; - } - else if (itstate != 0) - { - /* We are in a conditional block. Check the condition. */ - if (! condition_true (itstate >> 4, status)) - { - /* Advance to the next executed instruction. */ - pc += thumb_insn_size (inst1); - itstate = thumb_advance_itstate (itstate); + std::vector <std::pair <CORE_ADDR, arm_breakpoint_kinds>> itblock_next_pcs + = thumb_get_next_pcs_raw_itblock + (pc, inst1, status, itstate, self->ops->read_mem_uint, + byte_order_for_code); - while (itstate != 0 && ! condition_true (itstate >> 4, status)) - { - inst1 = self->ops->read_mem_uint (pc, 2, byte_order_for_code); - - pc += thumb_insn_size (inst1); - itstate = thumb_advance_itstate (itstate); - } - - VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc)); - return next_pcs; - } - else if ((itstate & 0x0f) == 0x08) - { - /* This is the last instruction of the conditional - block, and it is executed. We can handle it normally - because the following instruction is not conditional, - and we must handle it normally because it is - permitted to branch. Fall through. */ - } - else - { - int cond_negated; - - /* There are conditional instructions after this one. - If this instruction modifies the flags, then we can - not predict what the next executed instruction will - be. Fortunately, this instruction is architecturally - forbidden to branch; we know it will fall through. - Start by skipping past it. */ - pc += thumb_insn_size (inst1); - itstate = thumb_advance_itstate (itstate); - - /* Set a breakpoint on the following instruction. */ - gdb_assert ((itstate & 0x0f) != 0); - VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc)); - - cond_negated = (itstate >> 4) & 1; - - /* Skip all following instructions with the same - condition. If there is a later instruction in the IT - block with the opposite condition, set the other - breakpoint there. If not, then set a breakpoint on - the instruction after the IT block. */ - do - { - inst1 = self->ops->read_mem_uint (pc, 2, byte_order_for_code); - pc += thumb_insn_size (inst1); - itstate = thumb_advance_itstate (itstate); - } - while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated); - - VEC_safe_push (CORE_ADDR, next_pcs, MAKE_THUMB_ADDR (pc)); - - return next_pcs; - } - } + for(auto const &nextpc : itblock_next_pcs) { + VEC_safe_push (CORE_ADDR, next_pcs, nextpc.first); + } } else if (itstate & 0x0f) { diff --git a/gdb/arch/arm-get-next-pcs.h b/gdb/arch/arm-get-next-pcs.h index 2300ac1..830844b 100644 --- a/gdb/arch/arm-get-next-pcs.h +++ b/gdb/arch/arm-get-next-pcs.h @@ -20,6 +20,7 @@ #ifndef ARM_GET_NEXT_PCS_H #define ARM_GET_NEXT_PCS_H 1 #include "gdb_vecs.h" +#include <vector> /* Forward declaration. */ struct arm_get_next_pcs; @@ -63,4 +64,12 @@ void arm_get_next_pcs_ctor (struct arm_get_next_pcs *self, /* Find the next possible PCs after the current instruction executes. */ VEC (CORE_ADDR) *arm_get_next_pcs (struct arm_get_next_pcs *self); +/* Return the next possible PCs after PC if those are in a thumb2 it block. */ +std::vector <std::pair <CORE_ADDR, arm_breakpoint_kinds>> +thumb_get_next_pcs_raw_itblock +(CORE_ADDR pc, unsigned short inst1, + ULONGEST status, ULONGEST itstate, + ULONGEST (*read_mem_uint) (CORE_ADDR memaddr, int len, int byte_order), + int byte_order_for_code); + #endif /* ARM_GET_NEXT_PCS_H */ diff --git a/gdb/gdbserver/linux-aarch32-low.c b/gdb/gdbserver/linux-aarch32-low.c index 2b710ba..ff4869f 100644 --- a/gdb/gdbserver/linux-aarch32-low.c +++ b/gdb/gdbserver/linux-aarch32-low.c @@ -18,6 +18,7 @@ #include "server.h" #include "arch/arm.h" #include "arch/arm-linux.h" +#include "arch/arm-get-next-pcs.h" #include "linux-low.h" #include "linux-aarch32-low.h" @@ -28,6 +29,8 @@ #include <elf.h> #endif +#include <vector> + /* Correct in either endianness. */ #define arm_abi_breakpoint 0xef9f0001UL @@ -287,8 +290,31 @@ arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr) { if (arm_is_thumb_mode ()) { - *pcptr = MAKE_THUMB_ADDR (*pcptr); - return arm_breakpoint_kind_from_pc (pcptr); + struct regcache *regcache = get_thread_regcache (current_thread, 1); + CORE_ADDR pc = regcache_read_pc (regcache); + ULONGEST status, itstate; + unsigned short inst1; + + *pcptr = UNMAKE_THUMB_ADDR (*pcptr); + + inst1 = get_next_pcs_read_memory_unsigned_integer + (pc, 2, 0); + status = regcache_raw_get_unsigned (regcache, ARM_PS_REGNUM); + itstate = ((status >> 8) & 0xfc) | ((status >> 25) & 0x3); + + std::vector <std::pair <CORE_ADDR, arm_breakpoint_kinds>> itblock_next_pcs + = thumb_get_next_pcs_raw_itblock + (pc, inst1, status, itstate, + get_next_pcs_read_memory_unsigned_integer, + 0); + + /* If this PC is in an itblock let thumb_get_next_pcs_raw_itblock + decide the kind. */ + for(auto const &nextpc : itblock_next_pcs) { + if (MAKE_THUMB_ADDR (*pcptr) == nextpc.first) + return nextpc.second; + } + return ARM_BP_KIND_THUMB; } else { @@ -296,6 +322,22 @@ arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr) } } +/* Read memory from the inferiror. + BYTE_ORDER is ignored and there to keep compatiblity with GDB's + read_memory_unsigned_integer. */ +ULONGEST +get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr, + int len, + int byte_order) +{ + ULONGEST res; + + res = 0; + target_read_memory (memaddr, (unsigned char *) &res, len); + + return res; +} + void initialize_low_arch_aarch32 (void) { diff --git a/gdb/gdbserver/linux-aarch32-low.h b/gdb/gdbserver/linux-aarch32-low.h index fecfcbe..77fca32 100644 --- a/gdb/gdbserver/linux-aarch32-low.h +++ b/gdb/gdbserver/linux-aarch32-low.h @@ -27,6 +27,9 @@ int arm_breakpoint_kind_from_pc (CORE_ADDR *pcptr); const gdb_byte *arm_sw_breakpoint_from_kind (int kind , int *size); int arm_breakpoint_kind_from_current_state (CORE_ADDR *pcptr); int arm_breakpoint_at (CORE_ADDR where); +ULONGEST get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr, + int len, + int byte_order); void initialize_low_arch_aarch32 (void); diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c index fc2b348..fc6b5cc 100644 --- a/gdb/gdbserver/linux-arm-low.c +++ b/gdb/gdbserver/linux-arm-low.c @@ -139,11 +139,6 @@ static int arm_regmap[] = { 64 }; -/* Forward declarations needed for get_next_pcs ops. */ -static ULONGEST get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr, - int len, - int byte_order); - static CORE_ADDR get_next_pcs_addr_bits_remove (struct arm_get_next_pcs *self, CORE_ADDR val); @@ -252,22 +247,6 @@ get_next_pcs_is_thumb (struct arm_get_next_pcs *self) return arm_is_thumb_mode (); } -/* Read memory from the inferiror. - BYTE_ORDER is ignored and there to keep compatiblity with GDB's - read_memory_unsigned_integer. */ -static ULONGEST -get_next_pcs_read_memory_unsigned_integer (CORE_ADDR memaddr, - int len, - int byte_order) -{ - ULONGEST res; - - res = 0; - target_read_memory (memaddr, (unsigned char *) &res, len); - - return res; -} - /* Fetch the thread-local storage pointer for libthread_db. */ ps_err_e diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c index 6e6926a..f3845cf 100644 --- a/gdb/gdbserver/mem-break.c +++ b/gdb/gdbserver/mem-break.c @@ -855,7 +855,21 @@ set_breakpoint_type_at (enum bkpt_type type, CORE_ADDR where, { int err_ignored; CORE_ADDR placed_address = where; - int breakpoint_kind = target_breakpoint_kind_from_pc (&placed_address); + int breakpoint_kind; + + /* Get the kind of breakpoint to PLACED_ADDRESS except single-step + breakpoint. Get the kind of single-step breakpoint according to + the current register state. */ + if (type == single_step_breakpoint) + { + breakpoint_kind + = target_breakpoint_kind_from_current_state (&placed_address); + } + else + { + breakpoint_kind + = target_breakpoint_kind_from_pc (&placed_address); + } return set_breakpoint (type, raw_bkpt_type_sw, placed_address, breakpoint_kind, handler, ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-04-03 13:18 ` Antoine Tremblay @ 2017-04-03 15:18 ` Yao Qi 2017-04-03 16:57 ` Antoine Tremblay 0 siblings, 1 reply; 38+ messages in thread From: Yao Qi @ 2017-04-03 15:18 UTC (permalink / raw) To: Antoine Tremblay; +Cc: Pedro Alves, gdb-patches Antoine Tremblay <antoine.tremblay@ericsson.com> writes: > + if ((inst1 & 0xff00) == 0xbf00 && (inst1 & 0x000f) != 0) > + { > + /* An IT instruction. Because this instruction does not > + modify the flags, we can accurately predict the next > + executed instruction. */ > + itstate = inst1 & 0x00ff; > + pc += thumb_insn_size (inst1); > + > + while (itstate != 0 && ! condition_true (itstate >> 4, status)) > + { > + inst1 = read_mem_uint (pc, 2,byte_order_for_code); > + pc += thumb_insn_size (inst1); > + itstate = thumb_advance_itstate (itstate); > + } > + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> > + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); It is incorrect to choose ARM_BP_KIND_THUMB2 if the instruction is 16-bit. IMO, this function should only tell whether PC is in IT block nor not. It shouldn't involve any breakpoint kinds selection. > + return next_pcs; > + } > + else if (itstate != 0) > + { > + /* We are in a conditional block. Check the condition. */ > + if (! condition_true (itstate >> 4, status)) > + { > + /* Advance to the next executed instruction. */ > + pc += thumb_insn_size (inst1); > + itstate = thumb_advance_itstate (itstate); > + > + while (itstate != 0 && ! condition_true (itstate >> 4, status)) > + { > + inst1 = read_mem_uint (pc, 2, byte_order_for_code); > + > + pc += thumb_insn_size (inst1); > + itstate = thumb_advance_itstate (itstate); > + } > + If all the following instructions' condition is false, breakpoint should be set on the first instruction out side of IT block. We can still use 16-bit thumb breakpoint. > + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> > + (MAKE_THUMB_ADDR (pc), > ARM_BP_KIND_THUMB2)); The same issue. > + return next_pcs; > + } > + else if ((itstate & 0x0f) == 0x08) > + { > + /* This is the last instruction of the conditional > + block, and it is executed. We can handle it normally > + because the following instruction is not conditional, > + and we must handle it normally because it is > + permitted to branch. Fall through. */ How do we fall through now? > + } > + else > + { > + int cond_negated; > + > + /* There are conditional instructions after this one. > + If this instruction modifies the flags, then we can > + not predict what the next executed instruction will > + be. Fortunately, this instruction is archi2tecturally > + forbidden to branch; we know it will fall through. > + Start by skipping past it. */ > + pc += thumb_insn_size (inst1); > + itstate = thumb_advance_itstate (itstate); > + > + /* Set a breakpoint on the following instruction. */ > + gdb_assert ((itstate & 0x0f) != 0); > + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> > + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); > + > + cond_negated = (itstate >> 4) & 1; > + > + /* Skip all following instructions with the same > + condition. If there is a later instruction in the IT > + block with the opposite condition, set the other > + breakpoint there. If not, then set a breakpoint on > + the instruction after the IT block. */ > + do > + { > + inst1 = read_mem_uint (pc, 2, byte_order_for_code); > + pc += thumb_insn_size (inst1); > + itstate = thumb_advance_itstate (itstate); > + } > + while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated); > + > + if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated) > + { > + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> > + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); > + } > + else > + { > + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> > + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB)); > + } Why do you choose breakpoint in this way? > diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c > index 6e6926a..f3845cf 100644 > --- a/gdb/gdbserver/mem-break.c > +++ b/gdb/gdbserver/mem-break.c > @@ -855,7 +855,21 @@ set_breakpoint_type_at (enum bkpt_type type, CORE_ADDR where, > { > int err_ignored; > CORE_ADDR placed_address = where; > - int breakpoint_kind = target_breakpoint_kind_from_pc (&placed_address); > + int breakpoint_kind; > + > + /* Get the kind of breakpoint to PLACED_ADDRESS except single-step > + breakpoint. Get the kind of single-step breakpoint according to > + the current register state. */ > + if (type == single_step_breakpoint) > + { > + breakpoint_kind > + = target_breakpoint_kind_from_current_state (&placed_address); I read my patch again, but it looks wrong. If we single-step an instruction with a state change, like bx or blx, current get_next_pcs correctly marked the address bit. However, with the change like this, we'll get the wrong breakpoint kind. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-04-03 15:18 ` Yao Qi @ 2017-04-03 16:57 ` Antoine Tremblay 0 siblings, 0 replies; 38+ messages in thread From: Antoine Tremblay @ 2017-04-03 16:57 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, Pedro Alves, gdb-patches Yao Qi writes: > Antoine Tremblay <antoine.tremblay@ericsson.com> writes: > >> + if ((inst1 & 0xff00) == 0xbf00 && (inst1 & 0x000f) != 0) >> + { >> + /* An IT instruction. Because this instruction does not >> + modify the flags, we can accurately predict the next >> + executed instruction. */ >> + itstate = inst1 & 0x00ff; >> + pc += thumb_insn_size (inst1); >> + >> + while (itstate != 0 && ! condition_true (itstate >> 4, status)) >> + { >> + inst1 = read_mem_uint (pc, 2,byte_order_for_code); >> + pc += thumb_insn_size (inst1); >> + itstate = thumb_advance_itstate (itstate); >> + } >> + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> >> + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); > > It is incorrect to choose ARM_BP_KIND_THUMB2 if the instruction is > 16-bit. Good point this does not look correct indeed, a call to breakpoint_from_pc would be better at this point. > IMO, this function should only tell whether PC is in IT block > nor not. It shouldn't involve any breakpoint kinds selection. > Yes I think you're right, I could make it return std::pair<CORE_ADDR, bool (is_it_block ())> And use breakpoint_from_pc if true , and BP_KIND_THUMB if false. >> + return next_pcs; >> + } >> + else if (itstate != 0) >> + { >> + /* We are in a conditional block. Check the condition. */ >> + if (! condition_true (itstate >> 4, status)) >> + { >> + /* Advance to the next executed instruction. */ >> + pc += thumb_insn_size (inst1); >> + itstate = thumb_advance_itstate (itstate); >> + >> + while (itstate != 0 && ! condition_true (itstate >> 4, status)) >> + { >> + inst1 = read_mem_uint (pc, 2, byte_order_for_code); >> + >> + pc += thumb_insn_size (inst1); >> + itstate = thumb_advance_itstate (itstate); >> + } >> + > > If all the following instructions' condition is false, breakpoint should > be set on the first instruction out side of IT block. We can still use > 16-bit thumb breakpoint. > >> + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> >> + (MAKE_THUMB_ADDR (pc), >> ARM_BP_KIND_THUMB2)); > > The same issue. > >> + return next_pcs; >> + } >> + else if ((itstate & 0x0f) == 0x08) >> + { >> + /* This is the last instruction of the conditional >> + block, and it is executed. We can handle it normally >> + because the following instruction is not conditional, >> + and we must handle it normally because it is >> + permitted to branch. Fall through. */ > > How do we fall through now? No breakpoints are added to the vector, so it falls through. > >> + } >> + else >> + { >> + int cond_negated; >> + >> + /* There are conditional instructions after this one. >> + If this instruction modifies the flags, then we can >> + not predict what the next executed instruction will >> + be. Fortunately, this instruction is archi2tecturally >> + forbidden to branch; we know it will fall through. >> + Start by skipping past it. */ >> + pc += thumb_insn_size (inst1); >> + itstate = thumb_advance_itstate (itstate); >> + >> + /* Set a breakpoint on the following instruction. */ >> + gdb_assert ((itstate & 0x0f) != 0); >> + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> >> + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); >> + >> + cond_negated = (itstate >> 4) & 1; >> + >> + /* Skip all following instructions with the same >> + condition. If there is a later instruction in the IT >> + block with the opposite condition, set the other >> + breakpoint there. If not, then set a breakpoint on >> + the instruction after the IT block. */ >> + do >> + { >> + inst1 = read_mem_uint (pc, 2, byte_order_for_code); >> + pc += thumb_insn_size (inst1); >> + itstate = thumb_advance_itstate (itstate); >> + } >> + while (itstate != 0 && ((itstate >> 4) & 1) == cond_negated); >> + >> + if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated) >> + { >> + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> >> + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB2)); >> + } >> + else >> + { >> + next_pcs.push_back (std::pair<CORE_ADDR, arm_breakpoint_kinds> >> + (MAKE_THUMB_ADDR (pc), ARM_BP_KIND_THUMB)); >> + } > > Why do you choose breakpoint in this way? > In the case of if (itstate != 0 && ((itstate >> 4) & 1) == cond_negated) we are in the IT block But if that is false we're after the IT block so it doesn't need a thumb2 breakpoint. (See the comment above in the code) >> diff --git a/gdb/gdbserver/mem-break.c b/gdb/gdbserver/mem-break.c >> index 6e6926a..f3845cf 100644 >> --- a/gdb/gdbserver/mem-break.c >> +++ b/gdb/gdbserver/mem-break.c >> @@ -855,7 +855,21 @@ set_breakpoint_type_at (enum bkpt_type type, CORE_ADDR where, >> { >> int err_ignored; >> CORE_ADDR placed_address = where; >> - int breakpoint_kind = target_breakpoint_kind_from_pc (&placed_address); >> + int breakpoint_kind; >> + >> + /* Get the kind of breakpoint to PLACED_ADDRESS except single-step >> + breakpoint. Get the kind of single-step breakpoint according to >> + the current register state. */ >> + if (type == single_step_breakpoint) >> + { >> + breakpoint_kind >> + = target_breakpoint_kind_from_current_state (&placed_address); > > I read my patch again, but it looks wrong. If we single-step an > instruction with a state change, like bx or blx, current get_next_pcs > correctly marked the address bit. However, with the change like this, > we'll get the wrong breakpoint kind. You're right, that's a problem however I think it could be fixed in arm_breakpoint_kind_from_current_state by adding a case where placed_address != current_pc in which case the thumb bit would be the deciding factor rather then arm_is_thumb_mode ()... I'll resubmit a proper patch with those fixes. ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-01-30 13:29 ` Antoine Tremblay 2017-02-03 16:13 ` Pedro Alves @ 2017-02-16 22:32 ` Yao Qi 2017-02-17 2:17 ` Antoine Tremblay 1 sibling, 1 reply; 38+ messages in thread From: Yao Qi @ 2017-02-16 22:32 UTC (permalink / raw) To: Antoine Tremblay; +Cc: gdb-patches On 17-01-30 08:28:29, Antoine Tremblay wrote: > > Yao Qi writes: > > > On Fri, Jan 27, 2017 at 6:24 PM, Antoine Tremblay > > <antoine.tremblay@ericsson.com> wrote: > >> > >> Yao Qi writes: > >> > >>> On Fri, Jan 27, 2017 at 4:06 PM, Antoine Tremblay > >>> <antoine.tremblay@ericsson.com> wrote: > > > >>> If emulation is complex, probably > >>> we can partially fix this problem by "always using 16-bit thumb instruction > >>> if program is out of IT block". > >>> > >> > >> I would be against that since it would leave the feature partially > >> working and crashing the program when it fails... > >> > >> It would also be a regression compared to previous GDBServer. > > > > There won't be any regression. 16-bit thumb breakpoint works pretty well > > for any thumb instructions (16-bit and 32-bit) if program is out of IT block. > > The 32-bit thumb-2 breakpoint was added for single step IT block. > > Yes so there will be a regression for single step inside an IT block if > we keep using the 32 bit breakpoint since this one can be non atomic if > it's not aligned properly. > It does fail, but not a regression, because current GDBserver fails to write 32-bit thumb breakpoint to 2-byte aligned 32-bit instruction atomically, regardless the program is within IT block or not. My suggestion is that "let us fix this problem when program is out of IT block by using 16-bit thumb breakpoint". That will fix the issue of atomic write when program is out of IT block, and leave the problem there when program is within IT block. Why is it a regression? > We could write a test case for it and it would fail like schedlock > did. But it would not with the single stepping controlled by GDB like it > was before. > > > > >> Also, IT blocks are a common place to have a breakpoint/tracepoint. > >> > > > > We don't change anything when setting breakpoint inside IT block. > > Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on > 2 bytes like discussed before. > That is just the sub-set of the original problem. > > > >>>> I think it would be better to get the current single stepping working > >>>> with the stop all threads logic since GDBServer was working like that > >>>> when GDB was doing the single stepping anyway. This would fix the current > >>>> regression. > >>>> > >>>> Then work could be done to improve GDBServer to be better at > >>>> non-stopping. > >>>> > >>>> WDYT ? > >>>> > >>> > >>> Sounds like we are applying the ARM linux limitation to a general > >>> place. > >>> Other software single step targets may write breakpoint in atomic way, > >>> so we don't need to stop all threads. Even in -marm mode, we don't > >>> have to stop all threads on inserting breakpoints. > >> > >> Well ARM is the only software single stepping target, while I agreee > >> that we would be affecting general code, I think that since there is > >> no software single step breakpoint target that supports atomic > >> breakpoint writes at the moment it's normal that the run control > >> doesn't support that. > > > > No, ARM Linux ptrace POKETEXT is _atomic_ if the address is 4-byte > > aligned, so 32-bit arm breakpoint and 16-bit thumb breakpoint can be > > written atomically. 32-bit thumb-2 breakpoint can be written atomically > > too if the address is 4-byte aligned. > > > > The only problem we have is inserting a breakpoint on a 2-byte aligned > > 32-bit thumb-2 instruction inside IT block, we can not use neither 16-bit thumb > > breakpoint nor 32-bit thumb breakpoint. Everything works fine in other > > cases. > > > > I mean software single stepping as a whole, which means considering all > cases, is not atomic. I don't see how we could leave that case > unaddressed ? > > Especially since it will crash the inferior ? > I am thinking how do we make progress here. Nowadays, the multi-threaded program may crash almost everywhere, but we can partially fix it to an extent that the multi-threaded program may only crash in side IT block. Is it a progress? I feel it is a good example to apply "divide and conquer" to a complicated engineering problem. We can easily fix the first half of this problem, and then think about the second half. > >> > >> I don't count -marm as an arch since there no way to check that all the > >> program including shared libs etc is -marm, I don't think we could make > >> the distinction in GDBServer AFAIK. > > > > We can check with -mthumb. My hack in last email fixes fails in > > schedlock.exp in thumb mode. > > Yes but schedlock.exp is not made to test the 2 byte aligned thumb2 > 32-bit breakpoint IIRC. > We can write one test for single step 2-byte aligned thumb-2 32-bit instruction. No problem. -- Yao (齐尧) ^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 2017-02-16 22:32 ` Yao Qi @ 2017-02-17 2:17 ` Antoine Tremblay 0 siblings, 0 replies; 38+ messages in thread From: Antoine Tremblay @ 2017-02-17 2:17 UTC (permalink / raw) To: Yao Qi; +Cc: Antoine Tremblay, gdb-patches Yao Qi writes: > On 17-01-30 08:28:29, Antoine Tremblay wrote: >> >> Yao Qi writes: >> >> > On Fri, Jan 27, 2017 at 6:24 PM, Antoine Tremblay >> > <antoine.tremblay@ericsson.com> wrote: >> >> >> >> Yao Qi writes: >> >> >> >>> On Fri, Jan 27, 2017 at 4:06 PM, Antoine Tremblay >> >>> <antoine.tremblay@ericsson.com> wrote: >> > >> >>> If emulation is complex, probably >> >>> we can partially fix this problem by "always using 16-bit thumb instruction >> >>> if program is out of IT block". >> >>> >> >> >> >> I would be against that since it would leave the feature partially >> >> working and crashing the program when it fails... >> >> >> >> It would also be a regression compared to previous GDBServer. >> > >> > There won't be any regression. 16-bit thumb breakpoint works pretty well >> > for any thumb instructions (16-bit and 32-bit) if program is out of IT block. >> > The 32-bit thumb-2 breakpoint was added for single step IT block. >> >> Yes so there will be a regression for single step inside an IT block if >> we keep using the 32 bit breakpoint since this one can be non atomic if >> it's not aligned properly. >> > > It does fail, but not a regression, because current GDBserver fails to > write 32-bit thumb breakpoint to 2-byte aligned 32-bit instruction > atomically, regardless the program is within IT block or not. I mean it's a regression compared to GDB 7.11. In 7.11 GDBServer will stop all threads write the 4 bytes and then restart all threads so the issue is not present. Yes it wasn't atomic before either but it did not create an issue due to the stopping of the threads. Or maybe I misunderstand what you mean... ? > My > suggestion is that "let us fix this problem when program is out of IT > block by using 16-bit thumb breakpoint". That will fix the issue > of atomic write when program is out of IT block, and leave the problem > there when program is within IT block. Why is it a regression? > Well like I said before in 7.11 because GDBServer stopped the threads to write the 4 bytes, it did not have to be atomic and schedlock.exp etc passed, single stepping a program even with a IT block did not fail. I would like to re-validate this since you're introducing doubt into my mind but I can't at the moment, I hope my memory serves me right but I have not retested this now. >> We could write a test case for it and it would fail like schedlock >> did. But it would not with the single stepping controlled by GDB like it >> was before. >> >> > >> >> Also, IT blocks are a common place to have a breakpoint/tracepoint. >> >> >> > >> > We don't change anything when setting breakpoint inside IT block. >> >> Well that's a problem if we write a 32 bit thumb2 breakpoint aligned on >> 2 bytes like discussed before. >> > > That is just the sub-set of the original problem. > >> > >> >>>> I think it would be better to get the current single stepping working >> >>>> with the stop all threads logic since GDBServer was working like that >> >>>> when GDB was doing the single stepping anyway. This would fix the current >> >>>> regression. >> >>>> >> >>>> Then work could be done to improve GDBServer to be better at >> >>>> non-stopping. >> >>>> >> >>>> WDYT ? >> >>>> >> >>> >> >>> Sounds like we are applying the ARM linux limitation to a general >> >>> place. >> >>> Other software single step targets may write breakpoint in atomic way, >> >>> so we don't need to stop all threads. Even in -marm mode, we don't >> >>> have to stop all threads on inserting breakpoints. >> >> >> >> Well ARM is the only software single stepping target, while I agreee >> >> that we would be affecting general code, I think that since there is >> >> no software single step breakpoint target that supports atomic >> >> breakpoint writes at the moment it's normal that the run control >> >> doesn't support that. >> > >> > No, ARM Linux ptrace POKETEXT is _atomic_ if the address is 4-byte >> > aligned, so 32-bit arm breakpoint and 16-bit thumb breakpoint can be >> > written atomically. 32-bit thumb-2 breakpoint can be written atomically >> > too if the address is 4-byte aligned. >> > >> > The only problem we have is inserting a breakpoint on a 2-byte aligned >> > 32-bit thumb-2 instruction inside IT block, we can not use neither 16-bit thumb >> > breakpoint nor 32-bit thumb breakpoint. Everything works fine in other >> > cases. >> > >> >> I mean software single stepping as a whole, which means considering all >> cases, is not atomic. I don't see how we could leave that case >> unaddressed ? >> >> Especially since it will crash the inferior ? >> > > I am thinking how do we make progress here. Nowadays, the multi-threaded > program may crash almost everywhere, but we can partially fix it to an > extent that the multi-threaded program may only crash in side IT block. > Is it a progress? I feel it is a good example to apply "divide and > conquer" to a complicated engineering problem. We can easily fix the > first half of this problem, and then think about the second half. > Well I agree with the divide an conquer approach, I would just have divided it another way by stopping all threads so that we fix all the problem right now, and then think about the second half which would be to allow non-stop operations. The solution to the program crashing in the IT block seems non-trivial and I don't know how much time will pass before a fix is done. I'm afraid this would linger for a long time but maybe I'm wrong, do you plan to address the second part for 8.0 too ? I would feel better if GDB worked for all cases meanwhile. This is more important to me than not stopping the threads, especially since in 7.11 the threads were stopped. My point is that if we can fix the problem completely now while we think about a better solution isn't that preferable to leaving GDB partially fixed ? >> >> >> >> I don't count -marm as an arch since there no way to check that all the >> >> program including shared libs etc is -marm, I don't think we could make >> >> the distinction in GDBServer AFAIK. >> > >> > We can check with -mthumb. My hack in last email fixes fails in >> > schedlock.exp in thumb mode. >> >> Yes but schedlock.exp is not made to test the 2 byte aligned thumb2 >> 32-bit breakpoint IIRC. >> > > We can write one test for single step 2-byte aligned thumb-2 32-bit > instruction. No problem. ^ permalink raw reply [flat|nested] 38+ messages in thread
end of thread, other threads:[~2017-04-03 16:57 UTC | newest] Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-29 12:07 [PATCH 1/2] This patch fixes GDBServer's run control for single stepping 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 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
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).