diff mbox

[1/2] This patch fixes GDBServer's run control for single stepping

Message ID wwoktw69l31p.fsf@ericsson.com
State New
Headers show

Commit Message

Antoine Tremblay March 31, 2017, 6:22 p.m. UTC 
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

Comments

Yao Qi April 3, 2017, 12:41 p.m. UTC | #1 
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.
diff mbox

Patch

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
+}