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

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

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 492 bytes --]

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.

A one line fix corrects this problem (attached).

Michael


[-- Attachment #2: insn_num.txt --]
[-- Type: text/plain, Size: 570 bytes --]

2009-10-12  Michael Snyder  <msnyder@vmware.com>

	* record.c (record_list_release_first): Do not decrement
	record_insn_num.

Index: record.c
===================================================================
RCS file: /cvs/src/src/gdb/record.c,v
retrieving revision 1.20
diff -u -p -r1.20 record.c
--- record.c	27 Sep 2009 02:49:34 -0000	1.20
+++ record.c	12 Oct 2009 16:33:44 -0000
@@ -209,8 +211,6 @@ record_list_release_first (void)
       if (type == record_end)
 	break;
     }
-
-  record_insn_num--;
 }
 
 /* Add a struct record_entry to record_arch_list.  */

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

[Jiang Jilin]

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

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,

[-- Attachment #2: 0001-Fix-off-by-one-error-in-record.c-record_list_releas.patch.txt --]
[-- Type: text/plain, Size: 955 bytes --]

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,

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

[Hui Zhu]

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

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

[Jiang Jilin]

On Tue, Oct 13, 2009 at 2:46 PM, Hui Zhu <teawater@gmail.com> wrote:
> 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?

Agree with you. :)

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


-- 
Jiang

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

[Michael Snyder]

[-- Attachment #1: Type: text/plain, Size: 580 bytes --]

Hui Zhu wrote:
> 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.

Hah, sorry, I completely missed the implications for 
set_record_insn_max_num.  Thanks Jilin for catching it.

New diff:


[-- Attachment #2: insn_num2.txt --]
[-- Type: text/plain, Size: 1517 bytes --]

2009-10-12  Michael Snyder  <msnyder@vmware.com>

	* record.c (record_list_release_first): Do not decrement
	record_insn_num.
	(set_insn_num_max): Remove printf.
	Decrement record_insn_num in the loop.

Index: record.c
===================================================================
RCS file: /cvs/src/src/gdb/record.c,v
retrieving revision 1.20
diff -u -p -r1.20 record.c
--- record.c	27 Sep 2009 02:49:34 -0000	1.20
+++ record.c	13 Oct 2009 18:04:44 -0000
@@ -177,6 +177,11 @@ record_list_release_next (void)
     }
 }
 
+/* Delete the first instruction from the beginning of the log, to make
+   room for adding a new instruction at the end of the log.
+
+   Note -- this function does not modify record_insn_num.  */
+
 static void
 record_list_release_first (void)
 {
@@ -209,8 +214,6 @@ record_list_release_first (void)
       if (type == record_end)
 	break;
     }
-
-  record_insn_num--;
 }
 
 /* Add a struct record_entry to record_arch_list.  */
@@ -1260,12 +1263,12 @@ set_record_insn_max_num (char *args, int
 {
   if (record_insn_num > record_insn_max_num && record_insn_max_num)
     {
-      printf_unfiltered (_("Record instructions number is bigger than "
-		           "record instructions max number.  Auto delete "
-		           "the first ones?\n"));
-
+      /* Count down record_insn_num while releasing records from list.  */
       while (record_insn_num > record_insn_max_num)
-	record_list_release_first ();
+	{
+	  record_list_release_first ();
+	  record_insn_num--;
+	}
     }
 }
 

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

[Hui Zhu]

Thanks Michael, I think this patch is OK with me.

Hui

On Wed, Oct 14, 2009 at 02:01, Michael Snyder <msnyder@vmware.com> wrote:
> Hui Zhu wrote:
>>
>> 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.
>
> Hah, sorry, I completely missed the implications for
> set_record_insn_max_num.  Thanks Jilin for catching it.
>
> New diff:
>
>
> 2009-10-12  Michael Snyder  <msnyder@vmware.com>
>
>        * record.c (record_list_release_first): Do not decrement
>        record_insn_num.
>        (set_insn_num_max): Remove printf.
>        Decrement record_insn_num in the loop.
>
> Index: record.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/record.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 record.c
> --- record.c    27 Sep 2009 02:49:34 -0000      1.20
> +++ record.c    13 Oct 2009 18:04:44 -0000
> @@ -177,6 +177,11 @@ record_list_release_next (void)
>     }
>  }
>
> +/* Delete the first instruction from the beginning of the log, to make
> +   room for adding a new instruction at the end of the log.
> +
> +   Note -- this function does not modify record_insn_num.  */
> +
>  static void
>  record_list_release_first (void)
>  {
> @@ -209,8 +214,6 @@ record_list_release_first (void)
>       if (type == record_end)
>        break;
>     }
> -
> -  record_insn_num--;
>  }
>
>  /* Add a struct record_entry to record_arch_list.  */
> @@ -1260,12 +1263,12 @@ set_record_insn_max_num (char *args, int
>  {
>   if (record_insn_num > record_insn_max_num && record_insn_max_num)
>     {
> -      printf_unfiltered (_("Record instructions number is bigger than "
> -                          "record instructions max number.  Auto delete "
> -                          "the first ones?\n"));
> -
> +      /* Count down record_insn_num while releasing records from list.  */
>       while (record_insn_num > record_insn_max_num)
> -       record_list_release_first ();
> +       {
> +         record_list_release_first ();
> +         record_insn_num--;
> +       }
>     }
>  }
>
>
>

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

[Michael Snyder]

OK, going to commit it.

Hui Zhu wrote:
> Thanks Michael, I think this patch is OK with me.
> 
> Hui
> 
> On Wed, Oct 14, 2009 at 02:01, Michael Snyder <msnyder@vmware.com> wrote:
>> Hui Zhu wrote:
>>> 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.
>> Hah, sorry, I completely missed the implications for
>> set_record_insn_max_num.  Thanks Jilin for catching it.
>>
>> New diff:
>>
>>
>> 2009-10-12  Michael Snyder  <msnyder@vmware.com>
>>
>>        * record.c (record_list_release_first): Do not decrement
>>        record_insn_num.
>>        (set_insn_num_max): Remove printf.
>>        Decrement record_insn_num in the loop.
>>
>> Index: record.c
>> ===================================================================
>> RCS file: /cvs/src/src/gdb/record.c,v
>> retrieving revision 1.20
>> diff -u -p -r1.20 record.c
>> --- record.c    27 Sep 2009 02:49:34 -0000      1.20
>> +++ record.c    13 Oct 2009 18:04:44 -0000
>> @@ -177,6 +177,11 @@ record_list_release_next (void)
>>     }
>>  }
>>
>> +/* Delete the first instruction from the beginning of the log, to make
>> +   room for adding a new instruction at the end of the log.
>> +
>> +   Note -- this function does not modify record_insn_num.  */
>> +
>>  static void
>>  record_list_release_first (void)
>>  {
>> @@ -209,8 +214,6 @@ record_list_release_first (void)
>>       if (type == record_end)
>>        break;
>>     }
>> -
>> -  record_insn_num--;
>>  }
>>
>>  /* Add a struct record_entry to record_arch_list.  */
>> @@ -1260,12 +1263,12 @@ set_record_insn_max_num (char *args, int
>>  {
>>   if (record_insn_num > record_insn_max_num && record_insn_max_num)
>>     {
>> -      printf_unfiltered (_("Record instructions number is bigger than "
>> -                          "record instructions max number.  Auto delete "
>> -                          "the first ones?\n"));
>> -
>> +      /* Count down record_insn_num while releasing records from list.  */
>>       while (record_insn_num > record_insn_max_num)
>> -       record_list_release_first ();
>> +       {
>> +         record_list_release_first ();
>> +         record_insn_num--;
>> +       }
>>     }
>>  }
>>
>>
>>

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

[Michael Snyder]

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,