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

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

[Anton Blanchard]

(My apologies, I thought I'd got these patches merged but obviously I
hadn't).

The current ppc64 single step over atomic sequence testcase is broken.
Convert the test into assembly and use stepi to step through it.

Anton
--

2013-07-29  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 

Index: gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
===================================================================
--- gdb.orig/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
+++ gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
@@ -27,9 +27,9 @@ if {![istarget "powerpc*"] || ![is_lp64_
 }
 
 set testfile "ppc64-atomic-inst"
-set srcfile ${testfile}.c
+set srcfile ${testfile}.s
 set binfile ${objdir}/${subdir}/${testfile}
-set compile_flags {debug quiet}
+set compile_flags "debug quiet additional_flags=-I${srcdir}/gdb.asm"
 
 if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $compile_flags] != "" } {
     unsupported "Testcase compile failed."
@@ -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"
Index: gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.c
===================================================================
--- gdb.orig/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-2013 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;
-}
Index: gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
===================================================================
--- /dev/null
+++ gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
@@ -0,0 +1,26 @@
+        .include "common.inc"
+        .include "powerpc64.inc"
+
+.global main
+gdbasm_declare main
+	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

[PATCH 2/3] 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:       7f e0 30 a8     ldarx   r31,0,r6
c00000000003ac50:       7c 80 f8 79     andc.   r0,r4,r31
c00000000003ac54:       40 82 02 94     bne-    c00000000003aee8 <htab_wrong_access>
c00000000003ac58:       73 e0 08 00     andi.   r0,r31,2048
c00000000003ac5c:       40 82 01 b0     bne-    c00000000003ae0c <htab_bail_ok>
c00000000003ac60:       54 9e f6 30     rlwinm  r30,r4,30,24,24
c00000000003ac64:       7f de fb 78     or      r30,r30,r31
c00000000003ac68:       63 de 09 00     ori     r30,r30,2304
c00000000003ac6c:       67 de 10 00     oris    r30,r30,4096
c00000000003ac70:       7f c0 31 ad     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.

Anton
--

2013-07-29  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.

Index: b/gdb/rs6000-tdep.c
===================================================================
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1087,7 +1087,7 @@ ppc_deal_with_atomic_sequence (struct fr
   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);
@@ -1096,7 +1096,6 @@ ppc_deal_with_atomic_sequence (struct fr
   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
@@ -1110,24 +1109,20 @@ ppc_deal_with_atomic_sequence (struct fr
       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++;
         }
 
@@ -1146,18 +1141,29 @@ ppc_deal_with_atomic_sequence (struct fr
   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;
 }
Index: b/gdb/breakpoint.c
===================================================================
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14845,11 +14845,10 @@ deprecated_remove_raw_breakpoint (struct
   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.  */
 
@@ -14858,19 +14857,17 @@ insert_single_step_breakpoint (struct gd
 			       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
@@ -14891,8 +14888,13 @@ insert_single_step_breakpoint (struct gd
 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.  */
@@ -14900,22 +14902,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
@@ -14928,7 +14929,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]);
@@ -14945,7 +14946,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]);
@@ -14960,7 +14961,7 @@ single_step_breakpoint_inserted_here_p (
 {
   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
Index: b/gdb/breakpoint.h
===================================================================
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1444,8 +1444,10 @@ extern void add_solib_catchpoint (char *
    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);

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

[Ulrich Weigand]

Anton Blanchard wrote:

> 2013-07-29  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.

This is OK.

Thanks,
Ulrich

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

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

[Anton Blanchard]

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

Anton
--

2013-07-29  Anton Blanchard  <anton@samba.org>

	* gdb.arch/ppc64-atomic-inst.s: Add second and third branch
	inside atomic sequence.

Index: b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
===================================================================
--- a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
+++ b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
@@ -10,17 +10,29 @@ gdbasm_declare 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

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

[Ulrich Weigand]

Anton Blanchard wrote:

> 2013-07-29  Anton Blanchard  <anton@samba.org>
> 
> 	* gdb.arch/ppc64-atomic-inst.s: Add second and third branch
> 	inside atomic sequence.

This is OK.

Thanks,
Ulrich

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

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

[Edjunior Barbosa Machado]

On 07/29/2013 04:38 AM, Anton Blanchard wrote:
> 
> (My apologies, I thought I'd got these patches merged but obviously I
> hadn't).
> 
> The current ppc64 single step over atomic sequence testcase is broken.
> Convert the test into assembly and use stepi to step through it.

Hi Anton,

thanks for reworking this patch! I do recall we discussed this in the
mailing list last year
(http://sourceware.org/ml/gdb-patches/2012-06/msg00162.html).

> 
> Anton
> --
> 
> 2013-07-29  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 
> 

(just a nitpick: missing period at the end of the lines.)

> Index: gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
> ===================================================================
> --- gdb.orig/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
> +++ gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
> @@ -27,9 +27,9 @@ if {![istarget "powerpc*"] || ![is_lp64_
>  }
> 
>  set testfile "ppc64-atomic-inst"
> -set srcfile ${testfile}.c
> +set srcfile ${testfile}.s
>  set binfile ${objdir}/${subdir}/${testfile}
> -set compile_flags {debug quiet}
> +set compile_flags "debug quiet additional_flags=-I${srcdir}/gdb.asm"

I've noticed it fails to build with the following:

spawn -ignore SIGHUP gcc ../../../gdb.git/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s -I../../../gdb.git/gdb/testsuite/gdb.asm -g -lm -o /home/emachado/gdb/build/gdb/testsuite/gdb.arch/ppc64-atomic-inst^M
../../../gdb.git/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s: Assembler messages:^M
../../../gdb.git/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s:1: Error: can't open common.inc for reading: No such file or directory^M
../../../gdb.git/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s:2: Error: can't open powerpc64.inc for reading: No such file or directory^M
../../../gdb.git/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s:5: Error: Unrecognized opcode: `gdbasm_declare'^M
compiler exited with status 1

This seems to happen because -I option is not passed to the assembler. I've
tried adding this parameter using -Wa as:

set compile_flags "debug quiet additional_flags=-Wa,-I${srcdir}/gdb.asm"

and it fixes the build failure. With this change, the testcase passes OK, also
including the other 2 patches you sent on this thread.

Thanks and regards,
-- 
Edjunior

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

[Anton Blanchard]

Hi Edjunior,

> This seems to happen because -I option is not passed to the
> assembler. I've tried adding this parameter using -Wa as:
> 
> set compile_flags "debug quiet
>     additional_flags=-Wa,-I${srcdir}/gdb.asm"
> 
> and it fixes the build failure. With this change, the testcase passes
> OK, also including the other 2 patches you sent on this thread.

Thanks for the fix! Incorporated below.

Anton
--

The current ppc64 single step over atomic sequence testcase is broken.
Convert the test into assembly and use stepi to step through it.

--

2013-07-29  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.

Index: b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
===================================================================
--- a/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
+++ b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
@@ -27,9 +27,9 @@ if {![istarget "powerpc*"] || ![is_lp64_
 }
 
 set testfile "ppc64-atomic-inst"
-set srcfile ${testfile}.c
+set srcfile ${testfile}.s
 set binfile ${objdir}/${subdir}/${testfile}
-set compile_flags {debug quiet}
+set compile_flags "debug quiet additional_flags=-Wa,-I${srcdir}/gdb.asm"
 
 if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable $compile_flags] != "" } {
     unsupported "Testcase compile failed."
@@ -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"
Index: b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.c
===================================================================
--- 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-2013 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;
-}
Index: b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
===================================================================
--- /dev/null
+++ b/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
@@ -0,0 +1,26 @@
+        .include "common.inc"
+        .include "powerpc64.inc"
+
+.global main
+gdbasm_declare main
+	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

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

[Ulrich Weigand]

Anton Blanchard wrote:

> > This seems to happen because -I option is not passed to the
> > assembler. I've tried adding this parameter using -Wa as:
> > 
> > set compile_flags "debug quiet
> >     additional_flags=-Wa,-I${srcdir}/gdb.asm"
> > 
> > and it fixes the build failure. With this change, the testcase passes
> > OK, also including the other 2 patches you sent on this thread.
> 
> Thanks for the fix! Incorporated below.

I think it might be best to just get rid of those .include statements;
depending on an .inc file from another directory seems surprising.

It seems you only need it for the gdbasm_declare macro; since this
file is ppc64 specific anyway, why don't you just hard-code the
.opd generation in this source file?

Also, the assembler source file probably ought to keep the
copyright header.   A comment why this test needs to use
assembler source also would be good.

Thanks,
Ulrich

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

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

[Anton Blanchard]

Hi Uli,

> I think it might be best to just get rid of those .include statements;
> depending on an .inc file from another directory seems surprising.
> 
> It seems you only need it for the gdbasm_declare macro; since this
> file is ppc64 specific anyway, why don't you just hard-code the
> .opd generation in this source file?

Considering the issues we have had with getting the correct include
paths passed into the assembler, I tend to agree. It only saves a couple
of lines.

> Also, the assembler source file probably ought to keep the
> copyright header.   A comment why this test needs to use
> assembler source also would be good.

Thanks, will incorporate that into the next spin.

Anton

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

[Anton Blanchard]

The current ppc64 single step over atomic sequence testcase is broken.
Convert the test into assembly and use stepi to step through it.

Anton
--

2012-06-05  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 

Index: gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
===================================================================
--- gdb.orig/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
+++ gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.exp
@@ -27,7 +27,7 @@ if {![istarget "powerpc*"] || ![is_lp64_
 }
 
 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"
Index: gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.c
===================================================================
--- gdb.orig/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-2012 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;
-}
Index: gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
===================================================================
--- /dev/null
+++ gdb/gdb/testsuite/gdb.arch/ppc64-atomic-inst.s
@@ -0,0 +1,26 @@
+        .include "gdb.asm/common.inc"
+        .include "gdb.asm/powerpc64.inc"
+
+.global main
+gdbasm_declare main
+	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

[PATCH 2/3] 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:       7f e0 30 a8     ldarx   r31,0,r6
c00000000003ac50:       7c 80 f8 79     andc.   r0,r4,r31
c00000000003ac54:       40 82 02 94     bne-    c00000000003aee8 <htab_wrong_access>
c00000000003ac58:       73 e0 08 00     andi.   r0,r31,2048
c00000000003ac5c:       40 82 01 b0     bne-    c00000000003ae0c <htab_bail_ok>
c00000000003ac60:       54 9e f6 30     rlwinm  r30,r4,30,24,24
c00000000003ac64:       7f de fb 78     or      r30,r30,r31
c00000000003ac68:       63 de 09 00     ori     r30,r30,2304
c00000000003ac6c:       67 de 10 00     oris    r30,r30,4096
c00000000003ac70:       7f c0 31 ad     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.

Anton
--

2012-06-05  Anton Blanchard  <anton@samba.org>

	* gdb/breakpoint.h: Define NR_SINGLE_STEP_BREAKPOINTS
	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
	than two breakpoints.
	* gdb/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

Index: gdb/gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/gdb/rs6000-tdep.c	2012-06-06 12:59:47.857432494 +1000
+++ gdb/gdb/rs6000-tdep.c	2012-06-06 12:59:57.409592812 +1000
@@ -1087,7 +1087,7 @@ ppc_deal_with_atomic_sequence (struct fr
   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[NR_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);
@@ -1096,7 +1096,6 @@ ppc_deal_with_atomic_sequence (struct fr
   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
@@ -1110,24 +1109,20 @@ ppc_deal_with_atomic_sequence (struct fr
       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 >= (NR_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++;
         }
 
@@ -1146,18 +1141,29 @@ ppc_deal_with_atomic_sequence (struct fr
   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;
 }
Index: gdb/gdb/breakpoint.c
===================================================================
--- gdb.orig/gdb/breakpoint.c	2012-06-06 12:59:47.841432226 +1000
+++ gdb/gdb/breakpoint.c	2012-06-06 12:59:57.417592947 +1000
@@ -14394,11 +14394,10 @@ deprecated_remove_raw_breakpoint (struct
   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[NR_SINGLE_STEP_BREAKPOINTS];
+static struct gdbarch *single_step_gdbarch[NR_SINGLE_STEP_BREAKPOINTS];
 
 /* Create and insert a breakpoint for software single step.  */
 
@@ -14407,19 +14406,17 @@ insert_single_step_breakpoint (struct gd
 			       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 < NR_SINGLE_STEP_BREAKPOINTS; i++)
+    if (single_step_breakpoints[i] == NULL)
+        break;
+
+  gdb_assert (i < NR_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
@@ -14440,8 +14437,13 @@ insert_single_step_breakpoint (struct gd
 int
 single_step_breakpoints_inserted (void)
 {
-  return (single_step_breakpoints[0] != NULL
-          || single_step_breakpoints[1] != NULL);
+  int i;
+
+  for (i = 0; i < NR_SINGLE_STEP_BREAKPOINTS; i++)
+    if (single_step_breakpoints[i] != NULL)
+      return 1;
+
+  return 0;
 }
 
 /* Remove and delete any breakpoints used for software single step.  */
@@ -14449,22 +14451,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 < NR_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
@@ -14477,7 +14478,7 @@ cancel_single_step_breakpoints (void)
 {
   int i;
 
-  for (i = 0; i < 2; i++)
+  for (i = 0; i < NR_SINGLE_STEP_BREAKPOINTS; i++)
     if (single_step_breakpoints[i])
       {
 	xfree (single_step_breakpoints[i]);
@@ -14494,7 +14495,7 @@ detach_single_step_breakpoints (void)
 {
   int i;
 
-  for (i = 0; i < 2; i++)
+  for (i = 0; i < NR_SINGLE_STEP_BREAKPOINTS; i++)
     if (single_step_breakpoints[i])
       target_remove_breakpoint (single_step_gdbarch[i],
 				single_step_breakpoints[i]);
@@ -14509,7 +14510,7 @@ single_step_breakpoint_inserted_here_p (
 {
   int i;
 
-  for (i = 0; i < 2; i++)
+  for (i = 0; i < NR_SINGLE_STEP_BREAKPOINTS; i++)
     {
       struct bp_target_info *bp_tgt = single_step_breakpoints[i];
       if (bp_tgt
Index: gdb/gdb/breakpoint.h
===================================================================
--- gdb.orig/gdb/breakpoint.h	2012-06-06 12:59:47.829432024 +1000
+++ gdb/gdb/breakpoint.h	2012-06-06 12:59:57.417592947 +1000
@@ -1394,8 +1394,9 @@ extern int is_catchpoint (struct breakpo
    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.  */
+#define NR_SINGLE_STEP_BREAKPOINTS 4
+
 extern void insert_single_step_breakpoint (struct gdbarch *,
 					   struct address_space *, 
 					   CORE_ADDR);

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

[Joel Brobecker]

Anton,

Thanks for the patch. I am not a real specialist of the software
single-step implementation, even though I worked on it a really
long time ago. But your changes mostly make sense to me (and seem
to be relatively mechanical in nature).

> 2012-06-05  Anton Blanchard  <anton@samba.org>
> 
> 	* gdb/breakpoint.h: Define NR_SINGLE_STEP_BREAKPOINTS
> 	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
> 	than two breakpoints.
> 	* gdb/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

Mostly OK. I would like you to change the name of the macro to
MAX_SINGLE_STEP_BREAKPOINTS, though. Can you, please?

Other comments and questions inline.

> -  int bc_insn_count = 0; /* Conditional branch instruction count.  */

Thanks for deleting that variable that seems redundant with
last_breakpoint...

> +          if (last_breakpoint >= (NR_SINGLE_STEP_BREAKPOINTS-1))
> +            return 0; /* too many conditional branches found, fallback

Can you remove the extra parens which are useless in this case?
And binary operators should have a space before and after. Thus:

        if (last_breakpoint > MAX_SINGLE_STEP_BREAKPOINTS - 1)

> Index: gdb/gdb/breakpoint.h
[...]
> -/* Manage a software single step breakpoint (or two).  Insert may be
> -   called twice before remove is called.  */
> +/* Manage software single step breakpoints.  */
> +#define NR_SINGLE_STEP_BREAKPOINTS 4
> +

Just curious: Why did you remove the second sentence from the comment?
Is it no longer true? Maybe it is the "twice" that should be changed
into "multiple times"?

Thanks,
-- 
Joel

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

[Anton Blanchard]

Hi Joel,

Thanks for the review.

> Mostly OK. I would like you to change the name of the macro to
> MAX_SINGLE_STEP_BREAKPOINTS, though. Can you, please?

Makes sense, done.

> > +          if (last_breakpoint >= (NR_SINGLE_STEP_BREAKPOINTS-1))
> > +            return 0; /* too many conditional branches found,
> > fallback
> 
> Can you remove the extra parens which are useless in this case?
> And binary operators should have a space before and after.

Done.

> > Index: gdb/gdb/breakpoint.h
> [...]
> > -/* Manage a software single step breakpoint (or two).  Insert may
> > be
> > -   called twice before remove is called.  */
> > +/* Manage software single step breakpoints.  */
> > +#define NR_SINGLE_STEP_BREAKPOINTS 4
> > +
> 
> Just curious: Why did you remove the second sentence from the comment?
> Is it no longer true? Maybe it is the "twice" that should be changed
> into "multiple times"?

Agreed. Here is a new version.

Anton
--

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:       7f e0 30 a8     ldarx   r31,0,r6
c00000000003ac50:       7c 80 f8 79     andc.   r0,r4,r31
c00000000003ac54:       40 82 02 94     bne-    c00000000003aee8 <htab_wrong_access>
c00000000003ac58:       73 e0 08 00     andi.   r0,r31,2048
c00000000003ac5c:       40 82 01 b0     bne-    c00000000003ae0c <htab_bail_ok>
c00000000003ac60:       54 9e f6 30     rlwinm  r30,r4,30,24,24
c00000000003ac64:       7f de fb 78     or      r30,r30,r31
c00000000003ac68:       63 de 09 00     ori     r30,r30,2304
c00000000003ac6c:       67 de 10 00     oris    r30,r30,4096
c00000000003ac70:       7f c0 31 ad     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.

2012-06-05  Anton Blanchard  <anton@samba.org>

	* gdb/breakpoint.h: Define MAX_SINGLE_STEP_BREAKPOINTS
	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
	than two breakpoints.
	* gdb/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

Index: b/gdb/rs6000-tdep.c
===================================================================
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -1087,7 +1087,7 @@ ppc_deal_with_atomic_sequence (struct fr
   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);
@@ -1096,7 +1096,6 @@ ppc_deal_with_atomic_sequence (struct fr
   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
@@ -1110,24 +1109,20 @@ ppc_deal_with_atomic_sequence (struct fr
       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++;
         }
 
@@ -1146,18 +1141,29 @@ ppc_deal_with_atomic_sequence (struct fr
   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;
 }
Index: b/gdb/breakpoint.c
===================================================================
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -14741,11 +14741,10 @@ deprecated_remove_raw_breakpoint (struct
   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.  */
 
@@ -14754,19 +14753,17 @@ insert_single_step_breakpoint (struct gd
 			       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
@@ -14787,8 +14784,13 @@ insert_single_step_breakpoint (struct gd
 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.  */
@@ -14796,22 +14798,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
@@ -14824,7 +14825,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]);
@@ -14841,7 +14842,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]);
@@ -14856,7 +14857,7 @@ single_step_breakpoint_inserted_here_p (
 {
   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
Index: b/gdb/breakpoint.h
===================================================================
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -1408,8 +1408,10 @@ extern int is_catchpoint (struct breakpo
    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);

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

[Joel Brobecker]

> Here is a new version.
[...]
> 2012-06-05  Anton Blanchard  <anton@samba.org>
> 
> 	* gdb/breakpoint.h: Define MAX_SINGLE_STEP_BREAKPOINTS
> 	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
> 	than two breakpoints.
> 	* gdb/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

Thanks for sending the new version, and sorry for dropping the ball on
this one.

This version of the patch is OK.

> Index: b/gdb/rs6000-tdep.c
> ===================================================================
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -1087,7 +1087,7 @@ ppc_deal_with_atomic_sequence (struct fr
>    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);
> @@ -1096,7 +1096,6 @@ ppc_deal_with_atomic_sequence (struct fr
>    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
> @@ -1110,24 +1109,20 @@ ppc_deal_with_atomic_sequence (struct fr
>        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++;
>          }
>  
> @@ -1146,18 +1141,29 @@ ppc_deal_with_atomic_sequence (struct fr
>    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;
>  }
> Index: b/gdb/breakpoint.c
> ===================================================================
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -14741,11 +14741,10 @@ deprecated_remove_raw_breakpoint (struct
>    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.  */
>  
> @@ -14754,19 +14753,17 @@ insert_single_step_breakpoint (struct gd
>  			       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
> @@ -14787,8 +14784,13 @@ insert_single_step_breakpoint (struct gd
>  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.  */
> @@ -14796,22 +14798,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
> @@ -14824,7 +14825,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]);
> @@ -14841,7 +14842,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]);
> @@ -14856,7 +14857,7 @@ single_step_breakpoint_inserted_here_p (
>  {
>    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
> Index: b/gdb/breakpoint.h
> ===================================================================
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -1408,8 +1408,10 @@ extern int is_catchpoint (struct breakpo
>     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);

-- 
Joel

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

[Pedro Alves]

Hi Anton,

Just some notes on the ChangeLog formatting:

On 07/16/2012 07:33 AM, Anton Blanchard wrote:

> 2012-06-05  Anton Blanchard  <anton@samba.org>
> 
> 	* gdb/breakpoint.h: Define MAX_SINGLE_STEP_BREAKPOINTS
> 	* rs6000-tdep.c (ppc_deal_with_atomic_sequence): Allow for more
> 	than two breakpoints.
> 	* gdb/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

- Please add a period at the end of these sentences.
- Remove the gdb/ from the breakpoint.h|c entries.

I suggest:

2012-09-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.

-- 
Pedro Alves