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

Michael Snyder
2009-10-13T17:25:33Z

Jiang Jilin 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.

I understand your first point (about caller vs. callee), but
I don't understand what you mean about "handling"
set_record_insn_max_num. In what way does it need handling?

There is still a user command to set and show that variable:

add_setshow_zinteger_cmd ("insn-number-max", no_class,
&record_insn_max_num,
_("Set record/replay buffer limit."),
_("Show record/replay buffer limit."), _("\
Set the maximum number of instructions to be stored in the\n\
record/replay buffer. Zero means unlimited. Default is 200000."),

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