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

[Hui Zhu <teawater at gmail dot 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,
>