Re: [reverse RFC] Fix some bugs

[teawater <teawater@gmail.com>]

On Fri, Sep 19, 2008 at 02:40, Michael Snyder <msnyder@vmware.com> wrote:
> teawater wrote:
>>
>> Thanks for the test and bug report of Michael. It help me a lot.
>>
>> 2008-09-19  Hui Zhu  <teawater@gmail.com>
>>
>>        * record.c (record_wait): Fix the bug of forware step after
>>        reverse step.
>>        Make debug message more clean.
>>        Remove variable record_list_status.
>>
>>        * i386-tdep.c (i386_record): Fix the bug of "0x80 ... 0x83".
>
> Hui, are all these changes related?
> It looks a little like you might have combined several
> smaller changes into one patch.  In order to make review
> easier, we like to have unrelated patches submitted separately.
>
> If they are in fact related, could you explain please?
> Maybe add a few more comments?

Sorry for it. I will add some comments in this function.

Thanks,
Hui

>
> Thanks a lot!
>
>
>> ------------------------------------------------------------------------
>>
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -1,3 +1,12 @@
>> +2008-09-19  Hui Zhu  <teawater@gmail.com>
>> +
>> +       * record.c (record_wait): Fix the bug of forware step after
>> +       reverse step.
>> +       Make debug message more clean.
>> +       Remove variable record_list_status.
>> +
>> +       * i386-tdep.c (i386_record): Fix the bug of "0x80 ... 0x83".
>> +
>>  2008-09-15  Michael Snyder  <msnyder@vmware.com>
>>          * infrun.c (proceed): No need to singlestep over a breakpoint
>> --- a/i386-tdep.c
>> +++ b/i386-tdep.c
>> @@ -3037,11 +3037,7 @@ reswitch:
>>            }
>>          else
>>            {
>> -             if (ot == OT_BYTE)
>> -               {
>> -                 reg &= 0x3;
>> -               }
>> -             if (record_arch_list_add_reg (reg))
>> +             if (record_arch_list_add_reg (rm))
>>                {
>>                  return (-1);
>>                }
>> --- a/record.c
>> +++ b/record.c
>> @@ -33,7 +33,6 @@ int record_debug = 0;
>>   record_t record_first;
>>  record_t *record_list = &record_first;
>> -int record_list_status = 1;    /* 0 normal 1 to the begin 2 to the end */
>>  record_t *record_arch_list_head = NULL;
>>  record_t *record_arch_list_tail = NULL;
>>  struct regcache *record_regcache = NULL;
>> @@ -446,7 +445,6 @@ record_open (char *name, int from_tty)
>>     /* Reset */
>>   record_insn_num = 0;
>> -  record_list_status = 1;
>>   record_execdir = EXEC_FORWARD;
>>   record_list = &record_first;
>>   record_list->next = NULL;
>> @@ -546,30 +544,31 @@ record_wait (ptid_t ptid, struct target_
>>          Then set it to terminal_ours to make GDB get the signal.  */
>>       target_terminal_ours ();
>>  -      /* Loop over the record log, looking for the next place to stop.
>>  */
>> +      /* In EXEC_FORWARD mode,, record_list point to the tail of prev
>> +         instruction. */
>> +      if (record_execdir == EXEC_FORWARD && record_list->next)
>> +        {
>> +         record_list = record_list->next;
>> +       }
>> +
>> +      /* Loop over the record_list, looking for the next place to stop.
>>  */
>>       do
>>        {
>>          /* check state */
>> -         if ((record_execdir == EXEC_REVERSE && !record_list->prev
>> -              && record_list_status == 1) -             ||
>> (record_execdir != EXEC_REVERSE
>> -                 && !record_list->next
>> -                 && record_list_status == 2))
>> +         if (record_execdir == EXEC_REVERSE && record_list ==
>> &record_first)
>>            {
>> -             if (record_list_status == 2)
>> -               {
>> -                 fprintf_unfiltered (gdb_stdlog,
>> -                                     "Record: running to the end of
>> record list.\n");
>> -               }
>> -             else if (record_list_status == 1)
>> -               {
>> -                 fprintf_unfiltered (gdb_stdlog,
>> -                                     "Record: running to the begin of
>> record list.\n");
>> -               }
>> +             fprintf_unfiltered (gdb_stdlog,
>> +                                 "Record: running to the begin of record
>> list.\n");
>> +             stop_soon = STOP_QUIETLY;
>> +             break;
>> +           }
>> +         if (record_execdir != EXEC_REVERSE && !record_list->next)
>> +           {
>> +             fprintf_unfiltered (gdb_stdlog,
>> +                                 "Record: running to the end of record
>> list.\n");
>>              stop_soon = STOP_QUIETLY;
>>              break;
>>            }
>> -         record_list_status = 0;
>>            /* set ptid, register and memory according to record_list */
>>          if (record_list->type == record_reg)
>> @@ -579,7 +578,8 @@ record_wait (ptid_t ptid, struct target_
>>              if (record_debug > 1)
>>                {
>>                  fprintf_unfiltered (gdb_stdlog,
>> -                                     "Record: record_reg to inferior num
>> = %d.\n",
>> +                                     "Record: record_reg 0x%s to inferior
>> num = %d.\n",
>> +                                     paddr_nz ((CORE_ADDR)record_list),
>>                                      record_list->u.reg.num);
>>                }
>>              regcache_cooked_read (regcache, record_list->u.reg.num, reg);
>> @@ -594,7 +594,8 @@ record_wait (ptid_t ptid, struct target_
>>              if (record_debug > 1)
>>                {
>>                  fprintf_unfiltered (gdb_stdlog,
>> -                                     "Record: record_mem to inferior addr
>> = 0x%s len = %d.\n",
>> +                                     "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);
>>                }
>> @@ -625,19 +626,13 @@ record_wait (ptid_t ptid, struct target_
>>              if (record_debug > 1)
>>                {
>>                  fprintf_unfiltered (gdb_stdlog,
>> -                                     "Record: record_end to inferior
>> need_dasm = %d.\n",
>> +                                     "Record: record_end 0x%s to inferior
>> need_dasm = %d.\n",
>> +                                     paddr_nz ((CORE_ADDR)record_list),
>>                                      record_list->u.need_dasm);
>>                }
>>                if (record_execdir == EXEC_FORWARD)
>>                {
>> -                 if (record_list == &record_first)
>> -                   {
>> -                     /* The first record_t, not a really record_t.
>> -                        Goto next record_t. */
>> -                     goto next;
>> -                   }
>> -
>>                  need_dasm = record_list->u.need_dasm;
>>                }
>>              if (need_dasm)
>> @@ -706,21 +701,19 @@ record_wait (ptid_t ptid, struct target_
>>            }
>>   next:
>> -         if (record_execdir == EXEC_REVERSE)
>> -           {
>> -             if (record_list->prev && continue_flag)
>> -               record_list = record_list->prev;
>> -             else
>> -               record_list_status = 1;
>> -           }
>> -         else
>> +         if (continue_flag)
>>            {
>> -             if (record_list->next)
>> -               record_list = record_list->next;
>> +             if (record_execdir == EXEC_REVERSE)
>> +               {
>> +                 if (record_list->prev)
>> +                   record_list = record_list->prev;
>> +               }
>>              else
>> -               record_list_status = 2;
>> +               {
>> +                 if (record_list->next)
>> +                   record_list = record_list->next;
>> +               }
>>            }
>> -
>>        }
>>       while (continue_flag);
>>
>
>