[reverse/record] adjust_pc_after_break in reverse execution mode?

[reverse/record] adjust_pc_after_break in reverse execution mode?

[Pedro Alves]

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

Just noticed this, while looking at the code, so I tried it out against
the record target (x86) on the reverse-20080930-branch branch.

4       int main ()
5       {
6               asm ("nop");
7               asm ("nop");
8               asm ("nop");
9               asm ("nop");
10      }

(gdb) disassemble
Dump of assembler code for function main:
0x08048344 <main+0>:    lea    0x4(%esp),%ecx
0x08048348 <main+4>:    and    $0xfffffff0,%esp
0x0804834b <main+7>:    pushl  -0x4(%ecx)
0x0804834e <main+10>:   push   %ebp
0x0804834f <main+11>:   mov    %esp,%ebp
0x08048351 <main+13>:   push   %ecx
0x08048352 <main+14>:   nop
0x08048353 <main+15>:   nop
0x08048354 <main+16>:   nop
0x08048355 <main+17>:   nop
0x08048356 <main+18>:   pop    %ecx

Now let's try reverse continuing until hitting a breakpoint at 0x8048353 (line 7):

  (gdb) b 7
  Breakpoint 1 at 0x8048353: file nop.c, line 7.
  (gdb) start
  Temporary breakpoint 2 at 0x8048352: file nop.c, line 6.
  Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/nop

  Temporary breakpoint 2, main () at nop.c:6
  6               asm ("nop");
  (gdb) record
  (gdb) n

  Breakpoint 1, main () at nop.c:7
  7               asm ("nop");
  (gdb) n
  8               asm ("nop");
  (gdb) n
  9               asm ("nop");
  (gdb) p $pc
  $1 = (void (*)()) 0x8048355 <main+17>
  (gdb) reverse-continue
  Continuing.

  Breakpoint 1, main () at nop.c:7
  7               asm ("nop");
  (gdb) p $pc
  $1 = (void (*)()) 0x8048353 <main+15>
  (gdb)                             

Now, let's try reverse continuing to a breakpoint at 0x8048353 (line 6),
but this time, let's also sneak a breakpoint at 0x8048352 (line 6):

  (gdb) start
  Temporary breakpoint 1 at 0x8048352: file nop.c, line 6.
  Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/nop

  Temporary breakpoint 1, main () at nop.c:6
  6               asm ("nop");
  (gdb) b 6
  Breakpoint 2 at 0x8048352: file nop.c, line 6.
  (gdb) b 7
  Breakpoint 3 at 0x8048353: file nop.c, line 7.
  (gdb) record
  (gdb) n

  Breakpoint 3, main () at nop.c:7
  7               asm ("nop");
  (gdb) n
  8               asm ("nop");
  (gdb) n
  9               asm ("nop");
  (gdb) p $pc
  $1 = (void (*)()) 0x8048355 <main+17>
  (gdb) reverse-continue
  Continuing.

  Breakpoint 2, main () at nop.c:6
  6               asm ("nop");
  (gdb) p $pc
  $1 = (void (*)()) 0x8048352 <main+14>

Oh-oh.  Not good.

So, in the second example, reverse execution should continue until
breakpoint 3, but, adjust_pc_after_break finds a breakpoint
at `PC - decr_pc_after_break' (1 on x86), adjusts the PC, and then we
report breakpoint 2 being hit.  The first example didn't trip on the
problem, because there was no breakpoint at `PC - 1' when GDB went to
look if adjustment was needed.

I'm guessing the attached patch should be correct for all
targets/archs, or could it be your targets are behaving differently?

-- 
Pedro Alves

[-- Attachment #2: adjust_pc_reverse.diff --]
[-- Type: text/x-diff, Size: 1781 bytes --]

2008-10-18  Pedro Alves  <pedro@codesourcery.com>

	* infrun.c (adjust_pc_after_break): Do nothing if executing in
	reverse.

---
 gdb/infrun.c |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2008-10-18 02:06:15.000000000 +0100
+++ src/gdb/infrun.c	2008-10-18 02:09:36.000000000 +0100
@@ -1787,6 +1787,33 @@ adjust_pc_after_break (struct execution_
   if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP)
     return;
 
+  /* In reverse execution, when a breakpoint is hit, the instruction
+     under it has already been de-executed.  The reported PC always
+     points at the breakpoint address, so adjusting it further would
+     be wrong.  E.g., consider:
+
+       B1         0x08000000 :   INSN1
+       B2         0x08000001 :   INSN2
+		  0x08000002 :   INSN3
+	    PC -> 0x08000003 :   INSN4
+
+      Say you're stopped at 0x08000003 as above.  Reverse continuing
+      from that point should hit B2 as below.  Reading the PC when the
+      SIGTRAP is reported should read 0x08000001 and INSN2 should have
+      been de-executed already.
+
+       B1         0x08000000 :   INSN1
+       B2   PC -> 0x08000001 :   INSN2
+		  0x08000002 :   INSN3
+		  0x08000003 :   INSN4
+
+      If we tried to adjust the PC on for example, a
+      decr_pc_after_break == 1 architecture, we would wrongly further
+      adjust the PC to 0x08000000 and report a hit on B1, although the
+      INSN1 effects hadn't been de-executed yet.  */
+  if (execution_direction == EXEC_REVERSE)
+    return;
+
   /* If this target does not decrement the PC after breakpoints, then
      we have nothing to do.  */
   regcache = get_thread_regcache (ecs->ptid);

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Michael Snyder]

Grump grump -- there's a "consecutive.exp" test in the testsuite,
but your example shows it to be inadequate -- it places two bps
on consecutive instructions, but doesn't make sure that they are
one byte in size.

So naturally, my derived "consecutive-reverse.exp" test
(see the branch) has the same failing.

Got any ideas how we could address this, testsuite-wise?
Ideally we'd like it to be arch-independent...

Pedro Alves wrote:
> Just noticed this, while looking at the code, so I tried it out against
> the record target (x86) on the reverse-20080930-branch branch.
> 
> 4       int main ()
> 5       {
> 6               asm ("nop");
> 7               asm ("nop");
> 8               asm ("nop");
> 9               asm ("nop");
> 10      }
> 
> (gdb) disassemble
> Dump of assembler code for function main:
> 0x08048344 <main+0>:    lea    0x4(%esp),%ecx
> 0x08048348 <main+4>:    and    $0xfffffff0,%esp
> 0x0804834b <main+7>:    pushl  -0x4(%ecx)
> 0x0804834e <main+10>:   push   %ebp
> 0x0804834f <main+11>:   mov    %esp,%ebp
> 0x08048351 <main+13>:   push   %ecx
> 0x08048352 <main+14>:   nop
> 0x08048353 <main+15>:   nop
> 0x08048354 <main+16>:   nop
> 0x08048355 <main+17>:   nop
> 0x08048356 <main+18>:   pop    %ecx
> 
> Now let's try reverse continuing until hitting a breakpoint at 0x8048353 (line 7):
> 
>   (gdb) b 7
>   Breakpoint 1 at 0x8048353: file nop.c, line 7.
>   (gdb) start
>   Temporary breakpoint 2 at 0x8048352: file nop.c, line 6.
>   Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/nop
> 
>   Temporary breakpoint 2, main () at nop.c:6
>   6               asm ("nop");
>   (gdb) record
>   (gdb) n
> 
>   Breakpoint 1, main () at nop.c:7
>   7               asm ("nop");
>   (gdb) n
>   8               asm ("nop");
>   (gdb) n
>   9               asm ("nop");
>   (gdb) p $pc
>   $1 = (void (*)()) 0x8048355 <main+17>
>   (gdb) reverse-continue
>   Continuing.
> 
>   Breakpoint 1, main () at nop.c:7
>   7               asm ("nop");
>   (gdb) p $pc
>   $1 = (void (*)()) 0x8048353 <main+15>
>   (gdb)
> 
> Now, let's try reverse continuing to a breakpoint at 0x8048353 (line 6),
> but this time, let's also sneak a breakpoint at 0x8048352 (line 6):
> 
>   (gdb) start
>   Temporary breakpoint 1 at 0x8048352: file nop.c, line 6.
>   Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/nop
> 
>   Temporary breakpoint 1, main () at nop.c:6
>   6               asm ("nop");
>   (gdb) b 6
>   Breakpoint 2 at 0x8048352: file nop.c, line 6.
>   (gdb) b 7
>   Breakpoint 3 at 0x8048353: file nop.c, line 7.
>   (gdb) record
>   (gdb) n
> 
>   Breakpoint 3, main () at nop.c:7
>   7               asm ("nop");
>   (gdb) n
>   8               asm ("nop");
>   (gdb) n
>   9               asm ("nop");
>   (gdb) p $pc
>   $1 = (void (*)()) 0x8048355 <main+17>
>   (gdb) reverse-continue
>   Continuing.
> 
>   Breakpoint 2, main () at nop.c:6
>   6               asm ("nop");
>   (gdb) p $pc
>   $1 = (void (*)()) 0x8048352 <main+14>
> 
> Oh-oh.  Not good.
> 
> So, in the second example, reverse execution should continue until
> breakpoint 3, but, adjust_pc_after_break finds a breakpoint
> at `PC - decr_pc_after_break' (1 on x86), adjusts the PC, and then we
> report breakpoint 2 being hit.  The first example didn't trip on the
> problem, because there was no breakpoint at `PC - 1' when GDB went to
> look if adjustment was needed.
> 
> I'm guessing the attached patch should be correct for all
> targets/archs, or could it be your targets are behaving differently?
> 
> --
> Pedro Alves
> 
> 
> ------------------------------------------------------------------------
> 
> 2008-10-18  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* infrun.c (adjust_pc_after_break): Do nothing if executing in
> 	reverse.
> 
> ---
>  gdb/infrun.c |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> Index: src/gdb/infrun.c
> ===================================================================
> --- src.orig/gdb/infrun.c	2008-10-18 02:06:15.000000000 +0100
> +++ src/gdb/infrun.c	2008-10-18 02:09:36.000000000 +0100
> @@ -1787,6 +1787,33 @@ adjust_pc_after_break (struct execution_
>    if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP)
>      return;
>  
> +  /* In reverse execution, when a breakpoint is hit, the instruction
> +     under it has already been de-executed.  The reported PC always
> +     points at the breakpoint address, so adjusting it further would
> +     be wrong.  E.g., consider:
> +
> +       B1         0x08000000 :   INSN1
> +       B2         0x08000001 :   INSN2
> +		  0x08000002 :   INSN3
> +	    PC -> 0x08000003 :   INSN4
> +
> +      Say you're stopped at 0x08000003 as above.  Reverse continuing
> +      from that point should hit B2 as below.  Reading the PC when the
> +      SIGTRAP is reported should read 0x08000001 and INSN2 should have
> +      been de-executed already.
> +
> +       B1         0x08000000 :   INSN1
> +       B2   PC -> 0x08000001 :   INSN2
> +		  0x08000002 :   INSN3
> +		  0x08000003 :   INSN4
> +
> +      If we tried to adjust the PC on for example, a
> +      decr_pc_after_break == 1 architecture, we would wrongly further
> +      adjust the PC to 0x08000000 and report a hit on B1, although the
> +      INSN1 effects hadn't been de-executed yet.  */
> +  if (execution_direction == EXEC_REVERSE)
> +    return;
> +
>    /* If this target does not decrement the PC after breakpoints, then
>       we have nothing to do.  */
>    regcache = get_thread_regcache (ecs->ptid);

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

Great! Please check it in maintree.

On Sat, Oct 18, 2008 at 09:10, Pedro Alves <pedro@codesourcery.com> wrote:
> Just noticed this, while looking at the code, so I tried it out against
> the record target (x86) on the reverse-20080930-branch branch.
>
> 4       int main ()
> 5       {
> 6               asm ("nop");
> 7               asm ("nop");
> 8               asm ("nop");
> 9               asm ("nop");
> 10      }
>
> (gdb) disassemble
> Dump of assembler code for function main:
> 0x08048344 <main+0>:    lea    0x4(%esp),%ecx
> 0x08048348 <main+4>:    and    $0xfffffff0,%esp
> 0x0804834b <main+7>:    pushl  -0x4(%ecx)
> 0x0804834e <main+10>:   push   %ebp
> 0x0804834f <main+11>:   mov    %esp,%ebp
> 0x08048351 <main+13>:   push   %ecx
> 0x08048352 <main+14>:   nop
> 0x08048353 <main+15>:   nop
> 0x08048354 <main+16>:   nop
> 0x08048355 <main+17>:   nop
> 0x08048356 <main+18>:   pop    %ecx
>
> Now let's try reverse continuing until hitting a breakpoint at 0x8048353 (line 7):
>
>  (gdb) b 7
>  Breakpoint 1 at 0x8048353: file nop.c, line 7.
>  (gdb) start
>  Temporary breakpoint 2 at 0x8048352: file nop.c, line 6.
>  Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/nop
>
>  Temporary breakpoint 2, main () at nop.c:6
>  6               asm ("nop");
>  (gdb) record
>  (gdb) n
>
>  Breakpoint 1, main () at nop.c:7
>  7               asm ("nop");
>  (gdb) n
>  8               asm ("nop");
>  (gdb) n
>  9               asm ("nop");
>  (gdb) p $pc
>  $1 = (void (*)()) 0x8048355 <main+17>
>  (gdb) reverse-continue
>  Continuing.
>
>  Breakpoint 1, main () at nop.c:7
>  7               asm ("nop");
>  (gdb) p $pc
>  $1 = (void (*)()) 0x8048353 <main+15>
>  (gdb)
>
> Now, let's try reverse continuing to a breakpoint at 0x8048353 (line 6),
> but this time, let's also sneak a breakpoint at 0x8048352 (line 6):
>
>  (gdb) start
>  Temporary breakpoint 1 at 0x8048352: file nop.c, line 6.
>  Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/nop
>
>  Temporary breakpoint 1, main () at nop.c:6
>  6               asm ("nop");
>  (gdb) b 6
>  Breakpoint 2 at 0x8048352: file nop.c, line 6.
>  (gdb) b 7
>  Breakpoint 3 at 0x8048353: file nop.c, line 7.
>  (gdb) record
>  (gdb) n
>
>  Breakpoint 3, main () at nop.c:7
>  7               asm ("nop");
>  (gdb) n
>  8               asm ("nop");
>  (gdb) n
>  9               asm ("nop");
>  (gdb) p $pc
>  $1 = (void (*)()) 0x8048355 <main+17>
>  (gdb) reverse-continue
>  Continuing.
>
>  Breakpoint 2, main () at nop.c:6
>  6               asm ("nop");
>  (gdb) p $pc
>  $1 = (void (*)()) 0x8048352 <main+14>
>
> Oh-oh.  Not good.
>
> So, in the second example, reverse execution should continue until
> breakpoint 3, but, adjust_pc_after_break finds a breakpoint
> at `PC - decr_pc_after_break' (1 on x86), adjusts the PC, and then we
> report breakpoint 2 being hit.  The first example didn't trip on the
> problem, because there was no breakpoint at `PC - 1' when GDB went to
> look if adjustment was needed.
>
> I'm guessing the attached patch should be correct for all
> targets/archs, or could it be your targets are behaving differently?
>
> --
> Pedro Alves
>

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Pedro Alves]

A Saturday 18 October 2008 02:21:38, Michael Snyder wrote:
> Grump grump -- there's a "consecutive.exp" test in the testsuite,
> but your example shows it to be inadequate -- it places two bps
> on consecutive instructions, but doesn't make sure that they are
> one byte in size.
> 
> So naturally, my derived "consecutive-reverse.exp" test
> (see the branch) has the same failing.
> 
> Got any ideas how we could address this, testsuite-wise?
> Ideally we'd like it to be arch-independent...
> 

I think asm ("nop") like below is your best bet.  Is there any assembler
that doesn't understand "nop"?  I believe it should pretty much be a safe
bet that nop will be the smallest possible sized instruction on
variable sized instruction archs, and the same size of a decr_pc_after_break
(or of a breakpoint insn).  Else, the .exp could -DNOP_ASM depending on target,
and the code could use 'asm (NOP_ASM);'.  NOP_ASM could be any instruction
other than jmps and branches, doesn't really have to be a "nop".

> Pedro Alves wrote:
> > Just noticed this, while looking at the code, so I tried it out against
> > the record target (x86) on the reverse-20080930-branch branch.
> > 
> > 4       int main ()
> > 5       {
> > 6               asm ("nop");
> > 7               asm ("nop");
> > 8               asm ("nop");
> > 9               asm ("nop");
> > 10      }
> > 
> > (gdb) disassemble
> > Dump of assembler code for function main:
> > 0x08048344 <main+0>:    lea    0x4(%esp),%ecx
> > 0x08048348 <main+4>:    and    $0xfffffff0,%esp
> > 0x0804834b <main+7>:    pushl  -0x4(%ecx)
> > 0x0804834e <main+10>:   push   %ebp
> > 0x0804834f <main+11>:   mov    %esp,%ebp
> > 0x08048351 <main+13>:   push   %ecx
> > 0x08048352 <main+14>:   nop
> > 0x08048353 <main+15>:   nop
> > 0x08048354 <main+16>:   nop
> > 0x08048355 <main+17>:   nop
> > 0x08048356 <main+18>:   pop    %ecx
> > 
> > Now let's try reverse continuing until hitting a breakpoint at 0x8048353 (line 7):
> > 
> >   (gdb) b 7
> >   Breakpoint 1 at 0x8048353: file nop.c, line 7.
> >   (gdb) start
> >   Temporary breakpoint 2 at 0x8048352: file nop.c, line 6.
> >   Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/nop
> > 
> >   Temporary breakpoint 2, main () at nop.c:6
> >   6               asm ("nop");
> >   (gdb) record
> >   (gdb) n
> > 
> >   Breakpoint 1, main () at nop.c:7
> >   7               asm ("nop");
> >   (gdb) n
> >   8               asm ("nop");
> >   (gdb) n
> >   9               asm ("nop");
> >   (gdb) p $pc
> >   $1 = (void (*)()) 0x8048355 <main+17>
> >   (gdb) reverse-continue
> >   Continuing.
> > 
> >   Breakpoint 1, main () at nop.c:7
> >   7               asm ("nop");
> >   (gdb) p $pc
> >   $1 = (void (*)()) 0x8048353 <main+15>
> >   (gdb)
> > 
> > Now, let's try reverse continuing to a breakpoint at 0x8048353 (line 6),
> > but this time, let's also sneak a breakpoint at 0x8048352 (line 6):
> > 
> >   (gdb) start
> >   Temporary breakpoint 1 at 0x8048352: file nop.c, line 6.
> >   Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/nop
> > 
> >   Temporary breakpoint 1, main () at nop.c:6
> >   6               asm ("nop");
> >   (gdb) b 6
> >   Breakpoint 2 at 0x8048352: file nop.c, line 6.
> >   (gdb) b 7
> >   Breakpoint 3 at 0x8048353: file nop.c, line 7.
> >   (gdb) record
> >   (gdb) n
> > 
> >   Breakpoint 3, main () at nop.c:7
> >   7               asm ("nop");
> >   (gdb) n
> >   8               asm ("nop");
> >   (gdb) n
> >   9               asm ("nop");
> >   (gdb) p $pc
> >   $1 = (void (*)()) 0x8048355 <main+17>
> >   (gdb) reverse-continue
> >   Continuing.
> > 
> >   Breakpoint 2, main () at nop.c:6
> >   6               asm ("nop");
> >   (gdb) p $pc
> >   $1 = (void (*)()) 0x8048352 <main+14>
> > 
> > Oh-oh.  Not good.
> > 
> > So, in the second example, reverse execution should continue until
> > breakpoint 3, but, adjust_pc_after_break finds a breakpoint
> > at `PC - decr_pc_after_break' (1 on x86), adjusts the PC, and then we
> > report breakpoint 2 being hit.  The first example didn't trip on the
> > problem, because there was no breakpoint at `PC - 1' when GDB went to
> > look if adjustment was needed.
> > 
> > I'm guessing the attached patch should be correct for all
> > targets/archs, or could it be your targets are behaving differently?
> > 
> > --
> > Pedro Alves
> > 
> > 
> > ------------------------------------------------------------------------
> > 
> > 2008-10-18  Pedro Alves  <pedro@codesourcery.com>
> > 
> > 	* infrun.c (adjust_pc_after_break): Do nothing if executing in
> > 	reverse.
> > 
> > ---
> >  gdb/infrun.c |   27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> > 
> > Index: src/gdb/infrun.c
> > ===================================================================
> > --- src.orig/gdb/infrun.c	2008-10-18 02:06:15.000000000 +0100
> > +++ src/gdb/infrun.c	2008-10-18 02:09:36.000000000 +0100
> > @@ -1787,6 +1787,33 @@ adjust_pc_after_break (struct execution_
> >    if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP)
> >      return;
> >  
> > +  /* In reverse execution, when a breakpoint is hit, the instruction
> > +     under it has already been de-executed.  The reported PC always
> > +     points at the breakpoint address, so adjusting it further would
> > +     be wrong.  E.g., consider:
> > +
> > +       B1         0x08000000 :   INSN1
> > +       B2         0x08000001 :   INSN2
> > +		  0x08000002 :   INSN3
> > +	    PC -> 0x08000003 :   INSN4
> > +
> > +      Say you're stopped at 0x08000003 as above.  Reverse continuing
> > +      from that point should hit B2 as below.  Reading the PC when the
> > +      SIGTRAP is reported should read 0x08000001 and INSN2 should have
> > +      been de-executed already.
> > +
> > +       B1         0x08000000 :   INSN1
> > +       B2   PC -> 0x08000001 :   INSN2
> > +		  0x08000002 :   INSN3
> > +		  0x08000003 :   INSN4
> > +
> > +      If we tried to adjust the PC on for example, a
> > +      decr_pc_after_break == 1 architecture, we would wrongly further
> > +      adjust the PC to 0x08000000 and report a hit on B1, although the
> > +      INSN1 effects hadn't been de-executed yet.  */
> > +  if (execution_direction == EXEC_REVERSE)
> > +    return;
> > +
> >    /* If this target does not decrement the PC after breakpoints, then
> >       we have nothing to do.  */
> >    regcache = get_thread_regcache (ecs->ptid);
> 
> 



-- 
Pedro Alves

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

Agree with it.

And I think we don't need worry about it very much.
  breakpoint_pc = regcache_read_pc (regcache)
		  - gdbarch_decr_pc_after_break (gdbarch);
Cause most of arch are RISC that have same size insn set (Or most of
them are same such as MIPS16).

In this test, we need use a insn that size is same with breakpint insn.
X86's nop is same breakpint insn. How about other CISC?


On Sat, Oct 18, 2008 at 11:08, Pedro Alves <pedro@codesourcery.com> wrote:
> A Saturday 18 October 2008 02:21:38, Michael Snyder wrote:
>> Grump grump -- there's a "consecutive.exp" test in the testsuite,
>> but your example shows it to be inadequate -- it places two bps
>> on consecutive instructions, but doesn't make sure that they are
>> one byte in size.
>>
>> So naturally, my derived "consecutive-reverse.exp" test
>> (see the branch) has the same failing.
>>
>> Got any ideas how we could address this, testsuite-wise?
>> Ideally we'd like it to be arch-independent...
>>
>
> I think asm ("nop") like below is your best bet.  Is there any assembler
> that doesn't understand "nop"?  I believe it should pretty much be a safe
> bet that nop will be the smallest possible sized instruction on
> variable sized instruction archs, and the same size of a decr_pc_after_break
> (or of a breakpoint insn).  Else, the .exp could -DNOP_ASM depending on target,
> and the code could use 'asm (NOP_ASM);'.  NOP_ASM could be any instruction
> other than jmps and branches, doesn't really have to be a "nop".
>
>> Pedro Alves wrote:
>> > Just noticed this, while looking at the code, so I tried it out against
>> > the record target (x86) on the reverse-20080930-branch branch.
>> >
>> > 4       int main ()
>> > 5       {
>> > 6               asm ("nop");
>> > 7               asm ("nop");
>> > 8               asm ("nop");
>> > 9               asm ("nop");
>> > 10      }
>> >
>> > (gdb) disassemble
>> > Dump of assembler code for function main:
>> > 0x08048344 <main+0>:    lea    0x4(%esp),%ecx
>> > 0x08048348 <main+4>:    and    $0xfffffff0,%esp
>> > 0x0804834b <main+7>:    pushl  -0x4(%ecx)
>> > 0x0804834e <main+10>:   push   %ebp
>> > 0x0804834f <main+11>:   mov    %esp,%ebp
>> > 0x08048351 <main+13>:   push   %ecx
>> > 0x08048352 <main+14>:   nop
>> > 0x08048353 <main+15>:   nop
>> > 0x08048354 <main+16>:   nop
>> > 0x08048355 <main+17>:   nop
>> > 0x08048356 <main+18>:   pop    %ecx
>> >
>> > Now let's try reverse continuing until hitting a breakpoint at 0x8048353 (line 7):
>> >
>> >   (gdb) b 7
>> >   Breakpoint 1 at 0x8048353: file nop.c, line 7.
>> >   (gdb) start
>> >   Temporary breakpoint 2 at 0x8048352: file nop.c, line 6.
>> >   Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/nop
>> >
>> >   Temporary breakpoint 2, main () at nop.c:6
>> >   6               asm ("nop");
>> >   (gdb) record
>> >   (gdb) n
>> >
>> >   Breakpoint 1, main () at nop.c:7
>> >   7               asm ("nop");
>> >   (gdb) n
>> >   8               asm ("nop");
>> >   (gdb) n
>> >   9               asm ("nop");
>> >   (gdb) p $pc
>> >   $1 = (void (*)()) 0x8048355 <main+17>
>> >   (gdb) reverse-continue
>> >   Continuing.
>> >
>> >   Breakpoint 1, main () at nop.c:7
>> >   7               asm ("nop");
>> >   (gdb) p $pc
>> >   $1 = (void (*)()) 0x8048353 <main+15>
>> >   (gdb)
>> >
>> > Now, let's try reverse continuing to a breakpoint at 0x8048353 (line 6),
>> > but this time, let's also sneak a breakpoint at 0x8048352 (line 6):
>> >
>> >   (gdb) start
>> >   Temporary breakpoint 1 at 0x8048352: file nop.c, line 6.
>> >   Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/nop
>> >
>> >   Temporary breakpoint 1, main () at nop.c:6
>> >   6               asm ("nop");
>> >   (gdb) b 6
>> >   Breakpoint 2 at 0x8048352: file nop.c, line 6.
>> >   (gdb) b 7
>> >   Breakpoint 3 at 0x8048353: file nop.c, line 7.
>> >   (gdb) record
>> >   (gdb) n
>> >
>> >   Breakpoint 3, main () at nop.c:7
>> >   7               asm ("nop");
>> >   (gdb) n
>> >   8               asm ("nop");
>> >   (gdb) n
>> >   9               asm ("nop");
>> >   (gdb) p $pc
>> >   $1 = (void (*)()) 0x8048355 <main+17>
>> >   (gdb) reverse-continue
>> >   Continuing.
>> >
>> >   Breakpoint 2, main () at nop.c:6
>> >   6               asm ("nop");
>> >   (gdb) p $pc
>> >   $1 = (void (*)()) 0x8048352 <main+14>
>> >
>> > Oh-oh.  Not good.
>> >
>> > So, in the second example, reverse execution should continue until
>> > breakpoint 3, but, adjust_pc_after_break finds a breakpoint
>> > at `PC - decr_pc_after_break' (1 on x86), adjusts the PC, and then we
>> > report breakpoint 2 being hit.  The first example didn't trip on the
>> > problem, because there was no breakpoint at `PC - 1' when GDB went to
>> > look if adjustment was needed.
>> >
>> > I'm guessing the attached patch should be correct for all
>> > targets/archs, or could it be your targets are behaving differently?
>> >
>> > --
>> > Pedro Alves
>> >
>> >
>> > ------------------------------------------------------------------------
>> >
>> > 2008-10-18  Pedro Alves  <pedro@codesourcery.com>
>> >
>> >     * infrun.c (adjust_pc_after_break): Do nothing if executing in
>> >     reverse.
>> >
>> > ---
>> >  gdb/infrun.c |   27 +++++++++++++++++++++++++++
>> >  1 file changed, 27 insertions(+)
>> >
>> > Index: src/gdb/infrun.c
>> > ===================================================================
>> > --- src.orig/gdb/infrun.c   2008-10-18 02:06:15.000000000 +0100
>> > +++ src/gdb/infrun.c        2008-10-18 02:09:36.000000000 +0100
>> > @@ -1787,6 +1787,33 @@ adjust_pc_after_break (struct execution_
>> >    if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP)
>> >      return;
>> >
>> > +  /* In reverse execution, when a breakpoint is hit, the instruction
>> > +     under it has already been de-executed.  The reported PC always
>> > +     points at the breakpoint address, so adjusting it further would
>> > +     be wrong.  E.g., consider:
>> > +
>> > +       B1         0x08000000 :   INSN1
>> > +       B2         0x08000001 :   INSN2
>> > +             0x08000002 :   INSN3
>> > +       PC -> 0x08000003 :   INSN4
>> > +
>> > +      Say you're stopped at 0x08000003 as above.  Reverse continuing
>> > +      from that point should hit B2 as below.  Reading the PC when the
>> > +      SIGTRAP is reported should read 0x08000001 and INSN2 should have
>> > +      been de-executed already.
>> > +
>> > +       B1         0x08000000 :   INSN1
>> > +       B2   PC -> 0x08000001 :   INSN2
>> > +             0x08000002 :   INSN3
>> > +             0x08000003 :   INSN4
>> > +
>> > +      If we tried to adjust the PC on for example, a
>> > +      decr_pc_after_break == 1 architecture, we would wrongly further
>> > +      adjust the PC to 0x08000000 and report a hit on B1, although the
>> > +      INSN1 effects hadn't been de-executed yet.  */
>> > +  if (execution_direction == EXEC_REVERSE)
>> > +    return;
>> > +
>> >    /* If this target does not decrement the PC after breakpoints, then
>> >       we have nothing to do.  */
>> >    regcache = get_thread_regcache (ecs->ptid);
>>
>>
>
>
>
> --
> Pedro Alves
>

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Pedro Alves]

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

On Saturday 18 October 2008 04:06:29, teawater wrote:
> Great! Please check it in maintree.

Done.  I've revised the text a tiny bit, like below.

-- 
Pedro Alves

[-- Attachment #2: adjust_pc_reverse.diff --]
[-- Type: text/x-diff, Size: 1885 bytes --]

2008-10-18  Pedro Alves  <pedro@codesourcery.com>

	* infrun.c (adjust_pc_after_break): Do nothing if executing in
	reverse.

---
 gdb/infrun.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Index: src/gdb/infrun.c
===================================================================
--- src.orig/gdb/infrun.c	2008-10-18 00:43:46.000000000 +0100
+++ src/gdb/infrun.c	2008-10-18 04:18:55.000000000 +0100
@@ -1826,6 +1826,35 @@ adjust_pc_after_break (struct execution_
   if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP)
     return;
 
+  /* In reverse execution, when a breakpoint is hit, the instruction
+     under it has already been de-executed.  The reported PC always
+     points at the breakpoint address, so adjusting it further would
+     be wrong.  E.g., consider this case on a decr_pc_after_break == 1
+     architecture:
+
+       B1         0x08000000 :   INSN1
+       B2         0x08000001 :   INSN2
+		  0x08000002 :   INSN3
+	    PC -> 0x08000003 :   INSN4
+
+     Say you're stopped at 0x08000003 as above.  Reverse continuing
+     from that point should hit B2 as below.  Reading the PC when the
+     SIGTRAP is reported should read 0x08000001 and INSN2 should have
+     been de-executed already.
+
+       B1         0x08000000 :   INSN1
+       B2   PC -> 0x08000001 :   INSN2
+		  0x08000002 :   INSN3
+		  0x08000003 :   INSN4
+
+     We can't apply the same logic as for forward execution, because
+     we would wrongly adjust the PC to 0x08000000, since there's a
+     breakpoint at PC - 1.  We'd then report a hit on B1, although
+     INSN1 hadn't been de-executed yet.  Doing nothing is the correct
+     behaviour.  */
+  if (execution_direction == EXEC_REVERSE)
+    return;
+
   /* If this target does not decrement the PC after breakpoints, then
      we have nothing to do.  */
   regcache = get_thread_regcache (ecs->ptid);

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Andreas Schwab]

Pedro Alves <pedro@codesourcery.com> writes:

> I think asm ("nop") like below is your best bet.  Is there any assembler
> that doesn't understand "nop"?

ia64 wants "nop 0".

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
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."

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

I try "asm ("nop 0");" in with x86-32 gcc got insn:
 8048352:       0f 1f 05 00 00 00 00    nopl   0x0

On Sat, Oct 18, 2008 at 16:41, Andreas Schwab <schwab@suse.de> wrote:
> Pedro Alves <pedro@codesourcery.com> writes:
>
>> I think asm ("nop") like below is your best bet.  Is there any assembler
>> that doesn't understand "nop"?
>
> ia64 wants "nop 0".
>
> Andreas.
>
> --
> Andreas Schwab, SuSE Labs, schwab@suse.de
> 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."
>

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Daniel Jacobowitz]

On Sat, Oct 18, 2008 at 04:08:42AM +0100, Pedro Alves wrote:
> I think asm ("nop") like below is your best bet.  Is there any assembler
> that doesn't understand "nop"?

Compare with GCC's configure script, which has a list of nops for
various architectures.  Or compare with asm-source.exp and its
per-arch fragments.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Michael Snyder]

After codgitating for a bit (that's "thinking" when you're over 50),
I've decided that you're right.

However, I have a new concern -- I'm worried about what it will do
when it's replaying but going forward.

Could you possibly revisit your test and see what it does
if you record all the way to line 9 or 10, then back up
to line 6, then continue with breakpoints at 6 and 7?

Pedro Alves wrote:
> Just noticed this, while looking at the code, so I tried it out against
> the record target (x86) on the reverse-20080930-branch branch.
> 
> 4       int main ()
> 5       {
> 6               asm ("nop");
> 7               asm ("nop");
> 8               asm ("nop");
> 9               asm ("nop");
> 10      }
> 
> (gdb) disassemble
> Dump of assembler code for function main:
> 0x08048344 <main+0>:    lea    0x4(%esp),%ecx
> 0x08048348 <main+4>:    and    $0xfffffff0,%esp
> 0x0804834b <main+7>:    pushl  -0x4(%ecx)
> 0x0804834e <main+10>:   push   %ebp
> 0x0804834f <main+11>:   mov    %esp,%ebp
> 0x08048351 <main+13>:   push   %ecx
> 0x08048352 <main+14>:   nop
> 0x08048353 <main+15>:   nop
> 0x08048354 <main+16>:   nop
> 0x08048355 <main+17>:   nop
> 0x08048356 <main+18>:   pop    %ecx
> 
> Now let's try reverse continuing until hitting a breakpoint at 0x8048353 (line 7):
> 
>   (gdb) b 7
>   Breakpoint 1 at 0x8048353: file nop.c, line 7.
>   (gdb) start
>   Temporary breakpoint 2 at 0x8048352: file nop.c, line 6.
>   Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/nop
> 
>   Temporary breakpoint 2, main () at nop.c:6
>   6               asm ("nop");
>   (gdb) record
>   (gdb) n
> 
>   Breakpoint 1, main () at nop.c:7
>   7               asm ("nop");
>   (gdb) n
>   8               asm ("nop");
>   (gdb) n
>   9               asm ("nop");
>   (gdb) p $pc
>   $1 = (void (*)()) 0x8048355 <main+17>
>   (gdb) reverse-continue
>   Continuing.
> 
>   Breakpoint 1, main () at nop.c:7
>   7               asm ("nop");
>   (gdb) p $pc
>   $1 = (void (*)()) 0x8048353 <main+15>
>   (gdb)
> 
> Now, let's try reverse continuing to a breakpoint at 0x8048353 (line 6),
> but this time, let's also sneak a breakpoint at 0x8048352 (line 6):
> 
>   (gdb) start
>   Temporary breakpoint 1 at 0x8048352: file nop.c, line 6.
>   Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/nop
> 
>   Temporary breakpoint 1, main () at nop.c:6
>   6               asm ("nop");
>   (gdb) b 6
>   Breakpoint 2 at 0x8048352: file nop.c, line 6.
>   (gdb) b 7
>   Breakpoint 3 at 0x8048353: file nop.c, line 7.
>   (gdb) record
>   (gdb) n
> 
>   Breakpoint 3, main () at nop.c:7
>   7               asm ("nop");
>   (gdb) n
>   8               asm ("nop");
>   (gdb) n
>   9               asm ("nop");
>   (gdb) p $pc
>   $1 = (void (*)()) 0x8048355 <main+17>
>   (gdb) reverse-continue
>   Continuing.
> 
>   Breakpoint 2, main () at nop.c:6
>   6               asm ("nop");
>   (gdb) p $pc
>   $1 = (void (*)()) 0x8048352 <main+14>
> 
> Oh-oh.  Not good.
> 
> So, in the second example, reverse execution should continue until
> breakpoint 3, but, adjust_pc_after_break finds a breakpoint
> at `PC - decr_pc_after_break' (1 on x86), adjusts the PC, and then we
> report breakpoint 2 being hit.  The first example didn't trip on the
> problem, because there was no breakpoint at `PC - 1' when GDB went to
> look if adjustment was needed.
> 
> I'm guessing the attached patch should be correct for all
> targets/archs, or could it be your targets are behaving differently?
> 
> --
> Pedro Alves
> 
> 
> ------------------------------------------------------------------------
> 
> 2008-10-18  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* infrun.c (adjust_pc_after_break): Do nothing if executing in
> 	reverse.
> 
> ---
>  gdb/infrun.c |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> Index: src/gdb/infrun.c
> ===================================================================
> --- src.orig/gdb/infrun.c	2008-10-18 02:06:15.000000000 +0100
> +++ src/gdb/infrun.c	2008-10-18 02:09:36.000000000 +0100
> @@ -1787,6 +1787,33 @@ adjust_pc_after_break (struct execution_
>    if (ecs->ws.value.sig != TARGET_SIGNAL_TRAP)
>      return;
>  
> +  /* In reverse execution, when a breakpoint is hit, the instruction
> +     under it has already been de-executed.  The reported PC always
> +     points at the breakpoint address, so adjusting it further would
> +     be wrong.  E.g., consider:
> +
> +       B1         0x08000000 :   INSN1
> +       B2         0x08000001 :   INSN2
> +		  0x08000002 :   INSN3
> +	    PC -> 0x08000003 :   INSN4
> +
> +      Say you're stopped at 0x08000003 as above.  Reverse continuing
> +      from that point should hit B2 as below.  Reading the PC when the
> +      SIGTRAP is reported should read 0x08000001 and INSN2 should have
> +      been de-executed already.
> +
> +       B1         0x08000000 :   INSN1
> +       B2   PC -> 0x08000001 :   INSN2
> +		  0x08000002 :   INSN3
> +		  0x08000003 :   INSN4
> +
> +      If we tried to adjust the PC on for example, a
> +      decr_pc_after_break == 1 architecture, we would wrongly further
> +      adjust the PC to 0x08000000 and report a hit on B1, although the
> +      INSN1 effects hadn't been de-executed yet.  */
> +  if (execution_direction == EXEC_REVERSE)
> +    return;
> +
>    /* If this target does not decrement the PC after breakpoints, then
>       we have nothing to do.  */
>    regcache = get_thread_regcache (ecs->ptid);

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Pedro Alves]

On Sunday 19 October 2008 23:39:20, Michael Snyder wrote:
> After codgitating for a bit (that's "thinking" when you're over 50),
> I've decided that you're right.
> 
> However, I have a new concern -- I'm worried about what it will do
> when it's replaying but going forward.
> 
> Could you possibly revisit your test and see what it does
> if you record all the way to line 9 or 10, then back up
> to line 6, then continue with breakpoints at 6 and 7?

Eh, you're right.  It's broken.

(gdb) record
(gdb) b 6
Breakpoint 2 at 0x8048352: file nop.c, line 6.
(gdb) b 7
Breakpoint 3 at 0x8048353: file nop.c, line 7.
(gdb) n

Breakpoint 3, main () at nop.c:7
7               asm ("nop");
(gdb) n
8               asm ("nop");
(gdb)
9               asm ("nop");
(gdb) n
10      }
(gdb) rc
Continuing.

Breakpoint 3, main () at nop.c:7
7               asm ("nop");
(gdb) rn

No more reverse-execution history.
main () at nop.c:6
6               asm ("nop");
(gdb) n

Breakpoint 2, main () at nop.c:6
6               asm ("nop");
(gdb)
8               asm ("nop");
(gdb)
9               asm ("nop");
(gdb)



-- 
Pedro Alves

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Michael Snyder]

Pedro Alves wrote:
> On Sunday 19 October 2008 23:39:20, Michael Snyder wrote:
>> After codgitating for a bit (that's "thinking" when you're over 50),
>> I've decided that you're right.
>>
>> However, I have a new concern -- I'm worried about what it will do
>> when it's replaying but going forward.
>>
>> Could you possibly revisit your test and see what it does
>> if you record all the way to line 9 or 10, then back up
>> to line 6, then continue with breakpoints at 6 and 7?
> 
> Eh, you're right.  It's broken.

Thought so.

See, the problem is that "adjust_pc_after_break" is assuming
memory-breakpoint semantics, but Process Record/Replay actually
implements hardware-breakpoint semantics.  It watches the
instruction-address "bus" and stops when the PC matches the
address of a breakpoint.

I suspect this is probably a problem with other record/replay
back-ends too, but I haven't confirmed it yet.

Still, I think that the patch you committed was correct
for the reverse case.  This is a corner case that reveals
that "reverse" and "replay" are not synonymous.

> (gdb) record
> (gdb) b 6
> Breakpoint 2 at 0x8048352: file nop.c, line 6.
> (gdb) b 7
> Breakpoint 3 at 0x8048353: file nop.c, line 7.
> (gdb) n
> 
> Breakpoint 3, main () at nop.c:7
> 7               asm ("nop");
> (gdb) n
> 8               asm ("nop");
> (gdb)
> 9               asm ("nop");
> (gdb) n
> 10      }
> (gdb) rc
> Continuing.
> 
> Breakpoint 3, main () at nop.c:7
> 7               asm ("nop");
> (gdb) rn
> 
> No more reverse-execution history.
> main () at nop.c:6
> 6               asm ("nop");
> (gdb) n
> 
> Breakpoint 2, main () at nop.c:6
> 6               asm ("nop");
> (gdb)
> 8               asm ("nop");
> (gdb)
> 9               asm ("nop");
> (gdb)
> 
> 
> 
> --
> Pedro Alves

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Daniel Jacobowitz]

On Sun, Oct 19, 2008 at 05:39:30PM -0700, Michael Snyder wrote:
> See, the problem is that "adjust_pc_after_break" is assuming
> memory-breakpoint semantics, but Process Record/Replay actually
> implements hardware-breakpoint semantics.  It watches the
> instruction-address "bus" and stops when the PC matches the
> address of a breakpoint.

Don't x86 hardware breakpoints behave the same as x86 software
breakpoints in this regard?

I'd suggest the replay target make this as simple as possible for the
rest of GDB: increment the PC by decr_pc_after_break.  Or is that
going to cause some other problem?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Pedro Alves]

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

A Monday 20 October 2008 01:39:30, Michael Snyder escreveu:
> Pedro Alves wrote:
> > On Sunday 19 October 2008 23:39:20, Michael Snyder wrote:
> >> After codgitating for a bit (that's "thinking" when you're over 50),
> >> I've decided that you're right.
> >>
> >> However, I have a new concern -- I'm worried about what it will do
> >> when it's replaying but going forward.
> >>
> >> Could you possibly revisit your test and see what it does
> >> if you record all the way to line 9 or 10, then back up
> >> to line 6, then continue with breakpoints at 6 and 7?
> > 
> > Eh, you're right.  It's broken.
> 
> Thought so.
> 
> See, the problem is that "adjust_pc_after_break" is assuming
> memory-breakpoint semantics, but Process Record/Replay actually
> implements hardware-breakpoint semantics.  It watches the
> instruction-address "bus" and stops when the PC matches the
> address of a breakpoint.
> 
> I suspect this is probably a problem with other record/replay
> back-ends too, but I haven't confirmed it yet.
> 
> Still, I think that the patch you committed was correct
> for the reverse case.  

> This is a corner case that reveals 
> that "reverse" and "replay" are not synonymous.

They certainly aren't.  When replaying, I believe it's just best to
behave as close as possible to when it the inferior is really running.
From the inferior control side, GDB be mostly as agnostic about
"replay" vs normal run as possibly.

IIUC from reading the code, I see two issues.

 1) When going forward and in reply mode, breakpoint hits are being checked
    *after* a record item is replays.  IIUC, we should check *before*,
    and report an adjusted PC.

 2) Un-inserted breakpoints weren't accounted for AFAICT (GDB will
    un-inserted breakpoints temporarily when stepping over them).
    Maybe they are, I got lost.  :-)  There's a loop going through the
    bp_location_chain.  Can you get rid of that and use
    regular_breakpoint_inserted_here_p or similars?

Below is a 10 minutes hack at it, as a starting point.  Replay stil
isn't perfect, mainly because I got lost in the record_wait maze --- that,
needs a bit of clean up.  :-)

> 
> > (gdb) record
> > (gdb) b 6
> > Breakpoint 2 at 0x8048352: file nop.c, line 6.
> > (gdb) b 7
> > Breakpoint 3 at 0x8048353: file nop.c, line 7.
> > (gdb) n
> > 
> > Breakpoint 3, main () at nop.c:7
> > 7               asm ("nop");
> > (gdb) n
> > 8               asm ("nop");
> > (gdb)
> > 9               asm ("nop");
> > (gdb) n
> > 10      }
> > (gdb) rc
> > Continuing.
> > 
> > Breakpoint 3, main () at nop.c:7
> > 7               asm ("nop");
> > (gdb) rn
> > 
> > No more reverse-execution history.
> > main () at nop.c:6
> > 6               asm ("nop");
> > (gdb) n
> > 
> > Breakpoint 2, main () at nop.c:6
> > 6               asm ("nop");
> > (gdb)
> > 8               asm ("nop");
> > (gdb)
> > 9               asm ("nop");
> > (gdb)
> > 
> > 
> > 
> > --
> > Pedro Alves
> 
> 



-- 
Pedro Alves

[-- Attachment #2: record_decr_pc.diff --]
[-- Type: text/x-diff, Size: 8073 bytes --]

---
 gdb/record.c |  159 +++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 101 insertions(+), 58 deletions(-)

Index: src/gdb/record.c
===================================================================
--- src.orig/gdb/record.c	2008-10-20 00:48:50.000000000 +0100
+++ src/gdb/record.c	2008-10-20 13:02:38.000000000 +0100
@@ -497,6 +497,9 @@ record_wait (ptid_t ptid, struct target_
       int continue_flag = 1;
       int first_record_end = 1;
       struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
+      CORE_ADDR pc;
+      record_t *curr_record;
+      int first = 1;
 
       record_get_sig = 0;
       act.sa_handler = record_sig_handler;
@@ -512,20 +515,13 @@ record_wait (ptid_t ptid, struct target_
          Then set it to terminal_ours to make GDB get the signal.  */
       target_terminal_ours ();
 
-      /* In EXEC_FORWARD mode, record_list point to the tail of prev
-         instruction.  */
-      if (execution_direction == EXEC_FORWARD && record_list->next)
-        {
-	  record_list = record_list->next;
-	}
-
       /* Loop over the record_list, looking for the next place to
 	 stop.  */
       status->kind = TARGET_WAITKIND_STOPPED;
       do
 	{
 	  /* Check for beginning and end of log.  */
-	  if (execution_direction == EXEC_REVERSE 
+	  if (execution_direction == EXEC_REVERSE
 	      && record_list == &record_first)
 	    {
 	      /* Hit beginning of record log in reverse.  */
@@ -539,8 +535,51 @@ record_wait (ptid_t ptid, struct target_
 	      break;
 	    }
 
+	  /* Check for breakpoint hits in forward execution.  */
+	  pc = read_pc ();
+	  if (execution_direction == EXEC_FORWARD
+	      && regular_breakpoint_inserted_here_p (pc)
+	      /* && !single-stepping */)
+	    {
+	      status->kind = TARGET_WAITKIND_STOPPED;
+	      status->value.sig = TARGET_SIGNAL_TRAP;
+	      if (software_breakpoint_inserted_here_p (pc))
+		pc += gdbarch_decr_pc_after_break (current_gdbarch);
+	      write_pc (pc);
+
+	      if (sigaction (SIGALRM, &old_act, NULL))
+		perror_with_name (_("Process record: sigaction"));
+
+	      discard_cleanups (old_cleanups);
+	      return inferior_ptid;
+	    }
+
+	  if (first)
+	    {
+	      first = 0;
+	      /* In EXEC_FORWARD mode, record_list point to the tail of prev
+		 instruction.  */
+	      if (execution_direction == EXEC_FORWARD && record_list->next)
+		{
+		  record_list = record_list->next;
+		}
+	    }
+
+	  curr_record = record_list;
+
+	  if (execution_direction == EXEC_REVERSE)
+	    {
+	      if (record_list->prev)
+		record_list = record_list->prev;
+	    }
+	  else
+	    {
+	      if (record_list->next)
+		record_list = record_list->next;
+	    }
+
 	  /* set ptid, register and memory according to record_list */
-	  if (record_list->type == record_reg)
+	  if (curr_record->type == record_reg)
 	    {
 	      /* reg */
 	      gdb_byte reg[MAX_REGISTER_SIZE];
@@ -548,43 +587,43 @@ record_wait (ptid_t ptid, struct target_
 		{
 		  fprintf_unfiltered (gdb_stdlog,
 				      "Process record: record_reg 0x%s to inferior num = %d.\n",
-				      paddr_nz ((CORE_ADDR)record_list),
-				      record_list->u.reg.num);
+				      paddr_nz ((CORE_ADDR)curr_record),
+				      curr_record->u.reg.num);
 		}
-	      regcache_cooked_read (regcache, record_list->u.reg.num, reg);
-	      regcache_cooked_write (regcache, record_list->u.reg.num,
-				     record_list->u.reg.val);
-	      memcpy (record_list->u.reg.val, reg, MAX_REGISTER_SIZE);
+	      regcache_cooked_read (regcache, curr_record->u.reg.num, reg);
+	      regcache_cooked_write (regcache, curr_record->u.reg.num,
+				     curr_record->u.reg.val);
+	      memcpy (curr_record->u.reg.val, reg, MAX_REGISTER_SIZE);
 	    }
-	  else if (record_list->type == record_mem)
+	  else if (curr_record->type == record_mem)
 	    {
 	      /* mem */
-	      gdb_byte *mem = alloca (record_list->u.mem.len);
+	      gdb_byte *mem = alloca (curr_record->u.mem.len);
 	      if (record_debug > 1)
 		{
 		  fprintf_unfiltered (gdb_stdlog,
 				      "Process record: record_mem 0x%s to inferior addr = 0x%s len = %d.\n",
-				      paddr_nz ((CORE_ADDR)record_list),
-				      paddr_nz (record_list->u.mem.addr),
-				      record_list->u.mem.len);
+				      paddr_nz ((CORE_ADDR)curr_record),
+				      paddr_nz (curr_record->u.mem.addr),
+				      curr_record->u.mem.len);
 		}
 	      if (target_read_memory
-		  (record_list->u.mem.addr, mem, record_list->u.mem.len))
+		  (curr_record->u.mem.addr, mem, curr_record->u.mem.len))
 		{
 		  error (_("Process record: read memory addr = 0x%s len = %d error."),
-			 paddr_nz (record_list->u.mem.addr),
-			 record_list->u.mem.len);
+			 paddr_nz (curr_record->u.mem.addr),
+			 curr_record->u.mem.len);
 		}
 	      if (target_write_memory
-		  (record_list->u.mem.addr, record_list->u.mem.val,
-		   record_list->u.mem.len))
+		  (curr_record->u.mem.addr, curr_record->u.mem.val,
+		   curr_record->u.mem.len))
 		{
 		  error (_
 			 ("Process record: write memory addr = 0x%s len = %d error."),
-			 paddr_nz (record_list->u.mem.addr),
-			 record_list->u.mem.len);
+			 paddr_nz (curr_record->u.mem.addr),
+			 curr_record->u.mem.len);
 		}
-	      memcpy (record_list->u.mem.val, mem, record_list->u.mem.len);
+	      memcpy (curr_record->u.mem.val, mem, curr_record->u.mem.len);
 	    }
 	  else
 	    {
@@ -596,13 +635,13 @@ record_wait (ptid_t ptid, struct target_
 		{
 		  fprintf_unfiltered (gdb_stdlog,
 				      "Process record: record_end 0x%s to inferior need_dasm = %d.\n",
-				      paddr_nz ((CORE_ADDR)record_list),
-				      record_list->u.need_dasm);
+				      paddr_nz ((CORE_ADDR)curr_record),
+				      curr_record->u.need_dasm);
 		}
 
 	      if (execution_direction == EXEC_FORWARD)
 		{
-		  need_dasm = record_list->u.need_dasm;
+		  need_dasm = curr_record->u.need_dasm;
 		}
 	      if (need_dasm)
 		{
@@ -631,45 +670,48 @@ record_wait (ptid_t ptid, struct target_
 		      continue_flag = 0;
 		    }
 
-		  /* check breakpoint */
-		  tmp_pc = read_pc ();
-		  for (bl = bp_location_chain; bl; bl = bl->global_next)
+		  if (execution_direction == EXEC_REVERSE)
 		    {
-		      b = bl->owner;
-		      gdb_assert (b);
-		      if (b->enable_state != bp_enabled
-			  && b->enable_state != bp_permanent)
-			continue;
-
-		      if (b->type == bp_watchpoint || b->type == bp_catch_fork
-			  || b->type == bp_catch_vfork
-			  || b->type == bp_catch_exec
-			  || b->type == bp_hardware_watchpoint
-			  || b->type == bp_read_watchpoint
-			  || b->type == bp_access_watchpoint)
-			{
-			  continue;
-			}
-		      if (bl->address == tmp_pc)
+		      /* check breakpoint */
+		      tmp_pc = read_pc ();
+		      for (bl = bp_location_chain; bl; bl = bl->global_next)
 			{
-			  if (record_debug)
+			  b = bl->owner;
+			  gdb_assert (b);
+			  if (b->enable_state != bp_enabled
+			      && b->enable_state != bp_permanent)
+			    continue;
+
+			  if (b->type == bp_watchpoint || b->type == bp_catch_fork
+			      || b->type == bp_catch_vfork
+			      || b->type == bp_catch_exec
+			      || b->type == bp_hardware_watchpoint
+			      || b->type == bp_read_watchpoint
+			      || b->type == bp_access_watchpoint)
+			    {
+			      continue;
+			    }
+			  if (bl->address == tmp_pc)
 			    {
-			      fprintf_unfiltered (gdb_stdlog,
-						  "Process record: break at 0x%s.\n",
-						  paddr_nz (tmp_pc));
+			      if (record_debug)
+				{
+				  fprintf_unfiltered (gdb_stdlog,
+						      "Process record: break at 0x%s.\n",
+						      paddr_nz (tmp_pc));
+				}
+			      continue_flag = 0;
+			      break;
 			    }
-			  continue_flag = 0;
-			  break;
 			}
 		    }
 		}
 	      if (execution_direction == EXEC_REVERSE)
 		{
-		  need_dasm = record_list->u.need_dasm;
+		  need_dasm = curr_record->u.need_dasm;
 		}
 	    }
 
-next:
+#if 0
 	  if (continue_flag)
 	    {
 	      if (execution_direction == EXEC_REVERSE)
@@ -683,6 +725,7 @@ next:
 		    record_list = record_list->next;
 		}
 	    }
+#endif
 	}
       while (continue_flag);
 

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

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

Thanks Pedro.

I make a patch too.


2008-10-20  Hui Zhu  <teawater@gmail.com>

	* record.c (record_wait): Check breakpint before forward
	execute in replay mode.
	Check breakpoint use function "breakpoint_inserted_here_p".

Thanks,
Hui

On Mon, Oct 20, 2008 at 20:09, Pedro Alves <pedro@codesourcery.com> wrote:
> A Monday 20 October 2008 01:39:30, Michael Snyder escreveu:
>> Pedro Alves wrote:
>> > On Sunday 19 October 2008 23:39:20, Michael Snyder wrote:
>> >> After codgitating for a bit (that's "thinking" when you're over 50),
>> >> I've decided that you're right.
>> >>
>> >> However, I have a new concern -- I'm worried about what it will do
>> >> when it's replaying but going forward.
>> >>
>> >> Could you possibly revisit your test and see what it does
>> >> if you record all the way to line 9 or 10, then back up
>> >> to line 6, then continue with breakpoints at 6 and 7?
>> >
>> > Eh, you're right.  It's broken.
>>
>> Thought so.
>>
>> See, the problem is that "adjust_pc_after_break" is assuming
>> memory-breakpoint semantics, but Process Record/Replay actually
>> implements hardware-breakpoint semantics.  It watches the
>> instruction-address "bus" and stops when the PC matches the
>> address of a breakpoint.
>>
>> I suspect this is probably a problem with other record/replay
>> back-ends too, but I haven't confirmed it yet.
>>
>> Still, I think that the patch you committed was correct
>> for the reverse case.
>
>> This is a corner case that reveals
>> that "reverse" and "replay" are not synonymous.
>
> They certainly aren't.  When replaying, I believe it's just best to
> behave as close as possible to when it the inferior is really running.
> From the inferior control side, GDB be mostly as agnostic about
> "replay" vs normal run as possibly.
>
> IIUC from reading the code, I see two issues.
>
>  1) When going forward and in reply mode, breakpoint hits are being checked
>    *after* a record item is replays.  IIUC, we should check *before*,
>    and report an adjusted PC.
>
>  2) Un-inserted breakpoints weren't accounted for AFAICT (GDB will
>    un-inserted breakpoints temporarily when stepping over them).
>    Maybe they are, I got lost.  :-)  There's a loop going through the
>    bp_location_chain.  Can you get rid of that and use
>    regular_breakpoint_inserted_here_p or similars?
>
> Below is a 10 minutes hack at it, as a starting point.  Replay stil
> isn't perfect, mainly because I got lost in the record_wait maze --- that,
> needs a bit of clean up.  :-)
>
>>
>> > (gdb) record
>> > (gdb) b 6
>> > Breakpoint 2 at 0x8048352: file nop.c, line 6.
>> > (gdb) b 7
>> > Breakpoint 3 at 0x8048353: file nop.c, line 7.
>> > (gdb) n
>> >
>> > Breakpoint 3, main () at nop.c:7
>> > 7               asm ("nop");
>> > (gdb) n
>> > 8               asm ("nop");
>> > (gdb)
>> > 9               asm ("nop");
>> > (gdb) n
>> > 10      }
>> > (gdb) rc
>> > Continuing.
>> >
>> > Breakpoint 3, main () at nop.c:7
>> > 7               asm ("nop");
>> > (gdb) rn
>> >
>> > No more reverse-execution history.
>> > main () at nop.c:6
>> > 6               asm ("nop");
>> > (gdb) n
>> >
>> > Breakpoint 2, main () at nop.c:6
>> > 6               asm ("nop");
>> > (gdb)
>> > 8               asm ("nop");
>> > (gdb)
>> > 9               asm ("nop");
>> > (gdb)
>> >
>> >
>> >
>> > --
>> > Pedro Alves
>>
>>
>
>
>
> --
> Pedro Alves
>

[-- Attachment #2: record_wait_breakpoint.txt --]
[-- Type: text/plain, Size: 2708 bytes --]

--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2008-10-20  Hui Zhu  <teawater@gmail.com>
+
+	* record.c (record_wait): Check breakpint before forward
+	execute in replay mode.
+	Check breakpoint use function "breakpoint_inserted_here_p".
+
 2008-10-19  Hui Zhu  <teawater@gmail.com>
 
 	* infrun.c (handle_inferior_event): Set "stop_pc" when
--- a/record.c
+++ b/record.c
@@ -498,6 +498,23 @@ record_wait (ptid_t ptid, struct target_
       int first_record_end = 1;
       struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
 
+      /* Check breakpoint when forward execute.  */
+      if (execution_direction == EXEC_FORWARD)
+	{
+	  if (breakpoint_inserted_here_p (read_pc ()))
+	    {
+	      if (record_debug)
+		{
+		  fprintf_unfiltered (gdb_stdlog,
+				      "Process record: break at 0x%s.\n",
+				      paddr_nz (regcache_read_pc
+						(get_thread_regcache
+						 (ptid))));
+		}
+	      goto replay_out;
+	    }
+	}
+
       record_get_sig = 0;
       act.sa_handler = record_sig_handler;
       act.sa_mask = record_maskall;
@@ -588,10 +605,6 @@ record_wait (ptid_t ptid, struct target_
 	    }
 	  else
 	    {
-	      CORE_ADDR tmp_pc;
-	      struct bp_location *bl;
-	      struct breakpoint *b;
-
 	      if (record_debug > 1)
 		{
 		  fprintf_unfiltered (gdb_stdlog,
@@ -632,35 +645,17 @@ record_wait (ptid_t ptid, struct target_
 		    }
 
 		  /* check breakpoint */
-		  tmp_pc = read_pc ();
-		  for (bl = bp_location_chain; bl; bl = bl->global_next)
+		  if (breakpoint_inserted_here_p (read_pc ()))
 		    {
-		      b = bl->owner;
-		      gdb_assert (b);
-		      if (b->enable_state != bp_enabled
-			  && b->enable_state != bp_permanent)
-			continue;
-
-		      if (b->type == bp_watchpoint || b->type == bp_catch_fork
-			  || b->type == bp_catch_vfork
-			  || b->type == bp_catch_exec
-			  || b->type == bp_hardware_watchpoint
-			  || b->type == bp_read_watchpoint
-			  || b->type == bp_access_watchpoint)
+		      if (record_debug)
 			{
-			  continue;
-			}
-		      if (bl->address == tmp_pc)
-			{
-			  if (record_debug)
-			    {
-			      fprintf_unfiltered (gdb_stdlog,
-						  "Process record: break at 0x%s.\n",
-						  paddr_nz (tmp_pc));
-			    }
-			  continue_flag = 0;
-			  break;
+			  fprintf_unfiltered (gdb_stdlog,
+					      "Process record: break at 0x%s.\n",
+					      paddr_nz (regcache_read_pc
+							(get_thread_regcache
+							 (ptid))));
 			}
+		      continue_flag = 0;
 		    }
 		}
 	      if (execution_direction == EXEC_REVERSE)
@@ -691,6 +686,7 @@ next:
 	  perror_with_name (_("Process record: sigaction"));
 	}
 
+replay_out:
       if (record_get_sig)
 	{
 	  status->value.sig = TARGET_SIGNAL_INT;

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Pedro Alves]

On Monday 20 October 2008 01:39:30, Michael Snyder wrote:
> See, the problem is that "adjust_pc_after_break" is assuming
> memory-breakpoint semantics, but Process Record/Replay actually
> implements hardware-breakpoint semantics.  It watches the
> instruction-address "bus" and stops when the PC matches the
> address of a breakpoint.
> 
> I suspect this is probably a problem with other record/replay
> back-ends too, but I haven't confirmed it yet.
> 

But that is wrong.  If GDB is telling the target to insert
software breakpoints, and the target is accepting them, then
GDB assumes software breakpoints semantics for that particular
architecture.  That's the `target_insert_breakpoint' semantics
(native/remote, doesn't matter).

For remote, if the stub is accepting Z0, those breakpoint should behave
as memory breakpoints.  See from the manual:

"
`Z0,addr,length'
Insert (`Z0') or remove (`z0') a memory breakpoint at address addr of size length. 

`Z1,addr,length'
    Insert (`Z1') or remove (`z1') a hardware breakpoint at address addr of size length. 
    A hardware breakpoint is implemented using a mechanism that is not dependant on being able to modify the target's memory. 
"

Notice that adjust_pc_after_break does this:

  /* Check whether there actually is a *software breakpoint* inserted
     at that location.  */
  if (software_breakpoint_inserted_here_p (breakpoint_pc))
      ^^^^^^^^
    {

If the stub on a decr_pc_after_break arch lies to GDB, then, PC
adjustment will be broken, even in normal forward debugging.

I believe the correct thing for the target to do is to
report `PC + decr_pc_after_breakpoint' on forward (replay or normal forward)
breakpoint hits, if it is telling GDB that it succesfully
inserted a software breakpoint.

-- 
Pedro Alves

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Michael Snyder]

Pedro Alves wrote:

> I believe the correct thing for the target to do is to
> report `PC + decr_pc_after_breakpoint' on forward (replay or normal forward)
> breakpoint hits, if it is telling GDB that it succesfully
> inserted a software breakpoint.

Yeah, you and Daniel have both said essentially the same.
And I think you're right.

Hui, can you do this?

Unfortunately, right now I think you are handling the
breakpoints in record.c (which is architecture-agnostic).
Maybe you'll have to check the decr_pc_after_break value.

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

I think your mean is check breakpoint in address
read_pc()+gdbarch_decr_pc_after_break (gdbarch) in record_wait, right?

On Tue, Oct 21, 2008 at 01:46, Michael Snyder <msnyder@vmware.com> wrote:
> Pedro Alves wrote:
>
>> I believe the correct thing for the target to do is to
>> report `PC + decr_pc_after_breakpoint' on forward (replay or normal
>> forward)
>> breakpoint hits, if it is telling GDB that it succesfully
>> inserted a software breakpoint.
>
> Yeah, you and Daniel have both said essentially the same.
> And I think you're right.
>
> Hui, can you do this?
>
> Unfortunately, right now I think you are handling the
> breakpoints in record.c (which is architecture-agnostic).
> Maybe you'll have to check the decr_pc_after_break value.
>
>

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Pedro Alves]

On Tuesday 21 October 2008 00:36:12, teawater wrote:
> I think your mean is check breakpoint in address
> read_pc()+gdbarch_decr_pc_after_break (gdbarch) in record_wait, right?

Taking x86 as an example, when you're doing normal debugging and you
hit a breakpoint (SIGTRAP), the first read_pc GDB does to check where
what breakpoint was hit, will read back `breakpoint_PC + 1' --- GDB takes care
getting rid of that `+ 1' offset in infrun.c:adjust_pc_after_break.  The
idea is for you to do the same as the kernel/hardware would --- still
check for breakpoints at read_pc, but increment PC by 1 before reporting the
breakpoint to GDB's core.  E.g., see the `pc += gdbarch...' line from
the patch I posted previously, something like:

record.c:record_wait ()
{
...
+         /* Check for breakpoint hits in forward execution.  */
+         pc = read_pc ();
+         if (execution_direction == EXEC_FORWARD
+             && regular_breakpoint_inserted_here_p (pc)
+             /* && !single-stepping */)
+           {
+             status->kind = TARGET_WAITKIND_STOPPED;
+             status->value.sig = TARGET_SIGNAL_TRAP;
+             if (software_breakpoint_inserted_here_p (pc))
+               {
+                 pc += gdbarch_decr_pc_after_break (gdbarch);
+                 write_pc (pc);
+               }
+

-- 
Pedro Alves

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

In replay mode, it can't change value of register.

So I just can check if some address have breakpoint and stop execute
in this address.

On Tue, Oct 21, 2008 at 08:21, Pedro Alves <pedro@codesourcery.com> wrote:
> On Tuesday 21 October 2008 00:36:12, teawater wrote:
>> I think your mean is check breakpoint in address
>> read_pc()+gdbarch_decr_pc_after_break (gdbarch) in record_wait, right?
>
> Taking x86 as an example, when you're doing normal debugging and you
> hit a breakpoint (SIGTRAP), the first read_pc GDB does to check where
> what breakpoint was hit, will read back `breakpoint_PC + 1' --- GDB takes care
> getting rid of that `+ 1' offset in infrun.c:adjust_pc_after_break.  The
> idea is for you to do the same as the kernel/hardware would --- still
> check for breakpoints at read_pc, but increment PC by 1 before reporting the
> breakpoint to GDB's core.  E.g., see the `pc += gdbarch...' line from
> the patch I posted previously, something like:
>
> record.c:record_wait ()
> {
> ...
> +         /* Check for breakpoint hits in forward execution.  */
> +         pc = read_pc ();
> +         if (execution_direction == EXEC_FORWARD
> +             && regular_breakpoint_inserted_here_p (pc)
> +             /* && !single-stepping */)
> +           {
> +             status->kind = TARGET_WAITKIND_STOPPED;
> +             status->value.sig = TARGET_SIGNAL_TRAP;
> +             if (software_breakpoint_inserted_here_p (pc))
> +               {
> +                 pc += gdbarch_decr_pc_after_break (gdbarch);
> +                 write_pc (pc);
> +               }
> +
>
> --
> Pedro Alves
>

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

I see this problem again. And I found that function
"adjust_pc_after_break" that deal with gdbarch_decr_pc_after_break is
unless for replay mode.
How about let function "adjust_pc_after_break" return directly if in
replay mode?

On Tue, Oct 21, 2008 at 08:56, teawater <teawater@gmail.com> wrote:
> In replay mode, it can't change value of register.
>
> So I just can check if some address have breakpoint and stop execute
> in this address.
>
> On Tue, Oct 21, 2008 at 08:21, Pedro Alves <pedro@codesourcery.com> wrote:
>> On Tuesday 21 October 2008 00:36:12, teawater wrote:
>>> I think your mean is check breakpoint in address
>>> read_pc()+gdbarch_decr_pc_after_break (gdbarch) in record_wait, right?
>>
>> Taking x86 as an example, when you're doing normal debugging and you
>> hit a breakpoint (SIGTRAP), the first read_pc GDB does to check where
>> what breakpoint was hit, will read back `breakpoint_PC + 1' --- GDB takes care
>> getting rid of that `+ 1' offset in infrun.c:adjust_pc_after_break.  The
>> idea is for you to do the same as the kernel/hardware would --- still
>> check for breakpoints at read_pc, but increment PC by 1 before reporting the
>> breakpoint to GDB's core.  E.g., see the `pc += gdbarch...' line from
>> the patch I posted previously, something like:
>>
>> record.c:record_wait ()
>> {
>> ...
>> +         /* Check for breakpoint hits in forward execution.  */
>> +         pc = read_pc ();
>> +         if (execution_direction == EXEC_FORWARD
>> +             && regular_breakpoint_inserted_here_p (pc)
>> +             /* && !single-stepping */)
>> +           {
>> +             status->kind = TARGET_WAITKIND_STOPPED;
>> +             status->value.sig = TARGET_SIGNAL_TRAP;
>> +             if (software_breakpoint_inserted_here_p (pc))
>> +               {
>> +                 pc += gdbarch_decr_pc_after_break (gdbarch);
>> +                 write_pc (pc);
>> +               }
>> +
>>
>> --
>> Pedro Alves
>>
>

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

2008-10-21  Hui Zhu  <teawater@gmail.com>

	* record.c (record_wait): Check breakpint before forward
	execute in replay mode.
	Check breakpoint use function "breakpoint_inserted_here_p"
	in replay mode.
	Set pc if forward execute and gdbarch_decr_pc_after_break
	is not 0 in replay mode.

On Tue, Oct 21, 2008 at 14:51, teawater <teawater@gmail.com> wrote:
> Sorry for understand your mean so later Pedro. I made a new patch that
> Set pc if forward execute and gdbarch_decr_pc_after_break is not 0 in
> replay mode. How do you think about it?
>
>
> And I think 20080930 branch is need your "adjust_pc_reverse.diff". Do
> you mind I check it in?
>
> On Tue, Oct 21, 2008 at 08:21, Pedro Alves <pedro@codesourcery.com> wrote:
>> On Tuesday 21 October 2008 00:36:12, teawater wrote:
>>> I think your mean is check breakpoint in address
>>> read_pc()+gdbarch_decr_pc_after_break (gdbarch) in record_wait, right?
>>
>> Taking x86 as an example, when you're doing normal debugging and you
>> hit a breakpoint (SIGTRAP), the first read_pc GDB does to check where
>> what breakpoint was hit, will read back `breakpoint_PC + 1' --- GDB takes care
>> getting rid of that `+ 1' offset in infrun.c:adjust_pc_after_break.  The
>> idea is for you to do the same as the kernel/hardware would --- still
>> check for breakpoints at read_pc, but increment PC by 1 before reporting the
>> breakpoint to GDB's core.  E.g., see the `pc += gdbarch...' line from
>> the patch I posted previously, something like:
>>
>> record.c:record_wait ()
>> {
>> ...
>> +         /* Check for breakpoint hits in forward execution.  */
>> +         pc = read_pc ();
>> +         if (execution_direction == EXEC_FORWARD
>> +             && regular_breakpoint_inserted_here_p (pc)
>> +             /* && !single-stepping */)
>> +           {
>> +             status->kind = TARGET_WAITKIND_STOPPED;
>> +             status->value.sig = TARGET_SIGNAL_TRAP;
>> +             if (software_breakpoint_inserted_here_p (pc))
>> +               {
>> +                 pc += gdbarch_decr_pc_after_break (gdbarch);
>> +                 write_pc (pc);
>> +               }
>> +
>>
>> --
>> Pedro Alves
>>
>

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

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

Sorry for understand your mean so later Pedro. I made a new patch that
Set pc if forward execute and gdbarch_decr_pc_after_break is not 0 in
replay mode. How do you think about it?


And I think 20080930 branch is need your "adjust_pc_reverse.diff". Do
you mind I check it in?

On Tue, Oct 21, 2008 at 08:21, Pedro Alves <pedro@codesourcery.com> wrote:
> On Tuesday 21 October 2008 00:36:12, teawater wrote:
>> I think your mean is check breakpoint in address
>> read_pc()+gdbarch_decr_pc_after_break (gdbarch) in record_wait, right?
>
> Taking x86 as an example, when you're doing normal debugging and you
> hit a breakpoint (SIGTRAP), the first read_pc GDB does to check where
> what breakpoint was hit, will read back `breakpoint_PC + 1' --- GDB takes care
> getting rid of that `+ 1' offset in infrun.c:adjust_pc_after_break.  The
> idea is for you to do the same as the kernel/hardware would --- still
> check for breakpoints at read_pc, but increment PC by 1 before reporting the
> breakpoint to GDB's core.  E.g., see the `pc += gdbarch...' line from
> the patch I posted previously, something like:
>
> record.c:record_wait ()
> {
> ...
> +         /* Check for breakpoint hits in forward execution.  */
> +         pc = read_pc ();
> +         if (execution_direction == EXEC_FORWARD
> +             && regular_breakpoint_inserted_here_p (pc)
> +             /* && !single-stepping */)
> +           {
> +             status->kind = TARGET_WAITKIND_STOPPED;
> +             status->value.sig = TARGET_SIGNAL_TRAP;
> +             if (software_breakpoint_inserted_here_p (pc))
> +               {
> +                 pc += gdbarch_decr_pc_after_break (gdbarch);
> +                 write_pc (pc);
> +               }
> +
>
> --
> Pedro Alves
>

[-- Attachment #2: record_wait_breakpoint.txt --]
[-- Type: text/plain, Size: 2821 bytes --]

--- a/record.c
+++ b/record.c
@@ -497,6 +497,30 @@ record_wait (ptid_t ptid, struct target_
       int continue_flag = 1;
       int first_record_end = 1;
       struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
+      CORE_ADDR tmp_pc;
+
+      /* Check breakpoint when forward execute.  */
+      if (execution_direction == EXEC_FORWARD)
+	{
+	  tmp_pc = regcache_read_pc (regcache);
+	  if (breakpoint_inserted_here_p (tmp_pc))
+	    {
+	      if (record_debug)
+		{
+		  fprintf_unfiltered (gdb_stdlog,
+				      "Process record: break at 0x%s.\n",
+				      paddr_nz (tmp_pc));
+		}
+	      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache)))
+		{
+		  regcache_write_pc (regcache,
+				     tmp_pc +
+				     gdbarch_decr_pc_after_break
+				     (get_regcache_arch (regcache)));
+		}
+	      goto replay_out;
+	    }
+	}
 
       record_get_sig = 0;
       act.sa_handler = record_sig_handler;
@@ -588,10 +612,6 @@ record_wait (ptid_t ptid, struct target_
 	    }
 	  else
 	    {
-	      CORE_ADDR tmp_pc;
-	      struct bp_location *bl;
-	      struct breakpoint *b;
-
 	      if (record_debug > 1)
 		{
 		  fprintf_unfiltered (gdb_stdlog,
@@ -632,35 +652,24 @@ record_wait (ptid_t ptid, struct target_
 		    }
 
 		  /* check breakpoint */
-		  tmp_pc = read_pc ();
-		  for (bl = bp_location_chain; bl; bl = bl->global_next)
+		  tmp_pc = regcache_read_pc (regcache);
+		  if (breakpoint_inserted_here_p (tmp_pc))
 		    {
-		      b = bl->owner;
-		      gdb_assert (b);
-		      if (b->enable_state != bp_enabled
-			  && b->enable_state != bp_permanent)
-			continue;
-
-		      if (b->type == bp_watchpoint || b->type == bp_catch_fork
-			  || b->type == bp_catch_vfork
-			  || b->type == bp_catch_exec
-			  || b->type == bp_hardware_watchpoint
-			  || b->type == bp_read_watchpoint
-			  || b->type == bp_access_watchpoint)
+		      if (record_debug)
 			{
-			  continue;
+			  fprintf_unfiltered (gdb_stdlog,
+					      "Process record: break at 0x%s.\n",
+					      paddr_nz (tmp_pc));
 			}
-		      if (bl->address == tmp_pc)
+		      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
+			  && execution_direction == EXEC_FORWARD)
 			{
-			  if (record_debug)
-			    {
-			      fprintf_unfiltered (gdb_stdlog,
-						  "Process record: break at 0x%s.\n",
-						  paddr_nz (tmp_pc));
-			    }
-			  continue_flag = 0;
-			  break;
+			  regcache_write_pc (regcache,
+					     tmp_pc +
+					     gdbarch_decr_pc_after_break
+					     (get_regcache_arch (regcache)));
 			}
+		      continue_flag = 0;
 		    }
 		}
 	      if (execution_direction == EXEC_REVERSE)
@@ -691,6 +700,7 @@ next:
 	  perror_with_name (_("Process record: sigaction"));
 	}
 
+replay_out:
       if (record_get_sig)
 	{
 	  status->value.sig = TARGET_SIGNAL_INT;

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

Sorry I send too much Email. I found that:

      if (singlestep_breakpoints_inserted_p
	  || !ptid_equal (ecs->ptid, inferior_ptid)
	  || !currently_stepping (ecs->event_thread)
	  || ecs->event_thread->prev_pc == breakpoint_pc)
	regcache_write_pc (regcache, breakpoint_pc);

Before write_pc, there are a lot of thing to check. Do we  need to
check it in record_wait?
If so, it actually useless cause it will be set back in adjust_pc_after_break?
Maybe we can let adjust_pc_after_break disable in replay mode.

How do you think?




On Tue, Oct 21, 2008 at 08:21, Pedro Alves <pedro@codesourcery.com> wrote:
> On Tuesday 21 October 2008 00:36:12, teawater wrote:
>> I think your mean is check breakpoint in address
>> read_pc()+gdbarch_decr_pc_after_break (gdbarch) in record_wait, right?
>
> Taking x86 as an example, when you're doing normal debugging and you
> hit a breakpoint (SIGTRAP), the first read_pc GDB does to check where
> what breakpoint was hit, will read back `breakpoint_PC + 1' --- GDB takes care
> getting rid of that `+ 1' offset in infrun.c:adjust_pc_after_break.  The
> idea is for you to do the same as the kernel/hardware would --- still
> check for breakpoints at read_pc, but increment PC by 1 before reporting the
> breakpoint to GDB's core.  E.g., see the `pc += gdbarch...' line from
> the patch I posted previously, something like:
>
> record.c:record_wait ()
> {
> ...
> +         /* Check for breakpoint hits in forward execution.  */
> +         pc = read_pc ();
> +         if (execution_direction == EXEC_FORWARD
> +             && regular_breakpoint_inserted_here_p (pc)
> +             /* && !single-stepping */)
> +           {
> +             status->kind = TARGET_WAITKIND_STOPPED;
> +             status->value.sig = TARGET_SIGNAL_TRAP;
> +             if (software_breakpoint_inserted_here_p (pc))
> +               {
> +                 pc += gdbarch_decr_pc_after_break (gdbarch);
> +                 write_pc (pc);
> +               }
> +
>
> --
> Pedro Alves
>

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Michael Snyder]

teawater wrote:
> Sorry I send too much Email. I found that:
> 
>       if (singlestep_breakpoints_inserted_p
>           || !ptid_equal (ecs->ptid, inferior_ptid)
>           || !currently_stepping (ecs->event_thread)
>           || ecs->event_thread->prev_pc == breakpoint_pc)
>         regcache_write_pc (regcache, breakpoint_pc);
> 
> Before write_pc, there are a lot of thing to check. Do we  need to
> check it in record_wait?
> If so, it actually useless cause it will be set back in adjust_pc_after_break?
> Maybe we can let adjust_pc_after_break disable in replay mode.
> 
> How do you think?

I think we should leave adjust_pc_after_break alone,
and change record_wait so that it adjusts the pc by
adding decr_pc_after_break(gdbarch) when appropriate.

Whenever possible, gdb should not need to know the difference
between replay and live debugging.  This keeps things simple,
and preserves modularity.

Of course, you don't have access to the "ecs" object, which
is local to infrun.  But you do know whether or not gdb is
stepping.  And (for now) you know that there is only one
thread, so you can (for now) ignore the thread id (ptid).

The value of "step" that was passed to record_resume
came from "currently_stepping", so you should be able
to use that.

Something close to the patch that Pedro posted should work...

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

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

I see.
I make a patch for it.

2008-10-21  Hui Zhu  <teawater@gmail.com>

	* record.c (record_wait): Check breakpint before forward
	execute in replay mode.
	Check breakpoint use function "breakpoint_inserted_here_p"
	in replay mode.
	Set pc if forward execute, gdbarch_decr_pc_after_break is not
	0 and this is not single step in replay mode.

Thanks,
Hui


On Wed, Oct 22, 2008 at 02:30, Michael Snyder <msnyder@vmware.com> wrote:
> teawater wrote:
>>
>> Sorry I send too much Email. I found that:
>>
>>      if (singlestep_breakpoints_inserted_p
>>          || !ptid_equal (ecs->ptid, inferior_ptid)
>>          || !currently_stepping (ecs->event_thread)
>>          || ecs->event_thread->prev_pc == breakpoint_pc)
>>        regcache_write_pc (regcache, breakpoint_pc);
>>
>> Before write_pc, there are a lot of thing to check. Do we  need to
>> check it in record_wait?
>> If so, it actually useless cause it will be set back in
>> adjust_pc_after_break?
>> Maybe we can let adjust_pc_after_break disable in replay mode.
>>
>> How do you think?
>
> I think we should leave adjust_pc_after_break alone,
> and change record_wait so that it adjusts the pc by
> adding decr_pc_after_break(gdbarch) when appropriate.
>
> Whenever possible, gdb should not need to know the difference
> between replay and live debugging.  This keeps things simple,
> and preserves modularity.
>
> Of course, you don't have access to the "ecs" object, which
> is local to infrun.  But you do know whether or not gdb is
> stepping.  And (for now) you know that there is only one
> thread, so you can (for now) ignore the thread id (ptid).
>
> The value of "step" that was passed to record_resume
> came from "currently_stepping", so you should be able
> to use that.
>
> Something close to the patch that Pedro posted should work...
>
>
>
>

[-- Attachment #2: record_wait_breakpoint.txt --]
[-- Type: text/plain, Size: 2878 bytes --]

--- a/record.c
+++ b/record.c
@@ -497,6 +497,31 @@ record_wait (ptid_t ptid, struct target_
       int continue_flag = 1;
       int first_record_end = 1;
       struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
+      CORE_ADDR tmp_pc;
+
+      /* Check breakpoint when forward execute.  */
+      if (execution_direction == EXEC_FORWARD)
+	{
+	  tmp_pc = regcache_read_pc (regcache);
+	  if (breakpoint_inserted_here_p (tmp_pc))
+	    {
+	      if (record_debug)
+		{
+		  fprintf_unfiltered (gdb_stdlog,
+				      "Process record: break at 0x%s.\n",
+				      paddr_nz (tmp_pc));
+		}
+	      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
+		  && !record_resume_step)
+		{
+		  regcache_write_pc (regcache,
+				     tmp_pc +
+				     gdbarch_decr_pc_after_break
+				     (get_regcache_arch (regcache)));
+		}
+	      goto replay_out;
+	    }
+	}
 
       record_get_sig = 0;
       act.sa_handler = record_sig_handler;
@@ -588,10 +613,6 @@ record_wait (ptid_t ptid, struct target_
 	    }
 	  else
 	    {
-	      CORE_ADDR tmp_pc;
-	      struct bp_location *bl;
-	      struct breakpoint *b;
-
 	      if (record_debug > 1)
 		{
 		  fprintf_unfiltered (gdb_stdlog,
@@ -632,35 +653,25 @@ record_wait (ptid_t ptid, struct target_
 		    }
 
 		  /* check breakpoint */
-		  tmp_pc = read_pc ();
-		  for (bl = bp_location_chain; bl; bl = bl->global_next)
+		  tmp_pc = regcache_read_pc (regcache);
+		  if (breakpoint_inserted_here_p (tmp_pc))
 		    {
-		      b = bl->owner;
-		      gdb_assert (b);
-		      if (b->enable_state != bp_enabled
-			  && b->enable_state != bp_permanent)
-			continue;
-
-		      if (b->type == bp_watchpoint || b->type == bp_catch_fork
-			  || b->type == bp_catch_vfork
-			  || b->type == bp_catch_exec
-			  || b->type == bp_hardware_watchpoint
-			  || b->type == bp_read_watchpoint
-			  || b->type == bp_access_watchpoint)
+		      if (record_debug)
 			{
-			  continue;
+			  fprintf_unfiltered (gdb_stdlog,
+					      "Process record: break at 0x%s.\n",
+					      paddr_nz (tmp_pc));
 			}
-		      if (bl->address == tmp_pc)
+		      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
+			  && execution_direction == EXEC_FORWARD
+			  && !record_resume_step)
 			{
-			  if (record_debug)
-			    {
-			      fprintf_unfiltered (gdb_stdlog,
-						  "Process record: break at 0x%s.\n",
-						  paddr_nz (tmp_pc));
-			    }
-			  continue_flag = 0;
-			  break;
+			  regcache_write_pc (regcache,
+					     tmp_pc +
+					     gdbarch_decr_pc_after_break
+					     (get_regcache_arch (regcache)));
 			}
+		      continue_flag = 0;
 		    }
 		}
 	      if (execution_direction == EXEC_REVERSE)
@@ -691,6 +702,7 @@ next:
 	  perror_with_name (_("Process record: sigaction"));
 	}
 
+replay_out:
       if (record_get_sig)
 	{
 	  status->value.sig = TARGET_SIGNAL_INT;

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Michael Snyder]

Hui, can you hold off on this change?

What I'm finding is that I can reproduce the bad behavior
that Pedro demonstrated on the 20080930 branch, but ONLY
if I omit Pedro's change in adjust_pc_after_break.  If I
add Pedro's change, I can no longer reproduce any bad
behavior.

I've added Pedro's change to the branch now -- why don't
you temporarily take out all these changes, and then see
if you can make it manifest a problem.


teawater wrote:
> Sorry for understand your mean so later Pedro. I made a new patch that
> Set pc if forward execute and gdbarch_decr_pc_after_break is not 0 in
> replay mode. How do you think about it?
> 
> 
> And I think 20080930 branch is need your "adjust_pc_reverse.diff". Do
> you mind I check it in?
> 
> On Tue, Oct 21, 2008 at 08:21, Pedro Alves <pedro@codesourcery.com> wrote:
>> On Tuesday 21 October 2008 00:36:12, teawater wrote:
>>> I think your mean is check breakpoint in address
>>> read_pc()+gdbarch_decr_pc_after_break (gdbarch) in record_wait, right?
>> Taking x86 as an example, when you're doing normal debugging and you
>> hit a breakpoint (SIGTRAP), the first read_pc GDB does to check where
>> what breakpoint was hit, will read back `breakpoint_PC + 1' --- GDB takes care
>> getting rid of that `+ 1' offset in infrun.c:adjust_pc_after_break.  The
>> idea is for you to do the same as the kernel/hardware would --- still
>> check for breakpoints at read_pc, but increment PC by 1 before reporting the
>> breakpoint to GDB's core.  E.g., see the `pc += gdbarch...' line from
>> the patch I posted previously, something like:
>>
>> record.c:record_wait ()
>> {
>> ...
>> +         /* Check for breakpoint hits in forward execution.  */
>> +         pc = read_pc ();
>> +         if (execution_direction == EXEC_FORWARD
>> +             && regular_breakpoint_inserted_here_p (pc)
>> +             /* && !single-stepping */)
>> +           {
>> +             status->kind = TARGET_WAITKIND_STOPPED;
>> +             status->value.sig = TARGET_SIGNAL_TRAP;
>> +             if (software_breakpoint_inserted_here_p (pc))
>> +               {
>> +                 pc += gdbarch_decr_pc_after_break (gdbarch);
>> +                 write_pc (pc);
>> +               }
>> +
>>
>> --
>> Pedro Alves
>>
>>
>> ------------------------------------------------------------------------
>>
>> --- a/record.c
>> +++ b/record.c
>> @@ -497,6 +497,30 @@ record_wait (ptid_t ptid, struct target_
>>        int continue_flag = 1;
>>        int first_record_end = 1;
>>        struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
>> +      CORE_ADDR tmp_pc;
>> +
>> +      /* Check breakpoint when forward execute.  */
>> +      if (execution_direction == EXEC_FORWARD)
>> +	{
>> +	  tmp_pc = regcache_read_pc (regcache);
>> +	  if (breakpoint_inserted_here_p (tmp_pc))
>> +	    {
>> +	      if (record_debug)
>> +		{
>> +		  fprintf_unfiltered (gdb_stdlog,
>> +				      "Process record: break at 0x%s.\n",
>> +				      paddr_nz (tmp_pc));
>> +		}
>> +	      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache)))
>> +		{
>> +		  regcache_write_pc (regcache,
>> +				     tmp_pc +
>> +				     gdbarch_decr_pc_after_break
>> +				     (get_regcache_arch (regcache)));
>> +		}
>> +	      goto replay_out;
>> +	    }
>> +	}
>>  
>>        record_get_sig = 0;
>>        act.sa_handler = record_sig_handler;
>> @@ -588,10 +612,6 @@ record_wait (ptid_t ptid, struct target_
>>  	    }
>>  	  else
>>  	    {
>> -	      CORE_ADDR tmp_pc;
>> -	      struct bp_location *bl;
>> -	      struct breakpoint *b;
>> -
>>  	      if (record_debug > 1)
>>  		{
>>  		  fprintf_unfiltered (gdb_stdlog,
>> @@ -632,35 +652,24 @@ record_wait (ptid_t ptid, struct target_
>>  		    }
>>  
>>  		  /* check breakpoint */
>> -		  tmp_pc = read_pc ();
>> -		  for (bl = bp_location_chain; bl; bl = bl->global_next)
>> +		  tmp_pc = regcache_read_pc (regcache);
>> +		  if (breakpoint_inserted_here_p (tmp_pc))
>>  		    {
>> -		      b = bl->owner;
>> -		      gdb_assert (b);
>> -		      if (b->enable_state != bp_enabled
>> -			  && b->enable_state != bp_permanent)
>> -			continue;
>> -
>> -		      if (b->type == bp_watchpoint || b->type == bp_catch_fork
>> -			  || b->type == bp_catch_vfork
>> -			  || b->type == bp_catch_exec
>> -			  || b->type == bp_hardware_watchpoint
>> -			  || b->type == bp_read_watchpoint
>> -			  || b->type == bp_access_watchpoint)
>> +		      if (record_debug)
>>  			{
>> -			  continue;
>> +			  fprintf_unfiltered (gdb_stdlog,
>> +					      "Process record: break at 0x%s.\n",
>> +					      paddr_nz (tmp_pc));
>>  			}
>> -		      if (bl->address == tmp_pc)
>> +		      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
>> +			  && execution_direction == EXEC_FORWARD)
>>  			{
>> -			  if (record_debug)
>> -			    {
>> -			      fprintf_unfiltered (gdb_stdlog,
>> -						  "Process record: break at 0x%s.\n",
>> -						  paddr_nz (tmp_pc));
>> -			    }
>> -			  continue_flag = 0;
>> -			  break;
>> +			  regcache_write_pc (regcache,
>> +					     tmp_pc +
>> +					     gdbarch_decr_pc_after_break
>> +					     (get_regcache_arch (regcache)));
>>  			}
>> +		      continue_flag = 0;
>>  		    }
>>  		}
>>  	      if (execution_direction == EXEC_REVERSE)
>> @@ -691,6 +700,7 @@ next:
>>  	  perror_with_name (_("Process record: sigaction"));
>>  	}
>>  
>> +replay_out:
>>        if (record_get_sig)
>>  	{
>>  	  status->value.sig = TARGET_SIGNAL_INT;

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Michael Snyder]

Hi Pedro,

I duplicated your test case, and found that I could
reproduce the behavior that you show below, but only
so long as the branch did not contain your
"adjust_pc_after_break" patch.

Once I added that patch to the branch, this behavior
seemed to go away.

If I look carefully at what you did below, it seems like
the forward-replay problem only shows up immediately after
the reverse-replay problem manifests.  And my experiments
reflect the same thing.

The branch is now patched.  Could you spare a moment to
play with it, and see if you can make it break again?

Thanks!

Pedro Alves wrote:
> On Sunday 19 October 2008 23:39:20, Michael Snyder wrote:
>> After codgitating for a bit (that's "thinking" when you're over 50),
>> I've decided that you're right.
>>
>> However, I have a new concern -- I'm worried about what it will do
>> when it's replaying but going forward.
>>
>> Could you possibly revisit your test and see what it does
>> if you record all the way to line 9 or 10, then back up
>> to line 6, then continue with breakpoints at 6 and 7?
> 
> Eh, you're right.  It's broken.
> 
> (gdb) record
> (gdb) b 6
> Breakpoint 2 at 0x8048352: file nop.c, line 6.
> (gdb) b 7
> Breakpoint 3 at 0x8048353: file nop.c, line 7.
> (gdb) n
> 
> Breakpoint 3, main () at nop.c:7
> 7               asm ("nop");
> (gdb) n
> 8               asm ("nop");
> (gdb)
> 9               asm ("nop");
> (gdb) n
> 10      }
> (gdb) rc
> Continuing.
> 
> Breakpoint 3, main () at nop.c:7
> 7               asm ("nop");
> (gdb) rn
> 
> No more reverse-execution history.
> main () at nop.c:6
> 6               asm ("nop");
> (gdb) n
> 
> Breakpoint 2, main () at nop.c:6
> 6               asm ("nop");
> (gdb)
> 8               asm ("nop");
> (gdb)
> 9               asm ("nop");
> (gdb)
> 
> 
> 
> --
> Pedro Alves

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Pedro Alves]

A Friday 24 October 2008 00:26:43, Michael Snyder wrote:
> Hi Pedro,
> 
> I duplicated your test case, and found that I could
> reproduce the behavior that you show below, but only
> so long as the branch did not contain your
> "adjust_pc_after_break" patch.
> 
> Once I added that patch to the branch, this behavior
> seemed to go away.
> 
> If I look carefully at what you did below, it seems like
> the forward-replay problem only shows up immediately after
> the reverse-replay problem manifests.  And my experiments
> reflect the same thing.
> 
> The branch is now patched.  Could you spare a moment to
> play with it, and see if you can make it break again?

I've done so a bit this morning, and came to a similar
conclusion, although I noticed Hui's change to set stop_pc in
TARGET_WAITKIND_NO_HISTORY, also also required.  I was wanting
to find time to play a little bit more, but since you're on to it...

I think the issue here, is that when proceeding (continuing) from B1
below,

 B1: PC -->  0x80000001    INSN1
 B2:         0x80000002    INSN2

GDB will always do a single-step to get over B1.  Then, the record
target replays INSN1, and then notices that there's a breakpoint
at 0x80000002.  Remember that GDB told the target to single-step (over
a breakpoint), and to do so, removed all breakpoints from
the inferior.  Hence, the adjust_pc_after_break checks to see if there's
a breakpoint inserted at `0x80000002 - 1', it will find there isn't one
(no breakpoint is inserted while doing the single-step over breakpoints
operation).

In sum, it appears that decr_pc_after_break doesn't matter when you have
continguous breakpoints, as long as you get from from B1's address to B2's
address by single-stepping.  All is good then, it appears!

Without Hui's stop_pc change, when we'd go backwards and hit the
start (end, whatever) of history, we'd get us a wrong stop_pc.  Then,
proceed while doing this check:

     if (pc == stop_pc && breakpoint_here_p (pc) 
	  && execution_direction != EXEC_REVERSE)

pc == stop_pc would fail, and hence the target would not be told
to single-step over the breakpoint, producing the bad effects we were
seeing.  (*)


Hope I'm making sense.  This gave me a bit of a headache
this morning.  :-)

(*) BTW, it seemed that TARGET_WAITKIND_NO_HISTORY overrides the
last event the target would report?  Should'nt the last event in
history be reported normally, and only *on the next* resume we'd
get a TARGET_WAITKIND_NO_HISTORY?  I was wondering if you'd not lose
a possible interesting event, just because it happened to be on
the edge of the history.

> 
> Thanks!
> 
> Pedro Alves wrote:
> > On Sunday 19 October 2008 23:39:20, Michael Snyder wrote:
> >> After codgitating for a bit (that's "thinking" when you're over 50),
> >> I've decided that you're right.
> >>
> >> However, I have a new concern -- I'm worried about what it will do
> >> when it's replaying but going forward.
> >>
> >> Could you possibly revisit your test and see what it does
> >> if you record all the way to line 9 or 10, then back up
> >> to line 6, then continue with breakpoints at 6 and 7?
> > 
> > Eh, you're right.  It's broken.
> > 
> > (gdb) record
> > (gdb) b 6
> > Breakpoint 2 at 0x8048352: file nop.c, line 6.
> > (gdb) b 7
> > Breakpoint 3 at 0x8048353: file nop.c, line 7.
> > (gdb) n
> > 
> > Breakpoint 3, main () at nop.c:7
> > 7               asm ("nop");
> > (gdb) n
> > 8               asm ("nop");
> > (gdb)
> > 9               asm ("nop");
> > (gdb) n
> > 10      }
> > (gdb) rc
> > Continuing.
> > 
> > Breakpoint 3, main () at nop.c:7
> > 7               asm ("nop");
> > (gdb) rn
> > 
> > No more reverse-execution history.
> > main () at nop.c:6
> > 6               asm ("nop");
> > (gdb) n
> > 
> > Breakpoint 2, main () at nop.c:6
> > 6               asm ("nop");
> > (gdb)
> > 8               asm ("nop");
> > (gdb)
> > 9               asm ("nop");
> > (gdb)
> > 
> > 
> > 
> > --
> > Pedro Alves
> 
> 



-- 
Pedro Alves

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Pedro Alves]

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

> I've done so a bit this morning, and came to a similar
> conclusion, although I noticed Hui's change to set stop_pc in
> TARGET_WAITKIND_NO_HISTORY, also also required.  I was wanting
> to find time to play a little bit more, but since you're on to it...

BTW, I noticed that, while reviewing Hui's latest patch
(which was missing setting the waitkind to TARGET_WAITKIND_STOPPED,
hence could never work :-) ).  While doing so, I dejagnufied the
nop testcase into the attached.  Maybe you'll find it useful.

-- 
Pedro Alves

[-- Attachment #2: record_wait_breakpoint_test.diff --]
[-- Type: text/x-diff, Size: 4479 bytes --]

2008-10-24  Pedro Alves  <pedro@codesourcery.com>

	* gdb.base/decr-pc-rev.c, gdb.base/decr-pc-rev.exp: New test.

---
 gdb/testsuite/gdb.base/decr-pc-rev.c   |   26 +++++++++
 gdb/testsuite/gdb.base/decr-pc-rev.exp |   86 +++++++++++++++++++++++++++++++++
 2 files changed, 112 insertions(+)

Index: src/gdb/testsuite/gdb.base/decr-pc-rev.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/decr-pc-rev.c	2008-10-23 15:21:02.000000000 +0100
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2008 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main ()
+{
+  asm ("nop"); /* first insn */
+  asm ("nop"); /* second insn */
+  asm ("nop"); /* third insn */
+  asm ("nop"); /* fourth insn */
+  return 0;
+}
Index: src/gdb/testsuite/gdb.base/decr-pc-rev.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ src/gdb/testsuite/gdb.base/decr-pc-rev.exp	2008-10-23 15:21:02.000000000 +0100
@@ -0,0 +1,86 @@
+#   Copyright 2008 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+# Test PC adjustment behaviour on decr_pc_after_break archs in reverse
+# and replay modes, in the presence of breakpoints at consecutive
+# instruction addresses.
+
+set testfile "decr-pc-rev"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug nowarnings}] != "" } {
+    untested consecutive.exp
+    return -1
+}
+
+if [get_compiler_info ${binfile}] {
+    return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if [target_info exists gdb_stub] {
+    gdb_step_for_stub;
+}
+
+if ![runto_main] then {
+    perror "couldn't run to breakpoint"
+    continue
+}
+
+# These breakpoint should be placed at consecutive instructions, such
+# that the address difference between them is equal to
+# decr_pc_after_break on this arquitecture.
+
+set bp_location1 [gdb_get_line_number "first insn"]
+set bp_location2 [gdb_get_line_number "second insn"]
+
+# Enable recording.
+
+gdb_test "record" "" "enable record target"
+
+# Step through the breakpoints creating some history.
+
+gdb_test "next" ".*second insn.*" "next (1)"
+gdb_test "next" ".*third insn.*" "next (2)"
+gdb_test "next" ".*fourth insn.*" "next (3)"
+gdb_test "next" ".*return 0.*" "next (4)"
+
+# Set consecutive breakpoints.
+
+gdb_test "break $bp_location1" \
+    ".*Breakpoint .*$srcfile, line $bp_location1\\." \
+    "first breakpoint line number"
+
+gdb_test "break $bp_location2" \
+    "Breakpoint.*at.* file .*$srcfile, line $bp_location2\\." \
+    "second breakpoint line number"
+
+# Test that reverse-continue doesn't mess with decr_pc_after_break
+
+gdb_test "rc" ".*second insn.*" "reverse continue to breakpoint"
+gdb_test "rn" ".*first insn.*" "reverse next to begining of times"
+
+# Test that replay behaves as normal play.
+gdb_test "next" ".*second insn.*" "next (1) in replay"
+gdb_test "next" ".*third insn.*" "next (2) in replay"

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Michael Snyder]

Pedro Alves wrote:
> A Friday 24 October 2008 00:26:43, Michael Snyder wrote:
>> Hi Pedro,
>>
>> I duplicated your test case, and found that I could
>> reproduce the behavior that you show below, but only
>> so long as the branch did not contain your
>> "adjust_pc_after_break" patch.
>>
>> Once I added that patch to the branch, this behavior
>> seemed to go away.
>>
>> If I look carefully at what you did below, it seems like
>> the forward-replay problem only shows up immediately after
>> the reverse-replay problem manifests.  And my experiments
>> reflect the same thing.
>>
>> The branch is now patched.  Could you spare a moment to
>> play with it, and see if you can make it break again?
> 
> I've done so a bit this morning, and came to a similar
> conclusion, although I noticed Hui's change to set stop_pc in
> TARGET_WAITKIND_NO_HISTORY, also also required.  I was wanting
> to find time to play a little bit more, but since you're on to it...
> 
> I think the issue here, is that when proceeding (continuing) from B1
> below,
> 
>  B1: PC -->  0x80000001    INSN1
>  B2:         0x80000002    INSN2
> 
> GDB will always do a single-step to get over B1.  Then, the record
> target replays INSN1, and then notices that there's a breakpoint
> at 0x80000002.  Remember that GDB told the target to single-step (over
> a breakpoint), and to do so, removed all breakpoints from
> the inferior.  Hence, the adjust_pc_after_break checks to see if there's
> a breakpoint inserted at `0x80000002 - 1', it will find there isn't one
> (no breakpoint is inserted while doing the single-step over breakpoints
> operation).

Yes, I was reaching the same conclusion.

> In sum, it appears that decr_pc_after_break doesn't matter when you have
> continguous breakpoints, as long as you get from from B1's address to B2's
> address by single-stepping.  All is good then, it appears!

I agree, at least that is the conclusion I am leaning toward.


> (*) BTW, it seemed that TARGET_WAITKIND_NO_HISTORY overrides the
> last event the target would report?  Should'nt the last event in
> history be reported normally, and only *on the next* resume we'd
> get a TARGET_WAITKIND_NO_HISTORY?  I was wondering if you'd not lose
> a possible interesting event, just because it happened to be on
> the edge of the history.

Yes, it seems like if there is a breakpoint at the very last
(or first) instruction in the history, GDB will report
"no history" rather than "breakpoint".

I'm not *terribly* happy about that, but it's also
not the worst thing that could happen.  Maybe we can
get around to looking at it once we feel that everything
more urgent has been handled.

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Michael Snyder]

Thank you!
I'll put it on the branch for now...

Does it make sense to include a forward version of this test,
for normal (non-reverse/non-replay) targets?

Pedro Alves wrote:
>> I've done so a bit this morning, and came to a similar
>> conclusion, although I noticed Hui's change to set stop_pc in
>> TARGET_WAITKIND_NO_HISTORY, also also required.  I was wanting
>> to find time to play a little bit more, but since you're on to it...
> 
> BTW, I noticed that, while reviewing Hui's latest patch
> (which was missing setting the waitkind to TARGET_WAITKIND_STOPPED,
> hence could never work :-) ).  While doing so, I dejagnufied the
> nop testcase into the attached.  Maybe you'll find it useful.
> 
> --
> Pedro Alves
> 
> 
> ------------------------------------------------------------------------
> 
> 2008-10-24  Pedro Alves  <pedro@codesourcery.com>
> 
> 	* gdb.base/decr-pc-rev.c, gdb.base/decr-pc-rev.exp: New test.
> 
> ---
>  gdb/testsuite/gdb.base/decr-pc-rev.c   |   26 +++++++++
>  gdb/testsuite/gdb.base/decr-pc-rev.exp |   86 +++++++++++++++++++++++++++++++++
>  2 files changed, 112 insertions(+)
> 
> Index: src/gdb/testsuite/gdb.base/decr-pc-rev.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ src/gdb/testsuite/gdb.base/decr-pc-rev.c	2008-10-23 15:21:02.000000000 +0100
> @@ -0,0 +1,26 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2008 Free Software Foundation, Inc.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +int
> +main ()
> +{
> +  asm ("nop"); /* first insn */
> +  asm ("nop"); /* second insn */
> +  asm ("nop"); /* third insn */
> +  asm ("nop"); /* fourth insn */
> +  return 0;
> +}
> Index: src/gdb/testsuite/gdb.base/decr-pc-rev.exp
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ src/gdb/testsuite/gdb.base/decr-pc-rev.exp	2008-10-23 15:21:02.000000000 +0100
> @@ -0,0 +1,86 @@
> +#   Copyright 2008 Free Software Foundation, Inc.
> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +if $tracelevel then {
> +    strace $tracelevel
> +}
> +
> +# Test PC adjustment behaviour on decr_pc_after_break archs in reverse
> +# and replay modes, in the presence of breakpoints at consecutive
> +# instruction addresses.
> +
> +set testfile "decr-pc-rev"
> +set srcfile ${testfile}.c
> +set binfile ${objdir}/${subdir}/${testfile}
> +
> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug nowarnings}] != "" } {
> +    untested consecutive.exp
> +    return -1
> +}
> +
> +if [get_compiler_info ${binfile}] {
> +    return -1
> +}
> +
> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}
> +
> +if [target_info exists gdb_stub] {
> +    gdb_step_for_stub;
> +}
> +
> +if ![runto_main] then {
> +    perror "couldn't run to breakpoint"
> +    continue
> +}
> +
> +# These breakpoint should be placed at consecutive instructions, such
> +# that the address difference between them is equal to
> +# decr_pc_after_break on this arquitecture.
> +
> +set bp_location1 [gdb_get_line_number "first insn"]
> +set bp_location2 [gdb_get_line_number "second insn"]
> +
> +# Enable recording.
> +
> +gdb_test "record" "" "enable record target"
> +
> +# Step through the breakpoints creating some history.
> +
> +gdb_test "next" ".*second insn.*" "next (1)"
> +gdb_test "next" ".*third insn.*" "next (2)"
> +gdb_test "next" ".*fourth insn.*" "next (3)"
> +gdb_test "next" ".*return 0.*" "next (4)"
> +
> +# Set consecutive breakpoints.
> +
> +gdb_test "break $bp_location1" \
> +    ".*Breakpoint .*$srcfile, line $bp_location1\\." \
> +    "first breakpoint line number"
> +
> +gdb_test "break $bp_location2" \
> +    "Breakpoint.*at.* file .*$srcfile, line $bp_location2\\." \
> +    "second breakpoint line number"
> +
> +# Test that reverse-continue doesn't mess with decr_pc_after_break
> +
> +gdb_test "rc" ".*second insn.*" "reverse continue to breakpoint"
> +gdb_test "rn" ".*first insn.*" "reverse next to begining of times"
> +
> +# Test that replay behaves as normal play.
> +gdb_test "next" ".*second insn.*" "next (1) in replay"
> +gdb_test "next" ".*third insn.*" "next (2) in replay"

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Pedro Alves]

On Friday 24 October 2008 01:37:31, Michael Snyder wrote:
> > In sum, it appears that decr_pc_after_break doesn't matter when you have
> > continguous breakpoints, as long as you get from from B1's address to B2's
> > address by single-stepping.  All is good then, it appears!
> 
> I agree, at least that is the conclusion I am leaning toward.
> 

Not so fast!  I knew I had to spend a little extra thinking about
it, 'cause I knew something was broken, just couldn't find what.  :-)
*as long as you get from from B1's address to B2's address
by single-stepping* was a restriction that doesn't always apply.

Here's a test that will fail in forward record/replay mode, but not
in normal "play" mode.

volatile int global_foo = 0;

int
main (int argc, char **argv)
{
  asm ("nop"); /* 1st insn */
  asm ("nop"); /* 2nd insn */
  asm ("nop"); /* 3rd insn */
  asm ("nop"); /* 4th insn */
  if (!global_foo)
    goto ahead;
  asm ("nop"); /* 5th insn */
  asm ("nop"); /* 6th insn */
  asm ("nop"); /* 7th insn */
  asm ("nop"); /* 8th insn */  <<< break 1 here
 ahead:
  asm ("nop"); /* 9th insn */  <<< break 2 here
 end:
  return 0;
}

If you let the program reply until break 2 is hit, and assuming insn
8th and 9th are assembled as contiguous (they do on x86 -O0 for me), you'll
see that adjust_pc_after_break will indeed make it appear that breakpoint
1 was hit.  Now, nops are nops, but real code could have something
else there...

/me goes back to bed.

-- 
Pedro Alves

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

Thanks Pedro and Michael,

I think the reason is P record let inferior step recycle in the
linux-nat target.
So when it break by breakpint, it will not let
(pc+gdbarch_decr_pc_after_break (gdbarch)). Then after
adjust_pc_after_break, The PC is error.

I will try to deal with it.

Hui

On Fri, Oct 24, 2008 at 09:50, Pedro Alves <pedro@codesourcery.com> wrote:
> On Friday 24 October 2008 01:37:31, Michael Snyder wrote:
>> > In sum, it appears that decr_pc_after_break doesn't matter when you have
>> > continguous breakpoints, as long as you get from from B1's address to B2's
>> > address by single-stepping.  All is good then, it appears!
>>
>> I agree, at least that is the conclusion I am leaning toward.
>>
>
> Not so fast!  I knew I had to spend a little extra thinking about
> it, 'cause I knew something was broken, just couldn't find what.  :-)
> *as long as you get from from B1's address to B2's address
> by single-stepping* was a restriction that doesn't always apply.
>
> Here's a test that will fail in forward record/replay mode, but not
> in normal "play" mode.
>
> volatile int global_foo = 0;
>
> int
> main (int argc, char **argv)
> {
>  asm ("nop"); /* 1st insn */
>  asm ("nop"); /* 2nd insn */
>  asm ("nop"); /* 3rd insn */
>  asm ("nop"); /* 4th insn */
>  if (!global_foo)
>    goto ahead;
>  asm ("nop"); /* 5th insn */
>  asm ("nop"); /* 6th insn */
>  asm ("nop"); /* 7th insn */
>  asm ("nop"); /* 8th insn */  <<< break 1 here
>  ahead:
>  asm ("nop"); /* 9th insn */  <<< break 2 here
>  end:
>  return 0;
> }
>
> If you let the program reply until break 2 is hit, and assuming insn
> 8th and 9th are assembled as contiguous (they do on x86 -O0 for me), you'll
> see that adjust_pc_after_break will indeed make it appear that breakpoint
> 1 was hit.  Now, nops are nops, but real code could have something
> else there...
>
> /me goes back to bed.
>
> --
> Pedro Alves
>

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

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

Hi buddies,

This is the new patch that fix the break bug.

But I think I still need to add some code to deal with signal.

2008-10-24  Hui Zhu  <teawater@gmail.com>

	* record.c (record_wait): Check breakpint before forward
	execute in replay mode.
	Check breakpoint use function "breakpoint_inserted_here_p"
	in replay mode.
	Set pc if forward execute, gdbarch_decr_pc_after_break is not
	0 and this is not single step in replay mode.

	* linux-nat.c (my_waitpid_record): Add
	gdbarch_decr_pc_after_break to pc if need.

Thanks,
Hui

On Fri, Oct 24, 2008 at 16:10, teawater <teawater@gmail.com> wrote:
> Thanks Pedro and Michael,
>
> I think the reason is P record let inferior step recycle in the
> linux-nat target.
> So when it break by breakpint, it will not let
> (pc+gdbarch_decr_pc_after_break (gdbarch)). Then after
> adjust_pc_after_break, The PC is error.
>
> I will try to deal with it.
>
> Hui
>
> On Fri, Oct 24, 2008 at 09:50, Pedro Alves <pedro@codesourcery.com> wrote:
>> On Friday 24 October 2008 01:37:31, Michael Snyder wrote:
>>> > In sum, it appears that decr_pc_after_break doesn't matter when you have
>>> > continguous breakpoints, as long as you get from from B1's address to B2's
>>> > address by single-stepping.  All is good then, it appears!
>>>
>>> I agree, at least that is the conclusion I am leaning toward.
>>>
>>
>> Not so fast!  I knew I had to spend a little extra thinking about
>> it, 'cause I knew something was broken, just couldn't find what.  :-)
>> *as long as you get from from B1's address to B2's address
>> by single-stepping* was a restriction that doesn't always apply.
>>
>> Here's a test that will fail in forward record/replay mode, but not
>> in normal "play" mode.
>>
>> volatile int global_foo = 0;
>>
>> int
>> main (int argc, char **argv)
>> {
>>  asm ("nop"); /* 1st insn */
>>  asm ("nop"); /* 2nd insn */
>>  asm ("nop"); /* 3rd insn */
>>  asm ("nop"); /* 4th insn */
>>  if (!global_foo)
>>    goto ahead;
>>  asm ("nop"); /* 5th insn */
>>  asm ("nop"); /* 6th insn */
>>  asm ("nop"); /* 7th insn */
>>  asm ("nop"); /* 8th insn */  <<< break 1 here
>>  ahead:
>>  asm ("nop"); /* 9th insn */  <<< break 2 here
>>  end:
>>  return 0;
>> }
>>
>> If you let the program reply until break 2 is hit, and assuming insn
>> 8th and 9th are assembled as contiguous (they do on x86 -O0 for me), you'll
>> see that adjust_pc_after_break will indeed make it appear that breakpoint
>> 1 was hit.  Now, nops are nops, but real code could have something
>> else there...
>>
>> /me goes back to bed.
>>
>> --
>> Pedro Alves
>>
>

[-- Attachment #2: record_wait_breakpoint.txt --]
[-- Type: text/plain, Size: 4292 bytes --]

--- a/linux-nat.c
+++ b/linux-nat.c
@@ -514,6 +514,7 @@ my_waitpid_record (int pid, int *status,
   struct bp_location *bl;
   struct breakpoint *b;
   CORE_ADDR pc;
+  CORE_ADDR decr_pc_after_break;
   struct lwp_info *lp;
 
 wait_begin:
@@ -530,7 +531,7 @@ wait_begin:
 
   if (WIFSTOPPED (*status) && WSTOPSIG (*status) == SIGTRAP)
     {
-      /* Check if there is a breakpoint */
+      /* Check if there is a breakpoint.  */
       pc = 0;
       registers_changed ();
       for (bl = bp_location_chain; bl; bl = bl->global_next)
@@ -603,6 +604,20 @@ wait_begin:
     }
 
 out:
+  /* Add gdbarch_decr_pc_after_break to pc because pc will be break address
+     add gdbarch_decr_pc_after_break when inferior non-step execute.  */
+  decr_pc_after_break = gdbarch_decr_pc_after_break
+    (get_regcache_arch (get_thread_regcache (pid_to_ptid (ret))));
+  if (decr_pc_after_break)
+    {
+      if (!pc)
+	{
+	  pc = regcache_read_pc (get_thread_regcache (pid_to_ptid (ret)));
+	}
+      regcache_write_pc (get_thread_regcache (pid_to_ptid (ret)),
+			 pc + decr_pc_after_break);
+    }
+
   return ret;
 }
 
--- a/record.c
+++ b/record.c
@@ -497,6 +497,33 @@ record_wait (ptid_t ptid, struct target_
       int continue_flag = 1;
       int first_record_end = 1;
       struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
+      CORE_ADDR tmp_pc;
+
+      status->kind = TARGET_WAITKIND_STOPPED;
+
+      /* Check breakpoint when forward execute.  */
+      if (execution_direction == EXEC_FORWARD)
+	{
+	  tmp_pc = regcache_read_pc (regcache);
+	  if (breakpoint_inserted_here_p (tmp_pc))
+	    {
+	      if (record_debug)
+		{
+		  fprintf_unfiltered (gdb_stdlog,
+				      "Process record: break at 0x%s.\n",
+				      paddr_nz (tmp_pc));
+		}
+	      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
+		  && !record_resume_step)
+		{
+		  regcache_write_pc (regcache,
+				     tmp_pc +
+				     gdbarch_decr_pc_after_break
+				     (get_regcache_arch (regcache)));
+		}
+	      goto replay_out;
+	    }
+	}
 
       record_get_sig = 0;
       act.sa_handler = record_sig_handler;
@@ -521,7 +548,6 @@ record_wait (ptid_t ptid, struct target_
 
       /* Loop over the record_list, looking for the next place to
 	 stop.  */
-      status->kind = TARGET_WAITKIND_STOPPED;
       do
 	{
 	  /* Check for beginning and end of log.  */
@@ -588,10 +614,6 @@ record_wait (ptid_t ptid, struct target_
 	    }
 	  else
 	    {
-	      CORE_ADDR tmp_pc;
-	      struct bp_location *bl;
-	      struct breakpoint *b;
-
 	      if (record_debug > 1)
 		{
 		  fprintf_unfiltered (gdb_stdlog,
@@ -632,35 +654,25 @@ record_wait (ptid_t ptid, struct target_
 		    }
 
 		  /* check breakpoint */
-		  tmp_pc = read_pc ();
-		  for (bl = bp_location_chain; bl; bl = bl->global_next)
+		  tmp_pc = regcache_read_pc (regcache);
+		  if (breakpoint_inserted_here_p (tmp_pc))
 		    {
-		      b = bl->owner;
-		      gdb_assert (b);
-		      if (b->enable_state != bp_enabled
-			  && b->enable_state != bp_permanent)
-			continue;
-
-		      if (b->type == bp_watchpoint || b->type == bp_catch_fork
-			  || b->type == bp_catch_vfork
-			  || b->type == bp_catch_exec
-			  || b->type == bp_hardware_watchpoint
-			  || b->type == bp_read_watchpoint
-			  || b->type == bp_access_watchpoint)
+		      if (record_debug)
 			{
-			  continue;
+			  fprintf_unfiltered (gdb_stdlog,
+					      "Process record: break at 0x%s.\n",
+					      paddr_nz (tmp_pc));
 			}
-		      if (bl->address == tmp_pc)
+		      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
+			  && execution_direction == EXEC_FORWARD
+			  && !record_resume_step)
 			{
-			  if (record_debug)
-			    {
-			      fprintf_unfiltered (gdb_stdlog,
-						  "Process record: break at 0x%s.\n",
-						  paddr_nz (tmp_pc));
-			    }
-			  continue_flag = 0;
-			  break;
+			  regcache_write_pc (regcache,
+					     tmp_pc +
+					     gdbarch_decr_pc_after_break
+					     (get_regcache_arch (regcache)));
 			}
+		      continue_flag = 0;
 		    }
 		}
 	      if (execution_direction == EXEC_REVERSE)
@@ -691,6 +703,7 @@ next:
 	  perror_with_name (_("Process record: sigaction"));
 	}
 
+replay_out:
       if (record_get_sig)
 	{
 	  status->value.sig = TARGET_SIGNAL_INT;

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

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

The old patch make my_waitpid_record set pc even if this is not a breakpoint.
So I make a new patch that my_waitpid_record just set pc when this is
a breakpoint.


2008-10-24  Hui Zhu  <teawater@gmail.com>

	* record.c (record_wait): Check breakpint before forward
	execute in replay mode.
	Check breakpoint use function "breakpoint_inserted_here_p"
	in replay mode.
	Set pc if forward execute, gdbarch_decr_pc_after_break is not
	0 and this is not single step in replay mode.

	* linux-nat.c (my_waitpid_record): Add
	gdbarch_decr_pc_after_break to pc if need.



On Fri, Oct 24, 2008 at 17:57, teawater <teawater@gmail.com> wrote:
> Hi buddies,
>
> This is the new patch that fix the break bug.
>
> But I think I still need to add some code to deal with signal.
>
> 2008-10-24  Hui Zhu  <teawater@gmail.com>
>
>        * record.c (record_wait): Check breakpint before forward
>        execute in replay mode.
>        Check breakpoint use function "breakpoint_inserted_here_p"
>        in replay mode.
>        Set pc if forward execute, gdbarch_decr_pc_after_break is not
>        0 and this is not single step in replay mode.
>
>        * linux-nat.c (my_waitpid_record): Add
>        gdbarch_decr_pc_after_break to pc if need.
>
> Thanks,
> Hui
>
> On Fri, Oct 24, 2008 at 16:10, teawater <teawater@gmail.com> wrote:
>> Thanks Pedro and Michael,
>>
>> I think the reason is P record let inferior step recycle in the
>> linux-nat target.
>> So when it break by breakpint, it will not let
>> (pc+gdbarch_decr_pc_after_break (gdbarch)). Then after
>> adjust_pc_after_break, The PC is error.
>>
>> I will try to deal with it.
>>
>> Hui
>>
>> On Fri, Oct 24, 2008 at 09:50, Pedro Alves <pedro@codesourcery.com> wrote:
>>> On Friday 24 October 2008 01:37:31, Michael Snyder wrote:
>>>> > In sum, it appears that decr_pc_after_break doesn't matter when you have
>>>> > continguous breakpoints, as long as you get from from B1's address to B2's
>>>> > address by single-stepping.  All is good then, it appears!
>>>>
>>>> I agree, at least that is the conclusion I am leaning toward.
>>>>
>>>
>>> Not so fast!  I knew I had to spend a little extra thinking about
>>> it, 'cause I knew something was broken, just couldn't find what.  :-)
>>> *as long as you get from from B1's address to B2's address
>>> by single-stepping* was a restriction that doesn't always apply.
>>>
>>> Here's a test that will fail in forward record/replay mode, but not
>>> in normal "play" mode.
>>>
>>> volatile int global_foo = 0;
>>>
>>> int
>>> main (int argc, char **argv)
>>> {
>>>  asm ("nop"); /* 1st insn */
>>>  asm ("nop"); /* 2nd insn */
>>>  asm ("nop"); /* 3rd insn */
>>>  asm ("nop"); /* 4th insn */
>>>  if (!global_foo)
>>>    goto ahead;
>>>  asm ("nop"); /* 5th insn */
>>>  asm ("nop"); /* 6th insn */
>>>  asm ("nop"); /* 7th insn */
>>>  asm ("nop"); /* 8th insn */  <<< break 1 here
>>>  ahead:
>>>  asm ("nop"); /* 9th insn */  <<< break 2 here
>>>  end:
>>>  return 0;
>>> }
>>>
>>> If you let the program reply until break 2 is hit, and assuming insn
>>> 8th and 9th are assembled as contiguous (they do on x86 -O0 for me), you'll
>>> see that adjust_pc_after_break will indeed make it appear that breakpoint
>>> 1 was hit.  Now, nops are nops, but real code could have something
>>> else there...
>>>
>>> /me goes back to bed.
>>>
>>> --
>>> Pedro Alves
>>>
>>
>

[-- Attachment #2: record_wait_breakpoint.txt --]
[-- Type: text/plain, Size: 4456 bytes --]

--- a/linux-nat.c
+++ b/linux-nat.c
@@ -514,7 +514,9 @@ my_waitpid_record (int pid, int *status,
   struct bp_location *bl;
   struct breakpoint *b;
   CORE_ADDR pc;
+  CORE_ADDR decr_pc_after_break;
   struct lwp_info *lp;
+  int is_breakpoint = 1;
 
 wait_begin:
   ret = my_waitpid (pid, status, flags);
@@ -530,7 +532,7 @@ wait_begin:
 
   if (WIFSTOPPED (*status) && WSTOPSIG (*status) == SIGTRAP)
     {
-      /* Check if there is a breakpoint */
+      /* Check if there is a breakpoint.  */
       pc = 0;
       registers_changed ();
       for (bl = bp_location_chain; bl; bl = bl->global_next)
@@ -602,7 +604,26 @@ wait_begin:
       goto wait_begin;
     }
 
+  is_breakpoint = 0;
+
 out:
+  /* Add gdbarch_decr_pc_after_break to pc because pc will be break at address
+     add gdbarch_decr_pc_after_break when inferior non-step execute.  */
+  if (is_breakpoint)
+    {
+      decr_pc_after_break = gdbarch_decr_pc_after_break
+	(get_regcache_arch (get_thread_regcache (pid_to_ptid (ret))));
+      if (decr_pc_after_break)
+	{
+	  if (!pc)
+	    {
+	      pc = regcache_read_pc (get_thread_regcache (pid_to_ptid (ret)));
+	    }
+	  regcache_write_pc (get_thread_regcache (pid_to_ptid (ret)),
+			     pc + decr_pc_after_break);
+	}
+    }
+
   return ret;
 }
 
--- a/record.c
+++ b/record.c
@@ -497,6 +497,33 @@ record_wait (ptid_t ptid, struct target_
       int continue_flag = 1;
       int first_record_end = 1;
       struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
+      CORE_ADDR tmp_pc;
+
+      status->kind = TARGET_WAITKIND_STOPPED;
+
+      /* Check breakpoint when forward execute.  */
+      if (execution_direction == EXEC_FORWARD)
+	{
+	  tmp_pc = regcache_read_pc (regcache);
+	  if (breakpoint_inserted_here_p (tmp_pc))
+	    {
+	      if (record_debug)
+		{
+		  fprintf_unfiltered (gdb_stdlog,
+				      "Process record: break at 0x%s.\n",
+				      paddr_nz (tmp_pc));
+		}
+	      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
+		  && !record_resume_step)
+		{
+		  regcache_write_pc (regcache,
+				     tmp_pc +
+				     gdbarch_decr_pc_after_break
+				     (get_regcache_arch (regcache)));
+		}
+	      goto replay_out;
+	    }
+	}
 
       record_get_sig = 0;
       act.sa_handler = record_sig_handler;
@@ -521,7 +548,6 @@ record_wait (ptid_t ptid, struct target_
 
       /* Loop over the record_list, looking for the next place to
 	 stop.  */
-      status->kind = TARGET_WAITKIND_STOPPED;
       do
 	{
 	  /* Check for beginning and end of log.  */
@@ -588,10 +614,6 @@ record_wait (ptid_t ptid, struct target_
 	    }
 	  else
 	    {
-	      CORE_ADDR tmp_pc;
-	      struct bp_location *bl;
-	      struct breakpoint *b;
-
 	      if (record_debug > 1)
 		{
 		  fprintf_unfiltered (gdb_stdlog,
@@ -632,35 +654,25 @@ record_wait (ptid_t ptid, struct target_
 		    }
 
 		  /* check breakpoint */
-		  tmp_pc = read_pc ();
-		  for (bl = bp_location_chain; bl; bl = bl->global_next)
+		  tmp_pc = regcache_read_pc (regcache);
+		  if (breakpoint_inserted_here_p (tmp_pc))
 		    {
-		      b = bl->owner;
-		      gdb_assert (b);
-		      if (b->enable_state != bp_enabled
-			  && b->enable_state != bp_permanent)
-			continue;
-
-		      if (b->type == bp_watchpoint || b->type == bp_catch_fork
-			  || b->type == bp_catch_vfork
-			  || b->type == bp_catch_exec
-			  || b->type == bp_hardware_watchpoint
-			  || b->type == bp_read_watchpoint
-			  || b->type == bp_access_watchpoint)
+		      if (record_debug)
 			{
-			  continue;
+			  fprintf_unfiltered (gdb_stdlog,
+					      "Process record: break at 0x%s.\n",
+					      paddr_nz (tmp_pc));
 			}
-		      if (bl->address == tmp_pc)
+		      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
+			  && execution_direction == EXEC_FORWARD
+			  && !record_resume_step)
 			{
-			  if (record_debug)
-			    {
-			      fprintf_unfiltered (gdb_stdlog,
-						  "Process record: break at 0x%s.\n",
-						  paddr_nz (tmp_pc));
-			    }
-			  continue_flag = 0;
-			  break;
+			  regcache_write_pc (regcache,
+					     tmp_pc +
+					     gdbarch_decr_pc_after_break
+					     (get_regcache_arch (regcache)));
 			}
+		      continue_flag = 0;
 		    }
 		}
 	      if (execution_direction == EXEC_REVERSE)
@@ -691,6 +703,7 @@ next:
 	  perror_with_name (_("Process record: sigaction"));
 	}
 
+replay_out:
       if (record_get_sig)
 	{
 	  status->value.sig = TARGET_SIGNAL_INT;

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

Hi Pedro and Michael,

How do you think about the patch in

http://sourceware.org/ml/gdb-patches/2008-10/msg00634.html

Thanks,
Hui


On Sat, Oct 25, 2008 at 15:07, teawater <teawater@gmail.com> wrote:
> The old patch make my_waitpid_record set pc even if this is not a breakpoint.
> So I make a new patch that my_waitpid_record just set pc when this is
> a breakpoint.
>
>
> 2008-10-24  Hui Zhu  <teawater@gmail.com>
>
>        * record.c (record_wait): Check breakpint before forward
>        execute in replay mode.
>        Check breakpoint use function "breakpoint_inserted_here_p"
>        in replay mode.
>        Set pc if forward execute, gdbarch_decr_pc_after_break is not
>        0 and this is not single step in replay mode.
>
>        * linux-nat.c (my_waitpid_record): Add
>        gdbarch_decr_pc_after_break to pc if need.
>
>
>
> On Fri, Oct 24, 2008 at 17:57, teawater <teawater@gmail.com> wrote:
>> Hi buddies,
>>
>> This is the new patch that fix the break bug.
>>
>> But I think I still need to add some code to deal with signal.
>>
>> 2008-10-24  Hui Zhu  <teawater@gmail.com>
>>
>>        * record.c (record_wait): Check breakpint before forward
>>        execute in replay mode.
>>        Check breakpoint use function "breakpoint_inserted_here_p"
>>        in replay mode.
>>        Set pc if forward execute, gdbarch_decr_pc_after_break is not
>>        0 and this is not single step in replay mode.
>>
>>        * linux-nat.c (my_waitpid_record): Add
>>        gdbarch_decr_pc_after_break to pc if need.
>>
>> Thanks,
>> Hui
>>
>> On Fri, Oct 24, 2008 at 16:10, teawater <teawater@gmail.com> wrote:
>>> Thanks Pedro and Michael,
>>>
>>> I think the reason is P record let inferior step recycle in the
>>> linux-nat target.
>>> So when it break by breakpint, it will not let
>>> (pc+gdbarch_decr_pc_after_break (gdbarch)). Then after
>>> adjust_pc_after_break, The PC is error.
>>>
>>> I will try to deal with it.
>>>
>>> Hui
>>>
>>> On Fri, Oct 24, 2008 at 09:50, Pedro Alves <pedro@codesourcery.com> wrote:
>>>> On Friday 24 October 2008 01:37:31, Michael Snyder wrote:
>>>>> > In sum, it appears that decr_pc_after_break doesn't matter when you have
>>>>> > continguous breakpoints, as long as you get from from B1's address to B2's
>>>>> > address by single-stepping.  All is good then, it appears!
>>>>>
>>>>> I agree, at least that is the conclusion I am leaning toward.
>>>>>
>>>>
>>>> Not so fast!  I knew I had to spend a little extra thinking about
>>>> it, 'cause I knew something was broken, just couldn't find what.  :-)
>>>> *as long as you get from from B1's address to B2's address
>>>> by single-stepping* was a restriction that doesn't always apply.
>>>>
>>>> Here's a test that will fail in forward record/replay mode, but not
>>>> in normal "play" mode.
>>>>
>>>> volatile int global_foo = 0;
>>>>
>>>> int
>>>> main (int argc, char **argv)
>>>> {
>>>>  asm ("nop"); /* 1st insn */
>>>>  asm ("nop"); /* 2nd insn */
>>>>  asm ("nop"); /* 3rd insn */
>>>>  asm ("nop"); /* 4th insn */
>>>>  if (!global_foo)
>>>>    goto ahead;
>>>>  asm ("nop"); /* 5th insn */
>>>>  asm ("nop"); /* 6th insn */
>>>>  asm ("nop"); /* 7th insn */
>>>>  asm ("nop"); /* 8th insn */  <<< break 1 here
>>>>  ahead:
>>>>  asm ("nop"); /* 9th insn */  <<< break 2 here
>>>>  end:
>>>>  return 0;
>>>> }
>>>>
>>>> If you let the program reply until break 2 is hit, and assuming insn
>>>> 8th and 9th are assembled as contiguous (they do on x86 -O0 for me), you'll
>>>> see that adjust_pc_after_break will indeed make it appear that breakpoint
>>>> 1 was hit.  Now, nops are nops, but real code could have something
>>>> else there...
>>>>
>>>> /me goes back to bed.
>>>>
>>>> --
>>>> Pedro Alves
>>>>
>>>
>>
>

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Michael Snyder]

teawater wrote:
> The old patch make my_waitpid_record set pc even if this is not a breakpoint.
> So I make a new patch that my_waitpid_record just set pc when this is
> a breakpoint.

Well, before I can evaluate the patch, I need a test case
to see what behavior it is fixing.  Doesn't have to be a
formal DEJAGNU script, just something like the printf example
that you posted for the other bug.

Right now, I am unable to get the reverse-20080930-branch
to exhibit any bad behavior that I could attribute to this
issue.  It seems to work just fine...


> 2008-10-24  Hui Zhu  <teawater@gmail.com>
> 
>         * record.c (record_wait): Check breakpint before forward
>         execute in replay mode.
>         Check breakpoint use function "breakpoint_inserted_here_p"
>         in replay mode.
>         Set pc if forward execute, gdbarch_decr_pc_after_break is not
>         0 and this is not single step in replay mode.
> 
>         * linux-nat.c (my_waitpid_record): Add
>         gdbarch_decr_pc_after_break to pc if need.
> 
> 
> 
> On Fri, Oct 24, 2008 at 17:57, teawater <teawater@gmail.com> wrote:
>> Hi buddies,
>>
>> This is the new patch that fix the break bug.
>>
>> But I think I still need to add some code to deal with signal.
>>
>> 2008-10-24  Hui Zhu  <teawater@gmail.com>
>>
>>        * record.c (record_wait): Check breakpint before forward
>>        execute in replay mode.
>>        Check breakpoint use function "breakpoint_inserted_here_p"
>>        in replay mode.
>>        Set pc if forward execute, gdbarch_decr_pc_after_break is not
>>        0 and this is not single step in replay mode.
>>
>>        * linux-nat.c (my_waitpid_record): Add
>>        gdbarch_decr_pc_after_break to pc if need.
>>
>> Thanks,
>> Hui
>>
>> On Fri, Oct 24, 2008 at 16:10, teawater <teawater@gmail.com> wrote:
>>> Thanks Pedro and Michael,
>>>
>>> I think the reason is P record let inferior step recycle in the
>>> linux-nat target.
>>> So when it break by breakpint, it will not let
>>> (pc+gdbarch_decr_pc_after_break (gdbarch)). Then after
>>> adjust_pc_after_break, The PC is error.
>>>
>>> I will try to deal with it.
>>>
>>> Hui
>>>
>>> On Fri, Oct 24, 2008 at 09:50, Pedro Alves <pedro@codesourcery.com> wrote:
>>>> On Friday 24 October 2008 01:37:31, Michael Snyder wrote:
>>>>>> In sum, it appears that decr_pc_after_break doesn't matter when you have
>>>>>> continguous breakpoints, as long as you get from from B1's address to B2's
>>>>>> address by single-stepping.  All is good then, it appears!
>>>>> I agree, at least that is the conclusion I am leaning toward.
>>>>>
>>>> Not so fast!  I knew I had to spend a little extra thinking about
>>>> it, 'cause I knew something was broken, just couldn't find what.  :-)
>>>> *as long as you get from from B1's address to B2's address
>>>> by single-stepping* was a restriction that doesn't always apply.
>>>>
>>>> Here's a test that will fail in forward record/replay mode, but not
>>>> in normal "play" mode.
>>>>
>>>> volatile int global_foo = 0;
>>>>
>>>> int
>>>> main (int argc, char **argv)
>>>> {
>>>>  asm ("nop"); /* 1st insn */
>>>>  asm ("nop"); /* 2nd insn */
>>>>  asm ("nop"); /* 3rd insn */
>>>>  asm ("nop"); /* 4th insn */
>>>>  if (!global_foo)
>>>>    goto ahead;
>>>>  asm ("nop"); /* 5th insn */
>>>>  asm ("nop"); /* 6th insn */
>>>>  asm ("nop"); /* 7th insn */
>>>>  asm ("nop"); /* 8th insn */  <<< break 1 here
>>>>  ahead:
>>>>  asm ("nop"); /* 9th insn */  <<< break 2 here
>>>>  end:
>>>>  return 0;
>>>> }
>>>>
>>>> If you let the program reply until break 2 is hit, and assuming insn
>>>> 8th and 9th are assembled as contiguous (they do on x86 -O0 for me), you'll
>>>> see that adjust_pc_after_break will indeed make it appear that breakpoint
>>>> 1 was hit.  Now, nops are nops, but real code could have something
>>>> else there...
>>>>
>>>> /me goes back to bed.
>>>>
>>>> --
>>>> Pedro Alves
>>>>
>>
>> ------------------------------------------------------------------------
>>
>> --- a/linux-nat.c
>> +++ b/linux-nat.c
>> @@ -514,7 +514,9 @@ my_waitpid_record (int pid, int *status,
>>    struct bp_location *bl;
>>    struct breakpoint *b;
>>    CORE_ADDR pc;
>> +  CORE_ADDR decr_pc_after_break;
>>    struct lwp_info *lp;
>> +  int is_breakpoint = 1;
>>  
>>  wait_begin:
>>    ret = my_waitpid (pid, status, flags);
>> @@ -530,7 +532,7 @@ wait_begin:
>>  
>>    if (WIFSTOPPED (*status) && WSTOPSIG (*status) == SIGTRAP)
>>      {
>> -      /* Check if there is a breakpoint */
>> +      /* Check if there is a breakpoint.  */
>>        pc = 0;
>>        registers_changed ();
>>        for (bl = bp_location_chain; bl; bl = bl->global_next)
>> @@ -602,7 +604,26 @@ wait_begin:
>>        goto wait_begin;
>>      }
>>  
>> +  is_breakpoint = 0;
>> +
>>  out:
>> +  /* Add gdbarch_decr_pc_after_break to pc because pc will be break at address
>> +     add gdbarch_decr_pc_after_break when inferior non-step execute.  */
>> +  if (is_breakpoint)
>> +    {
>> +      decr_pc_after_break = gdbarch_decr_pc_after_break
>> +	(get_regcache_arch (get_thread_regcache (pid_to_ptid (ret))));
>> +      if (decr_pc_after_break)
>> +	{
>> +	  if (!pc)
>> +	    {
>> +	      pc = regcache_read_pc (get_thread_regcache (pid_to_ptid (ret)));
>> +	    }
>> +	  regcache_write_pc (get_thread_regcache (pid_to_ptid (ret)),
>> +			     pc + decr_pc_after_break);
>> +	}
>> +    }
>> +
>>    return ret;
>>  }
>>  
>> --- a/record.c
>> +++ b/record.c
>> @@ -497,6 +497,33 @@ record_wait (ptid_t ptid, struct target_
>>        int continue_flag = 1;
>>        int first_record_end = 1;
>>        struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups, 0);
>> +      CORE_ADDR tmp_pc;
>> +
>> +      status->kind = TARGET_WAITKIND_STOPPED;
>> +
>> +      /* Check breakpoint when forward execute.  */
>> +      if (execution_direction == EXEC_FORWARD)
>> +	{
>> +	  tmp_pc = regcache_read_pc (regcache);
>> +	  if (breakpoint_inserted_here_p (tmp_pc))
>> +	    {
>> +	      if (record_debug)
>> +		{
>> +		  fprintf_unfiltered (gdb_stdlog,
>> +				      "Process record: break at 0x%s.\n",
>> +				      paddr_nz (tmp_pc));
>> +		}
>> +	      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
>> +		  && !record_resume_step)
>> +		{
>> +		  regcache_write_pc (regcache,
>> +				     tmp_pc +
>> +				     gdbarch_decr_pc_after_break
>> +				     (get_regcache_arch (regcache)));
>> +		}
>> +	      goto replay_out;
>> +	    }
>> +	}
>>  
>>        record_get_sig = 0;
>>        act.sa_handler = record_sig_handler;
>> @@ -521,7 +548,6 @@ record_wait (ptid_t ptid, struct target_
>>  
>>        /* Loop over the record_list, looking for the next place to
>>  	 stop.  */
>> -      status->kind = TARGET_WAITKIND_STOPPED;
>>        do
>>  	{
>>  	  /* Check for beginning and end of log.  */
>> @@ -588,10 +614,6 @@ record_wait (ptid_t ptid, struct target_
>>  	    }
>>  	  else
>>  	    {
>> -	      CORE_ADDR tmp_pc;
>> -	      struct bp_location *bl;
>> -	      struct breakpoint *b;
>> -
>>  	      if (record_debug > 1)
>>  		{
>>  		  fprintf_unfiltered (gdb_stdlog,
>> @@ -632,35 +654,25 @@ record_wait (ptid_t ptid, struct target_
>>  		    }
>>  
>>  		  /* check breakpoint */
>> -		  tmp_pc = read_pc ();
>> -		  for (bl = bp_location_chain; bl; bl = bl->global_next)
>> +		  tmp_pc = regcache_read_pc (regcache);
>> +		  if (breakpoint_inserted_here_p (tmp_pc))
>>  		    {
>> -		      b = bl->owner;
>> -		      gdb_assert (b);
>> -		      if (b->enable_state != bp_enabled
>> -			  && b->enable_state != bp_permanent)
>> -			continue;
>> -
>> -		      if (b->type == bp_watchpoint || b->type == bp_catch_fork
>> -			  || b->type == bp_catch_vfork
>> -			  || b->type == bp_catch_exec
>> -			  || b->type == bp_hardware_watchpoint
>> -			  || b->type == bp_read_watchpoint
>> -			  || b->type == bp_access_watchpoint)
>> +		      if (record_debug)
>>  			{
>> -			  continue;
>> +			  fprintf_unfiltered (gdb_stdlog,
>> +					      "Process record: break at 0x%s.\n",
>> +					      paddr_nz (tmp_pc));
>>  			}
>> -		      if (bl->address == tmp_pc)
>> +		      if (gdbarch_decr_pc_after_break (get_regcache_arch (regcache))
>> +			  && execution_direction == EXEC_FORWARD
>> +			  && !record_resume_step)
>>  			{
>> -			  if (record_debug)
>> -			    {
>> -			      fprintf_unfiltered (gdb_stdlog,
>> -						  "Process record: break at 0x%s.\n",
>> -						  paddr_nz (tmp_pc));
>> -			    }
>> -			  continue_flag = 0;
>> -			  break;
>> +			  regcache_write_pc (regcache,
>> +					     tmp_pc +
>> +					     gdbarch_decr_pc_after_break
>> +					     (get_regcache_arch (regcache)));
>>  			}
>> +		      continue_flag = 0;
>>  		    }
>>  		}
>>  	      if (execution_direction == EXEC_REVERSE)
>> @@ -691,6 +703,7 @@ next:
>>  	  perror_with_name (_("Process record: sigaction"));
>>  	}
>>  
>> +replay_out:
>>        if (record_get_sig)
>>  	{
>>  	  status->value.sig = TARGET_SIGNAL_INT;

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

Maybe I can checked it in first. :)

On Wed, Oct 29, 2008 at 06:51, Michael Snyder <msnyder@vmware.com> wrote:
> teawater wrote:
>>
>> The old patch make my_waitpid_record set pc even if this is not a
>> breakpoint.
>> So I make a new patch that my_waitpid_record just set pc when this is
>> a breakpoint.
>
> Well, before I can evaluate the patch, I need a test case
> to see what behavior it is fixing.  Doesn't have to be a
> formal DEJAGNU script, just something like the printf example
> that you posted for the other bug.
>
> Right now, I am unable to get the reverse-20080930-branch
> to exhibit any bad behavior that I could attribute to this
> issue.  It seems to work just fine...
>
>
>> 2008-10-24  Hui Zhu  <teawater@gmail.com>
>>
>>        * record.c (record_wait): Check breakpint before forward
>>        execute in replay mode.
>>        Check breakpoint use function "breakpoint_inserted_here_p"
>>        in replay mode.
>>        Set pc if forward execute, gdbarch_decr_pc_after_break is not
>>        0 and this is not single step in replay mode.
>>
>>        * linux-nat.c (my_waitpid_record): Add
>>        gdbarch_decr_pc_after_break to pc if need.
>>
>>
>>
>> On Fri, Oct 24, 2008 at 17:57, teawater <teawater@gmail.com> wrote:
>>>
>>> Hi buddies,
>>>
>>> This is the new patch that fix the break bug.
>>>
>>> But I think I still need to add some code to deal with signal.
>>>
>>> 2008-10-24  Hui Zhu  <teawater@gmail.com>
>>>
>>>       * record.c (record_wait): Check breakpint before forward
>>>       execute in replay mode.
>>>       Check breakpoint use function "breakpoint_inserted_here_p"
>>>       in replay mode.
>>>       Set pc if forward execute, gdbarch_decr_pc_after_break is not
>>>       0 and this is not single step in replay mode.
>>>
>>>       * linux-nat.c (my_waitpid_record): Add
>>>       gdbarch_decr_pc_after_break to pc if need.
>>>
>>> Thanks,
>>> Hui
>>>
>>> On Fri, Oct 24, 2008 at 16:10, teawater <teawater@gmail.com> wrote:
>>>>
>>>> Thanks Pedro and Michael,
>>>>
>>>> I think the reason is P record let inferior step recycle in the
>>>> linux-nat target.
>>>> So when it break by breakpint, it will not let
>>>> (pc+gdbarch_decr_pc_after_break (gdbarch)). Then after
>>>> adjust_pc_after_break, The PC is error.
>>>>
>>>> I will try to deal with it.
>>>>
>>>> Hui
>>>>
>>>> On Fri, Oct 24, 2008 at 09:50, Pedro Alves <pedro@codesourcery.com>
>>>> wrote:
>>>>>
>>>>> On Friday 24 October 2008 01:37:31, Michael Snyder wrote:
>>>>>>>
>>>>>>> In sum, it appears that decr_pc_after_break doesn't matter when you
>>>>>>> have
>>>>>>> continguous breakpoints, as long as you get from from B1's address to
>>>>>>> B2's
>>>>>>> address by single-stepping.  All is good then, it appears!
>>>>>>
>>>>>> I agree, at least that is the conclusion I am leaning toward.
>>>>>>
>>>>> Not so fast!  I knew I had to spend a little extra thinking about
>>>>> it, 'cause I knew something was broken, just couldn't find what.  :-)
>>>>> *as long as you get from from B1's address to B2's address
>>>>> by single-stepping* was a restriction that doesn't always apply.
>>>>>
>>>>> Here's a test that will fail in forward record/replay mode, but not
>>>>> in normal "play" mode.
>>>>>
>>>>> volatile int global_foo = 0;
>>>>>
>>>>> int
>>>>> main (int argc, char **argv)
>>>>> {
>>>>>  asm ("nop"); /* 1st insn */
>>>>>  asm ("nop"); /* 2nd insn */
>>>>>  asm ("nop"); /* 3rd insn */
>>>>>  asm ("nop"); /* 4th insn */
>>>>>  if (!global_foo)
>>>>>   goto ahead;
>>>>>  asm ("nop"); /* 5th insn */
>>>>>  asm ("nop"); /* 6th insn */
>>>>>  asm ("nop"); /* 7th insn */
>>>>>  asm ("nop"); /* 8th insn */  <<< break 1 here
>>>>>  ahead:
>>>>>  asm ("nop"); /* 9th insn */  <<< break 2 here
>>>>>  end:
>>>>>  return 0;
>>>>> }
>>>>>
>>>>> If you let the program reply until break 2 is hit, and assuming insn
>>>>> 8th and 9th are assembled as contiguous (they do on x86 -O0 for me),
>>>>> you'll
>>>>> see that adjust_pc_after_break will indeed make it appear that
>>>>> breakpoint
>>>>> 1 was hit.  Now, nops are nops, but real code could have something
>>>>> else there...
>>>>>
>>>>> /me goes back to bed.
>>>>>
>>>>> --
>>>>> Pedro Alves
>>>>>
>>>
>>> ------------------------------------------------------------------------
>>>
>>> --- a/linux-nat.c
>>> +++ b/linux-nat.c
>>> @@ -514,7 +514,9 @@ my_waitpid_record (int pid, int *status,
>>>   struct bp_location *bl;
>>>   struct breakpoint *b;
>>>   CORE_ADDR pc;
>>> +  CORE_ADDR decr_pc_after_break;
>>>   struct lwp_info *lp;
>>> +  int is_breakpoint = 1;
>>>   wait_begin:
>>>   ret = my_waitpid (pid, status, flags);
>>> @@ -530,7 +532,7 @@ wait_begin:
>>>     if (WIFSTOPPED (*status) && WSTOPSIG (*status) == SIGTRAP)
>>>     {
>>> -      /* Check if there is a breakpoint */
>>> +      /* Check if there is a breakpoint.  */
>>>       pc = 0;
>>>       registers_changed ();
>>>       for (bl = bp_location_chain; bl; bl = bl->global_next)
>>> @@ -602,7 +604,26 @@ wait_begin:
>>>       goto wait_begin;
>>>     }
>>>  +  is_breakpoint = 0;
>>> +
>>>  out:
>>> +  /* Add gdbarch_decr_pc_after_break to pc because pc will be break at
>>> address
>>> +     add gdbarch_decr_pc_after_break when inferior non-step execute.  */
>>> +  if (is_breakpoint)
>>> +    {
>>> +      decr_pc_after_break = gdbarch_decr_pc_after_break
>>> +       (get_regcache_arch (get_thread_regcache (pid_to_ptid (ret))));
>>> +      if (decr_pc_after_break)
>>> +       {
>>> +         if (!pc)
>>> +           {
>>> +             pc = regcache_read_pc (get_thread_regcache (pid_to_ptid
>>> (ret)));
>>> +           }
>>> +         regcache_write_pc (get_thread_regcache (pid_to_ptid (ret)),
>>> +                            pc + decr_pc_after_break);
>>> +       }
>>> +    }
>>> +
>>>   return ret;
>>>  }
>>>  --- a/record.c
>>> +++ b/record.c
>>> @@ -497,6 +497,33 @@ record_wait (ptid_t ptid, struct target_
>>>       int continue_flag = 1;
>>>       int first_record_end = 1;
>>>       struct cleanup *old_cleanups = make_cleanup (record_wait_cleanups,
>>> 0);
>>> +      CORE_ADDR tmp_pc;
>>> +
>>> +      status->kind = TARGET_WAITKIND_STOPPED;
>>> +
>>> +      /* Check breakpoint when forward execute.  */
>>> +      if (execution_direction == EXEC_FORWARD)
>>> +       {
>>> +         tmp_pc = regcache_read_pc (regcache);
>>> +         if (breakpoint_inserted_here_p (tmp_pc))
>>> +           {
>>> +             if (record_debug)
>>> +               {
>>> +                 fprintf_unfiltered (gdb_stdlog,
>>> +                                     "Process record: break at 0x%s.\n",
>>> +                                     paddr_nz (tmp_pc));
>>> +               }
>>> +             if (gdbarch_decr_pc_after_break (get_regcache_arch
>>> (regcache))
>>> +                 && !record_resume_step)
>>> +               {
>>> +                 regcache_write_pc (regcache,
>>> +                                    tmp_pc +
>>> +                                    gdbarch_decr_pc_after_break
>>> +                                    (get_regcache_arch (regcache)));
>>> +               }
>>> +             goto replay_out;
>>> +           }
>>> +       }
>>>         record_get_sig = 0;
>>>       act.sa_handler = record_sig_handler;
>>> @@ -521,7 +548,6 @@ record_wait (ptid_t ptid, struct target_
>>>         /* Loop over the record_list, looking for the next place to
>>>         stop.  */
>>> -      status->kind = TARGET_WAITKIND_STOPPED;
>>>       do
>>>        {
>>>          /* Check for beginning and end of log.  */
>>> @@ -588,10 +614,6 @@ record_wait (ptid_t ptid, struct target_
>>>            }
>>>          else
>>>            {
>>> -             CORE_ADDR tmp_pc;
>>> -             struct bp_location *bl;
>>> -             struct breakpoint *b;
>>> -
>>>              if (record_debug > 1)
>>>                {
>>>                  fprintf_unfiltered (gdb_stdlog,
>>> @@ -632,35 +654,25 @@ record_wait (ptid_t ptid, struct target_
>>>                    }
>>>                    /* check breakpoint */
>>> -                 tmp_pc = read_pc ();
>>> -                 for (bl = bp_location_chain; bl; bl = bl->global_next)
>>> +                 tmp_pc = regcache_read_pc (regcache);
>>> +                 if (breakpoint_inserted_here_p (tmp_pc))
>>>                    {
>>> -                     b = bl->owner;
>>> -                     gdb_assert (b);
>>> -                     if (b->enable_state != bp_enabled
>>> -                         && b->enable_state != bp_permanent)
>>> -                       continue;
>>> -
>>> -                     if (b->type == bp_watchpoint || b->type ==
>>> bp_catch_fork
>>> -                         || b->type == bp_catch_vfork
>>> -                         || b->type == bp_catch_exec
>>> -                         || b->type == bp_hardware_watchpoint
>>> -                         || b->type == bp_read_watchpoint
>>> -                         || b->type == bp_access_watchpoint)
>>> +                     if (record_debug)
>>>                        {
>>> -                         continue;
>>> +                         fprintf_unfiltered (gdb_stdlog,
>>> +                                             "Process record: break at
>>> 0x%s.\n",
>>> +                                             paddr_nz (tmp_pc));
>>>                        }
>>> -                     if (bl->address == tmp_pc)
>>> +                     if (gdbarch_decr_pc_after_break (get_regcache_arch
>>> (regcache))
>>> +                         && execution_direction == EXEC_FORWARD
>>> +                         && !record_resume_step)
>>>                        {
>>> -                         if (record_debug)
>>> -                           {
>>> -                             fprintf_unfiltered (gdb_stdlog,
>>> -                                                 "Process record: break
>>> at 0x%s.\n",
>>> -                                                 paddr_nz (tmp_pc));
>>> -                           }
>>> -                         continue_flag = 0;
>>> -                         break;
>>> +                         regcache_write_pc (regcache,
>>> +                                            tmp_pc +
>>> +                                            gdbarch_decr_pc_after_break
>>> +                                            (get_regcache_arch
>>> (regcache)));
>>>                        }
>>> +                     continue_flag = 0;
>>>                    }
>>>                }
>>>              if (execution_direction == EXEC_REVERSE)
>>> @@ -691,6 +703,7 @@ next:
>>>          perror_with_name (_("Process record: sigaction"));
>>>        }
>>>  +replay_out:
>>>       if (record_get_sig)
>>>        {
>>>          status->value.sig = TARGET_SIGNAL_INT;
>
>

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Pedro Alves]

On Tuesday 28 October 2008 22:51:52, Michael Snyder wrote:

> Well, before I can evaluate the patch, I need a test case
> to see what behavior it is fixing.  Doesn't have to be a
> formal DEJAGNU script, just something like the printf example
> that you posted for the other bug.
> 
> Right now, I am unable to get the reverse-20080930-branch
> to exhibit any bad behavior that I could attribute to this
> issue.  It seems to work just fine...

Wouldn't that be the extended nop+goto example I posted?

 http://sourceware.org/ml/gdb-patches/2008-10/msg00599.html

Hui, I'm now lost in this huge thread that never seems to
end, but I think that the last patch I saw, you still
missed that you should check for software_breakpoint_inserted_here_p
before doing the adjustment (see adjust_pc_after_break) --- it was
there in the first patch I posted to address this issue.

This decr after break business sucks.  For remote targets implementing
software breakpoints, it would probably be best if we had a remote protocol
feature with a corresponding property for this at the target_ops level that
overrides the gdbarch setting.  There are probably many targets out there
implementing Z0 breakpoints that do the adjustment themselves.  It's just
that it's not common to trip on it, so it gets by.

-- 
Pedro Alves

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Michael Snyder]

OK, I'm with you now.  Thanks a lot for clarifying.  ;-)

Pedro Alves wrote:
> On Thursday 30 October 2008 15:54:34, Michael Snyder wrote:
>> Pedro, yes, but I can no longer get it to exhibit  that behavior.
>> Can you?
>>
> 
> Sure, see below.
> 
> Head of ChangeLog:
> 
>  2008-10-24  Michael Snyder  <msnyder@vmware.com>
> 
>          * infrun.c (handle_inferior_event): Handle dynamic symbol
>          resolution in reverse.
> 
> Test app:
> 
> 18      volatile int global_foo = 0;
> 19
> 20      int
> 21      main (int argc, char **argv)
> 22      {
> 23        asm ("nop"); /* 1st insn */
> 24        asm ("nop"); /* 2nd insn */
> 25        asm ("nop"); /* 3rd insn */
> 26        asm ("nop"); /* 4th insn */
> 27        if (!global_foo)
> 28          goto ahead;
> 29        asm ("nop"); /* 5th insn */
> 30        asm ("nop"); /* 6th insn */
> 31        asm ("nop"); /* 7th insn */
> 32        asm ("nop"); /* 8th insn */  <<<<< bkpt here
> 33       ahead:
> 34        asm ("nop"); /* 9th insn */  <<<<< and here
> 35       end:
> 36        return 0;
> 37      }
> 
> Normal play:
> 
>> ./gdb ./testsuite/gdb.base/decr-pc-rev
> GNU gdb (GDB) 6.8.50.20080930-cvs
> [...]
> (gdb) start
> Temporary breakpoint 1 at 0x8048382: file ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c, line 23.
> Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/testsuite/gdb.base/decr-pc-rev
> 
> Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value optimized out>)
>     at ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c:23
> 23        asm ("nop"); /* 1st insn */
> (gdb) b 32
> Breakpoint 2 at 0x8048392: file ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c, line 32.
> (gdb) b 34
> Breakpoint 3 at 0x8048393: file ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c, line 34.
> (gdb) c
> Continuing.
> 
> Breakpoint 3, main (argc=<value optimized out>, argv=<value optimized out>)
>     at ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c:34
> 34      in ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c
> (gdb) p $pc
> $1 = (void (*)()) 0x8048393 <main+31>
> (gdb)
> 
> Ok, breakpoint 3 was hit (notice the goto at line 28, it's
> always executed because global_foo is always 0)
> 
> --------
> 
> Now the same, but while recording (replay exhibits the
> same symptom)
> 
>> ./gdb ./testsuite/gdb.base/decr-pc-rev
> GNU gdb (GDB) 6.8.50.20080930-cvs
> [...]
> (gdb) start
> Temporary breakpoint 1 at 0x8048382: file ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c, line 23.
> Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/testsuite/gdb.base/decr-pc-rev
> 
> Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value optimized out>)
>     at ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c:23
> 23        asm ("nop"); /* 1st insn */
> (gdb) record
>       ^^^^^^
> (gdb) c
> Continuing.
> 
> Breakpoint 2, main (argc=<value optimized out>, argv=<value optimized out>)
>     at ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c:32
> 32      in ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c
> (gdb) p $pc
> $1 = (void (*)()) 0x8048392 <main+30>
> (gdb)
> 
> Breakpoint 3 should've been hit, not 2.  The PC points at
> 0x8048392, but it should point at 0x8048393.
> 
> Feels like we're going in circles.  :-)
> 
> --
> Pedro Alves

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Pedro Alves]

On Thursday 30 October 2008 15:54:34, Michael Snyder wrote:
> Pedro, yes, but I can no longer get it to exhibit  that behavior.
> Can you?
> 

Sure, see below.

Head of ChangeLog:

 2008-10-24  Michael Snyder  <msnyder@vmware.com>
 
         * infrun.c (handle_inferior_event): Handle dynamic symbol
         resolution in reverse.

Test app:

18      volatile int global_foo = 0;
19
20      int
21      main (int argc, char **argv)
22      {
23        asm ("nop"); /* 1st insn */
24        asm ("nop"); /* 2nd insn */
25        asm ("nop"); /* 3rd insn */
26        asm ("nop"); /* 4th insn */
27        if (!global_foo)
28          goto ahead;
29        asm ("nop"); /* 5th insn */
30        asm ("nop"); /* 6th insn */
31        asm ("nop"); /* 7th insn */
32        asm ("nop"); /* 8th insn */  <<<<< bkpt here
33       ahead:
34        asm ("nop"); /* 9th insn */  <<<<< and here
35       end:
36        return 0;
37      }

Normal play:

>./gdb ./testsuite/gdb.base/decr-pc-rev
GNU gdb (GDB) 6.8.50.20080930-cvs
[...]
(gdb) start
Temporary breakpoint 1 at 0x8048382: file ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c, line 23.
Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/testsuite/gdb.base/decr-pc-rev

Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value optimized out>)
    at ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c:23
23        asm ("nop"); /* 1st insn */
(gdb) b 32
Breakpoint 2 at 0x8048392: file ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c, line 32.
(gdb) b 34
Breakpoint 3 at 0x8048393: file ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c, line 34.
(gdb) c
Continuing.

Breakpoint 3, main (argc=<value optimized out>, argv=<value optimized out>)
    at ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c:34
34      in ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c
(gdb) p $pc
$1 = (void (*)()) 0x8048393 <main+31>
(gdb)           

Ok, breakpoint 3 was hit (notice the goto at line 28, it's
always executed because global_foo is always 0)

--------

Now the same, but while recording (replay exhibits the
same symptom)

>./gdb ./testsuite/gdb.base/decr-pc-rev
GNU gdb (GDB) 6.8.50.20080930-cvs
[...]
(gdb) start
Temporary breakpoint 1 at 0x8048382: file ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c, line 23.
Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/testsuite/gdb.base/decr-pc-rev

Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value optimized out>)
    at ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c:23
23        asm ("nop"); /* 1st insn */
(gdb) record
      ^^^^^^ 
(gdb) c
Continuing.

Breakpoint 2, main (argc=<value optimized out>, argv=<value optimized out>)
    at ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c:32
32      in ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c
(gdb) p $pc
$1 = (void (*)()) 0x8048392 <main+30>
(gdb)      

Breakpoint 3 should've been hit, not 2.  The PC points at
0x8048392, but it should point at 0x8048393.

Feels like we're going in circles.  :-)

-- 
Pedro Alves

RE: [reverse/record] adjust_pc_after_break in reverse execution mode?

[Michael Snyder]

Pedro, yes, but I can no longer get it to exhibit  that behavior.
Can you?

Remember, there've been changes since then.


________________________________________
From: Pedro Alves [pedro@codesourcery.com]
Sent: Thursday, October 30, 2008 4:07 AM
To: gdb-patches@sourceware.org
Cc: Michael Snyder; teawater
Subject: Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

On Tuesday 28 October 2008 22:51:52, Michael Snyder wrote:

> Well, before I can evaluate the patch, I need a test case
> to see what behavior it is fixing.  Doesn't have to be a
> formal DEJAGNU script, just something like the printf example
> that you posted for the other bug.
>
> Right now, I am unable to get the reverse-20080930-branch
> to exhibit any bad behavior that I could attribute to this
> issue.  It seems to work just fine...

Wouldn't that be the extended nop+goto example I posted?

 http://sourceware.org/ml/gdb-patches/2008-10/msg00599.html

Hui, I'm now lost in this huge thread that never seems to
end, but I think that the last patch I saw, you still
missed that you should check for software_breakpoint_inserted_here_p
before doing the adjustment (see adjust_pc_after_break) --- it was
there in the first patch I posted to address this issue.

This decr after break business sucks.  For remote targets implementing
software breakpoints, it would probably be best if we had a remote protocol
feature with a corresponding property for this at the target_ops level that
overrides the gdbarch setting.  There are probably many targets out there
implementing Z0 breakpoints that do the adjustment themselves.  It's just
that it's not common to trip on it, so it gets by.

--
Pedro Alves

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

Hi Pedro,

On Thu, Oct 30, 2008 at 19:07, Pedro Alves <pedro@codesourcery.com> wrote:
>
> On Tuesday 28 October 2008 22:51:52, Michael Snyder wrote:
>
> > Well, before I can evaluate the patch, I need a test case
> > to see what behavior it is fixing.  Doesn't have to be a
> > formal DEJAGNU script, just something like the printf example
> > that you posted for the other bug.
> >
> > Right now, I am unable to get the reverse-20080930-branch
> > to exhibit any bad behavior that I could attribute to this
> > issue.  It seems to work just fine...
>
> Wouldn't that be the extended nop+goto example I posted?
>
>  http://sourceware.org/ml/gdb-patches/2008-10/msg00599.html

Yes,  It's OK with the newest patch.

>
> Hui, I'm now lost in this huge thread that never seems to
> end, but I think that the last patch I saw, you still
> missed that you should check for software_breakpoint_inserted_here_p
> before doing the adjustment (see adjust_pc_after_break) --- it was
> there in the first patch I posted to address this issue.
>
> This decr after break business sucks.  For remote targets implementing
> software breakpoints, it would probably be best if we had a remote protocol
> feature with a corresponding property for this at the target_ops level that
> overrides the gdbarch setting.  There are probably many targets out there
> implementing Z0 breakpoints that do the adjustment themselves.  It's just
> that it's not common to trip on it, so it gets by.

That is a really good idea.
Maybe we can add a special interface in target_ops tell the infrun.c
that this PC need adjust_pc_after_break or not.

Thanks,
Hui

Re: [reverse/record] adjust_pc_after_break in reverse execution mode?

[teawater]

I think the reason is P record let inferior step recycle in the
linux-nat target.
So when it break by breakpint, it will not let
(pc+gdbarch_decr_pc_after_break (gdbarch)). Then after
adjust_pc_after_break, The PC is error.

So I post patch to fix it.
http://sourceware.org/ml/gdb-patches/2008-10/msg00634.html
And I try it in Pedro's test is OK.


After that, Pedro said maybe we can add a target_ops level interface
tell infrun.c that this PC don't need adjust_pc_after_break. This
target can do it with itself.



So, what shell we do now? :)
Check my patch in or post a patch for this interface first?


This is just my understand of current stat. Maybe some part is wrong.


On Fri, Oct 31, 2008 at 00:15, Pedro Alves <pedro@codesourcery.com> wrote:
> On Thursday 30 October 2008 15:54:34, Michael Snyder wrote:
>> Pedro, yes, but I can no longer get it to exhibit  that behavior.
>> Can you?
>>
>
> Sure, see below.
>
> Head of ChangeLog:
>
>  2008-10-24  Michael Snyder  <msnyder@vmware.com>
>
>         * infrun.c (handle_inferior_event): Handle dynamic symbol
>         resolution in reverse.
>
> Test app:
>
> 18      volatile int global_foo = 0;
> 19
> 20      int
> 21      main (int argc, char **argv)
> 22      {
> 23        asm ("nop"); /* 1st insn */
> 24        asm ("nop"); /* 2nd insn */
> 25        asm ("nop"); /* 3rd insn */
> 26        asm ("nop"); /* 4th insn */
> 27        if (!global_foo)
> 28          goto ahead;
> 29        asm ("nop"); /* 5th insn */
> 30        asm ("nop"); /* 6th insn */
> 31        asm ("nop"); /* 7th insn */
> 32        asm ("nop"); /* 8th insn */  <<<<< bkpt here
> 33       ahead:
> 34        asm ("nop"); /* 9th insn */  <<<<< and here
> 35       end:
> 36        return 0;
> 37      }
>
> Normal play:
>
>>./gdb ./testsuite/gdb.base/decr-pc-rev
> GNU gdb (GDB) 6.8.50.20080930-cvs
> [...]
> (gdb) start
> Temporary breakpoint 1 at 0x8048382: file ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c, line 23.
> Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/testsuite/gdb.base/decr-pc-rev
>
> Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value optimized out>)
>    at ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c:23
> 23        asm ("nop"); /* 1st insn */
> (gdb) b 32
> Breakpoint 2 at 0x8048392: file ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c, line 32.
> (gdb) b 34
> Breakpoint 3 at 0x8048393: file ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c, line 34.
> (gdb) c
> Continuing.
>
> Breakpoint 3, main (argc=<value optimized out>, argv=<value optimized out>)
>    at ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c:34
> 34      in ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c
> (gdb) p $pc
> $1 = (void (*)()) 0x8048393 <main+31>
> (gdb)
>
> Ok, breakpoint 3 was hit (notice the goto at line 28, it's
> always executed because global_foo is always 0)
>
> --------
>
> Now the same, but while recording (replay exhibits the
> same symptom)
>
>>./gdb ./testsuite/gdb.base/decr-pc-rev
> GNU gdb (GDB) 6.8.50.20080930-cvs
> [...]
> (gdb) start
> Temporary breakpoint 1 at 0x8048382: file ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c, line 23.
> Starting program: /home/pedro/gdb/reverse-20080930-branch/build32/gdb/testsuite/gdb.base/decr-pc-rev
>
> Temporary breakpoint 1, main (argc=<value optimized out>, argv=<value optimized out>)
>    at ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c:23
> 23        asm ("nop"); /* 1st insn */
> (gdb) record
>      ^^^^^^
> (gdb) c
> Continuing.
>
> Breakpoint 2, main (argc=<value optimized out>, argv=<value optimized out>)
>    at ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c:32
> 32      in ../../../src/gdb/testsuite/gdb.base/decr-pc-rev.c
> (gdb) p $pc
> $1 = (void (*)()) 0x8048392 <main+30>
> (gdb)
>
> Breakpoint 3 should've been hit, not 2.  The PC points at
> 0x8048392, but it should point at 0x8048393.
>
> Feels like we're going in circles.  :-)
>
> --
> Pedro Alves
>