This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping
- From: Antoine Tremblay <antoine dot tremblay at ericsson dot com>
- To: Yao Qi <qiyaoltc at gmail dot com>
- Cc: Antoine Tremblay <antoine dot tremblay at ericsson dot com>, Pedro Alves <palves at redhat dot com>, "gdb-patches at sourceware dot org" <gdb-patches at sourceware dot org>
- Date: Fri, 31 Mar 2017 14:22:10 -0400
- Subject: Re: [PATCH 1/2] This patch fixes GDBServer's run control for single stepping
- Authentication-results: sourceware.org; auth=none
- Authentication-results: sourceware.org; dkim=none (message not signed) header.d=none;sourceware.org; dmarc=none action=none header.from=ericsson.com;
- References: <20161129120702.9490-1-antoine.tremblay@ericsson.com> <20170127150139.GB24676@E107787-LIN> <wwokwpdg5vxa.fsf@ericsson.com> <CAH=s-PP-i3v_Fr=QeWt9BQeJzjCHtW79nGYpJ9hF-Bb=OBo89Q@mail.gmail.com> <wwokr33o5pkb.fsf@ericsson.com> <CAH=s-PO98nCE4UB9ag+V=M2mBnZT0FOeHV3d7mFMLYe1+v=mFg@mail.gmail.com> <wwok8tps8yo2.fsf@ericsson.com> <2255ed6f-a146-026c-f871-00e9a33dfcf0@redhat.com> <wwokwpcp7fvn.fsf@ericsson.com> <b5fb81d1-66fc-68c2-9785-ffa487de59e0@redhat.com> <wwoktw7t7bzy.fsf@ericsson.com> <CAH=s-PPx+SjoE0DkTKKNqg4Dr4zHFNt6QeC-XXT_LoXVh004iw@mail.gmail.com> <wwokh93s1he3.fsf@ericsson.com> <CAH=s-PPrB=s6d9Q07W=-b8Sz9umh6_Lj24PyO4x99Z3QrtfmzQ@mail.gmail.com> <wwokzig4l0i1.fsf@ericsson.com> <86d1cy4umo.fsf@gmail.com> <wwokvaqqlipt.fsf@ericsson.com> <86d1cxwgpk.fsf@gmail.com>
- Spamdiagnosticmetadata: NSPM
- Spamdiagnosticoutput: 1:99
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
+}