Re: [patch] "single step" atomic instruction sequences as a whole.

[Emi SUZUKI <emi-suzuki at tjsys dot co dot jp>]

Hello Luis, 

From: Luis Machado <luisgpm at linux dot vnet dot ibm dot com>
Subject: Re: [patch] "single step" atomic instruction sequences as a whole.
Date: Wed, 28 Feb 2007 13:09:00 -0300

> Also, i haven't been able to reproduce the issue related at this post:
> (http://sourceware.org/ml/gdb-patches/2006-09/msg00060.html)

I think one of the reasons why is that you didn't build the target as
Emre did; 'atomic_dec' seemed to be inline expanded in that target.  
I could get it with -O3 option of gcc-4.1.0 on my machine running FC5.  

And I could reproduce the issue with your patch by setting a
breakpoint at where 'printf' preceded by 'atomic_dec' is before
running the target.  

But the fix is fairly easy: 
===================================================================
diff -u rs6000-tdep.c.old rs6000-tdep.c
--- rs6000-tdep.c.old   2007-03-02 19:33:12.000000000 +0900
+++ rs6000-tdep.c       2007-03-02 19:34:07.000000000 +0900
@@ -764,7 +764,7 @@
   if (last_break && breaks[1] == breaks[0])
     last_break = 0;

-  for (i= 0; i < last_break; ++i)
+  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"),
diff -u ppc-linux-tdep.c.old ppc-linux-tdep.c
--- ppc-linux-tdep.c.old        2007-03-02 19:33:33.000000000 +0900
+++ ppc-linux-tdep.c    2007-03-02 19:33:54.000000000 +0900
@@ -996,7 +996,7 @@
       if (last_break && breaks[1] == breaks[0])
        last_break = 0;

-      for (i= 0; i < last_break; ++i)
+      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"),
===================================================================

Another thing I want to notice you about Paul's patch is that the
patch for ppc-linux-tdep.c is not needed at all: they do exactly the
same and RS6000 is the superset of PowerPC.  
And I think that the load or store instructions which deal with
doublewords can be appeared as a part of the atomic sequence; Paul's
patch doesn't check it.  But maybe not needed for now.  

BTW, I had another mistake on my patch send last time...
Please replace the attached one.  

My best regards, 
-- 
Emi SUZUKI

diff -uBbEw -ruN -x CVS src/gdb/config/rs6000/tm-rs6000.h gdb/gdb/config/rs6000/tm-rs6000.h
--- src/gdb/config/rs6000/tm-rs6000.h	2007-03-02 21:42:44.000000000 +0900
+++ gdb/gdb/config/rs6000/tm-rs6000.h	2007-03-02 21:17:31.000000000 +0900
@@ -30,3 +30,9 @@
 #define	PROCESS_LINENUMBER_HOOK()	aix_process_linenos ()
 extern void aix_process_linenos (void);
 
+extern int rs6000_software_single_step_p (void);
+
+#ifdef SOFTWARE_SINGLE_STEP_P
+#undef SOFTWARE_SINGLE_STEP_P
+#endif
+#define SOFTWARE_SINGLE_STEP_P() rs6000_software_single_step_p()
diff -uBbEw -ruN -x CVS src/gdb/infrun.c gdb/gdb/infrun.c
--- src/gdb/infrun.c	2007-03-02 21:42:43.000000000 +0900
+++ gdb/gdb/infrun.c	2007-03-02 21:28:25.000000000 +0900
@@ -556,7 +556,9 @@
   if (breakpoint_here_p (read_pc ()) == permanent_breakpoint_here)
     SKIP_PERMANENT_BREAKPOINT ();
 
-  if (SOFTWARE_SINGLE_STEP_P () && step)
+  if (step)
+    {
+      if (SOFTWARE_SINGLE_STEP_P ())
     {
       /* Do it the hard way, w/temp breakpoints */
       SOFTWARE_SINGLE_STEP (sig, 1 /*insert-breakpoints */ );
@@ -565,6 +567,8 @@
       /* and do not pull these breakpoints until after a `wait' in
          `wait_for_inferior' */
       singlestep_breakpoints_inserted_p = 1;
+	}
+
       singlestep_ptid = inferior_ptid;
       singlestep_pc = read_pc ();
     }
@@ -1565,8 +1569,6 @@
 
   if (stepping_past_singlestep_breakpoint)
     {
-      gdb_assert (SOFTWARE_SINGLE_STEP_P ()
-		  && singlestep_breakpoints_inserted_p);
       gdb_assert (ptid_equal (singlestep_ptid, ecs->ptid));
       gdb_assert (!ptid_equal (singlestep_ptid, saved_singlestep_ptid));
 
@@ -1579,9 +1581,13 @@
 	{
 	  if (debug_infrun)
 	    fprintf_unfiltered (gdb_stdlog, "infrun: stepping_past_singlestep_breakpoint\n");
+
+	  if (singlestep_breakpoints_inserted_p)
+	    {
 	  /* Pull the single step breakpoints out of the target.  */
 	  SOFTWARE_SINGLE_STEP (0, 0);
 	  singlestep_breakpoints_inserted_p = 0;
+	    }
 
 	  ecs->random_signal = 0;
 
@@ -1615,7 +1621,7 @@
 	  if (!breakpoint_thread_match (stop_pc, ecs->ptid))
 	    thread_hop_needed = 1;
 	}
-      else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+      else if (singlestep_breakpoints_inserted_p)
 	{
 	  /* We have not context switched yet, so this should be true
 	     no matter which thread hit the singlestep breakpoint.  */
@@ -1686,7 +1692,7 @@
 	  /* Saw a breakpoint, but it was hit by the wrong thread.
 	     Just continue. */
 
-	  if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+	  if (singlestep_breakpoints_inserted_p)
 	    {
 	      /* Pull the single step breakpoints out of the target. */
 	      SOFTWARE_SINGLE_STEP (0, 0);
@@ -1735,7 +1741,7 @@
 	      return;
 	    }
 	}
-      else if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+      else if (singlestep_breakpoints_inserted_p)
 	{
 	  sw_single_step_trap_p = 1;
 	  ecs->random_signal = 0;
@@ -1757,7 +1763,7 @@
 	deprecated_context_hook (pid_to_thread_id (ecs->ptid));
     }
 
-  if (SOFTWARE_SINGLE_STEP_P () && singlestep_breakpoints_inserted_p)
+  if (singlestep_breakpoints_inserted_p)
     {
       /* Pull the single step breakpoints out of the target. */
       SOFTWARE_SINGLE_STEP (0, 0);
diff -uBbEw -ruN -x CVS src/gdb/rs6000-tdep.c gdb/gdb/rs6000-tdep.c
--- src/gdb/rs6000-tdep.c	2007-03-02 21:42:45.000000000 +0900
+++ gdb/gdb/rs6000-tdep.c	2007-03-02 21:15:39.000000000 +0900
@@ -696,6 +696,79 @@
     return little_breakpoint;
 }
 
+#define LWARX_MASK 0xfc0007fe
+#define LWARX_INSTRUCTION 0x7C000028
+#define LDARX_INSTRUCTION 0x7C000108
+#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))
+
+CORE_ADDR 
+rs6000_deal_with_atomic_sequence (CORE_ADDR pc, CORE_ADDR *branch)
+{
+  CORE_ADDR loc = pc;
+  CORE_ADDR bc = -1;
+  int insn = read_memory_integer (loc, PPC_INSN_SIZE);
+  int i;
+
+  /* Assume all atomic sequences start with an lwarx instruction. */
+  if ((insn & LWARX_MASK) != LWARX_INSTRUCTION 
+      && (insn & LWARX_MASK) != LDARX_INSTRUCTION)
+    return -1;
+
+  /* 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);
+
+      /* At most one conditional branch instruction is between the lwarx 
+	 and stwcx. instructions. */
+      if ((insn & BC_MASK) == BC_INSTRUCTION)
+	{
+	  bc = IMMEDIATE_PART (insn);
+	  if (!ABSOLUTE_P (insn))
+	    bc += 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)
+    error (_("Tried to step over an atomic sequence of instructions but could not find the end of the sequence."));
+
+  loc += PPC_INSN_SIZE;
+  insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+  if ((insn & BC_MASK) != BC_INSTRUCTION)
+    error (_("Tried to step over an atomic sequence of instructions but it did not end as expected."));
+
+  /* set the location of conditional branch instruction between the lwarx 
+     and stwcx. instruction if any.  */
+  if (bc != loc)
+    *branch = bc;
+  else
+    *branch = -1;
+
+  return loc;
+}
+
+/* SOFTWARE_SINGLE_STEP_P */
+int
+rs6000_software_single_step_p (void)
+{
+  CORE_ADDR branch;
+  return (rs6000_deal_with_atomic_sequence (read_pc (), &branch) != -1);
+}
 
 /* AIX does not support PT_STEP. Simulate it. */
 
@@ -715,8 +788,20 @@
     {
       loc = read_pc ();
 
-      insn = read_memory_integer (loc, 4);
+      /* check if running on an atomic sequence of instructions */
+      breaks[0] = rs6000_deal_with_atomic_sequence (loc, &breaks[1]);
 
+      if (breaks[0] != -1)
+	{
+	  printf_unfiltered (_("Stepping over an atomic sequence of instructions. \n\
+Beginning at %s, break at %s next time.\n"),
+			     core_addr_to_string (loc), 
+			     core_addr_to_string (breaks[0]));
+	  gdb_flush (gdb_stdout);
+	}
+      else
+	{
+	  insn = read_memory_integer (loc, PPC_INSN_SIZE);
       breaks[0] = loc + breakp_sz;
       opcode = insn >> 26;
       breaks[1] = branch_dest (opcode, insn, loc, breaks[0]);
@@ -724,6 +809,7 @@
       /* Don't put two breakpoints on the same address. */
       if (breaks[1] == breaks[0])
 	breaks[1] = -1;
+	}
 
       for (ii = 0; ii < 2; ++ii)
 	{
@@ -3442,6 +3529,7 @@
 
   set_gdbarch_inner_than (gdbarch, core_addr_lessthan);
   set_gdbarch_breakpoint_from_pc (gdbarch, rs6000_breakpoint_from_pc);
+  set_gdbarch_software_single_step (gdbarch, rs6000_software_single_step);
 
   /* Handle the 64-bit SVR4 minimal-symbol convention of using "FN"
      for the descriptor and ".FN" for the entry-point -- a user