[PATCH 1/4] Fix ppc64 single step over atomic sequence testcase

[PATCH 1/4] Fix ppc64 single step over atomic sequence testcase

[Anton Blanchard]

The current ppc64 single step over atomic sequence testcase is written
in C and breaks with some versions of gcc. Convert the test to
assembly and use stepi to step through it.

gdb/
2014-03-28  Anton Blanchard  <anton@samba.org>

	* gdb.arch/ppc64-atomic-inst.c: Remove.
	* gdb.arch/ppc64-atomic-inst.s: New file.
	* gdb.arch/ppc64-atomic-inst.exp: Adapt for asm based testcase.
---
 gdb/testsuite/gdb.arch/ppc64-atomic-inst.c   | 44 --------------------
 gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp | 15 +++++--
 gdb/testsuite/gdb.arch/ppc64-atomic-inst.s   | 61 ++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 48 deletions(-)
 delete mode 100644 gdb/testsuite/gdb.arch/ppc64-atomic-inst.c
 create mode 100644 gdb/testsuite/gdb.arch/ppc64-atomic-inst.s

diff --git a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.c b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.c
deleted file mode 100644
index 303e383..0000000
--- a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.c
+++ /dev/null
@@ -1,44 +0,0 @@
-/* This file is part of GDB, the GNU debugger.
-
-   Copyright 2008-2014 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 <stdio.h>
-
-int main()
-{
-  unsigned int word = 0;
-  unsigned int *word_addr = &word;
-  unsigned long dword = 0;
-  unsigned long *dword_addr = &dword;
-
-  __asm __volatile ("1:     lwarx   %0,0,%2\n"              \
-                    "       addi    %0,%0,1\n"              \
-                    "       stwcx.  %0,0,%2\n"              \
-                    "       bne-    1b"			    \
-                    : "=&b" (word), "=m" (*word_addr)       \
-                    : "b" (word_addr), "m" (*word_addr)     \
-                    : "cr0", "memory");			    \
-
-  __asm __volatile ("1:     ldarx   %0,0,%2\n"              \
-                    "       addi    %0,%0,1\n"              \
-                    "       stdcx.  %0,0,%2\n"              \
-                    "       bne-    1b"                     \
-                    : "=&b" (dword), "=m" (*dword_addr)     \
-                    : "b" (dword_addr), "m" (*dword_addr)   \
-                    : "cr0", "memory");                     \
-
-  return 0;
-}
diff --git a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
index f5f3b40..efcd82a 100644
--- a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
+++ b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
@@ -27,7 +27,7 @@ if {![istarget "powerpc*"] || ![is_lp64_target]} {
 }
 
 set testfile "ppc64-atomic-inst"
-set srcfile ${testfile}.c
+set srcfile ${testfile}.s
 set binfile ${objdir}/${subdir}/${testfile}
 set compile_flags {debug quiet}
 
@@ -50,11 +50,18 @@ set bp1 [gdb_get_line_number "lwarx"]
 gdb_breakpoint "$bp1" "Breakpoint $decimal at $hex" \
   "Set the breakpoint at the start of the sequence"
 
+set bp2 [gdb_get_line_number "ldarx"]
+gdb_breakpoint "$bp2" "Breakpoint $decimal at $hex" \
+  "Set the breakpoint at the start of the sequence"
+
 gdb_test continue "Continuing.*Breakpoint $decimal.*" \
   "Continue until breakpoint"
 
-gdb_test next ".*__asm __volatile.*" \
+gdb_test nexti "bne.*1b" \
   "Step through the lwarx/stwcx sequence"
 
-gdb_test next ".*return 0.*" \
-  "Step through the ldarx/stdcx sequence"
+gdb_test continue "Continuing.*Breakpoint $decimal.*" \
+  "Continue until breakpoint"
+
+gdb_test nexti "bne.*1b" \
+  "Step through the lwarx/stwcx sequence"
diff --git a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
new file mode 100644
index 0000000..15ccfd9
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
@@ -0,0 +1,61 @@
+/* This file is part of GDB, the GNU debugger.
+
+   Copyright 2008-2014 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/>.  */
+
+	.align 2
+	.globl main
+#if _CALL_ELF == 2
+	.type main,@function
+main:
+#else
+	.section ".opd","aw"
+	.align 3
+main:
+	.quad .main,.TOC.@tocbase,0
+	.size main,.-main
+	.previous
+	.globl .main
+	.type .main,@function
+.main:
+#endif
+
+	li	0,0
+	addi	4,1,-8
+
+	stw	0,0(4)
+1:	lwarx	5,0,4
+	cmpwi	5,0
+	bne	2f
+	addi	5,5,1
+	stwcx.	5,0,4
+	bne	1b
+
+	std	0,0(4)
+2:	ldarx	5,0,4
+	cmpdi	5,0
+	bne	3f
+	addi	5,5,1
+	stdcx.	5,0,4
+	bne	1b
+
+3:	li	3,0
+	blr
+
+#if _CALL_ELF == 2
+	.size main,.-main
+#else
+	.size .main,.-.main
+#endif
-- 
1.8.3.2

[PATCH 2/4] Support up to 3 conditional branches in an atomic sequence

[Anton Blanchard]

gdb currently supports 1 conditional branch inside a ppc larx/stcx
critical region. Unfortunately there is existing code that contains more
than 1, for example in the ppc64 Linux kernel:

c00000000003ac18 <.__hash_page_4K>:
...
c00000000003ac4c:       ldarx   r31,0,r6
c00000000003ac50:       andc.   r0,r4,r31
c00000000003ac54:       bne-    c00000000003aee8 <htab_wrong_access>
c00000000003ac58:       andi.   r0,r31,2048
c00000000003ac5c:       bne-    c00000000003ae0c <htab_bail_ok>
c00000000003ac60:       rlwinm  r30,r4,30,24,24
c00000000003ac64:       or      r30,r30,r31
c00000000003ac68:       ori     r30,r30,2304
c00000000003ac6c:       oris    r30,r30,4096
c00000000003ac70:       stdcx.  r30,0,r6

If we try to single step through this we get stuck forever because
the reservation is never set when we step over the stdcx.

The following patch bumps the number to 3 conditional branches + 1
terminating branch. With this patch applied I can single step through
the offending function in the ppc64 Linux kernel.

gdb/
2014-03-28  Anton Blanchard  <anton@samba.org>

	* breakpoint.h (MAX_SINGLE_STEP_BREAKPOINTS): New define.
	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
	than two breakpoints.
	* breakpoint.c (insert_single_step_breakpoint): Likewise.
	(insert_single_step_breakpoint): Likewise.
	(single_step_breakpoints_inserted): Likewise.
	(cancel_single_step_breakpoints): Likewise.
	(detach_single_step_breakpoints): Likewise.
	(single_step_breakpoint_inserted_here_p): Likewise.
---
 gdb/breakpoint.c  | 63 ++++++++++++++++++++++++++++---------------------------
 gdb/breakpoint.h  |  6 ++++--
 gdb/rs6000-tdep.c | 46 ++++++++++++++++++++++------------------
 3 files changed, 62 insertions(+), 53 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 19af9df..b12ea94 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -15036,11 +15036,10 @@ deprecated_remove_raw_breakpoint (struct gdbarch *gdbarch, void *bp)
   return ret;
 }
 
-/* One (or perhaps two) breakpoints used for software single
-   stepping.  */
+/* Breakpoints used for software single stepping.  */
 
-static void *single_step_breakpoints[2];
-static struct gdbarch *single_step_gdbarch[2];
+static void *single_step_breakpoints[MAX_SINGLE_STEP_BREAKPOINTS];
+static struct gdbarch *single_step_gdbarch[MAX_SINGLE_STEP_BREAKPOINTS];
 
 /* Create and insert a breakpoint for software single step.  */
 
@@ -15049,19 +15048,17 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
 			       struct address_space *aspace, 
 			       CORE_ADDR next_pc)
 {
+  int i;
   void **bpt_p;
 
-  if (single_step_breakpoints[0] == NULL)
-    {
-      bpt_p = &single_step_breakpoints[0];
-      single_step_gdbarch[0] = gdbarch;
-    }
-  else
-    {
-      gdb_assert (single_step_breakpoints[1] == NULL);
-      bpt_p = &single_step_breakpoints[1];
-      single_step_gdbarch[1] = gdbarch;
-    }
+  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
+    if (single_step_breakpoints[i] == NULL)
+        break;
+
+  gdb_assert (i < MAX_SINGLE_STEP_BREAKPOINTS);
+
+  bpt_p = &single_step_breakpoints[i];
+  single_step_gdbarch[i] = gdbarch;
 
   /* NOTE drow/2006-04-11: A future improvement to this function would
      be to only create the breakpoints once, and actually put them on
@@ -15082,8 +15079,13 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
 int
 single_step_breakpoints_inserted (void)
 {
-  return (single_step_breakpoints[0] != NULL
-          || single_step_breakpoints[1] != NULL);
+  int i;
+
+  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
+    if (single_step_breakpoints[i] != NULL)
+      return 1;
+
+  return 0;
 }
 
 /* Remove and delete any breakpoints used for software single step.  */
@@ -15091,22 +15093,21 @@ single_step_breakpoints_inserted (void)
 void
 remove_single_step_breakpoints (void)
 {
+  int i;
+
   gdb_assert (single_step_breakpoints[0] != NULL);
 
   /* See insert_single_step_breakpoint for more about this deprecated
      call.  */
-  deprecated_remove_raw_breakpoint (single_step_gdbarch[0],
-				    single_step_breakpoints[0]);
-  single_step_gdbarch[0] = NULL;
-  single_step_breakpoints[0] = NULL;
 
-  if (single_step_breakpoints[1] != NULL)
-    {
-      deprecated_remove_raw_breakpoint (single_step_gdbarch[1],
-					single_step_breakpoints[1]);
-      single_step_gdbarch[1] = NULL;
-      single_step_breakpoints[1] = NULL;
-    }
+  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
+    if (single_step_breakpoints[i] != NULL)
+      {
+	deprecated_remove_raw_breakpoint (single_step_gdbarch[i],
+					  single_step_breakpoints[i]);
+	single_step_gdbarch[i] = NULL;
+	single_step_breakpoints[i] = NULL;
+      }
 }
 
 /* Delete software single step breakpoints without removing them from
@@ -15119,7 +15120,7 @@ cancel_single_step_breakpoints (void)
 {
   int i;
 
-  for (i = 0; i < 2; i++)
+  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
     if (single_step_breakpoints[i])
       {
 	xfree (single_step_breakpoints[i]);
@@ -15136,7 +15137,7 @@ detach_single_step_breakpoints (void)
 {
   int i;
 
-  for (i = 0; i < 2; i++)
+  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
     if (single_step_breakpoints[i])
       target_remove_breakpoint (single_step_gdbarch[i],
 				single_step_breakpoints[i]);
@@ -15151,7 +15152,7 @@ single_step_breakpoint_inserted_here_p (struct address_space *aspace,
 {
   int i;
 
-  for (i = 0; i < 2; i++)
+  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
     {
       struct bp_target_info *bp_tgt = single_step_breakpoints[i];
       if (bp_tgt
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index d8e88fc..65d3ecb 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1443,8 +1443,10 @@ extern void add_solib_catchpoint (char *arg, int is_load, int is_temp,
    deletes all breakpoints.  */
 extern void delete_command (char *arg, int from_tty);
 
-/* Manage a software single step breakpoint (or two).  Insert may be
-   called twice before remove is called.  */
+/* Manage software single step breakpoints.  Insert may be
+   called multiple times before remove is called.  */
+#define MAX_SINGLE_STEP_BREAKPOINTS 4
+
 extern void insert_single_step_breakpoint (struct gdbarch *,
 					   struct address_space *, 
 					   CORE_ADDR);
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index dbe3aa2..be14e39 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1088,7 +1088,7 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
   struct address_space *aspace = get_frame_address_space (frame);
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   CORE_ADDR pc = get_frame_pc (frame);
-  CORE_ADDR breaks[2] = {-1, -1};
+  CORE_ADDR breaks[MAX_SINGLE_STEP_BREAKPOINTS];
   CORE_ADDR loc = pc;
   CORE_ADDR closing_insn; /* Instruction that closes the atomic sequence.  */
   int insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
@@ -1097,7 +1097,6 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
   int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */  
   const int atomic_sequence_length = 16; /* Instruction sequence length.  */
   int opcode; /* Branch instruction's OPcode.  */
-  int bc_insn_count = 0; /* Conditional branch instruction count.  */
 
   /* Assume all atomic sequences start with a lwarx/ldarx instruction.  */
   if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
@@ -1111,24 +1110,20 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
       loc += PPC_INSN_SIZE;
       insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
 
-      /* Assume that there is at most one conditional branch in the atomic
-         sequence.  If a conditional branch is found, put a breakpoint in 
-         its destination address.  */
       if ((insn & BRANCH_MASK) == BC_INSN)
         {
           int immediate = ((insn & 0xfffc) ^ 0x8000) - 0x8000;
           int absolute = insn & 2;
 
-          if (bc_insn_count >= 1)
-            return 0; /* More than one conditional branch found, fallback 
+          if (last_breakpoint >= MAX_SINGLE_STEP_BREAKPOINTS - 1)
+            return 0; /* too many conditional branches found, fallback
                          to the standard single-step code.  */
  
 	  if (absolute)
-	    breaks[1] = immediate;
+	    breaks[last_breakpoint] = immediate;
 	  else
-	    breaks[1] = loc + immediate;
+	    breaks[last_breakpoint] = loc + immediate;
 
-	  bc_insn_count++;
 	  last_breakpoint++;
         }
 
@@ -1147,18 +1142,29 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
   insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
 
   /* Insert a breakpoint right after the end of the atomic sequence.  */
-  breaks[0] = loc;
+  breaks[last_breakpoint] = loc;
 
-  /* Check for duplicated breakpoints.  Check also for a breakpoint
-     placed (branch instruction's destination) anywhere in sequence.  */
-  if (last_breakpoint
-      && (breaks[1] == breaks[0]
-	  || (breaks[1] >= pc && breaks[1] <= closing_insn)))
-    last_breakpoint = 0;
-
-  /* Effectively inserts the breakpoints.  */
   for (index = 0; index <= last_breakpoint; index++)
-    insert_single_step_breakpoint (gdbarch, aspace, breaks[index]);
+    {
+      int index2;
+      int insert_bp = 1;
+
+      /* Check for a breakpoint placed (branch instruction's destination)
+	 anywhere in sequence.  */
+      if (breaks[index] >= pc && breaks[index] <= closing_insn)
+	continue;
+
+      /* Check for duplicated breakpoints.  */
+      for (index2 = 0; index2 < index; index2++)
+        {
+	  if (breaks[index] == breaks[index2])
+	    insert_bp = 0;
+	}
+
+      /* insert the breakpoint.  */
+      if (insert_bp)
+        insert_single_step_breakpoint (gdbarch, aspace, breaks[index]);
+    }
 
   return 1;
 }
-- 
1.8.3.2

[PATCH 3/4] Add multiple branches to ppc64 single step through atomic sequence testcase

[Anton Blanchard]

Test 3 conditional branches in an atomic sequence, 2 to the same
destination.

gdb/testsuite/
2014-03-28  Anton Blanchard  <anton@samba.org>

	* gdb.arch/ppc64-atomic-inst.s: Add second and third branch
	inside atomic sequence.
---
 gdb/testsuite/gdb.arch/ppc64-atomic-inst.s | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
index 15ccfd9..0521170 100644
--- a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
+++ b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
@@ -39,18 +39,30 @@ main:
 1:	lwarx	5,0,4
 	cmpwi	5,0
 	bne	2f
+	cmpwi	5,1
+	beq	3f
+	cmpwi	5,2
+	beq	3f	/* branch to same destination */
 	addi	5,5,1
 	stwcx.	5,0,4
 	bne	1b
 
-	std	0,0(4)
-2:	ldarx	5,0,4
+2:	nop
+
+3:	std	0,0(4)
+1:	ldarx	5,0,4
 	cmpdi	5,0
-	bne	3f
+	bne	2f
+	cmpdi	5,1
+	beq	3f
+	cmpwi	5,2
+	beq	3f	/* branch to same destination */
 	addi	5,5,1
 	stdcx.	5,0,4
 	bne	1b
 
+2:	nop
+
 3:	li	3,0
 	blr
 
-- 
1.8.3.2

[PATCH 4/4] Add lbarx/stbcx., lharx/sthcx. and lqarx/stqcx. single stepping

[Anton Blanchard]

Newer CPUs support byte, half word and quad word atomic update
sequences.

gdb/testsuite/
2014-03-28  Anton Blanchard  <anton@samba.org>

	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Add single step
	support for lbarx/stbcx., lharx/sthcx. and lqarx/stqcx.
---
 gdb/rs6000-tdep.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index be14e39..7257cc3 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1070,14 +1070,21 @@ ppc_displaced_step_hw_singlestep (struct gdbarch *gdbarch,
 
 /* Instruction masks used during single-stepping of atomic sequences.  */
 #define LWARX_MASK 0xfc0007fe
+#define LBARX_INSTRUCTION 0x7c000068
+#define LHARX_INSTRUCTION 0x7c0000e8
 #define LWARX_INSTRUCTION 0x7c000028
 #define LDARX_INSTRUCTION 0x7c0000A8
+#define LQARX_INSTRUCTION 0x7c000228
+
 #define STWCX_MASK 0xfc0007ff
+#define STBCX_INSTRUCTION 0x7c00056d
+#define STHCX_INSTRUCTION 0x7c0005ad
 #define STWCX_INSTRUCTION 0x7c00012d
 #define STDCX_INSTRUCTION 0x7c0001ad
+#define STQCX_INSTRUCTION 0x7c00016d
 
-/* Checks for an atomic sequence of instructions beginning with a LWARX/LDARX
-   instruction and ending with a STWCX/STDCX instruction.  If such a sequence
+/* Checks for an atomic sequence of instructions beginning with a l[bhwdq]arx
+   instruction and ending with a st[bhwdq]cx instruction.  If such a sequence
    is found, attempt to step through it.  A breakpoint is placed at the end of 
    the sequence.  */
 
@@ -1098,9 +1105,12 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
   const int atomic_sequence_length = 16; /* Instruction sequence length.  */
   int opcode; /* Branch instruction's OPcode.  */
 
-  /* Assume all atomic sequences start with a lwarx/ldarx instruction.  */
-  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
-      && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
+  /* Assume all atomic sequences start with a l[bhwdq]arx instruction.  */
+  if ((insn & LWARX_MASK) != LBARX_INSTRUCTION
+      && (insn & LWARX_MASK) != LHARX_INSTRUCTION
+      && (insn & LWARX_MASK) != LWARX_INSTRUCTION
+      && (insn & LWARX_MASK) != LDARX_INSTRUCTION
+      && (insn & LWARX_MASK) != LQARX_INSTRUCTION)
     return 0;
 
   /* Assume that no atomic sequence is longer than "atomic_sequence_length" 
@@ -1127,14 +1137,20 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
 	  last_breakpoint++;
         }
 
-      if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
-          || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
+      if ((insn & STWCX_MASK) == STBCX_INSTRUCTION
+          || (insn & STWCX_MASK) == STHCX_INSTRUCTION
+          || (insn & STWCX_MASK) == STWCX_INSTRUCTION
+          || (insn & STWCX_MASK) == STDCX_INSTRUCTION
+          || (insn & STWCX_MASK) == STQCX_INSTRUCTION)
         break;
     }
 
-  /* Assume that the atomic sequence ends with a stwcx/stdcx instruction.  */
-  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
-      && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
+  /* Assume that the atomic sequence ends with a st[bhwdq]cx instruction.  */
+  if ((insn & STWCX_MASK) != STBCX_INSTRUCTION
+      && (insn & STWCX_MASK) != STHCX_INSTRUCTION
+      && (insn & STWCX_MASK) != STWCX_INSTRUCTION
+      && (insn & STWCX_MASK) != STDCX_INSTRUCTION
+      && (insn & STWCX_MASK) != STQCX_INSTRUCTION)
     return 0;
 
   closing_insn = loc;
-- 
1.8.3.2

Re: [PATCH 1/4] Fix ppc64 single step over atomic sequence testcase

[Joel Brobecker]

> gdb/
> 2014-03-28  Anton Blanchard  <anton@samba.org>
> 
> 	* gdb.arch/ppc64-atomic-inst.c: Remove.
> 	* gdb.arch/ppc64-atomic-inst.s: New file.
> 	* gdb.arch/ppc64-atomic-inst.exp: Adapt for asm based testcase.

OK to commit.

>  set testfile "ppc64-atomic-inst"
> -set srcfile ${testfile}.c
> +set srcfile ${testfile}.s
>  set binfile ${objdir}/${subdir}/${testfile}
>  set compile_flags {debug quiet}

As a followup patch, you might want to adapt it to use standard_testfile
+ prepare_for_testing.

See: https://sourceware.org/gdb/wiki/GDBTestcaseCookbook

-- 
Joel

Re: [PATCH 2/4] Support up to 3 conditional branches in an atomic sequence

[Joel Brobecker]

On Fri, Mar 28, 2014 at 02:41:49PM +1100, Anton Blanchard wrote:
> gdb currently supports 1 conditional branch inside a ppc larx/stcx
> critical region. Unfortunately there is existing code that contains more
> than 1, for example in the ppc64 Linux kernel:
> 
> c00000000003ac18 <.__hash_page_4K>:
> ...
> c00000000003ac4c:       ldarx   r31,0,r6
> c00000000003ac50:       andc.   r0,r4,r31
> c00000000003ac54:       bne-    c00000000003aee8 <htab_wrong_access>
> c00000000003ac58:       andi.   r0,r31,2048
> c00000000003ac5c:       bne-    c00000000003ae0c <htab_bail_ok>
> c00000000003ac60:       rlwinm  r30,r4,30,24,24
> c00000000003ac64:       or      r30,r30,r31
> c00000000003ac68:       ori     r30,r30,2304
> c00000000003ac6c:       oris    r30,r30,4096
> c00000000003ac70:       stdcx.  r30,0,r6
> 
> If we try to single step through this we get stuck forever because
> the reservation is never set when we step over the stdcx.
> 
> The following patch bumps the number to 3 conditional branches + 1
> terminating branch. With this patch applied I can single step through
> the offending function in the ppc64 Linux kernel.
> 
> gdb/
> 2014-03-28  Anton Blanchard  <anton@samba.org>
> 
> 	* breakpoint.h (MAX_SINGLE_STEP_BREAKPOINTS): New define.
> 	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
> 	than two breakpoints.
> 	* breakpoint.c (insert_single_step_breakpoint): Likewise.
> 	(insert_single_step_breakpoint): Likewise.
> 	(single_step_breakpoints_inserted): Likewise.
> 	(cancel_single_step_breakpoints): Likewise.
> 	(detach_single_step_breakpoints): Likewise.
> 	(single_step_breakpoint_inserted_here_p): Likewise.

Overall, this looks like a nice generalization, but Pedro has been
more active in the breakpoint area than I have, so it would be nice
to have his feedback on this patch as well.

IIUC, it looks like MAX_SINGLE_STEP_BREAKPOINTS is actually not
the max, but MAX - 1. I was a little confused by that. Why not
change MAX to 3, and then adjust the array definition and code
accordingly? I think things will be slightly simpler to understand.


> ---
>  gdb/breakpoint.c  | 63 ++++++++++++++++++++++++++++---------------------------
>  gdb/breakpoint.h  |  6 ++++--
>  gdb/rs6000-tdep.c | 46 ++++++++++++++++++++++------------------
>  3 files changed, 62 insertions(+), 53 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 19af9df..b12ea94 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -15036,11 +15036,10 @@ deprecated_remove_raw_breakpoint (struct gdbarch *gdbarch, void *bp)
>    return ret;
>  }
>  
> -/* One (or perhaps two) breakpoints used for software single
> -   stepping.  */
> +/* Breakpoints used for software single stepping.  */
>  
> -static void *single_step_breakpoints[2];
> -static struct gdbarch *single_step_gdbarch[2];
> +static void *single_step_breakpoints[MAX_SINGLE_STEP_BREAKPOINTS];
> +static struct gdbarch *single_step_gdbarch[MAX_SINGLE_STEP_BREAKPOINTS];
>  
>  /* Create and insert a breakpoint for software single step.  */
>  
> @@ -15049,19 +15048,17 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
>  			       struct address_space *aspace, 
>  			       CORE_ADDR next_pc)
>  {
> +  int i;
>    void **bpt_p;
>  
> -  if (single_step_breakpoints[0] == NULL)
> -    {
> -      bpt_p = &single_step_breakpoints[0];
> -      single_step_gdbarch[0] = gdbarch;
> -    }
> -  else
> -    {
> -      gdb_assert (single_step_breakpoints[1] == NULL);
> -      bpt_p = &single_step_breakpoints[1];
> -      single_step_gdbarch[1] = gdbarch;
> -    }
> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
> +    if (single_step_breakpoints[i] == NULL)
> +        break;
> +
> +  gdb_assert (i < MAX_SINGLE_STEP_BREAKPOINTS);
> +
> +  bpt_p = &single_step_breakpoints[i];
> +  single_step_gdbarch[i] = gdbarch;
>  
>    /* NOTE drow/2006-04-11: A future improvement to this function would
>       be to only create the breakpoints once, and actually put them on
> @@ -15082,8 +15079,13 @@ insert_single_step_breakpoint (struct gdbarch *gdbarch,
>  int
>  single_step_breakpoints_inserted (void)
>  {
> -  return (single_step_breakpoints[0] != NULL
> -          || single_step_breakpoints[1] != NULL);
> +  int i;
> +
> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
> +    if (single_step_breakpoints[i] != NULL)
> +      return 1;
> +
> +  return 0;
>  }
>  
>  /* Remove and delete any breakpoints used for software single step.  */
> @@ -15091,22 +15093,21 @@ single_step_breakpoints_inserted (void)
>  void
>  remove_single_step_breakpoints (void)
>  {
> +  int i;
> +
>    gdb_assert (single_step_breakpoints[0] != NULL);
>  
>    /* See insert_single_step_breakpoint for more about this deprecated
>       call.  */
> -  deprecated_remove_raw_breakpoint (single_step_gdbarch[0],
> -				    single_step_breakpoints[0]);
> -  single_step_gdbarch[0] = NULL;
> -  single_step_breakpoints[0] = NULL;
>  
> -  if (single_step_breakpoints[1] != NULL)
> -    {
> -      deprecated_remove_raw_breakpoint (single_step_gdbarch[1],
> -					single_step_breakpoints[1]);
> -      single_step_gdbarch[1] = NULL;
> -      single_step_breakpoints[1] = NULL;
> -    }
> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
> +    if (single_step_breakpoints[i] != NULL)
> +      {
> +	deprecated_remove_raw_breakpoint (single_step_gdbarch[i],
> +					  single_step_breakpoints[i]);
> +	single_step_gdbarch[i] = NULL;
> +	single_step_breakpoints[i] = NULL;
> +      }
>  }
>  
>  /* Delete software single step breakpoints without removing them from
> @@ -15119,7 +15120,7 @@ cancel_single_step_breakpoints (void)
>  {
>    int i;
>  
> -  for (i = 0; i < 2; i++)
> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
>      if (single_step_breakpoints[i])
>        {
>  	xfree (single_step_breakpoints[i]);
> @@ -15136,7 +15137,7 @@ detach_single_step_breakpoints (void)
>  {
>    int i;
>  
> -  for (i = 0; i < 2; i++)
> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
>      if (single_step_breakpoints[i])
>        target_remove_breakpoint (single_step_gdbarch[i],
>  				single_step_breakpoints[i]);
> @@ -15151,7 +15152,7 @@ single_step_breakpoint_inserted_here_p (struct address_space *aspace,
>  {
>    int i;
>  
> -  for (i = 0; i < 2; i++)
> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
>      {
>        struct bp_target_info *bp_tgt = single_step_breakpoints[i];
>        if (bp_tgt
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index d8e88fc..65d3ecb 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -1443,8 +1443,10 @@ extern void add_solib_catchpoint (char *arg, int is_load, int is_temp,
>     deletes all breakpoints.  */
>  extern void delete_command (char *arg, int from_tty);
>  
> -/* Manage a software single step breakpoint (or two).  Insert may be
> -   called twice before remove is called.  */
> +/* Manage software single step breakpoints.  Insert may be
> +   called multiple times before remove is called.  */
> +#define MAX_SINGLE_STEP_BREAKPOINTS 4
> +
>  extern void insert_single_step_breakpoint (struct gdbarch *,
>  					   struct address_space *, 
>  					   CORE_ADDR);
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index dbe3aa2..be14e39 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1088,7 +1088,7 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>    struct address_space *aspace = get_frame_address_space (frame);
>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>    CORE_ADDR pc = get_frame_pc (frame);
> -  CORE_ADDR breaks[2] = {-1, -1};
> +  CORE_ADDR breaks[MAX_SINGLE_STEP_BREAKPOINTS];
>    CORE_ADDR loc = pc;
>    CORE_ADDR closing_insn; /* Instruction that closes the atomic sequence.  */
>    int insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
> @@ -1097,7 +1097,6 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>    int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */  
>    const int atomic_sequence_length = 16; /* Instruction sequence length.  */
>    int opcode; /* Branch instruction's OPcode.  */
> -  int bc_insn_count = 0; /* Conditional branch instruction count.  */
>  
>    /* Assume all atomic sequences start with a lwarx/ldarx instruction.  */
>    if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
> @@ -1111,24 +1110,20 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>        loc += PPC_INSN_SIZE;
>        insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
>  
> -      /* Assume that there is at most one conditional branch in the atomic
> -         sequence.  If a conditional branch is found, put a breakpoint in 
> -         its destination address.  */
>        if ((insn & BRANCH_MASK) == BC_INSN)
>          {
>            int immediate = ((insn & 0xfffc) ^ 0x8000) - 0x8000;
>            int absolute = insn & 2;
>  
> -          if (bc_insn_count >= 1)
> -            return 0; /* More than one conditional branch found, fallback 
> +          if (last_breakpoint >= MAX_SINGLE_STEP_BREAKPOINTS - 1)
> +            return 0; /* too many conditional branches found, fallback
>                           to the standard single-step code.  */
>   
>  	  if (absolute)
> -	    breaks[1] = immediate;
> +	    breaks[last_breakpoint] = immediate;
>  	  else
> -	    breaks[1] = loc + immediate;
> +	    breaks[last_breakpoint] = loc + immediate;
>  
> -	  bc_insn_count++;
>  	  last_breakpoint++;
>          }
>  
> @@ -1147,18 +1142,29 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>    insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
>  
>    /* Insert a breakpoint right after the end of the atomic sequence.  */
> -  breaks[0] = loc;
> +  breaks[last_breakpoint] = loc;
>  
> -  /* Check for duplicated breakpoints.  Check also for a breakpoint
> -     placed (branch instruction's destination) anywhere in sequence.  */
> -  if (last_breakpoint
> -      && (breaks[1] == breaks[0]
> -	  || (breaks[1] >= pc && breaks[1] <= closing_insn)))
> -    last_breakpoint = 0;
> -
> -  /* Effectively inserts the breakpoints.  */
>    for (index = 0; index <= last_breakpoint; index++)
> -    insert_single_step_breakpoint (gdbarch, aspace, breaks[index]);
> +    {
> +      int index2;
> +      int insert_bp = 1;
> +
> +      /* Check for a breakpoint placed (branch instruction's destination)
> +	 anywhere in sequence.  */
> +      if (breaks[index] >= pc && breaks[index] <= closing_insn)
> +	continue;
> +
> +      /* Check for duplicated breakpoints.  */
> +      for (index2 = 0; index2 < index; index2++)
> +        {
> +	  if (breaks[index] == breaks[index2])
> +	    insert_bp = 0;
> +	}
> +
> +      /* insert the breakpoint.  */
> +      if (insert_bp)
> +        insert_single_step_breakpoint (gdbarch, aspace, breaks[index]);
> +    }
>  
>    return 1;
>  }
> -- 
> 1.8.3.2

-- 
Joel

Re: [PATCH 1/4] Fix ppc64 single step over atomic sequence testcase

[Ulrich Weigand]

Anton Blanchard wrote:

> +++ b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s

> +#if _CALL_ELF == 2

I just noticed that a '.s' file is not run through
the preprocessor, and thus this fails (the # is
regarded as a comment ...).

You need to rename the file to .S (and update the
filename in the .exp file) for the test to work
successfully.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU/Linux compilers and toolchain
  Ulrich.Weigand@de.ibm.com

Re: [PATCH 3/4] Add multiple branches to ppc64 single step through atomic sequence testcase

[Joel Brobecker]

> Test 3 conditional branches in an atomic sequence, 2 to the same
> destination.
> 
> gdb/testsuite/
> 2014-03-28  Anton Blanchard  <anton@samba.org>
> 
> 	* gdb.arch/ppc64-atomic-inst.s: Add second and third branch
> 	inside atomic sequence.

Just a thought - is there no value in keeping the old assembly code
and creating an additional testcase? It would be very similar to
the one you already have, but then you'd be covering most cases...
Otherwise, the patch seems fine.

> ---
>  gdb/testsuite/gdb.arch/ppc64-atomic-inst.s | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
> index 15ccfd9..0521170 100644
> --- a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
> +++ b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
> @@ -39,18 +39,30 @@ main:
>  1:	lwarx	5,0,4
>  	cmpwi	5,0
>  	bne	2f
> +	cmpwi	5,1
> +	beq	3f
> +	cmpwi	5,2
> +	beq	3f	/* branch to same destination */
>  	addi	5,5,1
>  	stwcx.	5,0,4
>  	bne	1b
>  
> -	std	0,0(4)
> -2:	ldarx	5,0,4
> +2:	nop
> +
> +3:	std	0,0(4)
> +1:	ldarx	5,0,4
>  	cmpdi	5,0
> -	bne	3f
> +	bne	2f
> +	cmpdi	5,1
> +	beq	3f
> +	cmpwi	5,2
> +	beq	3f	/* branch to same destination */
>  	addi	5,5,1
>  	stdcx.	5,0,4
>  	bne	1b
>  
> +2:	nop
> +
>  3:	li	3,0
>  	blr
>  
> -- 
> 1.8.3.2

-- 
Joel

Re: [PATCH 4/4] Add lbarx/stbcx., lharx/sthcx. and lqarx/stqcx. single stepping

[Joel Brobecker]

> gdb/testsuite/
     ^^^^^^^^^^
Small typo. I'm sure you'll be adding the ChangeLog entry to the right
file, but we can also avoid this typo creeping into the revision log
of the commit that will eventually get pushed.

> 2014-03-28  Anton Blanchard  <anton@samba.org>
> 
> 	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Add single step
> 	support for lbarx/stbcx., lharx/sthcx. and lqarx/stqcx.

OK.

Thank you.

> ---
>  gdb/rs6000-tdep.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index be14e39..7257cc3 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1070,14 +1070,21 @@ ppc_displaced_step_hw_singlestep (struct gdbarch *gdbarch,
>  
>  /* Instruction masks used during single-stepping of atomic sequences.  */
>  #define LWARX_MASK 0xfc0007fe
> +#define LBARX_INSTRUCTION 0x7c000068
> +#define LHARX_INSTRUCTION 0x7c0000e8
>  #define LWARX_INSTRUCTION 0x7c000028
>  #define LDARX_INSTRUCTION 0x7c0000A8
> +#define LQARX_INSTRUCTION 0x7c000228
> +
>  #define STWCX_MASK 0xfc0007ff
> +#define STBCX_INSTRUCTION 0x7c00056d
> +#define STHCX_INSTRUCTION 0x7c0005ad
>  #define STWCX_INSTRUCTION 0x7c00012d
>  #define STDCX_INSTRUCTION 0x7c0001ad
> +#define STQCX_INSTRUCTION 0x7c00016d
>  
> -/* Checks for an atomic sequence of instructions beginning with a LWARX/LDARX
> -   instruction and ending with a STWCX/STDCX instruction.  If such a sequence
> +/* Checks for an atomic sequence of instructions beginning with a l[bhwdq]arx
> +   instruction and ending with a st[bhwdq]cx instruction.  If such a sequence
>     is found, attempt to step through it.  A breakpoint is placed at the end of 
>     the sequence.  */
>  
> @@ -1098,9 +1105,12 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>    const int atomic_sequence_length = 16; /* Instruction sequence length.  */
>    int opcode; /* Branch instruction's OPcode.  */
>  
> -  /* Assume all atomic sequences start with a lwarx/ldarx instruction.  */
> -  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
> -      && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
> +  /* Assume all atomic sequences start with a l[bhwdq]arx instruction.  */
> +  if ((insn & LWARX_MASK) != LBARX_INSTRUCTION
> +      && (insn & LWARX_MASK) != LHARX_INSTRUCTION
> +      && (insn & LWARX_MASK) != LWARX_INSTRUCTION
> +      && (insn & LWARX_MASK) != LDARX_INSTRUCTION
> +      && (insn & LWARX_MASK) != LQARX_INSTRUCTION)
>      return 0;
>  
>    /* Assume that no atomic sequence is longer than "atomic_sequence_length" 
> @@ -1127,14 +1137,20 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>  	  last_breakpoint++;
>          }
>  
> -      if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
> -          || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
> +      if ((insn & STWCX_MASK) == STBCX_INSTRUCTION
> +          || (insn & STWCX_MASK) == STHCX_INSTRUCTION
> +          || (insn & STWCX_MASK) == STWCX_INSTRUCTION
> +          || (insn & STWCX_MASK) == STDCX_INSTRUCTION
> +          || (insn & STWCX_MASK) == STQCX_INSTRUCTION)
>          break;
>      }
>  
> -  /* Assume that the atomic sequence ends with a stwcx/stdcx instruction.  */
> -  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
> -      && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
> +  /* Assume that the atomic sequence ends with a st[bhwdq]cx instruction.  */
> +  if ((insn & STWCX_MASK) != STBCX_INSTRUCTION
> +      && (insn & STWCX_MASK) != STHCX_INSTRUCTION
> +      && (insn & STWCX_MASK) != STWCX_INSTRUCTION
> +      && (insn & STWCX_MASK) != STDCX_INSTRUCTION
> +      && (insn & STWCX_MASK) != STQCX_INSTRUCTION)
>      return 0;
>  
>    closing_insn = loc;
> -- 
> 1.8.3.2

-- 
Joel

Re: [PATCH 2/4] Support up to 3 conditional branches in an atomic sequence

[Pedro Alves]

On 03/28/2014 01:12 PM, Joel Brobecker wrote:
> On Fri, Mar 28, 2014 at 02:41:49PM +1100, Anton Blanchard wrote:
>> gdb currently supports 1 conditional branch inside a ppc larx/stcx
>> critical region. Unfortunately there is existing code that contains more
>> than 1, for example in the ppc64 Linux kernel:
>>
>> c00000000003ac18 <.__hash_page_4K>:
>> ...
>> c00000000003ac4c:       ldarx   r31,0,r6
>> c00000000003ac50:       andc.   r0,r4,r31
>> c00000000003ac54:       bne-    c00000000003aee8 <htab_wrong_access>
>> c00000000003ac58:       andi.   r0,r31,2048
>> c00000000003ac5c:       bne-    c00000000003ae0c <htab_bail_ok>
>> c00000000003ac60:       rlwinm  r30,r4,30,24,24
>> c00000000003ac64:       or      r30,r30,r31
>> c00000000003ac68:       ori     r30,r30,2304
>> c00000000003ac6c:       oris    r30,r30,4096
>> c00000000003ac70:       stdcx.  r30,0,r6
>>
>> If we try to single step through this we get stuck forever because
>> the reservation is never set when we step over the stdcx.
>>
>> The following patch bumps the number to 3 conditional branches + 1
>> terminating branch. With this patch applied I can single step through
>> the offending function in the ppc64 Linux kernel.

Is there a hard limit?  Like, is there a limit on the number of instructions
that can appear inside a ppc larx/stcx region?

>>
>> gdb/
>> 2014-03-28  Anton Blanchard  <anton@samba.org>
>>
>> 	* breakpoint.h (MAX_SINGLE_STEP_BREAKPOINTS): New define.
>> 	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
>> 	than two breakpoints.
>> 	* breakpoint.c (insert_single_step_breakpoint): Likewise.
>> 	(insert_single_step_breakpoint): Likewise.
>> 	(single_step_breakpoints_inserted): Likewise.
>> 	(cancel_single_step_breakpoints): Likewise.
>> 	(detach_single_step_breakpoints): Likewise.
>> 	(single_step_breakpoint_inserted_here_p): Likewise.
> 
> Overall, this looks like a nice generalization, but Pedro has been
> more active in the breakpoint area than I have, so it would be nice
> to have his feedback on this patch as well.
> 
> IIUC, it looks like MAX_SINGLE_STEP_BREAKPOINTS is actually not
> the max, but MAX - 1. I was a little confused by that. Why not
> change MAX to 3, and then adjust the array definition and code
> accordingly? I think things will be slightly simpler to understand.

IMO that would be more confusing.  I read MAX_SINGLE_STEP_BREAKPOINTS
as meaning the "maximum number of of single-step breakpoints we
can create simultaneously".  I think you're reading it as
"the highest index possible into the single-step breakpoints
array" ?

Maybe it'd be clearer to just rename the macro, to, say
NUM_SINGLE_STEP_BREAKPOINTS?

>> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
>> +    if (single_step_breakpoints[i] == NULL)
>> +        break;

I notice something odd with the formatting.  E.g., above,
vs:

>> +  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
>> +    if (single_step_breakpoints[i] != NULL)
>> +      return 1;

Probably tab vs spaces -- please make use to use tabs
for 8 spaces.

>> --- a/gdb/rs6000-tdep.c
>> +++ b/gdb/rs6000-tdep.c
>> @@ -1088,7 +1088,7 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>>    struct address_space *aspace = get_frame_address_space (frame);
>>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>    CORE_ADDR pc = get_frame_pc (frame);
>> -  CORE_ADDR breaks[2] = {-1, -1};
>> +  CORE_ADDR breaks[MAX_SINGLE_STEP_BREAKPOINTS];

If we ever bump MAX_SINGLE_STEP_BREAKPOINTS further,
you'd still only want 4 here.

I think it'd be better if this was:

  /* 3 conditional branches + 1 terminating branch.  */
  CORE_ADDR breaks[4];

Followed by:

gdb_static_assert (MAX_SINGLE_STEP_BREAKPOINTS >= ARRAY_SIZE (breaks));

This way clearly documents that we need to support 4 sss breakpoints.
As it is, nothing in your patch leaves any indication in the source to
that effect, so the poor soul trying to revamp software single-step
breakpoints could miss this.

>>    CORE_ADDR loc = pc;
>>    CORE_ADDR closing_insn; /* Instruction that closes the atomic sequence.  */
>>    int insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
>> @@ -1097,7 +1097,6 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>>    int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */  
>>    const int atomic_sequence_length = 16; /* Instruction sequence length.  */
>>    int opcode; /* Branch instruction's OPcode.  */
>> -  int bc_insn_count = 0; /* Conditional branch instruction count.  */
>>  
>>    /* Assume all atomic sequences start with a lwarx/ldarx instruction.  */
>>    if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
>> @@ -1111,24 +1110,20 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>>        loc += PPC_INSN_SIZE;
>>        insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
>>  
>> -      /* Assume that there is at most one conditional branch in the atomic
>> -         sequence.  If a conditional branch is found, put a breakpoint in 
>> -         its destination address.  */
>>        if ((insn & BRANCH_MASK) == BC_INSN)
>>          {
>>            int immediate = ((insn & 0xfffc) ^ 0x8000) - 0x8000;
>>            int absolute = insn & 2;
>>  
>> -          if (bc_insn_count >= 1)
>> -            return 0; /* More than one conditional branch found, fallback 
>> +          if (last_breakpoint >= MAX_SINGLE_STEP_BREAKPOINTS - 1)
>> +            return 0; /* too many conditional branches found, fallback

No need for '>=' IFAICS.  Here I'd suggest:

          if (last_breakpoint == ARRAY_SIZE (breaks) - 1)
            {
              /* Too many conditional branches found, fallback
                 to the standard single-step code.  */
              return 0;
            }

Note "Too" should be capitalized.  also, our style nowadays says
to wrap the comment and statement in a {}s if it's multiline,
even though the old code wasn't doing that.

With those changes this looks good to me.
-- 
Pedro Alves

>>   
>>  	  if (absolute)
>> -	    breaks[1] = immediate;
>> +	    breaks[last_breakpoint] = immediate;
>>  	  else
>> -	    breaks[1] = loc + immediate;
>> +	    breaks[last_breakpoint] = loc + immediate;
>>  
>> -	  bc_insn_count++;
>>  	  last_breakpoint++;
>>          }
>>  
>> @@ -1147,18 +1142,29 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>>    insn = read_memory_integer (loc, PPC_INSN_SIZE, byte_order);
>>  
>>    /* Insert a breakpoint right after the end of the atomic sequence.  */
>> -  breaks[0] = loc;
>> +  breaks[last_breakpoint] = loc;
>>  
>> -  /* Check for duplicated breakpoints.  Check also for a breakpoint
>> -     placed (branch instruction's destination) anywhere in sequence.  */
>> -  if (last_breakpoint
>> -      && (breaks[1] == breaks[0]
>> -	  || (breaks[1] >= pc && breaks[1] <= closing_insn)))
>> -    last_breakpoint = 0;
>> -
>> -  /* Effectively inserts the breakpoints.  */
>>    for (index = 0; index <= last_breakpoint; index++)
>> -    insert_single_step_breakpoint (gdbarch, aspace, breaks[index]);
>> +    {
>> +      int index2;
>> +      int insert_bp = 1;
>> +
>> +      /* Check for a breakpoint placed (branch instruction's destination)
>> +	 anywhere in sequence.  */
>> +      if (breaks[index] >= pc && breaks[index] <= closing_insn)
>> +	continue;
>> +
>> +      /* Check for duplicated breakpoints.  */
>> +      for (index2 = 0; index2 < index; index2++)
>> +        {
>> +	  if (breaks[index] == breaks[index2])
>> +	    insert_bp = 0;
>> +	}
>> +
>> +      /* insert the breakpoint.  */
>> +      if (insert_bp)
>> +        insert_single_step_breakpoint (gdbarch, aspace, breaks[index]);
>> +    }
>>  
>>    return 1;
>>  }
>> -- 
>> 1.8.3.2
> 

Re: [PATCH 2/4] Support up to 3 conditional branches in an atomic sequence

[Pedro Alves]

On 03/28/2014 05:12 PM, Pedro Alves wrote:

>>> --- a/gdb/rs6000-tdep.c
>>> +++ b/gdb/rs6000-tdep.c
>>> @@ -1088,7 +1088,7 @@ ppc_deal_with_atomic_sequence (struct frame_info *frame)
>>>    struct address_space *aspace = get_frame_address_space (frame);
>>>    enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>>>    CORE_ADDR pc = get_frame_pc (frame);
>>> -  CORE_ADDR breaks[2] = {-1, -1};
>>> +  CORE_ADDR breaks[MAX_SINGLE_STEP_BREAKPOINTS];
> 
> If we ever bump MAX_SINGLE_STEP_BREAKPOINTS further,
> you'd still only want 4 here.
> 
> I think it'd be better if this was:
> 
>   /* 3 conditional branches + 1 terminating branch.  */
>   CORE_ADDR breaks[4];
> 
> Followed by:
> 
> gdb_static_assert (MAX_SINGLE_STEP_BREAKPOINTS >= ARRAY_SIZE (breaks));
> 
> This way clearly documents that we need to support 4 sss breakpoints.
> As it is, nothing in your patch leaves any indication in the source to
> that effect, so the poor soul trying to revamp software single-step
> breakpoints could miss this.

Sorry, I wrote this before realizing that there could even be more
condition branches in the region and writing the "is there a hard limit",
and then pushed send too soon.  So assuming there's no limit and we're
just trying to be practical in what we support, there's really no harm
in leaving this as:

 CORE_ADDR breaks[MAX_SINGLE_STEP_BREAKPOINTS];

but I still think a comment (+ the assert would be nice) is
missing.  Something like:

/* The ppc64 Linux kernel has regions with 3 conditional branches.
   (plus 1 for the terminating branch).  */
gdb_static_assert (MAX_SINGLE_STEP_BREAKPOINTS >= 4);

?

BTW, shouldn't GDB warn or even error out if too many
conditional branches are found?  I think that'd be better
than silently getting stuck forever.

-- 
Pedro Alves

Re: [PATCH 2/4] Support up to 3 conditional branches in an atomic sequence

[Joel Brobecker]

> > IIUC, it looks like MAX_SINGLE_STEP_BREAKPOINTS is actually not
> > the max, but MAX - 1. I was a little confused by that. Why not
> > change MAX to 3, and then adjust the array definition and code
> > accordingly? I think things will be slightly simpler to understand.
> 
> IMO that would be more confusing.  I read MAX_SINGLE_STEP_BREAKPOINTS
> as meaning the "maximum number of of single-step breakpoints we
> can create simultaneously".  I think you're reading it as
> "the highest index possible into the single-step breakpoints
> array" ?

Here is how I read the patch: MAX_SINGLE_STEP_BREAKPOINTS is the size
of the array, and we rely on the last element always being NULL
to determine the number of "live" elements we actually have.
Hence, to me, the maximum number of SS breakpoints we can handle
in practice is not MAX_SINGLE_STEP_BREAKPOINTS but 1 less. For
instance, I think the patch is trying to increase the number of
SS breakpoints to 3, and yet defines MAX_SINGLE_STEP_BREAKPOINTS
to 4.

Perhaps it's time to just use a vec? That way, we don't have
a limit at all anymore...

-- 
Joel

Re: [PATCH 2/4] Support up to 3 conditional branches in an atomic sequence

[Pedro Alves]

On 03/28/2014 05:32 PM, Joel Brobecker wrote:
>>> IIUC, it looks like MAX_SINGLE_STEP_BREAKPOINTS is actually not
>>> the max, but MAX - 1. I was a little confused by that. Why not
>>> change MAX to 3, and then adjust the array definition and code
>>> accordingly? I think things will be slightly simpler to understand.
>>
>> IMO that would be more confusing.  I read MAX_SINGLE_STEP_BREAKPOINTS
>> as meaning the "maximum number of of single-step breakpoints we
>> can create simultaneously".  I think you're reading it as
>> "the highest index possible into the single-step breakpoints
>> array" ?
> 
> Here is how I read the patch: MAX_SINGLE_STEP_BREAKPOINTS is the size
> of the array, and we rely on the last element always being NULL
> to determine the number of "live" elements we actually have.

But we can fill the whole array, there's no sentinel.  E.g.:

+  for (i = 0; i < MAX_SINGLE_STEP_BREAKPOINTS; i++)
+    if (single_step_breakpoints[i] == NULL)
+        break;
+
+  gdb_assert (i < MAX_SINGLE_STEP_BREAKPOINTS);
+

This just looks for the first empty slot.

> Hence, to me, the maximum number of SS breakpoints we can handle
> in practice is not MAX_SINGLE_STEP_BREAKPOINTS but 1 less.

Nope.  We can handle MAX_SINGLE_STEP_BREAKPOINTS.

> For
> instance, I think the patch is trying to increase the number of
> SS breakpoints to 3, and yet defines MAX_SINGLE_STEP_BREAKPOINTS
> to 4.

Anton is making the port handle 3 conditional branches + 1
terminating branch, so that's 4.  I guess it's either the
subject that's confusing you, or this, perhaps:

> -          if (bc_insn_count >= 1)
> -            return 0; /* More than one conditional branch found, fallback
> +          if (last_breakpoint >= MAX_SINGLE_STEP_BREAKPOINTS - 1)
> +            return 0; /* too many conditional branches found, fallback
>                           to the standard single-step code.  */

This is "- 1" here because that's leaving space for the terminating
branch.  At least, that's what I understood.

I blame lack of comments in the patch.  :-)

> Perhaps it's time to just use a vec? That way, we don't have
> a limit at all anymore...

Yeah...

-- 
Pedro Alves

Re: [PATCH 2/4] Support up to 3 conditional branches in an atomic sequence

[Joel Brobecker]

> But we can fill the whole array, there's no sentinel.  E.g.:

Ah, ok, I was indeed confused. Lack of comments I will blame also :)

Thanks for clarifying it for me.
-- 
Joel

Re: [PATCH 1/4] Fix ppc64 single step over atomic sequence testcase

[Anton Blanchard]

Hi,

Joel Brobecker <brobecker@adacore.com> wrote:

> As a followup patch, you might want to adapt it to use
> standard_testfile + prepare_for_testing.
> 
> See: https://sourceware.org/gdb/wiki/GDBTestcaseCookbook

Nice link. I took the opportunity to add these and make some other
modifications suggested.


"Ulrich Weigand" <uweigand@de.ibm.com> wrote:

> You need to rename the file to .S (and update the
> filename in the .exp file) for the test to work
> successfully.

Thanks Uli, fixed in the next spin.

Anton