Re: [RFA] Fix off-by-one error in record.c (record_list_release_first)

[Hui Zhu <teawater@gmail.com>]

Thanks Michael and Jilin,

I think the Michael's patch is more spend up.  Jilin's patch is more
clear and can handle set_record_insn_max_num.

I suggest we choice speed up.  Do you think it's OK?

If we choice it, Michael, could you please add some comments to
"record_list_release_first" or change it's name to
"record_list_release_first_without_update_xxx" to make it clear.
And please update set_record_insn_max_num.

Thanks,
Hui

On Tue, Oct 13, 2009 at 10:17, Jiang Jilin <freephp@gmail.com> wrote:
> On Tue, Oct 13, 2009 at 12:35 AM, Michael Snyder <msnyder@vmware.com> wrote:
>> Hui,
>>
>> My "info record" patch helped me to find a bug.
>> I found that, once we start calling record_list_release_first,
>> we start adding two instructions to the log for every one
>> instruction we remove.  Therefore the log continues to grow,
>> even though it is supposed to remain constant in size.
>>
>> This is because record_insn_num is not incremented if we
>> call record_list_release_first -- but record_list_release_first
>> does decrement it.
>
> Hi Michael,
>
> I've noticed this bug(maybe) earlier, but I prefer fixing the caller
> functions, not the callee function record_list_release_first. And
> your patch doesn't handle set_record_insn_max_num.
>
> Thanks!
>
> 2009-10-13  Michael Snyder  <msnyder@vmware.com>
>        Jiang Jilin  <freephp@gmail.com>
>
>        * record.c (record_message): Increase record_insn_num after
>        record_list_release_first.
>        (record_registers_change): Ditto.
>        (record_xfer_partial): Ditto.
>
> diff --git a/gdb/record.c b/gdb/record.c
> index 8b56010..0208dac 100644
> --- a/gdb/record.c
> +++ b/gdb/record.c
> @@ -438,8 +438,8 @@ record_message (void *args)
>
>   if (record_insn_num == record_insn_max_num && record_insn_max_num)
>     record_list_release_first ();
> -  else
> -    record_insn_num++;
> +
> +  record_insn_num++;
>
>   return 1;
>  }
> @@ -1008,8 +1008,8 @@ record_registers_change (struct regcache
> *regcache, int regnum)
>
>   if (record_insn_num == record_insn_max_num && record_insn_max_num)
>     record_list_release_first ();
> -  else
> -    record_insn_num++;
> +
> +  record_insn_num++;
>  }
>
>  static void
> @@ -1121,8 +1121,8 @@ record_xfer_partial (struct target_ops *ops,
> enum target_object object,
>
>       if (record_insn_num == record_insn_max_num && record_insn_max_num)
>        record_list_release_first ();
> -      else
> -       record_insn_num++;
> +
> +      record_insn_num++;
>     }
>
>   return record_beneath_to_xfer_partial (record_beneath_to_xfer_partial_ops,
>