Patch for gdb build on hppa hp-ux

Patch for gdb build on hppa hp-ux

[Steve Ellcey]

This patch fixes a build problem on hppa hp-ux.  This is the first of
two patches I have that allow me to build gdb on hppa hp-ux.  The build
has been broken (for me at least) for some time.  This is the only HP
specific change that was needed.  Tested by building gdb on hppa hp-ux.

OK to checkin?

Steve Ellcey
sje@cup.hp.com


2007-04-06  Steve Ellcey  <sje@cup.hp.com>

	* hpread.c (hpread_get_next_skip_over_anon_unions): Fix
	CHECK_TYPEDEF usage.

Index: hpread.c
===================================================================
RCS file: /cvs/src/src/gdb/hpread.c,v
retrieving revision 1.62
diff -p -u -r1.62 hpread.c
--- hpread.c	9 Jan 2007 17:58:51 -0000	1.62
+++ hpread.c	6 Apr 2007 21:47:21 -0000
@@ -6316,7 +6316,7 @@ hpread_get_next_skip_over_anon_unions (i
       /* Get type of item we're looking at now; recursively processes the types
          of these intermediate items we skip over, so they aren't lost. */
       anon_type = hpread_type_lookup ((*fieldp)->dfield.type, objfile);
-      anon_type = CHECK_TYPEDEF (anon_type);
+      CHECK_TYPEDEF (anon_type);
       bitoffset = (*fieldp)->dfield.bitoffset;
       name = VT (objfile) + (*fieldp)->dfield.name;
       /* First skip over one item to avoid stack death on recursion */

Re: Patch for gdb build on hppa hp-ux

[Daniel Jacobowitz]

On Fri, Apr 06, 2007 at 02:53:09PM -0700, Steve Ellcey wrote:
> This patch fixes a build problem on hppa hp-ux.  This is the first of
> two patches I have that allow me to build gdb on hppa hp-ux.  The build
> has been broken (for me at least) for some time.  This is the only HP
> specific change that was needed.  Tested by building gdb on hppa hp-ux.
> 
> OK to checkin?

OK, although this file will be going away shortly - I ran out of time
before ESC, but I'm going to finish removing the files we agreed on
this month.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Patch for gdb build on hppa hp-ux

[Daniel Jacobowitz]

On Fri, Apr 06, 2007 at 02:53:09PM -0700, Steve Ellcey wrote:
> This patch fixes a build problem on hppa hp-ux.  This is the first of
> two patches I have that allow me to build gdb on hppa hp-ux.  The build
> has been broken (for me at least) for some time.  This is the only HP
> specific change that was needed.  Tested by building gdb on hppa hp-ux.

Since you can build on HP/UX, could you test this patch for me?  It
removes support for HP aCC, both the custom C++ ABI (which has fingers
in a lot of other places too, but this is a start) and the custom
debug format.  It should not affect the testsuite when using gcc with
SOM.

Thanks in advance!

-- 
Daniel Jacobowitz
CodeSourcery

2007-04-08  Daniel Jacobowitz  <dan@codesourcery.com>

	* Makefile.in (SFILES): Remove hpacc-abi.c.
	(COMMON_OBS): Remove hpacc-abi.o.
	(ALLDEPFILES): Remove hpread.c and $(HPREAD_SOURCE).
	(hpacc-abi.o, hpread.o): Delete rules.
	* somread.c: Delete extern declarations from hpread.c.
	(som_symfile_read): Do not call do_pxdb or hpread_build_psymtabs.
	(som_symfile_finish): Do not call hpread_symfile_finish.
	(som_symfile_init): Do not call hpread_symfile_init.
	* config/pa/hppa64.mt (TDEPFILES): Remove hpread.o.
	* config/pa/hppahpux.mt (TDEPFILES): Likewise.
	* hpacc-abi.c, hpread.c: Deleted.

	* gdbint.texinfo (SOM): Correct location of the SOM reader.

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.890
diff -u -p -r1.890 Makefile.in
--- Makefile.in	30 Mar 2007 22:50:32 -0000	1.890
+++ Makefile.in	9 Apr 2007 02:09:23 -0000
@@ -538,7 +538,6 @@ SFILES = ada-exp.y ada-lang.c ada-typepr
 	frame-base.c \
 	frame-unwind.c \
 	gdbarch.c arch-utils.c gdbtypes.c gnu-v2-abi.c gnu-v3-abi.c \
-	hpacc-abi.c \
 	inf-loop.c \
 	infcall.c \
 	infcmd.c inflow.c infrun.c \
@@ -971,7 +970,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $
 	user-regs.o \
 	frame.o frame-unwind.o doublest.o \
 	frame-base.o \
-	gnu-v2-abi.o gnu-v3-abi.o hpacc-abi.o cp-abi.o cp-support.o \
+	gnu-v2-abi.o gnu-v3-abi.o cp-abi.o cp-support.o \
 	cp-namespace.o \
 	reggroups.o regset.o \
 	trad-frame.o \
@@ -1456,7 +1455,6 @@ ALLDEPFILES = \
 	hppa-tdep.c hppa-hpux-tdep.c hppa-hpux-nat.c \
 	hppa-linux-tdep.c hppa-linux-nat.c \
 	hppabsd-nat.c hppabsd-tdep.c \
-	hpread.c \
 	i386-tdep.c i386-linux-nat.c \
 	i386v4-nat.c i386-cygwin-tdep.c \
 	i386bsd-nat.c i386bsd-tdep.c i386fbsd-nat.c i386fbsd-tdep.c \
@@ -1485,7 +1483,7 @@ ALLDEPFILES = \
 	mips64obsd-nat.c mips64obsd-tdep.c \
 	nbsd-nat.c nbsd-tdep.c obsd-tdep.c \
 	solib-osf.c \
-	somread.c solib-som.c $(HPREAD_SOURCE) \
+	somread.c solib-som.c \
 	posix-hdep.c \
 	ppc-sysv-tdep.c ppc-linux-nat.c ppc-linux-tdep.c \
 	ppcnbsd-nat.c ppcnbsd-tdep.c \
@@ -2067,8 +2065,6 @@ go32-nat.o: go32-nat.c $(defs_h) $(infer
 h8300-tdep.o: h8300-tdep.c $(defs_h) $(value_h) $(arch_utils_h) $(regcache_h) \
 	$(gdbcore_h) $(objfiles_h) $(gdb_assert_h) $(dis_asm_h) \
 	 $(dwarf2_frame_h) $(frame_base_h) $(frame_unwind_h)
-hpacc-abi.o: hpacc-abi.c $(defs_h) $(value_h) $(gdb_regex_h) $(gdb_string_h) \
-	$(gdbtypes_h) $(gdbcore_h) $(cp_abi_h) $(gnu_v2_abi_h)
 hppabsd-nat.o: hppabsd-nat.c $(defs_h) $(inferior_h) $(regcache_h) \
 	$(target_h) $(hppa_tdep_h) $(inf_ptrace_h)
 hppabsd-tdep.o: hppabsd-tdep.c $(defs_h) $(arch_utils_h) $(symtab_h) \
@@ -2095,10 +2091,6 @@ hppa-tdep.o: hppa-tdep.c $(defs_h) $(bfd
 	$(symtab_h) $(dis_asm_h) $(trad_frame_h) $(frame_unwind_h) \
 	$(frame_base_h) $(gdbcore_h) $(gdbcmd_h) $(objfiles_h) \
 	$(hppa_tdep_h) $(gdbtypes_h)
-hpread.o: hpread.c $(defs_h) $(bfd_h) $(gdb_string_h) $(hp_symtab_h) \
-	$(syms_h) $(symtab_h) $(symfile_h) $(objfiles_h) $(buildsym_h) \
-	$(complaints_h) $(gdb_stabs_h) $(gdbtypes_h) $(demangle_h) \
-	$(solib_som_h) $(gdb_assert_h) $(hppa_tdep_h) $(gdb_string_h)
 hpux-thread.o: hpux-thread.c $(defs_h) $(gdbthread_h) $(target_h) \
 	$(inferior_h) $(regcache_h) $(gdb_stat_h) $(gdbcore_h) \
 	$(hppa_tdep_h)
Index: somread.c
===================================================================
RCS file: /cvs/src/src/gdb/somread.c,v
retrieving revision 1.32
diff -u -p -r1.32 somread.c
--- somread.c	9 Jan 2007 17:58:58 -0000	1.32
+++ somread.c	9 Apr 2007 02:09:23 -0000
@@ -40,16 +40,6 @@
 /* Prototypes for local functions.  */
 static int init_import_symbols (struct objfile *objfile);
 
-/* FIXME: These should really be in a common header somewhere */
-
-extern void hpread_build_psymtabs (struct objfile *, int);
-
-extern void hpread_symfile_finish (struct objfile *);
-
-extern void hpread_symfile_init (struct objfile *);
-
-extern void do_pxdb (bfd *);
-
 /*
 
    LOCAL FUNCTION
@@ -327,8 +317,6 @@ som_symfile_read (struct objfile *objfil
   bfd *abfd = objfile->obfd;
   struct cleanup *back_to;
 
-  do_pxdb (symfile_bfd_open (objfile->name));
-
   init_minimal_symbol_collection ();
   back_to = make_cleanup_discard_minimal_symbols ();
 
@@ -363,12 +351,6 @@ som_symfile_read (struct objfile *objfil
      This is emitted by gcc.  */
   stabsect_build_psymtabs (objfile, mainline,
 			   "$GDB_SYMBOLS$", "$GDB_STRINGS$", "$TEXT$");
-
-  /* Now read the native debug information. 
-     This builds the psymtab. This used to be done via a scan of
-     the DNTT, but is now done via the PXDB-built quick-lookup tables
-     together with a scan of the GNTT. See hp-psymtab-read.c. */
-  hpread_build_psymtabs (objfile, mainline);
 }
 
 /* Initialize anything that needs initializing when a completely new symbol
@@ -396,7 +378,6 @@ som_symfile_finish (struct objfile *objf
     {
       xfree (objfile->deprecated_sym_stab_info);
     }
-  hpread_symfile_finish (objfile);
 }
 
 /* SOM specific initialization routine for reading symbols.  */
@@ -408,7 +389,6 @@ som_symfile_init (struct objfile *objfil
      find this causes a significant slowdown in gdb then we could
      set it in the debug symbol readers only when necessary.  */
   objfile->flags |= OBJF_REORDERED;
-  hpread_symfile_init (objfile);
 }
 
 /* SOM specific parsing routine for section offsets.
Index: config/pa/hppa64.mt
===================================================================
RCS file: /cvs/src/src/gdb/config/pa/hppa64.mt,v
retrieving revision 1.8
diff -u -p -r1.8 hppa64.mt
--- config/pa/hppa64.mt	26 Mar 2006 08:18:17 -0000	1.8
+++ config/pa/hppa64.mt	9 Apr 2007 02:09:23 -0000
@@ -1,3 +1,3 @@
 # Target: HP PA-RISC 2.0 running HPUX 11.00 in wide mode
-TDEPFILES= hppa-tdep.o hppa-hpux-tdep.o solib-som.o solib-pa64.o somread.o hpread.o solib.o
+TDEPFILES= hppa-tdep.o hppa-hpux-tdep.o solib-som.o solib-pa64.o somread.o solib.o
 DEPRECATED_TM_FILE= tm-hppah.h
Index: config/pa/hppahpux.mt
===================================================================
RCS file: /cvs/src/src/gdb/config/pa/hppahpux.mt,v
retrieving revision 1.7
diff -u -p -r1.7 hppahpux.mt
--- config/pa/hppahpux.mt	13 Dec 2004 04:06:16 -0000	1.7
+++ config/pa/hppahpux.mt	9 Apr 2007 02:09:23 -0000
@@ -1,4 +1,4 @@
 # Target: HP PA-RISC running hpux
 MT_CFLAGS = -DPA_SOM_ONLY=1
-TDEPFILES= hppa-tdep.o hppa-hpux-tdep.o corelow.o somread.o hpread.o solib-som.o solib-pa64.o solib.o
+TDEPFILES= hppa-tdep.o hppa-hpux-tdep.o corelow.o somread.o solib-som.o solib-pa64.o solib.o
 DEPRECATED_TM_FILE= tm-hppah.h
Index: doc/gdbint.texinfo
===================================================================
RCS file: /cvs/src/src/gdb/doc/gdbint.texinfo,v
retrieving revision 1.257
diff -u -p -r1.257 gdbint.texinfo
--- doc/gdbint.texinfo	30 Mar 2007 17:21:48 -0000	1.257
+++ doc/gdbint.texinfo	9 Apr 2007 02:09:24 -0000
@@ -2112,7 +2112,7 @@ The basic ELF reader is in @file{elfread
 SOM is HP's object file and debug format (not to be confused with IBM's
 SOM, which is a cross-language ABI).
 
-The SOM reader is in @file{hpread.c}.
+The SOM reader is in @file{somread.c}.
 
 @section Debugging File Formats
 

Re: Patch for gdb build on hppa hp-ux

[Steve Ellcey]

Daniel Jacobowitz wrote:

> Since you can build on HP/UX, could you test this patch for me?  It
> removes support for HP aCC, both the custom C++ ABI (which has fingers
> in a lot of other places too, but this is a start) and the custom
> debug format.  It should not affect the testsuite when using gcc with
> SOM.
> 
> Thanks in advance!

Looks good.  After I added a local change to use -Wno-char-subscripts
when building gdb (patch to be submitted) I was able to build with your
patch.  I also ran the gdb testsuite and it went from 432 unexpected
errors down to 357 after adding your patch.  Most of the new passes are
in the gdb.cp section.

Steve Ellcey
sje@cup.hp.com

Re: Patch for gdb build on hppa hp-ux

[Daniel Jacobowitz]

On Mon, Apr 09, 2007 at 04:25:38PM -0700, Steve Ellcey wrote:
> Looks good.  After I added a local change to use -Wno-char-subscripts
> when building gdb (patch to be submitted) I was able to build with your
> patch.  I also ran the gdb testsuite and it went from 432 unexpected
> errors down to 357 after adding your patch.  Most of the new passes are
> in the gdb.cp section.

That's peculiar, but I'll accept it.  It must come from not calling
pxdb or trying to parse system libraries for debug information, since
the hpacc-abi.c code apparently has not been enabled in years; I went
trying to figure out how it was turned on today and lo and behold, it
never is.

I checked in the patch.  Steve, Eli, does this NEWS addition look
reasonable?  I believe it's accurate for what I've removed, but I am
not a user of HP-UX or aCC, so I'm guessing a little bit.

-- 
Daniel Jacobowitz
CodeSourcery

2007-04-10  Daniel Jacobowitz  <dan@codesourcery.com>

	* NEWS: Mention removal of HP aCC support.

Index: NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.220
diff -u -p -r1.220 NEWS
--- NEWS	31 Mar 2007 10:42:22 -0000	1.220
+++ NEWS	10 Apr 2007 12:04:27 -0000
@@ -127,6 +127,13 @@ DWARF 1 support
 	A debug information format.  The predecessor to DWARF 2 and 
 	DWARF 3, which are still supported.
 
+Support for the HP aCC compiler on HP-UX/PA-RISC
+
+	SOM-encapsulated symbolic debugging information, automatic
+	invocation of pxdb, and the aCC custom C++ ABI.  This does not
+	affect HP-UX for Itanic or GCC for HP-UX/PA-RISC.  Code compiled
+	with aCC can still be debugged on an assembly level.
+
 *** Changes in GDB 6.6
 
 * New targets

Re: Patch for gdb build on hppa hp-ux

[Daniel Jacobowitz]

On Tue, Apr 10, 2007 at 08:05:47AM -0400, Daniel Jacobowitz wrote:
> On Mon, Apr 09, 2007 at 04:25:38PM -0700, Steve Ellcey wrote:
> > Looks good.  After I added a local change to use -Wno-char-subscripts
> > when building gdb (patch to be submitted) I was able to build with your
> > patch.  I also ran the gdb testsuite and it went from 432 unexpected
> > errors down to 357 after adding your patch.  Most of the new passes are
> > in the gdb.cp section.
> 
> That's peculiar, but I'll accept it.  It must come from not calling
> pxdb or trying to parse system libraries for debug information, since
> the hpacc-abi.c code apparently has not been enabled in years; I went
> trying to figure out how it was turned on today and lo and behold, it
> never is.
> 
> I checked in the patch.  Steve, Eli, does this NEWS addition look
> reasonable?  I believe it's accurate for what I've removed, but I am
> not a user of HP-UX or aCC, so I'm guessing a little bit.

Sorry, meant to CC Eli on that last message obviously!

-- 
Daniel Jacobowitz
CodeSourcery

2007-04-10  Daniel Jacobowitz  <dan@codesourcery.com>

	* NEWS: Mention removal of HP aCC support.

Index: NEWS
===================================================================
RCS file: /cvs/src/src/gdb/NEWS,v
retrieving revision 1.220
diff -u -p -r1.220 NEWS
--- NEWS	31 Mar 2007 10:42:22 -0000	1.220
+++ NEWS	10 Apr 2007 12:04:27 -0000
@@ -127,6 +127,13 @@ DWARF 1 support
 	A debug information format.  The predecessor to DWARF 2 and 
 	DWARF 3, which are still supported.
 
+Support for the HP aCC compiler on HP-UX/PA-RISC
+
+	SOM-encapsulated symbolic debugging information, automatic
+	invocation of pxdb, and the aCC custom C++ ABI.  This does not
+	affect HP-UX for Itanic or GCC for HP-UX/PA-RISC.  Code compiled
+	with aCC can still be debugged on an assembly level.
+
 *** Changes in GDB 6.6
 
 * New targets

Re: Patch for gdb build on hppa hp-ux

[Steve Ellcey]

> +Support for the HP aCC compiler on HP-UX/PA-RISC
> +
> +	SOM-encapsulated symbolic debugging information, automatic
> +	invocation of pxdb, and the aCC custom C++ ABI.  This does not
> +	affect HP-UX for Itanic or GCC for HP-UX/PA-RISC.  Code compiled
> +	with aCC can still be debugged on an assembly level.
> +

I would prefer you say IA64 or Itanium and not Itanic.

Steve Ellcey
sje@cup.hp.com

Re: Patch for gdb build on hppa hp-ux

[Daniel Jacobowitz]

On Tue, Apr 10, 2007 at 10:32:07AM -0700, Steve Ellcey wrote:
> > +Support for the HP aCC compiler on HP-UX/PA-RISC
> > +
> > +	SOM-encapsulated symbolic debugging information, automatic
> > +	invocation of pxdb, and the aCC custom C++ ABI.  This does not
> > +	affect HP-UX for Itanic or GCC for HP-UX/PA-RISC.  Code compiled
> > +	with aCC can still be debugged on an assembly level.
> > +
> 
> I would prefer you say IA64 or Itanium and not Itanic.

Whoops, thank you.  I even looked it up and worked out that I should
say Itanium, and then forgot to fix it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Patch for gdb build on hppa hp-ux

[Eli Zaretskii]

> Date: Tue, 10 Apr 2007 08:05:47 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@gcc.gnu.org
> 
> I checked in the patch.  Steve, Eli, does this NEWS addition look
> reasonable?

Almost.

> +	SOM-encapsulated symbolic debugging information, automatic
> +	invocation of pxdb, and the aCC custom C++ ABI.

This is not a complete sentence.

>                                                          This does not
> +	affect HP-UX for Itanic or GCC for HP-UX/PA-RISC.
                         ^^^^^^
Itanium, I think.

Otherwise, okay.  Thanks.

Re: Patch for gdb build on hppa hp-ux

[Daniel Jacobowitz]

On Tue, Apr 10, 2007 at 10:03:27PM +0300, Eli Zaretskii wrote:
> > +	SOM-encapsulated symbolic debugging information, automatic
> > +	invocation of pxdb, and the aCC custom C++ ABI.
> 
> This is not a complete sentence.

This is a list of items and descriptions; the other items are:

target abug
target cpu32bug
target est
target rom68k

        Various m68k-only ROM monitors.

DWARF 1 support

        A debug information format.  The predecessor to DWARF 2 and
        DWARF 3, which are still supported.

Would you rather I used complete sentences throughout, or is that too
wordy?

I'm asking since I have a sizable patch deleting everything else
from the list of obsolete files, and that adds another dozen things to
this list.

-- 
Daniel Jacobowitz
CodeSourcery

Re: Patch for gdb build on hppa hp-ux

[Daniel Jacobowitz]

On Tue, Apr 10, 2007 at 04:22:13PM -0400, Daniel Jacobowitz wrote:
> On Tue, Apr 10, 2007 at 10:03:27PM +0300, Eli Zaretskii wrote:
> > > +	SOM-encapsulated symbolic debugging information, automatic
> > > +	invocation of pxdb, and the aCC custom C++ ABI.
> > 
> > This is not a complete sentence.

Since you approved similar descriptive items in the other deletion
patch, I checked this in with only the Itanium correction.  Please let
me know if you'd like me to change it.

-- 
Daniel Jacobowitz
CodeSourcery

[patch] "single step" atomic instruction sequences as a whole on PPC

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 229 bytes --]

Daniel,

Follows attached the ppc portion of the patch for single stepping of
atomic instruction sequences.

You did mention that it had some cosmetic issues, so i'd like to have
your thoughts and comments on that.

Thanks,
Luis

[-- Attachment #2: ppc-atomic-single-stepping.diff --]
[-- Type: text/x-patch, Size: 4466 bytes --]

2007-04-13  Luis Machado  <luisgpm@br.ibm.com>

	* rs6000-tdep.c: Defines masks for POWER instructions that set
	and use the reservation flag (LWARX,LDARX,STWCX,STDCX).
	* rs6000-tdep.c (deal_with_atomic_sequence): Handles single
	stepping through an atomic sequence of instructions.
	* rs6000-tdep.c (rs6000_software_single_step): Added a function
	call to check if we are stepping through an atomic sequence of 
	instructions.
	* rs6000-tdep.c (rs6000_gdbarch_init): Initializes a function to
	check for atomic instruction sequences while single stepping.

Index: gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/rs6000-tdep.c	2007-04-13 06:13:20.000000000 -0700
+++ gdb/rs6000-tdep.c	2007-04-13 08:52:43.000000000 -0700
@@ -719,6 +719,93 @@
     return little_breakpoint;
 }
 
+#define LWARX_MASK 0xfc0007fe
+#define LWARX_INSTRUCTION 0x7c000028
+#define LDARX_INSTRUCTION 0x7c0000A8
+#define STWCX_MASK 0xfc0007ff
+#define STWCX_INSTRUCTION 0x7c00012d
+#define STDCX_INSTRUCTION 0x7c0001ad
+#define BC_MASK 0xfc000000
+#define BC_INSTRUCTION 0x40000000
+#define IMMEDIATE_PART(insn)  (((insn & ~3) << 16) >> 16)
+#define ABSOLUTE_P(insn) ((int) ((insn >> 1) & 1))
+
+static int 
+deal_with_atomic_sequence (enum target_signal sig, int insert_breakpoints_p)
+{
+  CORE_ADDR pc = read_pc ();
+  CORE_ADDR breaks[2] = {-1, -1};
+  CORE_ADDR loc = pc;
+  int insn = read_memory_integer (loc, PPC_INSN_SIZE);
+  int last_break = 0;
+  int i;
+
+  if (insert_breakpoints_p)
+    {
+        
+  /* Assume all atomic sequences start with an lwarx or ldarx instruction. */
+  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
+   && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
+     return 0;
+
+  /* Assume that no atomic sequence is longer than 6 instructions. */
+  for (i = 1; i < 5; ++i)
+    {
+      loc += PPC_INSN_SIZE;
+      insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+      /* Assume at most one conditional branch instruction between
+ 	  the lwarx and stwcx instructions.*/
+      if ((insn & BC_MASK) == BC_INSTRUCTION)
+	    {
+	    last_break = 1;
+	    breaks[1] = IMMEDIATE_PART (insn);
+	    if ( ! ABSOLUTE_P(insn))
+	      breaks[1] += loc;
+	      continue;
+	    }
+
+        if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
+         || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
+	      break;
+    }
+
+  /* Assume that the atomic sequence ends with a stwcx instruction
+       followed by a conditional branch instruction. */
+  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
+   && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
+    warning (_("Tried to step over an atomic sequence of instructions at %s\n \
+                but could not find the end of the sequence."),
+            core_addr_to_string(pc));
+
+  loc += PPC_INSN_SIZE;
+  insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+  if ((insn & BC_MASK) != BC_INSTRUCTION)
+    warning (_("Tried to step over an atomic sequence of instructions at %s\n \
+                but the instruction sequence ended in an unexpected way."),
+            core_addr_to_string(pc));
+
+  breaks[0] = loc;
+
+  /* This should never happen, but make sure we don't put
+     two breakpoints on the same address. */
+  if (last_break && breaks[1] == breaks[0])
+    last_break = 0;
+
+  for (i = 0; i <= last_break; ++i)
+    insert_single_step_breakpoint (breaks[i]);
+
+  printf_unfiltered (_("Stepping over an atomic sequence of instructions beginning at %s\n"),
+		     core_addr_to_string (pc));
+
+  gdb_flush (gdb_stdout);
+    }
+  else
+      remove_single_step_breakpoints();
+
+  return 1;
+}
 
 /* AIX does not support PT_STEP. Simulate it. */
 
@@ -740,6 +827,9 @@
 
       insn = read_memory_integer (loc, 4);
 
+      if (deal_with_atomic_sequence (signal, insert_breakpoints_p))
+        return 1;
+      
       breaks[0] = loc + breakp_sz;
       opcode = insn >> 26;
       breaks[1] = branch_dest (opcode, insn, loc, breaks[0]);
@@ -3467,6 +3557,9 @@
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_breakpoint_from_pc (gdbarch, rs6000_breakpoint_from_pc);
 
+  /* Watch for locking instruction sequences during single stepping */
+  set_gdbarch_software_single_step(gdbarch, deal_with_atomic_sequence);
+  
   /* Handle the 64-bit SVR4 minimal-symbol convention of using "FN"
      for the descriptor and ".FN" for the entry-point -- a user
      specifying "break FN" will unexpectedly end up with a breakpoint

[RFC] "single step" atomic instruction sequences as a whole on PPC

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 291 bytes --]

Daniel,

Follows attached the ppc portion of the patch for single stepping of
atomic instruction sequences.

You did mention that it had some cosmetic issues, so i'd like to have
thoughts and comments on that.

I'm re-submitting this one as it didn't make its way to the list.

Thanks,
Luis

[-- Attachment #2: ppc-atomic-single-stepping.diff --]
[-- Type: text/x-patch, Size: 4509 bytes --]

2007-04-13  Paul Gilliam  <pgilliam@us.ibm.com>
						Luis Machado  <luisgpm@br.ibm.com>

	* rs6000-tdep.c: Defines masks for POWER instructions that set
	and use the reservation flag (LWARX,LDARX,STWCX,STDCX).
	* rs6000-tdep.c (deal_with_atomic_sequence): Handles single
	stepping through an atomic sequence of instructions.
	* rs6000-tdep.c (rs6000_software_single_step): Added a function
	call to check if we are stepping through an atomic sequence of 
	instructions.
	* rs6000-tdep.c (rs6000_gdbarch_init): Initializes a function to
	check for atomic instruction sequences while single stepping.

Index: gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/rs6000-tdep.c	2007-04-28 16:22:11.000000000 -0700
+++ gdb/rs6000-tdep.c	2007-04-28 16:25:17.000000000 -0700
@@ -719,6 +719,93 @@
     return little_breakpoint;
 }
 
+#define LWARX_MASK 0xfc0007fe
+#define LWARX_INSTRUCTION 0x7c000028
+#define LDARX_INSTRUCTION 0x7c0000A8
+#define STWCX_MASK 0xfc0007ff
+#define STWCX_INSTRUCTION 0x7c00012d
+#define STDCX_INSTRUCTION 0x7c0001ad
+#define BC_MASK 0xfc000000
+#define BC_INSTRUCTION 0x40000000
+#define IMMEDIATE_PART(insn)  (((insn & ~3) << 16) >> 16)
+#define ABSOLUTE_P(insn) ((int) ((insn >> 1) & 1))
+
+static int 
+deal_with_atomic_sequence (enum target_signal sig, int insert_breakpoints_p)
+{
+  CORE_ADDR pc = read_pc ();
+  CORE_ADDR breaks[2] = {-1, -1};
+  CORE_ADDR loc = pc;
+  int insn = read_memory_integer (loc, PPC_INSN_SIZE);
+  int last_break = 0;
+  int i;
+
+  if (insert_breakpoints_p)
+    {
+        
+  /* Assume all atomic sequences start with an lwarx or ldarx instruction. */
+  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
+   && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
+     return 0;
+
+  /* Assume that no atomic sequence is longer than 6 instructions. */
+  for (i = 1; i < 5; ++i)
+    {
+      loc += PPC_INSN_SIZE;
+      insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+      /* Assume at most one conditional branch instruction between
+ 	  the lwarx and stwcx instructions.*/
+      if ((insn & BC_MASK) == BC_INSTRUCTION)
+	    {
+	    last_break = 1;
+	    breaks[1] = IMMEDIATE_PART (insn);
+	    if ( ! ABSOLUTE_P(insn))
+	      breaks[1] += loc;
+	      continue;
+	    }
+
+        if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
+         || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
+	      break;
+    }
+
+  /* Assume that the atomic sequence ends with a stwcx instruction
+       followed by a conditional branch instruction. */
+  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
+   && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
+    warning (_("Tried to step over an atomic sequence of instructions at %s\n \
+                but could not find the end of the sequence."),
+            core_addr_to_string(pc));
+
+  loc += PPC_INSN_SIZE;
+  insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+  if ((insn & BC_MASK) != BC_INSTRUCTION)
+    warning (_("Tried to step over an atomic sequence of instructions at %s\n \
+                but the instruction sequence ended in an unexpected way."),
+            core_addr_to_string(pc));
+
+  breaks[0] = loc;
+
+  /* This should never happen, but make sure we don't put
+     two breakpoints on the same address. */
+  if (last_break && breaks[1] == breaks[0])
+    last_break = 0;
+
+  for (i = 0; i <= last_break; ++i)
+    insert_single_step_breakpoint (breaks[i]);
+
+  printf_unfiltered (_("Stepping over an atomic sequence of instructions beginning at %s\n"),
+		     core_addr_to_string (pc));
+
+  gdb_flush (gdb_stdout);
+    }
+  else
+      remove_single_step_breakpoints();
+
+  return 1;
+}
 
 /* AIX does not support PT_STEP. Simulate it. */
 
@@ -737,7 +824,10 @@
 
   insn = read_memory_integer (loc, 4);
 
-  breaks[0] = loc + breakp_sz;
+	if (deal_with_atomic_sequence (signal, insert_breakpoints_p))
+		return 1;
+
+	breaks[0] = loc + breakp_sz;
   opcode = insn >> 26;
   breaks[1] = branch_dest (opcode, insn, loc, breaks[0]);
 
@@ -3461,6 +3551,9 @@
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_breakpoint_from_pc (gdbarch, rs6000_breakpoint_from_pc);
 
+  /* Watch for locking instruction sequences during single stepping */
+  set_gdbarch_software_single_step(gdbarch, deal_with_atomic_sequence);
+  
   /* Handle the 64-bit SVR4 minimal-symbol convention of using "FN"
      for the descriptor and ".FN" for the entry-point -- a user
      specifying "break FN" will unexpectedly end up with a breakpoint

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Ulrich Weigand]

Luis Machado wrote:

> +static int 
> +deal_with_atomic_sequence (enum target_signal sig, int insert_breakpoints_p)

This doesn't actually work on mainline any more, the software_single_step
API was changed:
http://sourceware.org/ml/gdb-patches/2007-04/msg00218.html

Please update your patch accordingly.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Luis Machado]

Oops. Sorry about that Ulrich. I'll re-work it based on your
modification for the single stepping routines.

Luis

On Sun, 2007-04-29 at 01:36 +0200, Ulrich Weigand wrote:
> Luis Machado wrote:
> 
> > +static int 
> > +deal_with_atomic_sequence (enum target_signal sig, int insert_breakpoints_p)
> 
> This doesn't actually work on mainline any more, the software_single_step
> API was changed:
> http://sourceware.org/ml/gdb-patches/2007-04/msg00218.html
> 
> Please update your patch accordingly.
> 
> Bye,
> Ulrich
> 

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Ulrich Weigand]

Emi-san,

> I tried this one and am sure it was solved.  Thanks!
> I hope this will solve Jan's issue also.  

Thanks for testing!  I've committed this version now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Emi SUZUKI]

Ulrich, 

I tried this one and am sure it was solved.  Thanks!
I hope this will solve Jan's issue also.  

From: Ulrich Weigand <uweigand at de.ibm.com>
Subject: Re: [RFC] "single step" atomic instruction sequences as a whole on PPC
Date: Fri, 11 May 2007 00:58:28 +0200 (CEST)

> Daniel Jacobowitz wrote:
> > Maybe it would be more elegant to anticipate the day that single step
> > breakpoints are completely normal: make breakpoint_inserted_here_p
> > check them?  This is already the only caller.
> 
> Good point.  Something like the patch below?  I've also added the
> check to software_breakpoint_inserted_here_p; that seemed to make
> more sense that not.

-- 
Emi SUZUKI / emi-suzuki at tjsys.co.jp

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Daniel Jacobowitz]

On Fri, May 11, 2007 at 12:58:28AM +0200, Ulrich Weigand wrote:
> Daniel Jacobowitz wrote:
> > On Thu, May 10, 2007 at 11:30:53PM +0200, Ulrich Weigand wrote:
> > > This looks just like a problem we fixed for the combined debugger;
> > > cancel_breakpoints_callback should cancel SIGTRAP events caused by
> > > software single-step breakpoints just the same as those caused by
> > > other breakpoints.
> > 
> > Maybe it would be more elegant to anticipate the day that single step
> > breakpoints are completely normal: make breakpoint_inserted_here_p
> > check them?  This is already the only caller.
> 
> Good point.  Something like the patch below?  I've also added the
> check to software_breakpoint_inserted_here_p; that seemed to make
> more sense that not.
> 
> (I'm suspicious about adjust_pc_after_break, the only caller of
> software_breakpoint_inserted_here_p anyway; I think this may no 
> longer be correct after the software single-step changes ...)

Yes, while I was checking for callers of breakpoint_inserted_here_p
I noticed that.  There's no way this is correct any more; it only
works because PowerPC has DECR_PC_AFTER_BREAK == 0 and all other
targets have a software_single_step routine which always returns
the same value.

I think your patch is OK.  I suspect that it means the
singlestep_breakpoints_inserted_p check in adjust_pc_after_break is
now obsolete; what do you think?  That lets the function simplify a
bit further.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Ulrich Weigand]

Daniel Jacobowitz wrote:
> On Thu, May 10, 2007 at 11:30:53PM +0200, Ulrich Weigand wrote:
> > This looks just like a problem we fixed for the combined debugger;
> > cancel_breakpoints_callback should cancel SIGTRAP events caused by
> > software single-step breakpoints just the same as those caused by
> > other breakpoints.
> 
> Maybe it would be more elegant to anticipate the day that single step
> breakpoints are completely normal: make breakpoint_inserted_here_p
> check them?  This is already the only caller.

Good point.  Something like the patch below?  I've also added the
check to software_breakpoint_inserted_here_p; that seemed to make
more sense that not.

(I'm suspicious about adjust_pc_after_break, the only caller of
software_breakpoint_inserted_here_p anyway; I think this may no 
longer be correct after the software single-step changes ...)

Bye,
Ulrich


ChangeLog:

	* breakpoint.c (single_step_breakpoint_inserted_here_p): New function.
	(breakpoint_inserted_here_p): Call it.
	(software_breakpoint_inserted_here_p): Likewise.


diff -urNp gdb-orig/gdb/breakpoint.c gdb-head/gdb/breakpoint.c
--- gdb-orig/gdb/breakpoint.c	2007-05-11 00:15:25.160082872 +0200
+++ gdb-head/gdb/breakpoint.c	2007-05-11 00:22:39.846579732 +0200
@@ -202,6 +202,8 @@ static void tcatch_command (char *arg, i
 
 static void ep_skip_leading_whitespace (char **s);
 
+static int single_step_breakpoint_inserted_here_p (CORE_ADDR pc);
+
 /* Prototypes for exported functions. */
 
 /* If FALSE, gdb will not use hardware support for watchpoints, even
@@ -1841,6 +1843,10 @@ breakpoint_inserted_here_p (CORE_ADDR pc
 	}
     }
 
+  /* Also check for software single-step breakpoints.  */
+  if (single_step_breakpoint_inserted_here_p (pc))
+    return 1;
+
   return 0;
 }
 
@@ -1872,6 +1878,10 @@ software_breakpoint_inserted_here_p (COR
 	}
     }
 
+  /* Also check for software single-step breakpoints.  */
+  if (single_step_breakpoint_inserted_here_p (pc))
+    return 1;
+
   return 0;
 }
 
@@ -7951,6 +7961,23 @@ remove_single_step_breakpoints (void)
     }
 }
 
+/* Check whether a software single-step breakpoint is inserted at PC.  */
+
+static int
+single_step_breakpoint_inserted_here_p (CORE_ADDR pc)
+{
+  int i;
+
+  for (i = 0; i < 2; i++)
+    {
+      struct bp_target_info *bp_tgt = single_step_breakpoints[i];
+      if (bp_tgt && bp_tgt->placed_address == pc)
+	return 1;
+    }
+
+  return 0;
+}
+
 \f
 /* This help string is used for the break, hbreak, tbreak and thbreak commands.
    It is defined as a macro to prevent duplication.

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Daniel Jacobowitz]

On Thu, May 10, 2007 at 11:30:53PM +0200, Ulrich Weigand wrote:
> This looks just like a problem we fixed for the combined debugger;
> cancel_breakpoints_callback should cancel SIGTRAP events caused by
> software single-step breakpoints just the same as those caused by
> other breakpoints.

Maybe it would be more elegant to anticipate the day that single step
breakpoints are completely normal: make breakpoint_inserted_here_p
check them?  This is already the only caller.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Ulrich Weigand]

Hello Emi-san,

> When that multiple SIGTRAP events occured, GDB selects one event and
> cancels the other if the cause of SIGTRAP is a breakpoint hit, or just
> leave it pended.  The procedure will be done by `cancel_breakpoints_callback' 
> in linux-nat.c.  And the pended events will be deteced and noticed
> when the next time the target resumes.  
> 
> The probrem is that GDB doesn't check if the breakpoint is inserted
> for software single stepping when cancelling the trap event: when the
> event occured by a software single step breakpoint is not selected,
> GDB would not cancel it but leave it pended.  

This is cancel_breakpoints_callback in linux-nat.c, right?
 
> When the next time the target resumes, GDB restores the pended event.
> But if you have removed the watchpoint that the target get stopped by
> before resuming, GDB can never decide the cause of SIGTRAP anymore.
> The session log above shows the phenomenon.  

This looks just like a problem we fixed for the combined debugger;
cancel_breakpoints_callback should cancel SIGTRAP events caused by
software single-step breakpoints just the same as those caused by
other breakpoints.

Can you try whether the following patch fixes the problem for you?

Bye,
Ulrich


ChangeLog:

	* breakpoint.c (single_step_breakpoint_inserted_here_p): New function.
	* breakpoint.h (single_step_breakpoint_inserted_here_p): New prototype.
	* linux-nat.c (cancel_breakpoints_callback): Treat software single-
	step breakpoints the same way as regular software breakpoints.

diff -urNp gdb-orig/gdb/breakpoint.c gdb-head/gdb/breakpoint.c
--- gdb-orig/gdb/breakpoint.c	2007-05-08 14:37:32.000000000 +0200
+++ gdb-head/gdb/breakpoint.c	2007-05-10 22:34:36.776868666 +0200
@@ -7951,6 +7951,21 @@ remove_single_step_breakpoints (void)
     }
 }
 
+int
+single_step_breakpoint_inserted_here_p (CORE_ADDR pc)
+{
+  int i;
+
+  for (i = 0; i < 2; i++)
+    {
+      struct bp_target_info *bp_tgt = single_step_breakpoints[i];
+      if (bp_tgt && bp_tgt->placed_address == pc)
+	return 1;
+    }
+
+  return 0;
+}
+
 \f
 /* This help string is used for the break, hbreak, tbreak and thbreak commands.
    It is defined as a macro to prevent duplication.
diff -urNp gdb-orig/gdb/breakpoint.h gdb-head/gdb/breakpoint.h
--- gdb-orig/gdb/breakpoint.h	2007-04-13 19:13:45.000000000 +0200
+++ gdb-head/gdb/breakpoint.h	2007-05-10 22:34:36.829861036 +0200
@@ -832,6 +832,8 @@ extern int remove_hw_watchpoints (void);
    twice before remove is called.  */
 extern void insert_single_step_breakpoint (CORE_ADDR);
 extern void remove_single_step_breakpoints (void);
+extern int single_step_breakpoint_inserted_here_p (CORE_ADDR pc);
+
 
 /* Manage manual breakpoints, separate from the normal chain of
    breakpoints.  These functions are used in murky target-specific
diff -urNp gdb-orig/gdb/linux-nat.c gdb-head/gdb/linux-nat.c
--- gdb-orig/gdb/linux-nat.c	2007-05-07 02:18:12.000000000 +0200
+++ gdb-head/gdb/linux-nat.c	2007-05-10 22:34:36.860856573 +0200
@@ -1765,21 +1765,26 @@ cancel_breakpoints_callback (struct lwp_
      tripped on it.  */
 
   if (lp->status != 0
-      && WIFSTOPPED (lp->status) && WSTOPSIG (lp->status) == SIGTRAP
-      && breakpoint_inserted_here_p (read_pc_pid (lp->ptid) -
-				     DECR_PC_AFTER_BREAK))
+      && WIFSTOPPED (lp->status) && WSTOPSIG (lp->status) == SIGTRAP)
     {
-      if (debug_linux_nat)
-	fprintf_unfiltered (gdb_stdlog,
-			    "CBC: Push back breakpoint for %s\n",
-			    target_pid_to_str (lp->ptid));
+      CORE_ADDR break_pc;
+      break_pc = read_pc_pid (lp->ptid) - DECR_PC_AFTER_BREAK;
 
-      /* Back up the PC if necessary.  */
-      if (DECR_PC_AFTER_BREAK)
-	write_pc_pid (read_pc_pid (lp->ptid) - DECR_PC_AFTER_BREAK, lp->ptid);
+      if (breakpoint_inserted_here_p (break_pc)
+	  || single_step_breakpoint_inserted_here_p (break_pc))
+	{
+	  if (debug_linux_nat)
+	    fprintf_unfiltered (gdb_stdlog,
+				"CBC: Push back breakpoint for %s\n",
+				target_pid_to_str (lp->ptid));
 
-      /* Throw away the SIGTRAP.  */
-      lp->status = 0;
+	  /* Back up the PC if necessary.  */
+	  if (DECR_PC_AFTER_BREAK)
+	    write_pc_pid (break_pc, lp->ptid);
+
+	  /* Throw away the SIGTRAP.  */
+	  lp->status = 0;
+	}
     }
 
   return 0;


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Ulrich Weigand]

> 2007-05-09  Luis Machado  <luisgpm@br.ibm.com>
> 
>   * rs6000-tdep.c: (deal_with_atomic_sequence) Stores branch instruction's
>   opcode in the "opcode" variable and declares new variable "closing_insn".

I've committed this now.

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Emi SUZUKI]

[-- Attachment #1: Type: Text/Plain, Size: 4662 bytes --]

Hello Jan,

From: Jan Kratochvil <jan.kratochvil at redhat.com>
Subject: Re: [RFC] "single step" atomic instruction sequences as a whole on PPC
Date: Wed, 09 May 2007 20:33:19 +0200

> please check the attached two testcases and run them at least 100x etc.
> 
> Unfortunately the threaded one fails for me in some 7% of cases IMO due to
> a race at the `infrun.c' line:
> 	remove_status = remove_breakpoints ();

I'm not sure, but I guess the issue you may see on your testcase
resembles to the session log below: 

-- 
GNU gdb 6.6.50.20070510-cvs
Copyright (C) 2007 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "powerpc64-linux"...
Using host libthread_db library "/lib64/libthread_db.so.1".
(gdb) tb 161
Breakpoint 1 at 0x10001230: file gdb/testsuite/gdb.threads/atomic-seq-threaded.c, line 161.
(gdb) r
Starting program: gdb/testsuite/gdb.threads/atomic-seq-threaded
[Thread debugging using libthread_db enabled]
[New Thread 4160631792 (LWP 11609)]
[New Thread 4160627904 (LWP 11612)]
[Switching to Thread 4160631792 (LWP 11609)]
main (argc=1, argv=0xffe92484)
    at gdb/testsuite/gdb.threads/atomic-seq-threaded.c:161
161       assert (i == 0);                       /* _create_behind_ */
(gdb) b 167
Breakpoint 2 at 0x1000126c: file gdb/testsuite/gdb.threads/atomic-seq-threaded.c, line 167.
(gdb) b 151
Breakpoint 3 at 0x100011d0: file gdb/testsuite/gdb.threads/atomic-seq-threaded.c, line 151.
(gdb) b 133
Breakpoint 4 at 0x10001120: file gdb/testsuite/gdb.threads/atomic-seq-threaded.c, line 133.
(gdb) set can-use-hw-watchpoints 0
(gdb) watch unused
Watchpoint 5: unused
(gdb) c
Continuing.
[Switching to Thread 4160627904 (LWP 11612)]

Breakpoint 4, start1 (arg=0x0)
    at gdb/testsuite/gdb.threads/atomic-seq-threaded.c:133
133       return arg;                            /* _delete1_ */
(gdb) d
Delete all breakpoints? (y or n) y
(gdb) c
Continuing.

Program received signal SIGTRAP, Trace/breakpoint trap.
[Switching to Thread 4160631792 (LWP 11609)]
0x0ff3b748 in __libc_enable_asynccancel () from /lib/libc.so.6
(gdb) disassemble 0x0ff3b734 0x0ff3b750
Dump of assembler code from 0xff3b734 to 0xff3b750:
0x0ff3b734 <__libc_enable_asynccancel+52>:	addi    r9,r11,100
0x0ff3b738 <__libc_enable_asynccancel+56>:	lwarx   r10,0,r9
0x0ff3b73c <__libc_enable_asynccancel+60>:	cmpw    r10,r3
0x0ff3b740 <__libc_enable_asynccancel+64>:	bne-    0xff3b74c <__libc_enable_asynccancel+76>
0x0ff3b744 <__libc_enable_asynccancel+68>:	stwcx.  r0,0,r9
0x0ff3b748 <__libc_enable_asynccancel+72>:	bne-    0xff3b738 <__libc_enable_asynccancel+56>
0x0ff3b74c <__libc_enable_asynccancel+76>:	isync
End of assembler dump.
-- 

If it is, the line in `infrun.c' you have pointed is not the cause.
But whether it is or not, I think there is an issue for handling
multiple trap events while doing software single stepping.  

In the testcase, GDB always does hardware single stepping by setting a
software watchpoint.  And when a thread running through an atomic
sequence of instruction, it will be done by software one.  So, during
one thread is running through an atomic sequence of instructions, GDB
can detects multiple SIGTRAP events on the target process, as you have
already known.  

When that multiple SIGTRAP events occured, GDB selects one event and
cancels the other if the cause of SIGTRAP is a breakpoint hit, or just
leave it pended.  The procedure will be done by `cancel_breakpoints_callback' 
in linux-nat.c.  And the pended events will be deteced and noticed
when the next time the target resumes.  

The probrem is that GDB doesn't check if the breakpoint is inserted
for software single stepping when cancelling the trap event: when the
event occured by a software single step breakpoint is not selected,
GDB would not cancel it but leave it pended.  

When the next time the target resumes, GDB restores the pended event.
But if you have removed the watchpoint that the target get stopped by
before resuming, GDB can never decide the cause of SIGTRAP anymore.
The session log above shows the phenomenon.  

I attach the source code modified from yours, Jan, which is much
easier to reproduce the issue on my environment.  And I'm so sorry for
not sending any patch for resolving it, for my copyright assignment to
FSF is STILL NOT ready.  


And Luis, it's just FYI: 'Emi' is one of common female names in 
Japan :-)

-- 
Emi SUZUKI / emi-suzuki at tjsys.co.jp


[-- Attachment #2: atomic-seq-threaded.c --]
[-- Type: Text/Plain, Size: 4294 bytes --]

/* This testcase is part of GDB, the GNU debugger.

   Copyright 2007 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 2 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, write to the Free Software
   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
   MA 02110-1301, USA.  */

/* Test stepping over RISC atomic sequences.
   This variant testcases the code for stepping another thread while skipping
   over the atomic sequence in the former thread
   (STEPPING_PAST_SINGLESTEP_BREAKPOINT).
   Code comes from gcc/testsuite/gcc.dg/sync-2.c  */

/* { dg-options "-march=i486" { target { { i?86-*-* x86_64-*-* } && ilp32 } } } */
/* { dg-options "-mcpu=v9" { target sparc*-*-* } } */

/* Test functionality of the intrinsics for 'short' and 'char'.  */

#include <stdlib.h>
#include <string.h>
#include <pthread.h>
#include <assert.h>
#include <unistd.h>

#define LOOPS 2

static int unused;

static char AI[18];
static char init_qi[18] = { 3,5,7,9,0,0,0,0,-1,0,0,0,0,0,-1,0,0,0 };
static char test_qi[18] = { 3,5,7,9,1,4,22,-12,7,8,9,7,1,-12,7,8,9,7 };

static void
do_qi (void)
{
  if (__sync_fetch_and_add(AI+4, 1) != 0)
    abort ();
  if (__sync_fetch_and_add(AI+5, 4) != 0)
    abort ();
  if (__sync_fetch_and_add(AI+6, 22) != 0)
    abort ();
  if (__sync_fetch_and_sub(AI+7, 12) != 0)
    abort ();
  if (__sync_fetch_and_and(AI+8, 7) != (char)-1)
    abort ();
  if (__sync_fetch_and_or(AI+9, 8) != 0)
    abort ();
  if (__sync_fetch_and_xor(AI+10, 9) != 0)
    abort ();
  if (__sync_fetch_and_nand(AI+11, 7) != 0)
    abort ();

  if (__sync_add_and_fetch(AI+12, 1) != 1)
    abort ();
  if (__sync_sub_and_fetch(AI+13, 12) != (char)-12)
    abort ();
  if (__sync_and_and_fetch(AI+14, 7) != 7)
    abort ();
  if (__sync_or_and_fetch(AI+15, 8) != 8)
    abort ();
  if (__sync_xor_and_fetch(AI+16, 9) != 9)
    abort ();
  if (__sync_nand_and_fetch(AI+17, 7) != 7)
    abort ();
}

static short AL[18];
static short init_hi[18] = { 3,5,7,9,0,0,0,0,-1,0,0,0,0,0,-1,0,0,0 };
static short test_hi[18] = { 3,5,7,9,1,4,22,-12,7,8,9,7,1,-12,7,8,9,7 };

static void
do_hi (void)
{
  if (__sync_fetch_and_add(AL+4, 1) != 0)
    abort ();
  if (__sync_fetch_and_add(AL+5, 4) != 0)
    abort ();
  if (__sync_fetch_and_add(AL+6, 22) != 0)
    abort ();
  if (__sync_fetch_and_sub(AL+7, 12) != 0)
    abort ();
  if (__sync_fetch_and_and(AL+8, 7) != -1)
    abort ();
  if (__sync_fetch_and_or(AL+9, 8) != 0)
    abort ();
  if (__sync_fetch_and_xor(AL+10, 9) != 0)
    abort ();
  if (__sync_fetch_and_nand(AL+11, 7) != 0)
    abort ();

  if (__sync_add_and_fetch(AL+12, 1) != 1)
    abort ();
  if (__sync_sub_and_fetch(AL+13, 12) != -12)
    abort ();
  if (__sync_and_and_fetch(AL+14, 7) != 7)
    abort ();
  if (__sync_or_and_fetch(AL+15, 8) != 8)
    abort ();
  if (__sync_xor_and_fetch(AL+16, 9) != 9)
    abort ();
  if (__sync_nand_and_fetch(AL+17, 7) != 7)
    abort ();
}

static void *
start1 (void *arg)
{
  unsigned loop;
  sleep(1);

  for (loop = 0; loop < LOOPS; loop++)
    {
      memcpy(AI, init_qi, sizeof(init_qi));

      do_qi ();

      if (memcmp (AI, test_qi, sizeof(test_qi)))
	abort ();
    }

  return arg;						/* _delete1_ */
}

static void *
start2 (void *arg)
{
  unsigned loop;

  for (loop = 0; loop < LOOPS; loop++)
    {
      memcpy(AL, init_hi, sizeof(init_hi));

      do_hi ();

      if (memcmp (AL, test_hi, sizeof(test_hi)))
	abort ();
    }

  return arg;						/* _delete2_ */
}

int
main (int argc, char **argv)
{
  pthread_t thread;
  int i;

  i = pthread_create (&thread, NULL, start1, NULL);	/* _create_ */
  assert (i == 0);					/* _create_behind_ */

  sleep(1);

  start2 (NULL);

  i = pthread_join (thread, NULL);			/* _delete_ */
  assert (i == 0);

  return 0;						/* _success_ */
}

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

> I'd just do "opcode = insn >> 26" same as in rs6000_software_single_step.
> (In fact I'm wondering why branch_dest doesn't just that for itself ...).

Follows the updated patch. The "opcode" variable is now being assigned
the correct instruction's opcode value (only the corresponding bits).

As a consequence of this change, i've noticed problems with branch
instructions next to the stwcx/stdcx instructions (the end of the
sequence). Getting the destination address of this type of branch could
potentially (upon a failing branch condition) lead to the function
placing a breakpoint right at the stwcx/stdcx instruction, thus leading
us back to the same locking problem.

The variable "closing_insn" was created to check if the breakpoint at
the branch instruction's destination is right at the stwcx/stdcx
instruction. If so, ignore this breakpoint and consider only the
breakpoint after the closing of the atomic sequence.

Regards,
Luis

[-- Attachment #2: fix_opcode_var.diff --]
[-- Type: text/x-patch, Size: 2322 bytes --]

2007-05-09  Luis Machado  <luisgpm@br.ibm.com>

  * rs6000-tdep.c: (deal_with_atomic_sequence) Stores branch instruction's
  opcode in the "opcode" variable and declares new variable "closing_insn".
 

Index: gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/rs6000-tdep.c	2007-05-09 12:19:29.000000000 -0700
+++ gdb/rs6000-tdep.c	2007-05-09 17:36:13.000000000 -0700
@@ -729,12 +729,13 @@
   CORE_ADDR breaks[2] = {-1, -1};
   CORE_ADDR loc = pc;
   CORE_ADDR branch_bp; /* Breakpoint at branch instruction's destination.  */
+  CORE_ADDR closing_insn; /* Instruction that closes the atomic sequence.  */
   int insn = read_memory_integer (loc, PPC_INSN_SIZE);
   int insn_count;
   int index;
   int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */  
   const int atomic_sequence_length = 16; /* Instruction sequence length.  */
-  const int opcode = BC_INSTRUCTION; /* Branch instruction's OPcode.  */
+  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.  */
@@ -758,6 +759,7 @@
             return 0; /* More than one conditional branch found, fallback 
                          to the standard single-step code.  */
           
+          opcode = insn >> 26;
           branch_bp = branch_dest (opcode, insn, pc, breaks[0]);
           
           if (branch_bp != -1)
@@ -778,14 +780,19 @@
       && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
     return 0;
 
+  closing_insn = loc;
   loc += PPC_INSN_SIZE;
   insn = read_memory_integer (loc, PPC_INSN_SIZE);
 
   /* Insert a breakpoint right after the end of the atomic sequence.  */
   breaks[0] = loc;
 
-  /* Check for duplicated breakpoints.  */
-  if (last_breakpoint && (breaks[1] == breaks[0]))
+  /* Check for duplicated breakpoints.  Check also for a breakpoint
+     placed (branch instruction's destination) at the stwcx/stdcx 
+     instruction, this resets the reservation and take us back to the 
+     lwarx/ldarx instruction at the beginning of the atomic sequence.  */
+  if (last_breakpoint && ((breaks[1] == breaks[0]) 
+      || (breaks[1] == closing_insn)))
     last_breakpoint = 0;
 
   /* Effectively inserts the breakpoints.  */

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Ulrich Weigand]

Luis Machado wrote:

> About the instruction bits problem. Should i just literally assign a
> decimal number to the variable (16 in this case) or is it preferred to
> do a shift based on the hex value?

I'd just do "opcode = insn >> 26" same as in rs6000_software_single_step.
(In fact I'm wondering why branch_dest doesn't just that for itself ...).

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Luis Machado]

> Hi Luis,
> 
> please check the attached two testcases and run them at least 100x etc.
> 
> Unfortunately the threaded one fails for me in some 7% of cases IMO due to
> a race at the `infrun.c' line:
> 	remove_status = remove_breakpoints ();
> 
> The whole idea of running all the threads of the program to step over the
> atomic sequence is problematic as the other threads may hit the inserted
> breakpoint.  While this cases is handled it contains a race.
> 
> One way would be to use some temporary:
> 	if (scheduler_mode == schedlock_off)
> 	  scheduler_mode = schedlock_step;
> 
> But I believe one could use the PPC simulation code instead of the whole
> breakpoint/resume way?
> 
> 
> Regards,
> Jan

You had a fix for reducing the failures from 99% to 93%, right? Is this
available on HEAD?

Regards,
Luis

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Ulrich Weigand]

Daniel Jacobowitz wrote:

> I hadn't thought about this case, of trying to step over a touchy
> atomic sequence; I don't see any way we could make it work if we let
> more than one thread run at once.

However, in particular when atomic sequences are involved, the
current thread might be trying to acquire a lock held by another
thread.  If we keep running just the one thread, it'll never
get the lock ...

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Daniel Jacobowitz]

On Wed, May 09, 2007 at 08:33:19PM +0200, Jan Kratochvil wrote:
> Hi Luis,
> 
> please check the attached two testcases and run them at least 100x etc.
> 
> Unfortunately the threaded one fails for me in some 7% of cases IMO due to
> a race at the `infrun.c' line:
> 	remove_status = remove_breakpoints ();
> 
> The whole idea of running all the threads of the program to step over the
> atomic sequence is problematic as the other threads may hit the inserted
> breakpoint.  While this cases is handled it contains a race.
> 
> One way would be to use some temporary:
> 	if (scheduler_mode == schedlock_off)
> 	  scheduler_mode = schedlock_step;
> 
> But I believe one could use the PPC simulation code instead of the whole
> breakpoint/resume way?

This has come up before; I think we need to come up with a better
default behavior for "step" that does not do this.  Here's the last
discussion I remember:
  http://sourceware.org/ml/gdb/2006-11/msg00211.html

Should I try again?

I hadn't thought about this case, of trying to step over a touchy
atomic sequence; I don't see any way we could make it work if we let
more than one thread run at once.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 1241 bytes --]

Hi Luis,

please check the attached two testcases and run them at least 100x etc.

Unfortunately the threaded one fails for me in some 7% of cases IMO due to
a race at the `infrun.c' line:
	remove_status = remove_breakpoints ();

The whole idea of running all the threads of the program to step over the
atomic sequence is problematic as the other threads may hit the inserted
breakpoint.  While this cases is handled it contains a race.

One way would be to use some temporary:
	if (scheduler_mode == schedlock_off)
	  scheduler_mode = schedlock_step;

But I believe one could use the PPC simulation code instead of the whole
breakpoint/resume way?


Regards,
Jan


On Wed, 09 May 2007 20:21:15 +0200, Luis Machado wrote:
> Daniel,
> 
> > Does Ulrich's observation imply that there is no test case for this
> > neat feature?  I would recommend one in gdb.arch (and maybe a NEWS
> > entry too).
> 
> I could work on providing a test case for this feature to be included on
> the GDB testsuite, as soon as i get familiar with the syntax.
> 
> About the instruction bits problem. Should i just literally assign a
> decimal number to the variable (16 in this case) or is it preferred to
> do a shift based on the hex value?
> 
> Thanks!
> Luis

[-- Attachment #2: atomic-seq.tar.gz --]
[-- Type: application/x-gzip, Size: 2768 bytes --]

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Luis Machado]

Daniel,

> Does Ulrich's observation imply that there is no test case for this
> neat feature?  I would recommend one in gdb.arch (and maybe a NEWS
> entry too).

I could work on providing a test case for this feature to be included on
the GDB testsuite, as soon as i get familiar with the syntax.

About the instruction bits problem. Should i just literally assign a
decimal number to the variable (16 in this case) or is it preferred to
do a shift based on the hex value?

Thanks!
Luis

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Daniel Jacobowitz]

On Wed, May 09, 2007 at 03:05:26PM -0300, Luis Machado wrote:
> > Could you implement and test a patch to fix this, please?
> 
> Sure, no problem. I will update the patch, test and re-subtmit when it's
> ready.

Does Ulrich's observation imply that there is no test case for this
neat feature?  I would recommend one in gdb.arch (and maybe a NEWS
entry too).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Luis Machado]

> Actually, I think there's still a problem with the patch.  Sorry for
> not noticing earlier.  You call:
> 
> > +#define BC_INSTRUCTION 0x40000000
> > +  const int opcode = BC_INSTRUCTION; /* Branch instruction's OPcode.  */
> > +          branch_bp = branch_dest (opcode, insn, pc, breaks[0]);
> 
> branch_dest with 0x40000000 as opcode -- however, it appears the
> routine expects to be called with just the opcode bits, i.e. 16
> in the case of a conditional branch.
> 
> See the call in rs6000_software_single_step:
> >    opcode = insn >> 26;
> >    breaks[1] = branch_dest (opcode, insn, loc, breaks[0]);
> 
> Could you implement and test a patch to fix this, please?

Sure, no problem. I will update the patch, test and re-subtmit when it's
ready.

Regards,
Luis

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Ulrich Weigand]

Luis Machado wrote:

Actually, I think there's still a problem with the patch.  Sorry for
not noticing earlier.  You call:

> +#define BC_INSTRUCTION 0x40000000
> +  const int opcode = BC_INSTRUCTION; /* Branch instruction's OPcode.  */
> +          branch_bp = branch_dest (opcode, insn, pc, breaks[0]);

branch_dest with 0x40000000 as opcode -- however, it appears the
routine expects to be called with just the opcode bits, i.e. 16
in the case of a conditional branch.

See the call in rs6000_software_single_step:
>    opcode = insn >> 26;
>    breaks[1] = branch_dest (opcode, insn, loc, breaks[0]);

Could you implement and test a patch to fix this, please?

Thanks,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Ulrich Weigand]

Luis Machado wrote:

> Follows the refreshed version with a revamped Changelog and additional
> comments explaining what the function does. How does it look?

OK, thanks!

> 2007-05-07  Paul Gilliam  <pgilliam@us.ibm.com>
>             Luis Machado  <luisgpm@br.ibm.com>
> 
>   * rs6000-tdep.c: (LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION,
>   STWCX_MASK, STWCX_INSTRUCTION, STDCX_INSTRUCTION, BC_MASK,
>   BC_INSTRUCTION): Define.
>   (deal_with_atomic_sequence): New function.
>   (rs6000_software_single_step): Call deal_with_atomic_sequence.
>   (rs6000_gdbarch_init): Install deal_with_atomic_sequence as
>   gdbarch_software_single_step routine.

I've committed this version now.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 162 bytes --]

Ulrich,

Follows the refreshed version with a revamped Changelog and additional
comments explaining what the function does. How does it look?

Best regards.
Luis

[-- Attachment #2: ppc-atomic-single-stepping.diff --]
[-- Type: text/x-patch, Size: 4812 bytes --]

2007-05-07  Paul Gilliam  <pgilliam@us.ibm.com>
            Luis Machado  <luisgpm@br.ibm.com>

  * rs6000-tdep.c: (LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION,
  STWCX_MASK, STWCX_INSTRUCTION, STDCX_INSTRUCTION, BC_MASK,
  BC_INSTRUCTION): Define.
  (deal_with_atomic_sequence): New function.
  (rs6000_software_single_step): Call deal_with_atomic_sequence.
  (rs6000_gdbarch_init): Install deal_with_atomic_sequence as
  gdbarch_software_single_step routine.

Index: gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/rs6000-tdep.c	2007-05-07 07:20:39.000000000 -0700
+++ gdb/rs6000-tdep.c	2007-05-07 16:16:03.000000000 -0700
@@ -707,7 +707,95 @@
 }
 
 
-/* AIX does not support PT_STEP. Simulate it. */
+/* Instruction masks used during single-stepping of atomic sequences.  */
+#define LWARX_MASK 0xfc0007fe
+#define LWARX_INSTRUCTION 0x7c000028
+#define LDARX_INSTRUCTION 0x7c0000A8
+#define STWCX_MASK 0xfc0007ff
+#define STWCX_INSTRUCTION 0x7c00012d
+#define STDCX_INSTRUCTION 0x7c0001ad
+#define BC_MASK 0xfc000000
+#define BC_INSTRUCTION 0x40000000
+
+/* Checks for an atomic sequence of instructions beginning with a LWARX/LDARX
+   instruction and ending with a STWCX/STDCX instruction.  If such a sequence
+   is found, attempt to step through it.  A breakpoint is placed at the end of 
+   the sequence.  */
+
+static int 
+deal_with_atomic_sequence (struct regcache *regcache)
+{
+  CORE_ADDR pc = read_pc ();
+  CORE_ADDR breaks[2] = {-1, -1};
+  CORE_ADDR loc = pc;
+  CORE_ADDR branch_bp; /* Breakpoint at branch instruction's destination.  */
+  int insn = read_memory_integer (loc, PPC_INSN_SIZE);
+  int insn_count;
+  int index;
+  int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */  
+  const int atomic_sequence_length = 16; /* Instruction sequence length.  */
+  const int opcode = BC_INSTRUCTION; /* 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
+      && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
+    return 0;
+
+  /* Assume that no atomic sequence is longer than "atomic_sequence_length" 
+     instructions.  */
+  for (insn_count = 0; insn_count < atomic_sequence_length; ++insn_count)
+    {
+      loc += PPC_INSN_SIZE;
+      insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+      /* 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 & BC_MASK) == BC_INSTRUCTION)
+        {
+          if (bc_insn_count >= 1)
+            return 0; /* More than one conditional branch found, fallback 
+                         to the standard single-step code.  */
+          
+          branch_bp = branch_dest (opcode, insn, pc, breaks[0]);
+          
+          if (branch_bp != -1)
+            {
+              breaks[1] = branch_bp;
+              bc_insn_count++;
+              last_breakpoint++;
+            }
+        }
+
+      if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
+          || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
+        break;
+    }
+
+  /* Assume that the atomic sequence ends with a stwcx/stdcx instruction.  */
+  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
+      && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
+    return 0;
+
+  loc += PPC_INSN_SIZE;
+  insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+  /* Insert a breakpoint right after the end of the atomic sequence.  */
+  breaks[0] = loc;
+
+  /* Check for duplicated breakpoints.  */
+  if (last_breakpoint && (breaks[1] == breaks[0]))
+    last_breakpoint = 0;
+
+  /* Effectively inserts the breakpoints.  */
+  for (index = 0; index <= last_breakpoint; index++)
+    insert_single_step_breakpoint (breaks[index]);
+
+  return 1;
+}
+
+/* AIX does not support PT_STEP.  Simulate it.  */
 
 int
 rs6000_software_single_step (struct regcache *regcache)
@@ -724,6 +812,9 @@
 
   insn = read_memory_integer (loc, 4);
 
+  if (deal_with_atomic_sequence (regcache))
+    return 1;
+  
   breaks[0] = loc + breakp_sz;
   opcode = insn >> 26;
   breaks[1] = branch_dest (opcode, insn, loc, breaks[0]);
@@ -3448,6 +3539,9 @@
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_breakpoint_from_pc (gdbarch, rs6000_breakpoint_from_pc);
 
+  /* Handles single stepping of atomic sequences.  */
+  set_gdbarch_software_single_step (gdbarch, deal_with_atomic_sequence);
+  
   /* Handle the 64-bit SVR4 minimal-symbol convention of using "FN"
      for the descriptor and ".FN" for the entry-point -- a user
      specifying "break FN" will unexpectedly end up with a breakpoint

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Ulrich Weigand]

Luis Machado wrote:

> > It's much better, thanks.  Unfortunately there's still a number of
> > issues -- thanks for you patience in dealing with those.
> 
> Looks like i missed some. I'm getting used to the standard, however.
> Thanks for reviewing.

Looks good now, thanks.

> > I guess that's OK with me.  I'm wondering why we need the one message
> > that's still in there then -- I'd say either we warn whenever we find
> > a sequence we don't understand, or we never warn.
> 
> In order to make it more transparent to the user, i removed the messages
> during the process.

Fine with me.

Just one more thing:  the ChangeLog isn't quite right.  Sorry for not
noticing that earlier ...

> 2007-04-13  Paul Gilliam  <pgilliam@us.ibm.com>
>             Luis Machado  <luisgpm@br.ibm.com>
> 
> 	* rs6000-tdep.c: Defines masks for POWER instructions that set
> 	and use the reservation flag (LWARX,LDARX,STWCX,STDCX).
You need to mention those flags explicitly.

> 	* rs6000-tdep.c (deal_with_atomic_sequence): Handles single
> 	stepping through an atomic sequence of instructions.
> 	* rs6000-tdep.c (rs6000_software_single_step): Added a function
> 	call to check if we are stepping through an atomic sequence of 
> 	instructions.
> 	* rs6000-tdep.c (rs6000_gdbarch_init): Initializes a function to
> 	check for atomic instruction sequences while single stepping.

Do not describe *why* you're changing things, only *what* you're
changing.  For example, in this ChangeLog

	(deal_with_atomic_sequence): New function.

is sufficient.  To describe *why* you need this function, and what it
does, you should add a comment to the source file -- that's where 
people will look for this information.

Also, if you have multiple changes to the same file, you need to
list the file name only once.

I'd write a ChangeLog for this patch somewhat like:

	* rs6000-tdep.c (LWARX_MASK, LWARX_INSTRUCTION, LDARX_INSTRUCTION,
	STWCX_MASK, STWCX_INSTRUCTION, STDCX_INSTRUCTION, BC_MASK,
	BC_INSTRUCTION): Define.
	(deal_with_atomic_sequence): New function.
	(rs6000_software_single_step): Call deal_with_atomic_sequence.
	(rs6000_gdbarch_init): Install deal_with_atomic_sequence as
	gdbarch_software_single_step routine.


Can you add a comment to the patch before the new function that explains
what it does and why it is needed?  Thanks!

> +  CORE_ADDR branch_bp; /* Breakpoint at branch instruction's destination.  */
> +  int insn = read_memory_integer (loc, PPC_INSN_SIZE);
> +  int insn_count;
> +  int index; /* Index used for the "breaks" array.  */
> +  int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */  
> +  const int atomic_sequence_length = 16; /* Instruction sequence length.  */
> +  const int opcode = BC_INSTRUCTION; /* Branch instruction's OPcode.  */
> +  int bc_insn_count = 0; /* Conditional branch instruction count.  */

In exchange, you might want to get rid of those comments -- that a variable
called "index" is used as index doesn't need to be pointed out as comment ...

With the new comment and ChangeLog, the patch is OK.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 540 bytes --]

Ulrich,

> It's much better, thanks.  Unfortunately there's still a number of
> issues -- thanks for you patience in dealing with those.

Looks like i missed some. I'm getting used to the standard, however.
Thanks for reviewing.

> I guess that's OK with me.  I'm wondering why we need the one message
> that's still in there then -- I'd say either we warn whenever we find
> a sequence we don't understand, or we never warn.

In order to make it more transparent to the user, i removed the messages
during the process.

Best Regards,
Luis

[-- Attachment #2: ppc-atomic-single-stepping.diff --]
[-- Type: text/x-patch, Size: 4686 bytes --]

2007-04-13  Paul Gilliam  <pgilliam@us.ibm.com>
            Luis Machado  <luisgpm@br.ibm.com>

	* rs6000-tdep.c: Defines masks for POWER instructions that set
	and use the reservation flag (LWARX,LDARX,STWCX,STDCX).
	* rs6000-tdep.c (deal_with_atomic_sequence): Handles single
	stepping through an atomic sequence of instructions.
	* rs6000-tdep.c (rs6000_software_single_step): Added a function
	call to check if we are stepping through an atomic sequence of 
	instructions.
	* rs6000-tdep.c (rs6000_gdbarch_init): Initializes a function to
	check for atomic instruction sequences while single stepping.

Index: gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/rs6000-tdep.c	2007-05-07 07:20:39.000000000 -0700
+++ gdb/rs6000-tdep.c	2007-05-07 12:27:10.000000000 -0700
@@ -706,8 +706,89 @@
     return little_breakpoint;
 }
 
+#define LWARX_MASK 0xfc0007fe
+#define LWARX_INSTRUCTION 0x7c000028
+#define LDARX_INSTRUCTION 0x7c0000A8
+#define STWCX_MASK 0xfc0007ff
+#define STWCX_INSTRUCTION 0x7c00012d
+#define STDCX_INSTRUCTION 0x7c0001ad
+#define BC_MASK 0xfc000000
+#define BC_INSTRUCTION 0x40000000
+
+static int 
+deal_with_atomic_sequence (struct regcache *regcache)
+{
+  CORE_ADDR pc = read_pc ();
+  CORE_ADDR breaks[2] = {-1, -1};
+  CORE_ADDR loc = pc;
+  CORE_ADDR branch_bp; /* Breakpoint at branch instruction's destination.  */
+  int insn = read_memory_integer (loc, PPC_INSN_SIZE);
+  int insn_count;
+  int index; /* Index used for the "breaks" array.  */
+  int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */  
+  const int atomic_sequence_length = 16; /* Instruction sequence length.  */
+  const int opcode = BC_INSTRUCTION; /* Branch instruction's OPcode.  */
+  int bc_insn_count = 0; /* Conditional branch instruction count.  */
+
+  /* Assume all atomic sequences start with an lwarx/ldarx instruction.  */
+  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
+      && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
+    return 0;
+
+  /* Assume that no atomic sequence is longer than "atomic_sequence_length" 
+     instructions.  */
+  for (insn_count = 0; insn_count < atomic_sequence_length; ++insn_count)
+    {
+      loc += PPC_INSN_SIZE;
+      insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+      /* 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 & BC_MASK) == BC_INSTRUCTION)
+        {
+          if (bc_insn_count >= 1)
+            return 0; /* More than one conditional branch found, fallback 
+                         to the standard single-step code.  */
+          
+          branch_bp = branch_dest (opcode, insn, pc, breaks[0]);
+          
+          if (branch_bp != -1)
+            {
+              breaks[1] = branch_bp;
+              bc_insn_count++;
+              last_breakpoint++;
+            }
+        }
+
+      if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
+          || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
+        break;
+    }
+
+  /* Assume that the atomic sequence ends with a stwcx/stdcx instruction.  */
+  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
+      && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
+    return 0;
+
+  loc += PPC_INSN_SIZE;
+  insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+  /* Insert a breakpoint right after the end of the atomic sequence.  */
+  breaks[0] = loc;
+
+  /* Check for duplicated breakpoints.  */
+  if (last_breakpoint && (breaks[1] == breaks[0]))
+    last_breakpoint = 0;
+
+  /* Effectively inserts the breakpoints.  */
+  for (index = 0; index <= last_breakpoint; index++)
+    insert_single_step_breakpoint (breaks[index]);
+
+  return 1;
+}
 
-/* AIX does not support PT_STEP. Simulate it. */
+/* AIX does not support PT_STEP.  Simulate it.  */
 
 int
 rs6000_software_single_step (struct regcache *regcache)
@@ -724,6 +805,9 @@
 
   insn = read_memory_integer (loc, 4);
 
+  if (deal_with_atomic_sequence (regcache))
+    return 1;
+  
   breaks[0] = loc + breakp_sz;
   opcode = insn >> 26;
   breaks[1] = branch_dest (opcode, insn, loc, breaks[0]);
@@ -3448,6 +3532,9 @@
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_breakpoint_from_pc (gdbarch, rs6000_breakpoint_from_pc);
 
+  /* Handles single stepping of atomic sequences.  */
+  set_gdbarch_software_single_step (gdbarch, deal_with_atomic_sequence);
+  
   /* Handle the 64-bit SVR4 minimal-symbol convention of using "FN"
      for the descriptor and ".FN" for the entry-point -- a user
      specifying "break FN" will unexpectedly end up with a breakpoint

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Ulrich Weigand]

Luis Machado wrote:

> Thanks for the comments. I'm still getting used to the details of the
> coding standard.
> 
> The patch has been updated. I hope i've addressed all the coding format
> issues this time.

It's much better, thanks.  Unfortunately there's still a number of
issues -- thanks for you patience in dealing with those.

> The handling of multiple conditional branches was modified so that when
> GDB finds such a situation it just silently falls back to the standard
> way of doing single-step. That way we won't have too much text on the
> screen in case this situation happens often.

I guess that's OK with me.  I'm wondering why we need the one message
that's still in there then -- I'd say either we warn whenever we find
a sequence we don't understand, or we never warn.

> +  /* Assume all atomic sequences start with an lwarx or ldarx instruction.  */
> +  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
> +      && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
> +      return 0;
"return" is now indented two spaces too much.

> +  /* Assume that no atomic sequence is longer than "atomic_sequence_length 
Missing closing "

> +      /* Assume that there is at most one conditional branch in the atomic
> +         sequence. If a conditional branch is found, put a breakpoint in 
Two spaces after '.'

> +      warning (_("Tried to step over an atomic sequence of instructions at %s\n \
> +                  but could not find the end of the sequence."),
If we keep the warning, we need to remove the extra spaces at the beginning of
the second line.  See e.g. _initialize_printcmd for examples of long messages.

> +/* AIX does not support PT_STEP. Simulate it.  */
Two spaces not just after the last '.', but after each of them.

> +  /* Handles single stepping of atomic sequences */
Comment should end in '.'.


Otherwise, this looks technically OK to me.  Once the coding style issues
and the warning question are resolved, I think it can be committed.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

Ulrich,
> Thanks for the modifications.  There's still a number of coding style
> issues that I'll point out below.

Thanks for the comments. I'm still getting used to the details of the
coding standard.

The patch has been updated. I hope i've addressed all the coding format
issues this time.

The handling of multiple conditional branches was modified so that when
GDB finds such a situation it just silently falls back to the standard
way of doing single-step. That way we won't have too much text on the
screen in case this situation happens often.

Best Regards,
Luis

[-- Attachment #2: ppc-atomic-single-stepping.diff --]
[-- Type: text/x-patch, Size: 4849 bytes --]

2007-04-13  Luis Machado  <luisgpm@br.ibm.com>

	* rs6000-tdep.c: Defines masks for POWER instructions that set
	and use the reservation flag (LWARX,LDARX,STWCX,STDCX).
	* rs6000-tdep.c (deal_with_atomic_sequence): Handles single
	stepping through an atomic sequence of instructions.
	* rs6000-tdep.c (rs6000_software_single_step): Added a function
	call to check if we are stepping through an atomic sequence of 
	instructions.
	* rs6000-tdep.c (rs6000_gdbarch_init): Initializes a function to
	check for atomic instruction sequences while single stepping.

Index: gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/rs6000-tdep.c	2007-05-07 05:00:37.000000000 -0700
+++ gdb/rs6000-tdep.c	2007-05-07 06:57:27.000000000 -0700
@@ -706,8 +706,94 @@
     return little_breakpoint;
 }
 
+#define LWARX_MASK 0xfc0007fe
+#define LWARX_INSTRUCTION 0x7c000028
+#define LDARX_INSTRUCTION 0x7c0000A8
+#define STWCX_MASK 0xfc0007ff
+#define STWCX_INSTRUCTION 0x7c00012d
+#define STDCX_INSTRUCTION 0x7c0001ad
+#define BC_MASK 0xfc000000
+#define BC_INSTRUCTION 0x40000000
+
+static int 
+deal_with_atomic_sequence (struct regcache *regcache)
+{
+  CORE_ADDR pc = read_pc ();
+  CORE_ADDR breaks[2] = {-1, -1};
+  CORE_ADDR loc = pc;
+  CORE_ADDR branch_bp; /* Breakpoint at branch instruction's destination.  */
+  int insn = read_memory_integer (loc, PPC_INSN_SIZE);
+  int insn_count;
+  int index; /* Index used for the "breaks" array.  */
+  int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed).  */  
+  const int atomic_sequence_length = 16; /* Instruction sequence length.  */
+  const int opcode = BC_INSTRUCTION; /* Branch instruction's OPcode.  */
+  int bc_insn_count = 0; /* Conditional branch instruction count.  */
+
+  /* Assume all atomic sequences start with an lwarx or ldarx instruction.  */
+  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
+      && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
+      return 0;
+
+  /* Assume that no atomic sequence is longer than "atomic_sequence_length 
+     instructions.  */
+  for (insn_count = 0; insn_count < atomic_sequence_length; ++insn_count)
+    {
+      loc += PPC_INSN_SIZE;
+      insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+      /* 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 & BC_MASK) == BC_INSTRUCTION)
+        {
+          if (bc_insn_count >= 1)
+            return 0; /* More than one conditional branch found, fallback 
+                         to the standard single-step code.  */
+          
+          branch_bp = branch_dest (opcode, insn, pc, breaks[0]);
+          
+          if (branch_bp != -1)
+            {
+              breaks[1] = branch_bp;
+              bc_insn_count++;
+              last_breakpoint++;
+            }
+        }
+
+      if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
+          || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
+        break;
+    }
+
+  /* Assume that the atomic sequence ends with a stwcx/stdcx instruction.  */
+  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
+      && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
+    {
+      warning (_("Tried to step over an atomic sequence of instructions at %s\n \
+                  but could not find the end of the sequence."),
+                  core_addr_to_string (pc));
+      return 0;
+    }
 
-/* AIX does not support PT_STEP. Simulate it. */
+  loc += PPC_INSN_SIZE;
+  insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+  /* Insert a breakpoint right after the end of the atomic sequence.  */
+  breaks[0] = loc;
+
+  /* Check for duplicated breakpoints.  */
+  if (last_breakpoint && (breaks[1] == breaks[0]))
+    last_breakpoint = 0;
+
+  /* Effectively inserts the breakpoints.  */
+  for (index = 0; index <= last_breakpoint; index++)
+    insert_single_step_breakpoint (breaks[index]);
+
+  return 1;
+}
+
+/* AIX does not support PT_STEP. Simulate it.  */
 
 int
 rs6000_software_single_step (struct regcache *regcache)
@@ -724,6 +810,9 @@
 
   insn = read_memory_integer (loc, 4);
 
+  if (deal_with_atomic_sequence (regcache))
+    return 1;
+  
   breaks[0] = loc + breakp_sz;
   opcode = insn >> 26;
   breaks[1] = branch_dest (opcode, insn, loc, breaks[0]);
@@ -3448,6 +3537,9 @@
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_breakpoint_from_pc (gdbarch, rs6000_breakpoint_from_pc);
 
+  /* Handles single stepping of atomic sequences */
+  set_gdbarch_software_single_step (gdbarch, deal_with_atomic_sequence);
+  
   /* Handle the 64-bit SVR4 minimal-symbol convention of using "FN"
      for the descriptor and ".FN" for the entry-point -- a user
      specifying "break FN" will unexpectedly end up with a breakpoint

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Ulrich Weigand]

Luis Machado wrote:

> I've modified the patch according to your suggestions. Any additional
> comments on it?

Thanks for the modifications.  There's still a number of coding style
issues that I'll point out below.

> Any suggestions for the duplicate breakpoint detection? Maybe it could
> be handled in a better way.

Unfortunately your attempt to handle more than a single conditional
branch within the sequence doesn't work, as insert_single_step_breakpoint
cannot be called more than twice (it'll raise an assertion).

I think you should make the assumption that there will be at most
one conditional branch, but fail gracefully it that assumption is
in fact violated (e.g. by issuing a warning and returning 0 to fall
back on the standard single-step code).

> +static int 
> +deal_with_atomic_sequence(struct regcache *regcache)
Space before '('.

> +{
> +  CORE_ADDR pc = read_pc ();
> +  CORE_ADDR breaks[16] = {-1, -1, -1, -1, -1, -1, -1, -1,
> +                          -1, -1, -1, -1, -1, -1, -1, -1};
> +  CORE_ADDR branch_bp; /* Breakpoint at destination of a banch instruction */
Typo: "banch"
Comment should end with '.' followed by two spaces.

> +  int index; /* Index used for the "breaks" arrays */
> +  int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed) */  
> +  const int opcode = BC_INSTRUCTION; /* Branch instruction's OPcode */
Likewise.

> +  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
> +   && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
Indentation wrong.

> +      /* Check for conditiconal branches in the middle of the sequence
Typo "conditiconal"

> +          branch_bp = branch_dest(opcode, insn, pc, breaks[0]);
Space before '('.

> +      if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
> +       || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
Indentation.

> +  /* Assume that the atomic sequence ends with a stwcx/stdcx instruction */
Comment should end with full stop.

> +  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
> +   && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
Indentation.

> +      warning (_("\nTried to step over an atomic sequence of instructions at %s\n \
> +                  but could not find the end of the sequence."),
This leaves a lot of spaces in the warning text.

> +                  core_addr_to_string(pc));
Space before '('.

> +  /* Insert a breakpoint right after the end of the atomic sequence */
Comment should end with full stop.

> +  /* Effectively inserts all the breakpoints */
Likewise

> +  for (index = 0; index < last_breakpoint; index++)
> +    insert_single_step_breakpoint (breaks[index]);
> +
> +  gdb_flush (gdb_stdout);
I don't think this is necessary.

> +  if (deal_with_atomic_sequence(regcache))
Space before '('.

> +  /* Handles single stepping of atomic sequences */
Comment should end with full stop.
> +  set_gdbarch_software_single_step(gdbarch, deal_with_atomic_sequence);
Space before '('.


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[Luis Machado]

[-- Attachment #1: Type: text/plain, Size: 242 bytes --]

Ulrich,

Thanks for the comments. 

I've modified the patch according to your suggestions. Any additional
comments on it?

Any suggestions for the duplicate breakpoint detection? Maybe it could
be handled in a better way.

Best Regards.
Luis

[-- Attachment #2: ppc-atomic-single-stepping.diff --]
[-- Type: text/x-patch, Size: 4788 bytes --]

2007-04-13  Luis Machado  <luisgpm@br.ibm.com>

	* rs6000-tdep.c: Defines masks for POWER instructions that set
	and use the reservation flag (LWARX,LDARX,STWCX,STDCX).
	* rs6000-tdep.c (deal_with_atomic_sequence): Handles single
	stepping through an atomic sequence of instructions.
	* rs6000-tdep.c (rs6000_software_single_step): Added a function
	call to check if we are stepping through an atomic sequence of 
	instructions.
	* rs6000-tdep.c (rs6000_gdbarch_init): Initializes a function to
	check for atomic instruction sequences while single stepping.

Index: gdb/rs6000-tdep.c
===================================================================
--- gdb.orig/rs6000-tdep.c	2007-05-03 06:06:21.000000000 -0700
+++ gdb/rs6000-tdep.c	2007-05-03 07:45:09.000000000 -0700
@@ -719,8 +719,93 @@
     return little_breakpoint;
 }
 
+#define LWARX_MASK 0xfc0007fe
+#define LWARX_INSTRUCTION 0x7c000028
+#define LDARX_INSTRUCTION 0x7c0000A8
+#define STWCX_MASK 0xfc0007ff
+#define STWCX_INSTRUCTION 0x7c00012d
+#define STDCX_INSTRUCTION 0x7c0001ad
+#define BC_MASK 0xfc000000
+#define BC_INSTRUCTION 0x40000000
+
+static int 
+deal_with_atomic_sequence(struct regcache *regcache)
+{
+  CORE_ADDR pc = read_pc ();
+  CORE_ADDR breaks[16] = {-1, -1, -1, -1, -1, -1, -1, -1,
+                          -1, -1, -1, -1, -1, -1, -1, -1};
+  CORE_ADDR branch_bp; /* Breakpoint at destination of a banch instruction */
+  CORE_ADDR loc = pc;
+  int insn = read_memory_integer (loc, PPC_INSN_SIZE);
+  int insn_count;
+  int index; /* Index used for the "breaks" arrays */
+  int last_breakpoint = 0; /* Defaults to 0 (no breakpoints placed) */  
+  const int atomic_sequence_length = 16;
+  const int opcode = BC_INSTRUCTION; /* Branch instruction's OPcode */
+
+  /* Assume all atomic sequences start with an lwarx or ldarx instruction.  */
+  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
+   && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
+      return 0;
+
+  /* Assume that no atomic sequence is longer than "atomic_sequence_length 
+    instructions.  */
+  for (insn_count = 0; insn_count < atomic_sequence_length ; ++insn_count)
+    {
+      loc += PPC_INSN_SIZE;
+      insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+      /* Check for conditiconal branches in the middle of the sequence
+        and put breakpoints in their destinations */
+      if ((insn & BC_MASK) == BC_INSTRUCTION)
+        {
+          branch_bp = branch_dest(opcode, insn, pc, breaks[0]);
+
+          /* Make sure we don't have two breakpoints at the same address */
+          for (index = 0; index <= last_breakpoint; index++)
+            breaks[last_breakpoint] = (breaks[last_breakpoint] == branch_bp)?
+                                        -1:branch_bp;
+
+          if (breaks[last_breakpoint] != -1)
+            last_breakpoint++;
+        }
 
-/* AIX does not support PT_STEP. Simulate it. */
+      if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
+       || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
+        break;
+    }
+
+  /* Assume that the atomic sequence ends with a stwcx/stdcx instruction */
+  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
+   && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
+    {
+      warning (_("\nTried to step over an atomic sequence of instructions at %s\n \
+                  but could not find the end of the sequence."),
+                  core_addr_to_string(pc));
+      return 0;
+    }
+
+  loc += PPC_INSN_SIZE;
+  insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+  for (index = 0; index < last_breakpoint; index++)
+    if (loc == breaks[index])
+      break;
+
+  /* Insert a breakpoint right after the end of the atomic sequence */
+  if (index == last_breakpoint)
+    breaks[last_breakpoint++] = loc;
+
+  /* Effectively inserts all the breakpoints */
+  for (index = 0; index < last_breakpoint; index++)
+    insert_single_step_breakpoint (breaks[index]);
+
+  gdb_flush (gdb_stdout);
+
+  return 1;
+}
+
+/* AIX does not support PT_STEP. Simulate it.  */
 
 int
 rs6000_software_single_step (struct regcache *regcache)
@@ -737,6 +822,9 @@
 
   insn = read_memory_integer (loc, 4);
 
+  if (deal_with_atomic_sequence(regcache))
+    return 1;
+  
   breaks[0] = loc + breakp_sz;
   opcode = insn >> 26;
   breaks[1] = branch_dest (opcode, insn, loc, breaks[0]);
@@ -3461,6 +3549,9 @@
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_breakpoint_from_pc (gdbarch, rs6000_breakpoint_from_pc);
 
+  /* Handles single stepping of atomic sequences */
+  set_gdbarch_software_single_step(gdbarch, deal_with_atomic_sequence);
+  
   /* Handle the 64-bit SVR4 minimal-symbol convention of using "FN"
      for the descriptor and ".FN" for the entry-point -- a user
      specifying "break FN" will unexpectedly end up with a breakpoint

Re: [RFC] "single step" atomic instruction sequences as a whole on PPC

[uweigand]

Luis Machado wrote:

> Patch updated to reflect the inclusion of the regcache parameter.

Thanks!  A number of additional comments:

> +static int 
> +deal_with_atomic_sequence ()

Parameter is missing here.

> +{
> +  CORE_ADDR pc = read_pc ();
> +  CORE_ADDR breaks[2] = {-1, -1};
> +  CORE_ADDR loc = pc;
> +  int insn = read_memory_integer (loc, PPC_INSN_SIZE);
> +  int last_break = 0;
> +  int i;
> +
> +  /* Assume all atomic sequences start with an lwarx or ldarx instruction. */
Should have two spaces after '.' (multiple instances of this throughout the patch)

> +  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION
> +   && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
> +     return 0;
> +
> +  /* Assume that no atomic sequence is longer than 6 instructions. */
> +  for (i = 1; i < 5; ++i)
> +    {
> +      loc += PPC_INSN_SIZE;
> +      insn = read_memory_integer (loc, PPC_INSN_SIZE);
> +
> +      /* Assume at most one conditional branch instruction between
> + 	  the lwarx and stwcx instructions.*/

Well, what happens if there *is* more than one?  You should at least
detect that case ...

> +      if ((insn & BC_MASK) == BC_INSTRUCTION)
> +	    {
> +	    last_break = 1;
> +	    breaks[1] = IMMEDIATE_PART (insn);
> +	    if ( ! ABSOLUTE_P(insn))
Space before '('; no spaces around '!'.

> +	      breaks[1] += loc;
> +	      continue;
> +	    }

Why don't you use the existing branch_dest routine instead of 
re-implementing (parts of) it here?

> +        if ((insn & STWCX_MASK) == STWCX_INSTRUCTION
> +         || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
> +	      break;
> +    }
> +
> +  /* Assume that the atomic sequence ends with a stwcx instruction
> +       followed by a conditional branch instruction. */
> +  if ((insn & STWCX_MASK) != STWCX_INSTRUCTION
> +   && (insn & STWCX_MASK) != STDCX_INSTRUCTION)
> +    warning (_("Tried to step over an atomic sequence of instructions at %s\n \
> +                but could not find the end of the sequence."),
> +            core_addr_to_string(pc));

Shouldn't you return to the default single-step handling if you do not
understand the sequence you find?

> +
> +  loc += PPC_INSN_SIZE;
> +  insn = read_memory_integer (loc, PPC_INSN_SIZE);
> +
> +  if ((insn & BC_MASK) != BC_INSTRUCTION)
> +    warning (_("Tried to step over an atomic sequence of instructions at %s\n \
> +                but the instruction sequence ended in an unexpected way."),
> +            core_addr_to_string(pc));

I'm wondering why this test is necessary.  So what if there is no conditional
branch immediately after the stwcx?  That shouldn't really matter ...

> +
> +  breaks[0] = loc;
> +
> +  /* This should never happen, but make sure we don't put
> +     two breakpoints on the same address. */
> +  if (last_break && breaks[1] == breaks[0])
> +    last_break = 0;
> +
> +  for (i = 0; i <= last_break; ++i)
> +    insert_single_step_breakpoint (breaks[i]);
> +
> +  printf_unfiltered (_("Stepping over an atomic sequence of instructions beginning at %s\n"),
> +		     core_addr_to_string (pc));

Should we really output this message unconditionally?  If you're automatically
single-stepping (say, due to software watch points), this message could potentially
show up very frequently ...


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com