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

[Michael Snyder <msnyder@vmware.com>]

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);