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

[teawater <teawater@gmail.com>]

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