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

classic Classic list List threaded Threaded
35 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Luis Machado-5
Emi,

I tried your patch against the CVS head. GDB seems to handle this set of
instructions just fine:

0x40000119a00 <._IO_puts+80>:   beq-    cr7,0x40000119a3c <._IO_puts
+140>
0x40000119a04 <._IO_puts+84>:   li      r0,1
0x40000119a08 <._IO_puts+88>:   lwarx   r11,0,r3
0x40000119a0c <._IO_puts+92>:   cmpw    r11,r9
0x40000119a10 <._IO_puts+96>:   bne-    0x40000119a1c <._IO_puts+108>
0x40000119a14 <._IO_puts+100>:  stwcx.  r0,0,r3
0x40000119a18 <._IO_puts+104>:  bne+    0x40000119a08 <._IO_puts+88>
0x40000119a1c <._IO_puts+108>:  isync
0x40000119a20 <._IO_puts+112>:  cmpwi   cr7,r11,0
0x40000119a24 <._IO_puts+116>:  bne-    cr7,0x40000119b84 <._IO_puts
+468>

If i step through "lwarx", i end up with this:

Stepping over an atomic sequence of instructions.
Beginning at 0x0000040000119a08, break at 0x0000040000119a18 next time.
0x0000040000119a18 in ._IO_puts () from /lib64/power5/libc.so.6

So far so good. But if we continue executing the program we will
eventually reach a set of instructions like this one:

0x40000119ae8 <._IO_puts+312>:  lwarx   r9,0,r3
0x40000119aec <._IO_puts+316>:  stwcx.  r0,0,r3
0x40000119af0 <._IO_puts+320>:  bne+    0x40000119ae8 <._IO_puts+312>

At this point, GDB keeps stepping through the lwarx and stwcx
instructions endlessly. If i hit continue right after the first
breakpoint (0x40000119a08), the program ends normally

I tested the same instruction block with Paul's patch and this did not
happen, stating that it skipped the atomic instruction set as usual, and
ending the program.

I'm trying to figure out what is going on exactly.

Regards,
Luis

Reply | Threaded
Open this post in threaded view
|

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

Emi SUZUKI
Luis,

Thank you for testing and sorry for being late to be back on this.  

From: Luis Machado <luisgpm at linux.vnet.ibm.com>
Subject: Re: [patch] "single step" atomic instruction sequences as a whole.
Date: Sat, 17 Feb 2007 00:23:19 -0200

> So far so good. But if we continue executing the program we will
> eventually reach a set of instructions like this one:
>
> 0x40000119ae8 <._IO_puts+312>:  lwarx   r9,0,r3
> 0x40000119aec <._IO_puts+316>:  stwcx.  r0,0,r3
> 0x40000119af0 <._IO_puts+320>:  bne+    0x40000119ae8 <._IO_puts+312>
>
> At this point, GDB keeps stepping through the lwarx and stwcx
> instructions endlessly. If i hit continue right after the first
> breakpoint (0x40000119a08), the program ends normally
>
> I tested the same instruction block with Paul's patch and this did not
> happen, stating that it skipped the atomic instruction set as usual, and
> ending the program.
I have investigeted it and found that the code in my patch cannot
detect the region to skip from the sequence of instructions mentioned
above.  
But, in fact, I have diverted that part of codes from Paul's one :-(
I have been misguided by some useless codes in that patch... Yes, I
should have made myself understand more deeply about its behavior.  

Anyway, I have looked anew at that part of codes and improved it.  
Please try the attached version of patchset, if you don't mind.  
It's for CVS head.  

Best regards,
--
Emi SUZUKI

diff -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-01-10 02:59:05.000000000 +0900
+++ gdb/gdb/config/rs6000/tm-rs6000.h 2007-02-27 21:52:46.000000000 +0900
@@ -90,3 +90,9 @@
 
 extern void (*rs6000_set_host_arch_hook) (int);
 
+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 -ruN -x CVS src/gdb/infrun.c gdb/gdb/infrun.c
--- src/gdb/infrun.c 2007-02-27 21:34:23.000000000 +0900
+++ gdb/gdb/infrun.c 2007-02-27 21:28:54.000000000 +0900
@@ -556,15 +556,19 @@
   if (breakpoint_here_p (read_pc ()) == permanent_breakpoint_here)
     SKIP_PERMANENT_BREAKPOINT ();
 
-  if (SOFTWARE_SINGLE_STEP_P () && step)
+  if (step)
     {
-      /* Do it the hard way, w/temp breakpoints */
-      SOFTWARE_SINGLE_STEP (sig, 1 /*insert-breakpoints */ );
-      /* ...and don't ask hardware to do it.  */
-      step = 0;
-      /* and do not pull these breakpoints until after a `wait' in
-         `wait_for_inferior' */
-      singlestep_breakpoints_inserted_p = 1;
+      if (SOFTWARE_SINGLE_STEP_P ())
+ {
+  /* Do it the hard way, w/temp breakpoints */
+  SOFTWARE_SINGLE_STEP (sig, 1 /*insert-breakpoints */ );
+  /* ...and don't ask hardware to do it.  */
+  step = 0;
+  /* 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 ();
     }
@@ -1566,8 +1570,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));
 
@@ -1580,9 +1582,13 @@
  {
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog, "infrun: stepping_past_singlestep_breakpoint\n");
-  /* Pull the single step breakpoints out of the target.  */
-  SOFTWARE_SINGLE_STEP (0, 0);
-  singlestep_breakpoints_inserted_p = 0;
+
+  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;
 
@@ -1616,7 +1622,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.  */
@@ -1687,7 +1693,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);
@@ -1736,7 +1742,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;
@@ -1758,7 +1764,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 -ruN -x CVS src/gdb/rs6000-tdep.c gdb/gdb/rs6000-tdep.c
--- src/gdb/rs6000-tdep.c 2007-02-21 12:01:58.000000000 +0900
+++ gdb/gdb/rs6000-tdep.c 2007-02-27 21:00:20.000000000 +0900
@@ -701,6 +701,64 @@
     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 loc = pc;
+  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 instuction appears between
+ the lwarx/ldarx and stwcx/stdcx instructions.  But its target
+ address should be where the second conditional branch goes
+ when the branch is not taken.  */
+      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."));
+
+  return loc;
+}
+
+/* SOFTWARE_SINGLE_STEP_P */
+int
+rs6000_software_single_step_p (void)
+{
+  return (rs6000_deal_with_atomic_sequence (read_pc ()) != -1);
+}
 
 /* AIX does not support PT_STEP. Simulate it. */
 
@@ -720,15 +778,29 @@
     {
       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[0] = loc + breakp_sz;
-      opcode = insn >> 26;
-      breaks[1] = branch_dest (opcode, insn, loc, breaks[0]);
-
-      /* Don't put two breakpoints on the same address. */
-      if (breaks[1] == breaks[0])
- breaks[1] = -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);
+  breaks[1] = -1;
+ }
+      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]);
+
+  /* Don't put two breakpoints on the same address. */
+  if (breaks[1] == breaks[0])
+    breaks[1] = -1;
+ }
 
       for (ii = 0; ii < 2; ++ii)
  {
@@ -3446,6 +3518,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
@@ -3530,3 +3603,4 @@
   gdbarch_register (bfd_arch_rs6000, rs6000_gdbarch_init, rs6000_dump_tdep);
   gdbarch_register (bfd_arch_powerpc, rs6000_gdbarch_init, rs6000_dump_tdep);
 }
+
Reply | Threaded
Open this post in threaded view
|

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

Daniel Jacobowitz-2
On Tue, Feb 27, 2007 at 10:00:28PM +0900, Emi SUZUKI wrote:
> Anyway, I have looked anew at that part of codes and improved it.  
> Please try the attached version of patchset, if you don't mind.  
> It's for CVS head.  

Please forgive me if I've asked you this question before, but do you
have (or are you willing to get) a GDB copyright assignment?  I don't
think we can accept a patch of this size without one.

If not, that's OK - I'm sure we can come up with something.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

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

Emi SUZUKI
Daniel,

I'm glad to hear from you about the size of patch for which the
assignment is needed.  

From: Daniel Jacobowitz <drow at false.org>
Subject: Re: [patch] "single step" atomic instruction sequences as a whole.
Date: Tue, 27 Feb 2007 08:17:24 -0500

> Please forgive me if I've asked you this question before, but do you
> have (or are you willing to get) a GDB copyright assignment?  I don't
> think we can accept a patch of this size without one.

I have received a form from FSF for it (and got a sticker of gnu), and
the legal process in the campany is going on.  But unfortunately, my
boss told me that it will take long (he said that it means 3 months at
worst...)
Meanwhile they also demand me to make some contribution to the mainline.
So now I think I have a ground to ask for them to hurry.  

My best regards,
--
Emi SUZUKI

Reply | Threaded
Open this post in threaded view
|

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

Daniel Jacobowitz-2
On Wed, Feb 28, 2007 at 05:07:13PM +0900, Emi SUZUKI wrote:
> I have received a form from FSF for it (and got a sticker of gnu), and
> the legal process in the campany is going on.  But unfortunately, my
> boss told me that it will take long (he said that it means 3 months at
> worst...)

Unfortunately this is common :-(  Thank you - I'm glad to hear it is
in progress.  We'll wait.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

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

Luis Machado-5
Hi,

I refreshed Paul's patches to apply cleanly on HEAD. I did a test run
and it still works as expected.

->
(gdb) start
(gdb) x/g &puts
0x400001dc1d8 <puts>:   0x00000400000e99b0
(gdb) x/10i 0x00000400000e99b0 + 80
0x400000e9a00 <._IO_puts+80>:   beq-    cr7,0x400000e9a3c <._IO_puts+140>
0x400000e9a04 <._IO_puts+84>:   li      r0,1
0x400000e9a08 <._IO_puts+88>:   lwarx   r11,0,r3
0x400000e9a0c <._IO_puts+92>:   cmpw    r11,r9
0x400000e9a10 <._IO_puts+96>:   bne-    0x400000e9a1c <._IO_puts+108>
0x400000e9a14 <._IO_puts+100>:  stwcx.  r0,0,r3
0x400000e9a18 <._IO_puts+104>:  bne+    0x400000e9a08 <._IO_puts+88>
0x400000e9a1c <._IO_puts+108>:  isync
0x400000e9a20 <._IO_puts+112>:  cmpwi   cr7,r11,0
0x400000e9a24 <._IO_puts+116>:  bne-    cr7,0x400000e9b84 <._IO_puts+468>
(gdb) b *0x400000e9a04
Breakpoint 2 at 0x400000e9a04
(gdb) c
Continuing.

Breakpoint 2, 0x00000400000e9a04 in ._IO_puts () from /lib64/power5/libc.so.6
(gdb) si
0x00000400000e9a08 in ._IO_puts () from /lib64/power5/libc.so.6
(gdb)
Stepping over an atomic sequence of instructions beginning at 0x00000400000e9a08
0x00000400000e9a18 in ._IO_puts () from /lib64/power5/libc.so.6
(gdb) x/5i 0x00000400000e9a08
0x400000e9a08 <._IO_puts+88>:   lwarx   r11,0,r3
0x400000e9a0c <._IO_puts+92>:   cmpw    r11,r9
0x400000e9a10 <._IO_puts+96>:   bne-    0x400000e9a1c <._IO_puts+108>
0x400000e9a14 <._IO_puts+100>:  stwcx.  r0,0,r3
0x400000e9a18 <._IO_puts+104>:  bne+    0x400000e9a08 <._IO_puts+88>
(gdb)
<-

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

This is the test run on my power machine. No Internal errors at this
time.

->
(gdb) start
Breakpoint 1 at 0x10000674: file atomic.c, line 9.
Starting program: /home/luis/binaries/atomic64
warning: Unable to find dynamic linker breakpoint function.
GDB will be unable to debug shared library initializers
and track explicitly loaded dynamic code.
warning: Breakpoint address adjusted from 0x10010b58 to 0x100004f0.
main (argc=1, argv=0xfffff8351e8 "") at atomic.c:9
9       printf("atomic_step_test\n");
(gdb) n
atomic_step_test
10      atomic_set(&i,5);
(gdb)
11      printf("i=%d\n",atomic_read(&i));
(gdb)
i=5
12      atomic_dec(&i);
(gdb)
13      printf("i=%d\n",atomic_read(&i));
(gdb)
i=4
16      }
(gdb)
0x00000400000a0c8c in .generic_start_main ()
from /lib64/power5/libc.so.6
(gdb)
Single stepping until exit from function .generic_start_main,
which has no line number information.
0x00000400000150f0 in ._dl_fini () from /lib64/ld64.so.1
(gdb)
Single stepping until exit from function ._dl_fini,
which has no line number information.
Stepping over an atomic sequence of instructions beginning at
0x00000400000fa69c
Stepping over an atomic sequence of instructions beginning at
0x00000400000fa820

Program exited with code 04.
(gdb)
<-

Regards,
Luis

change-software-single-step.diff (16K) Download Attachment
ppc-atomic.single-step.diff (3K) Download Attachment
rs6000-atomic-single-step.diff (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Emi SUZUKI
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
Reply | Threaded
Open this post in threaded view
|

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

Andreas Schwab
Emi SUZUKI <[hidden email]> writes:

> +  /* 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."));

I don't think error should be called here.  It would probably be better to
just continue with the normal single-step here.

Andreas.

--
Andreas Schwab, SuSE Labs, [hidden email]
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
Reply | Threaded
Open this post in threaded view
|

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

Daniel Jacobowitz-2
On Tue, Mar 06, 2007 at 12:00:21PM +0100, Andreas Schwab wrote:
> Emi SUZUKI <[hidden email]> writes:
>
> > +  /* 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."));
>
> I don't think error should be called here.  It would probably be better to
> just continue with the normal single-step here.

Maybe a (once-only) warning?  It would be nice to let the user know
we're confused.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

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

Emi SUZUKI
Andreas and Daniel,

Thank you for the comments.  

From: Daniel Jacobowitz <drow at false.org>
Subject: Re: [patch] "single step" atomic instruction sequences as a whole.
Date: Tue, 06 Mar 2007 07:24:36 -0500

> On Tue, Mar 06, 2007 at 12:00:21PM +0100, Andreas Schwab wrote:
> > > +  /* 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."));
> >
> > I don't think error should be called here.  It would probably be better to
> > just continue with the normal single-step here.
>
> Maybe a (once-only) warning?  It would be nice to let the user know
> we're confused.
That's the last thing I have wondered what to do with respect to
Paul's patch.  
Maybe any errors should not be called in that function, because the
function is just for checking if there is any sequence of instructions
that should be avoided running through.  I have no idea about the
warning, though, it might be a help for debugging some of complecated
issues.  

Anyway, I found that calling warning instead of error, and returning -1
(indecates that any atomic sequences are not found) after warning
fulfills both suggestions.  I have done with the attached.  

Meanwhile the things about RS6000-AIX came to me: it does not support
hardware single stepping, so SOFTWARE_SINGLE_STEP_P should always
return true.  My patch has nothing to concern about it...

I have added a new file, tm-rs6000aix.h, to undef SOFTWARE_SINGLE_STEP_P
for only that target, but felt somewhat strange about the solution.  
I feel like adding some trick for SOFTWARE_SINGLE_STEP_P to gdbarch.c
rather than undef'ing it, but no idea has come to mind for now.  

My best regards,
--
Emi SUZUKI

diff -ruN -x CVS -x '*~' src/gdb/config/powerpc/aix.mt gdb/gdb/config/powerpc/aix.mt
--- src/gdb/config/powerpc/aix.mt 2006-02-11 05:56:15.000000000 +0900
+++ gdb/gdb/config/powerpc/aix.mt 2007-03-08 16:18:37.000000000 +0900
@@ -1,4 +1,4 @@
 # Target: PowerPC running AIX
 TDEPFILES= rs6000-tdep.o rs6000-aix-tdep.o \
            xcoffread.o ppc-sysv-tdep.o solib.o solib-svr4.o
-DEPRECATED_TM_FILE= config/rs6000/tm-rs6000.h
+DEPRECATED_TM_FILE= config/rs6000/tm-rs6000aix.h
diff -ruN -x CVS -x '*~' 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-06 20:43:40.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 -ruN -x CVS -x '*~' src/gdb/config/rs6000/tm-rs6000aix.h gdb/gdb/config/rs6000/tm-rs6000aix.h
--- src/gdb/config/rs6000/tm-rs6000aix.h 1970-01-01 09:00:00.000000000 +0900
+++ gdb/gdb/config/rs6000/tm-rs6000aix.h 2007-03-08 17:24:14.000000000 +0900
@@ -0,0 +1,33 @@
+/* Macro definitions for RS6000 running under AIX.  
+   Copyright 2007 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   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., 59 Temple Place - Suite 330,
+   Boston, MA 02111-1307, USA.  */
+
+#ifndef TM_RS6000AIX_H
+#define TM_RS6000AIX_H
+
+/* Use generic RS6000 definitions. */
+#include "rs6000/tm-rs6000.h"
+
+/* Almost all OS running on RS6000/PPC supports handling hardware single
+   step, but sometimes use software single step for skipping atomic
+   sequences of instructions.  Only RS6000-AIX always use software single
+   step, so we use the default function for it.  */
+#undef SOFTWARE_SINGLE_STEP_P
+
+#endif /* TM_RS6000AIX_H */
diff -ruN -x CVS -x '*~' 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,15 +556,19 @@
   if (breakpoint_here_p (read_pc ()) == permanent_breakpoint_here)
     SKIP_PERMANENT_BREAKPOINT ();
 
-  if (SOFTWARE_SINGLE_STEP_P () && step)
+  if (step)
     {
-      /* Do it the hard way, w/temp breakpoints */
-      SOFTWARE_SINGLE_STEP (sig, 1 /*insert-breakpoints */ );
-      /* ...and don't ask hardware to do it.  */
-      step = 0;
-      /* and do not pull these breakpoints until after a `wait' in
-         `wait_for_inferior' */
-      singlestep_breakpoints_inserted_p = 1;
+      if (SOFTWARE_SINGLE_STEP_P ())
+ {
+  /* Do it the hard way, w/temp breakpoints */
+  SOFTWARE_SINGLE_STEP (sig, 1 /*insert-breakpoints */ );
+  /* ...and don't ask hardware to do it.  */
+  step = 0;
+  /* 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");
-  /* Pull the single step breakpoints out of the target.  */
-  SOFTWARE_SINGLE_STEP (0, 0);
-  singlestep_breakpoints_inserted_p = 0;
+
+  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 -ruN -x CVS -x '*~' src/gdb/rs6000-aix-tdep.c gdb/gdb/rs6000-aix-tdep.c
--- src/gdb/rs6000-aix-tdep.c 2007-03-02 21:42:43.000000000 +0900
+++ gdb/gdb/rs6000-aix-tdep.c 2007-03-08 16:18:21.000000000 +0900
@@ -39,9 +39,6 @@
 static void
 rs6000_aix_init_osabi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
-  /* RS6000/AIX does not support PT_STEP.  Has to be simulated.  */
-  set_gdbarch_software_single_step (gdbarch, rs6000_software_single_step);
-
   /* Minimum possible text address in AIX.  */
   gdbarch_tdep (gdbarch)->text_segment_base = 0x10000000;
 }
diff -ruN -x CVS -x '*~' 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-07 18:01:01.000000000 +0900
@@ -696,6 +696,86 @@
     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
+      || (insn & STWCX_MASK) == STDCX_INSTRUCTION)
+    {
+      warning (_("Tried to step over an atomic sequence of instructions from %s but could not find the end of the sequence.", core_addr_to_string (pc)));
+      return -1;
+    }
+
+  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 from %s but it did not end as expected.", core_addr_to_string (pc)));
+      return -1;
+    }
+
+  /* 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,21 +795,35 @@
     {
       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]);
 
-      breaks[0] = loc + breakp_sz;
-      opcode = insn >> 26;
-      breaks[1] = branch_dest (opcode, insn, loc, breaks[0]);
-
-      /* Don't put two breakpoints on the same address. */
-      if (breaks[1] == breaks[0])
- breaks[1] = -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]);
+
+  /* Don't put two breakpoints on the same address. */
+  if (breaks[1] == breaks[0])
+    breaks[1] = -1;
+ }
 
       for (ii = 0; ii < 2; ++ii)
  {
   /* ignore invalid breakpoint. */
   if (breaks[ii] == -1)
     continue;
+
   insert_single_step_breakpoint (breaks[ii]);
  }
     }
@@ -3442,6 +3536,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
Reply | Threaded
Open this post in threaded view
|

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

Ulrich Weigand
Emi Suzuki wrote:

> Meanwhile the things about RS6000-AIX came to me: it does not support
> hardware single stepping, so SOFTWARE_SINGLE_STEP_P should always
> return true.  My patch has nothing to concern about it...
>
> I have added a new file, tm-rs6000aix.h, to undef SOFTWARE_SINGLE_STEP_P
> for only that target, but felt somewhat strange about the solution.  
> I feel like adding some trick for SOFTWARE_SINGLE_STEP_P to gdbarch.c
> rather than undef'ing it, but no idea has come to mind for now.  

We're trying to get rid of the tm.h files, and do everything strictly
via the gdbarch callbacks.  (This is also necessary for multi-arch
debugging.)  I'd much prefer a solution that does not add new tm.h
files (or contents).

Why don't we extend the gdbarch_software_single_step call with a return
value?  Common code would call the gdbarch routine, but if it returns
a nonzero value, it will fall back to using hardware single step after
all.


Bye,
Ulrich

--
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  [hidden email]
Reply | Threaded
Open this post in threaded view
|

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

Emi SUZUKI
Hello Ulrich,

Thank you for your suggestion.  

From: Ulrich Weigand <uweigand at de.ibm.com>
Subject: Re: [patch] "single step" atomic instruction sequences as a whole.
Date: Thu, 08 Mar 2007 17:15:42 +0100 (CET)

> Emi Suzuki wrote:
> > I have added a new file, tm-rs6000aix.h, to undef SOFTWARE_SINGLE_STEP_P
> > for only that target, but felt somewhat strange about the solution.  
> > I feel like adding some trick for SOFTWARE_SINGLE_STEP_P to gdbarch.c
> > rather than undef'ing it, but no idea has come to mind for now.  
>
> We're trying to get rid of the tm.h files, and do everything strictly
> via the gdbarch callbacks.  (This is also necessary for multi-arch
> debugging.)  I'd much prefer a solution that does not add new tm.h
> files (or contents).
Yes, that's exactly what I really intend, do everything via the
gdbarch callbacks.  

> Why don't we extend the gdbarch_software_single_step call with a return
> value?  Common code would call the gdbarch routine, but if it returns
> a nonzero value, it will fall back to using hardware single step after
> all.

It's like what the original patchset does for this issue (provided
http://sourceware.org/ml/gdb-patches/2006-06/msg00339.html and
http://sourceware.org/ml/gdb-patches/2006-06/msg00341.html).  
I have felt something uncomfortable to them, because it uses
SOFTWARE_SINGLE_STEP_P to check both if the target will do software
single stepping at the next time it proceed and if the target has done
software single stepping after it get stopped.  

But now I have noticed that calling gdbarch_software_single_step
*instead of* SOFTWARE_SINGLE_STEP_P will clear my confusion and avoid
adding new tm.h files.  I've changed the whole solution like below:

* make gdbarch_software_single_step return 1 (means non-zero) if it
  inserted software single step breakpoint, or return 0.  
* defined default_software_single_step in arch-utils.c, and make it
  the default of gdbarch_software_single_step.  
* call SOFTWARE_SINGLE_STEP instead of SOFTWARE_SINGLE_STEP_P, to
  check if software single stepping will be done when proceeding.  
* put an alternative way to check if software single stepping was
  done when stopped (mainly, it would be checking of
  singlestep_breakpoint_inserted_p).  

One thing I am not sure about my change is adjust_pc_after_break in
infrun.c.  And I am not familier with the use of gdbarch.sh, it is
much appreciated if anyone would review them.  

--
Emi SUZUKI

Index: gdb/infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.223
diff -u -r1.223 infrun.c
--- gdb/infrun.c 9 Mar 2007 16:20:42 -0000 1.223
+++ gdb/infrun.c 13 Mar 2007 05:37:27 -0000
@@ -552,15 +552,18 @@
   if (breakpoint_here_p (read_pc ()) == permanent_breakpoint_here)
     SKIP_PERMANENT_BREAKPOINT ();
 
-  if (SOFTWARE_SINGLE_STEP_P () && step)
+  if (step)
     {
-      /* Do it the hard way, w/temp breakpoints */
-      SOFTWARE_SINGLE_STEP (sig, 1 /*insert-breakpoints */ );
-      /* ...and don't ask hardware to do it.  */
-      step = 0;
-      /* and do not pull these breakpoints until after a `wait' in
-         `wait_for_inferior' */
-      singlestep_breakpoints_inserted_p = 1;
+      /* Try to do it the hard way, w/temp breakpoints */
+      if (SOFTWARE_SINGLE_STEP (sig, 1 /* insert-breakpoints */))
+ {
+  /* Succeeded, so don't ask hardware to do it.  */
+  step = 0;
+  /* 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 ();
     }
@@ -1196,22 +1199,18 @@
      breakpoint would be.  */
   breakpoint_pc = read_pc_pid (ecs->ptid) - DECR_PC_AFTER_BREAK;
 
-  if (SOFTWARE_SINGLE_STEP_P ())
+  if (singlestep_breakpoints_inserted_p)
     {
       /* When using software single-step, a SIGTRAP can only indicate
          an inserted breakpoint.  This actually makes things
          easier.  */
-      if (singlestep_breakpoints_inserted_p)
- /* When software single stepping, the instruction at [prev_pc]
-   is never a breakpoint, but the instruction following
-   [prev_pc] (in program execution order) always is.  Assume
-   that following instruction was reached and hence a software
-   breakpoint was hit.  */
- write_pc_pid (breakpoint_pc, ecs->ptid);
-      else if (software_breakpoint_inserted_here_p (breakpoint_pc))
- /* The inferior was free running (i.e., no single-step
-   breakpoints inserted) and it hit a software breakpoint.  */
- write_pc_pid (breakpoint_pc, ecs->ptid);
+
+      /* When software single stepping, the instruction at [prev_pc]
+ is never a breakpoint, but the instruction following
+ [prev_pc] (in program execution order) always is.  Assume
+ that following instruction was reached and hence a software
+ breakpoint was hit.  */
+      write_pc_pid (breakpoint_pc, ecs->ptid);
     }
   else
     {
@@ -1405,7 +1404,7 @@
       target_mourn_inferior ();
 
       print_stop_reason (SIGNAL_EXITED, stop_signal);
-      singlestep_breakpoints_inserted_p = 0; /*SOFTWARE_SINGLE_STEP_P() */
+      singlestep_breakpoints_inserted_p = 0; /* SOFTWARE_SINGLE_STEP() */
       stop_stepping (ecs);
       return;
 
@@ -1561,8 +1560,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));
 
@@ -1575,9 +1572,13 @@
  {
   if (debug_infrun)
     fprintf_unfiltered (gdb_stdlog, "infrun: stepping_past_singlestep_breakpoint\n");
-  /* Pull the single step breakpoints out of the target.  */
-  SOFTWARE_SINGLE_STEP (0, 0);
-  singlestep_breakpoints_inserted_p = 0;
+
+  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;
 
@@ -1611,7 +1612,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.  */
@@ -1682,7 +1683,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);
@@ -1731,7 +1732,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;
@@ -1753,7 +1754,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);
Index: gdb/cris-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/cris-tdep.c,v
retrieving revision 1.138
diff -u -r1.138 cris-tdep.c
--- gdb/cris-tdep.c 27 Feb 2007 20:17:18 -0000 1.138
+++ gdb/cris-tdep.c 13 Mar 2007 05:36:05 -0000
@@ -2119,7 +2119,7 @@
    digs through the opcodes in order to find all possible targets.
    Either one ordinary target or two targets for branches may be found.  */
 
-static void
+static int
 cris_software_single_step (enum target_signal ignore, int insert_breakpoints)
 {
   inst_env_type inst_env;
@@ -2152,6 +2152,8 @@
     }
   else
     remove_single_step_breakpoints ();
+
+  return 1;
 }
 
 /* Calculates the prefix value for quick offset addressing mode.  */
Index: gdb/rs6000-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.c,v
retrieving revision 1.265
diff -u -r1.265 rs6000-tdep.c
--- gdb/rs6000-tdep.c 27 Feb 2007 23:04:28 -0000 1.265
+++ gdb/rs6000-tdep.c 13 Mar 2007 05:38:22 -0000
@@ -696,10 +696,101 @@
     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
+
+int
+rs6000_deal_with_atomic_sequence (enum target_signal signal,
+  int insert_breakpoints_p)
+{
+  CORE_ADDR loc, pc;
+  int i, insn;
+  CORE_ADDR breaks[2] = { -1, -1 };
+
+  if (insert_breakpoints_p)
+    {
+      pc = read_pc ();
+      loc = pc;
+      insn = read_memory_integer (loc, PPC_INSN_SIZE);
+
+      /* Assume all atomic sequences start with an lwarx 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);
+
+  /* At most one conditional branch instruction is between the lwarx
+     and stwcx. instructions. */
+  if ((insn & BC_MASK) == BC_INSTRUCTION)
+    {
+      int opcode = BC_INSTRUCTION >> 26;
+      breaks[1] = branch_dest (opcode, insn, loc, -1);
+      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 from %s 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);
+
+      if ((insn & BC_MASK) != BC_INSTRUCTION)
+ {
+  warning (_("Tried to step over an atomic sequence of instructions from %s but it did not end as expected."), core_addr_to_string (pc));
+  return 0;
+ }
+
+      breaks[0] = loc;
+      printf_unfiltered (_("Stepping over an atomic sequence of instructions. \n\
+Beginning at %s, break at %s next time.\n"),
+ core_addr_to_string (pc),
+ core_addr_to_string (breaks[0]));
+
+      /* It cannot be happened, but just check it */
+      if (breaks[1] == loc)
+ breaks[1] = -1;
+
+      for (i = 0; i < 2; ++i)
+ {
+  /* ignore invalid breakpoint. */
+  if (breaks[i] == -1)
+    continue;
+
+  insert_single_step_breakpoint (breaks[i]);
+ }
+    }
+  else
+    remove_single_step_breakpoints ();
+
+  /* software single step breakpoint is inserted or removed */
+  return 1;
+}
 
 /* AIX does not support PT_STEP. Simulate it. */
 
-void
+int
 rs6000_software_single_step (enum target_signal signal,
      int insert_breakpoints_p)
 {
@@ -711,6 +802,10 @@
   CORE_ADDR breaks[2];
   int opcode;
 
+  /* check if running through the atomic sequence of instructions first. */
+  if (rs6000_deal_with_atomic_sequence (signal, insert_breakpoints_p))
+    return 1;
+
   if (insert_breakpoints_p)
     {
       loc = read_pc ();
@@ -738,6 +833,8 @@
 
   errno = 0; /* FIXME, don't ignore errors! */
   /* What errors?  {read,write}_memory call error().  */
+
+  return 1;
 }
 
 
@@ -3442,6 +3539,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_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
Index: gdb/mips-tdep.h
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.h,v
retrieving revision 1.19
diff -u -r1.19 mips-tdep.h
--- gdb/mips-tdep.h 9 Jan 2007 17:58:52 -0000 1.19
+++ gdb/mips-tdep.h 13 Mar 2007 05:38:01 -0000
@@ -103,7 +103,7 @@
 };
 
 /* Single step based on where the current instruction will take us.  */
-extern void mips_software_single_step (enum target_signal, int);
+extern int mips_software_single_step (enum target_signal, int);
 
 /* Tell if the program counter value in MEMADDR is in a MIPS16
    function.  */
Index: gdb/spu-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/spu-tdep.c,v
retrieving revision 1.10
diff -u -r1.10 spu-tdep.c
--- gdb/spu-tdep.c 9 Mar 2007 03:51:04 -0000 1.10
+++ gdb/spu-tdep.c 13 Mar 2007 05:38:39 -0000
@@ -1078,7 +1078,7 @@
 
 /* Software single-stepping support.  */
 
-void
+int
 spu_software_single_step (enum target_signal signal, int insert_breakpoints_p)
 {
   if (insert_breakpoints_p)
@@ -1093,7 +1093,7 @@
       pc = extract_unsigned_integer (buf, 4) & -4;
 
       if (target_read_memory (pc, buf, 4))
- return;
+ return 0;
       insn = extract_unsigned_integer (buf, 4);
 
        /* Next sequential instruction is at PC + 4, except if the current
@@ -1125,6 +1125,8 @@
     }
   else
     remove_single_step_breakpoints ();
+
+  return 1;
 }
 
 
Index: gdb/sparc64-sol2-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc64-sol2-tdep.c,v
retrieving revision 1.11
diff -u -r1.11 sparc64-sol2-tdep.c
--- gdb/sparc64-sol2-tdep.c 9 Jan 2007 17:58:58 -0000 1.11
+++ gdb/sparc64-sol2-tdep.c 13 Mar 2007 05:38:33 -0000
@@ -170,7 +170,7 @@
   tdep->plt_entry_size = 16;
 
   /* Solaris has kernel-assisted single-stepping support.  */
-  set_gdbarch_software_single_step (gdbarch, NULL);
+  set_gdbarch_software_single_step (gdbarch, default_software_single_step);
 }
 
 
Index: gdb/sparc-sol2-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc-sol2-tdep.c,v
retrieving revision 1.12
diff -u -r1.12 sparc-sol2-tdep.c
--- gdb/sparc-sol2-tdep.c 9 Jan 2007 17:58:58 -0000 1.12
+++ gdb/sparc-sol2-tdep.c 13 Mar 2007 05:38:24 -0000
@@ -188,7 +188,7 @@
   tdep->plt_entry_size = 12;
 
   /* Solaris has kernel-assisted single-stepping support.  */
-  set_gdbarch_software_single_step (gdbarch, NULL);
+  set_gdbarch_software_single_step (gdbarch, default_software_single_step);
 
   frame_unwind_append_sniffer (gdbarch, sparc32_sol2_sigtramp_frame_sniffer);
 }
Index: gdb/arch-utils.c
===================================================================
RCS file: /cvs/src/src/gdb/arch-utils.c,v
retrieving revision 1.143
diff -u -r1.143 arch-utils.c
--- gdb/arch-utils.c 26 Feb 2007 20:13:18 -0000 1.143
+++ gdb/arch-utils.c 13 Mar 2007 05:35:28 -0000
@@ -270,6 +270,13 @@
   return regno;
 }
 
+int
+default_software_single_step (enum target_signal sig,
+      int insert_breakpoints_p)
+{
+  return 0;
+}
+
 
 /* Functions to manipulate the endianness of the target.  */
 
Index: gdb/arch-utils.h
===================================================================
RCS file: /cvs/src/src/gdb/arch-utils.h,v
retrieving revision 1.86
diff -u -r1.86 arch-utils.h
--- gdb/arch-utils.h 26 Feb 2007 20:13:18 -0000 1.86
+++ gdb/arch-utils.h 13 Mar 2007 05:35:28 -0000
@@ -112,6 +112,9 @@
 int default_remote_register_number (struct gdbarch *gdbarch,
     int regno);
 
+extern int default_software_single_step (enum target_signal sig,
+ int insert_breakpoints_p);
+
 /* For compatibility with older architectures, returns
    (LEGACY_SIM_REGNO_IGNORE) when the register doesn't have a valid
    name.  */
Index: gdb/gdbarch.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.h,v
retrieving revision 1.294
diff -u -r1.294 gdbarch.h
--- gdb/gdbarch.h 8 Feb 2007 21:00:29 -0000 1.294
+++ gdb/gdbarch.h 13 Mar 2007 05:36:46 -0000
@@ -1153,23 +1153,8 @@
    FIXME/cagney/2001-01-18: The logic is backwards.  It should be asking if the target can
    single step.  If not, then implement single step using breakpoints. */
 
-#if defined (SOFTWARE_SINGLE_STEP)
-/* Legacy for systems yet to multi-arch SOFTWARE_SINGLE_STEP */
-#if !defined (SOFTWARE_SINGLE_STEP_P)
-#define SOFTWARE_SINGLE_STEP_P() (1)
-#endif
-#endif
-
-extern int gdbarch_software_single_step_p (struct gdbarch *gdbarch);
-#if !defined (GDB_TM_FILE) && defined (SOFTWARE_SINGLE_STEP_P)
-#error "Non multi-arch definition of SOFTWARE_SINGLE_STEP"
-#endif
-#if !defined (SOFTWARE_SINGLE_STEP_P)
-#define SOFTWARE_SINGLE_STEP_P() (gdbarch_software_single_step_p (current_gdbarch))
-#endif
-
-typedef void (gdbarch_software_single_step_ftype) (enum target_signal sig, int insert_breakpoints_p);
-extern void gdbarch_software_single_step (struct gdbarch *gdbarch, enum target_signal sig, int insert_breakpoints_p);
+typedef int (gdbarch_software_single_step_ftype) (enum target_signal sig, int insert_breakpoints_p);
+extern int gdbarch_software_single_step (struct gdbarch *gdbarch, enum target_signal sig, int insert_breakpoints_p);
 extern void set_gdbarch_software_single_step (struct gdbarch *gdbarch, gdbarch_software_single_step_ftype *software_single_step);
 #if !defined (GDB_TM_FILE) && defined (SOFTWARE_SINGLE_STEP)
 #error "Non multi-arch definition of SOFTWARE_SINGLE_STEP"
Index: gdb/arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.224
diff -u -r1.224 arm-tdep.c
--- gdb/arm-tdep.c 27 Feb 2007 20:17:18 -0000 1.224
+++ gdb/arm-tdep.c 13 Mar 2007 05:35:44 -0000
@@ -1907,7 +1907,7 @@
    single_step() is also called just after the inferior stops.  If we
    had set up a simulated single-step, we undo our damage.  */
 
-static void
+static int
 arm_software_single_step (enum target_signal sig, int insert_bpt)
 {
   /* NOTE: This may insert the wrong breakpoint instruction when
@@ -1922,6 +1922,8 @@
     }
   else
     remove_single_step_breakpoints ();
+
+  return 1;
 }
 
 #include "bfd-in2.h"
Index: gdb/mips-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-tdep.c,v
retrieving revision 1.405
diff -u -r1.405 mips-tdep.c
--- gdb/mips-tdep.c 7 Mar 2007 21:32:47 -0000 1.405
+++ gdb/mips-tdep.c 13 Mar 2007 05:38:00 -0000
@@ -2218,6 +2218,8 @@
     }
   else
     remove_single_step_breakpoints ();
+
+  return 1;
 }
 
 /* Test whether the PC points to the return instruction at the
Index: gdb/gdbarch.sh
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.sh,v
retrieving revision 1.376
diff -u -r1.376 gdbarch.sh
--- gdb/gdbarch.sh 28 Feb 2007 17:35:00 -0000 1.376
+++ gdb/gdbarch.sh 13 Mar 2007 05:37:00 -0000
@@ -622,7 +622,7 @@
 #
 # FIXME/cagney/2001-01-18: The logic is backwards.  It should be asking if the target can
 # single step.  If not, then implement single step using breakpoints.
-F:=:void:software_single_step:enum target_signal sig, int insert_breakpoints_p:sig, insert_breakpoints_p
+f:=:int:software_single_step:enum target_signal sig, int insert_breakpoints_p:sig, insert_breakpoints_p::default_software_single_step::0
 # Return non-zero if the processor is executing a delay slot and a
 # further single-step is needed before the instruction finishes.
 M::int:single_step_through_delay:struct frame_info *frame:frame
Index: gdb/gdbarch.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.c,v
retrieving revision 1.338
diff -u -r1.338 gdbarch.c
--- gdb/gdbarch.c 28 Feb 2007 17:34:58 -0000 1.338
+++ gdb/gdbarch.c 13 Mar 2007 05:36:32 -0000
@@ -447,6 +447,7 @@
   current_gdbarch->convert_from_func_ptr_addr = convert_from_func_ptr_addr_identity;
   current_gdbarch->addr_bits_remove = core_addr_identity;
   current_gdbarch->smash_text_address = core_addr_identity;
+  current_gdbarch->software_single_step = default_software_single_step;
   current_gdbarch->skip_trampoline_code = generic_skip_trampoline_code;
   current_gdbarch->skip_solib_resolver = generic_skip_solib_resolver;
   current_gdbarch->in_solib_return_trampoline = generic_in_solib_return_trampoline;
@@ -600,7 +601,7 @@
   /* Skip verify of convert_from_func_ptr_addr, invalid_p == 0 */
   /* Skip verify of addr_bits_remove, invalid_p == 0 */
   /* Skip verify of smash_text_address, invalid_p == 0 */
-  /* Skip verify of software_single_step, has predicate */
+  /* Skip verify of software_single_step, invalid_p == 0 */
   /* Skip verify of single_step_through_delay, has predicate */
   if (current_gdbarch->print_insn == 0)
     fprintf_unfiltered (log, "\n\tprint_insn");
@@ -1510,15 +1511,6 @@
   fprintf_unfiltered (file,
                       "gdbarch_dump: smash_text_address = <0x%lx>\n",
                       (long) current_gdbarch->smash_text_address);
-#ifdef SOFTWARE_SINGLE_STEP_P
-  fprintf_unfiltered (file,
-                      "gdbarch_dump: %s # %s\n",
-                      "SOFTWARE_SINGLE_STEP_P()",
-                      XSTRING (SOFTWARE_SINGLE_STEP_P ()));
-#endif
-  fprintf_unfiltered (file,
-                      "gdbarch_dump: gdbarch_software_single_step_p() = %d\n",
-                      gdbarch_software_single_step_p (current_gdbarch));
 #ifdef SOFTWARE_SINGLE_STEP
   fprintf_unfiltered (file,
                       "gdbarch_dump: %s # %s\n",
@@ -3283,20 +3275,13 @@
 }
 
 int
-gdbarch_software_single_step_p (struct gdbarch *gdbarch)
-{
-  gdb_assert (gdbarch != NULL);
-  return gdbarch->software_single_step != NULL;
-}
-
-void
 gdbarch_software_single_step (struct gdbarch *gdbarch, enum target_signal sig, int insert_breakpoints_p)
 {
   gdb_assert (gdbarch != NULL);
   gdb_assert (gdbarch->software_single_step != NULL);
   if (gdbarch_debug >= 2)
     fprintf_unfiltered (gdb_stdlog, "gdbarch_software_single_step called\n");
-  gdbarch->software_single_step (sig, insert_breakpoints_p);
+  return gdbarch->software_single_step (sig, insert_breakpoints_p);
 }
 
 void
Index: gdb/sparc-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/sparc-tdep.c,v
retrieving revision 1.178
diff -u -r1.178 sparc-tdep.c
--- gdb/sparc-tdep.c 27 Feb 2007 20:17:19 -0000 1.178
+++ gdb/sparc-tdep.c 13 Mar 2007 05:38:33 -0000
@@ -1176,7 +1176,7 @@
   return 0;
 }
 
-void
+int
 sparc_software_single_step (enum target_signal sig, int insert_breakpoints_p)
 {
   struct gdbarch *arch = current_gdbarch;
@@ -1206,6 +1206,8 @@
     }
   else
     remove_single_step_breakpoints ();
+
+  return 1;
 }
 
 static void
Index: gdb/alpha-tdep.h
===================================================================
RCS file: /cvs/src/src/gdb/alpha-tdep.h,v
retrieving revision 1.27
diff -u -r1.27 alpha-tdep.h
--- gdb/alpha-tdep.h 9 Jan 2007 17:58:49 -0000 1.27
+++ gdb/alpha-tdep.h 13 Mar 2007 05:35:23 -0000
@@ -107,7 +107,7 @@
 };
 
 extern unsigned int alpha_read_insn (CORE_ADDR pc);
-extern void alpha_software_single_step (enum target_signal, int);
+extern int alpha_software_single_step (enum target_signal, int);
 extern CORE_ADDR alpha_after_prologue (CORE_ADDR pc);
 
 extern void alpha_mdebug_init_abi (struct gdbarch_info, struct gdbarch *);
Index: gdb/rs6000-tdep.h
===================================================================
RCS file: /cvs/src/src/gdb/rs6000-tdep.h,v
retrieving revision 1.3
diff -u -r1.3 rs6000-tdep.h
--- gdb/rs6000-tdep.h 27 Feb 2007 23:04:28 -0000 1.3
+++ gdb/rs6000-tdep.h 13 Mar 2007 05:38:22 -0000
@@ -21,7 +21,7 @@
 
 #include "defs.h"
 
-extern void rs6000_software_single_step (enum target_signal signal,
+extern int rs6000_software_single_step (enum target_signal signal,
  int insert_breakpoints_p);
 
 /* Hook in rs6000-tdep.c for determining the TOC address when
Index: gdb/alpha-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/alpha-tdep.c,v
retrieving revision 1.162
diff -u -r1.162 alpha-tdep.c
--- gdb/alpha-tdep.c 27 Feb 2007 20:17:18 -0000 1.162
+++ gdb/alpha-tdep.c 13 Mar 2007 05:35:22 -0000
@@ -1518,7 +1518,7 @@
   return (pc + ALPHA_INSN_SIZE);
 }
 
-void
+int
 alpha_software_single_step (enum target_signal sig, int insert_breakpoints_p)
 {
   static CORE_ADDR next_pc;
@@ -1536,6 +1536,8 @@
       remove_single_step_breakpoints ();
       write_pc (next_pc);
     }
+
+  return 1;
 }
 
 
Reply | Threaded
Open this post in threaded view
|

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

Luis Machado-5
In reply to this post by Luis Machado-5
Hi,

This is a modified version of Paul's patch for the single stepping over
atomic instruction sequences.

The main modification is the removal of the portion of code that was
previously added to ppc-linux-tdep.* files. As some people on the
mailing list noticed, it was not really needed. The contents of that
file were transferred to the rs6000-tdep.c file.

rs6000-tdep.c now handles all the functionality for the single stepping
over locking instruction sets, and a suggested modification was included
in the code in order to fix possible internal errors due to inconsistent
handling of breakpoint creation/removal.

Follows the testsuite runs with and without the patch for HEAD.

Testsuite Summary with the patch applied

# of expected passes            11071
# of unexpected failures        356
# of expected failures          42
# of known failures             41
# of unresolved testcases       3
# of untested testcases         11
# of unsupported tests          18

Testsuite Summary without the patch

# of expected passes            11071
# of unexpected failures        357
# of expected failures          42
# of known failures             41
# of unresolved testcases       3
# of untested testcases         10
# of unsupported tests          18

It appears to have no regressions.

I am aware that Emi is currently working on a similar patch, based on
Paul's also. But since this is an issue that's been rolling for long and
that negatively impacts on more specific debugging activities, wouldn't
it be possible to include this patch as a fix while we wait for Emi's
copyright assignment to be ready? After his copyright assignment is
ready, it would be possible for him to incorporate additional changes
that he wrote on top of this one.

Regards,
Luis

single_stepping.diff (21K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Daniel Jacobowitz-2
On Thu, Mar 15, 2007 at 07:24:14PM -0300, Luis Machado wrote:
> I am aware that Emi is currently working on a similar patch, based on
> Paul's also. But since this is an issue that's been rolling for long and
> that negatively impacts on more specific debugging activities, wouldn't
> it be possible to include this patch as a fix while we wait for Emi's
> copyright assignment to be ready? After his copyright assignment is
> ready, it would be possible for him to incorporate additional changes
> that he wrote on top of this one.

I agree.  However, the patch can't be applied as-is.  There are a
couple of cosmetic issues, and at least one syntax error ("return 1",
no semicolon).  Let's do it in two pieces, please.

Could you post a patch which changes the type of the
software_single_step gdbarch method to return int, updates infrun.c,
and nothing else?  Then we can look at the PowerPC bits (which are
more interesting) separately.

When you're editing a bunch of tdep files, please use the
gdb_mbuild.sh script to make sure they all still compile.

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

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

Luis Machado-5
Daniel,

Thanks for your reply. The syntax problem was corrected.

I've ran the gdb_mbuild.sh script and had no problems that are directly
related to my patch. I had a compilation problem with the spu-elf, but
that seems to be in cvs-HEAD as well. I also changed the return type of
the software single step method for spu-elf and the local return values
from the method ("return" to "return 1"), since they were still void.

The wince.c file modification was droped due to its removal from the
tree.

> Could you post a patch which changes the type of the
> software_single_step gdbarch method to return int, updates infrun.c,
> and nothing else?  Then we can look at the PowerPC bits (which are
> more interesting) separately.

Attached follows the patch that only changes the infrun bits and also
the return type of the software_single_step methods for various archs.

> When you're editing a bunch of tdep files, please use the
> gdb_mbuild.sh script to make sure they all still compile.

Thanks for the hint on this.

Regards,
Luis

single_stepping.diff (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Daniel Jacobowitz-2
On Thu, Apr 12, 2007 at 09:09:24AM -0300, Luis Machado wrote:
> Daniel,
>
> Thanks for your reply. The syntax problem was corrected.
>
> I've ran the gdb_mbuild.sh script and had no problems that are directly
> related to my patch. I had a compilation problem with the spu-elf, but
> that seems to be in cvs-HEAD as well. I also changed the return type of
> the software single step method for spu-elf and the local return values
> from the method ("return" to "return 1"), since they were still void.

This version is basically OK.

> 2007-04-12  Luis Machado  <[hidden email]>
>
> * gdbarch.sh: Change the return type of software_single_step from
> void to int and reformatted some comments to <= 80 columns.

        * gdbarch.sh (software_single_step): Change the return type
        from void to int and reformatted some comments to <= 80
        columns.

> * alpha-tdep.c (alpha_software_single_step): Change the return type
> from void to int and always return 1.
> * alpha-tdep.h: Change the return type of alpha_software_single_step
> from void to int.
> * arm-tdep.c (arm_software_single_step): Change the return type from
> void to int and always return 1.

        * alpha-tdep.h (alpha_software_single_step): Likewise.
        * arm-tdep.c (arm_software_single_step): Likewise.

... et cetera.  No need to repeat.

> infrun.c (resume): Check the return value from SOFTWARE_SINGLE_STEP
> and act accordingly.  True means that the software_single_step
> breakpoints where inserted; false means they where not.

        * infrun.c (resume): Check the return value from
        SOFTWARE_SINGLE_STEP and act accordingly.

Note, part of this explanatory comment vanished because we don't put
"why" in ChangeLogs.  This is the sort of thing that one ought to know
when working with the code.  Therefore it should be a comment in the
code.  Please describe the return value in gdbarch.sh.

Thanks!

--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

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

Luis Machado-5
Daniel,

The changes were made according to the suggestions. Changelog updated
and the gdbarch now has a comment on the meaning of the new return value
for the software_single_step methods.

Patch attached.

Thanks!

Regards,
Luis

single_stepping.diff (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Daniel Jacobowitz-2
On Thu, Apr 12, 2007 at 09:53:59AM -0300, Luis Machado wrote:
> +# A return value of 1 means that the software_single_step breakpoints
> +# where inserted; 0 means they where not.

Sorry, two little things: "were" instead of "where", and you need to
rerun gdbarch.sh after changing it (this comment didn't get added to
gdbarch.c).

With those, the patch is OK to commit.  Ulrich, I guess you should do
it since Luis is with IBM?


--
Daniel Jacobowitz
CodeSourcery
Reply | Threaded
Open this post in threaded view
|

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

Luis Machado-5
The typos were corrected and the gdbarch.[c|h] files were updated with
gdbarch.sh, though the comment was just added to gdbarch.h, not
gdbarch.c, is this correct?

Regards,
Luis

single_stepping.diff (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Daniel Jacobowitz-2
On Thu, Apr 12, 2007 at 10:25:26AM -0300, Luis Machado wrote:
> The typos were corrected and the gdbarch.[c|h] files were updated with
> gdbarch.sh, though the comment was just added to gdbarch.h, not
> gdbarch.c, is this correct?

Yes, it is.  This version is fine.

--
Daniel Jacobowitz
CodeSourcery
12