[Patch] Watchpoint condition fix

[Patch] Watchpoint condition fix

[Marc Khouzam]

Repost to gdb-patches from gdb mailing list post:
http://sourceware.org/ml/gdb/2008-04/msg00078.html

Watchpoint conditions are broken because they
get deleted when a watchpoint gets re-inserted.
When watchpoints get re-inserted, the condition 
string should be always reparsed.

Ok?

2008-04-10  Marc Khouzam  <marc.khouzam@ericsson.com>

	* breakpoint.c (update_watchpoint): Always reparse
	condition.

Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.307
diff -u -r1.307 breakpoint.c
--- gdb/breakpoint.c    14 Mar 2008 18:57:43 -0000      1.307
+++ gdb/breakpoint.c    10 Apr 2008 12:25:32 -0000
@@ -986,7 +986,7 @@
            value_free (v);
        }
 
-      if (reparse && b->cond_string != NULL)
+      if (b->cond_string != NULL)
        {
          char *s = b->cond_string;
          if (b->loc->cond)

Re: [Patch] Watchpoint condition fix

[Joel Brobecker]

> Watchpoint conditions are broken because they get deleted when a
> watchpoint gets re-inserted.  When watchpoints get re-inserted, the
> condition string should be always reparsed.

I agree with you and Vladimir.

> @@ -986,7 +986,7 @@
>             value_free (v);
>         }
>  
> -      if (reparse && b->cond_string != NULL)
> +      if (b->cond_string != NULL)

This part of the change is OK, but I would like to see a comment
explaining why we have to re-evaluate the condition regardless of
the value of reparse.  Vladimir's explaination on the gdb@ mailing
list pretty much covers it.

>         {
>           char *s = b->cond_string;
>           if (b->loc->cond)

Also, you can remove the block of code that checks b->loc->cond
and frees when non-NULL, since we've just re-created the bplocs
earlier and we know that their cond field is NULL. As a result,
the temporary variable "s" shouldn't be necessary either.

As an aside, but this is not your doing, I think we have a memory leak.
We appear to be doing an xfree of each bp_location of our watchpoint,
but instead we should call free_bp_location (). Vladimir, do you agree?
(I will take care of doing that if you agree)

Thank you,
-- 
Joel

RE: [Patch] Watchpoint condition fix

[Marc Khouzam]

> This part of the change is OK, but I would like to see a comment

Done

> Also, you can remove the block of code that checks b->loc->cond
> and frees when non-NULL, since we've just re-created the bplocs
> earlier and we know that their cond field is NULL. 

Done

> As a result,
> the temporary variable "s" shouldn't be necessary either.

I'm not sure about this one.  s is pointing to b->cond_string
and not b->loc->cond.  Is it really unecessary?

Here is the updated patch (having kept the temp var s)

Marc


2008-04-17  Marc Khouzam  <marc.khouzam@ericsson.com>

       * breakpoint.c (update_watchpoint): Always reparse
       condition.


Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.308
diff -u -r1.308 breakpoint.c
--- gdb/breakpoint.c    15 Apr 2008 14:32:12 -0000      1.308
+++ gdb/breakpoint.c    17 Apr 2008 17:40:13 -0000
@@ -994,14 +994,14 @@
            value_free (v);
        }
 
-      if (reparse && b->cond_string != NULL)
+      /* We just regenerated the list of breakpoint locations.
+       * The new location does not have its condition field set to anything
+       * and therefore, we must always reparse the cond_string, independently
+       * of the value of the reparse flag.
+       */
+      if (b->cond_string != NULL)
        {
          char *s = b->cond_string;
-         if (b->loc->cond)
-           {
-             xfree (b->loc->cond);
-             b->loc->cond = NULL;
-           }
          b->loc->cond = parse_exp_1 (&s, b->exp_valid_block, 0);
        }
     }

Re: [Patch] Watchpoint condition fix

[Joel Brobecker]

> > As a result, the temporary variable "s" shouldn't be necessary
> > either.
> 
> I'm not sure about this one.  s is pointing to b->cond_string
> and not b->loc->cond.  Is it really unecessary?

You are right to keep it, but not for the reason you explained.
I forgot that parse_exp_1 advances the string pointer that it is given.
So if we didn't keep the temporary variable, we would screw b->cond_string
(oops!).

> 2008-04-17  Marc Khouzam  <marc.khouzam@ericsson.com>
> 
>        * breakpoint.c (update_watchpoint): Always reparse
>        condition.

This is OK after having fixed a tiny little detail:

> -      if (reparse && b->cond_string != NULL)
> +      /* We just regenerated the list of breakpoint locations.
> +       * The new location does not have its condition field set to anything
> +       * and therefore, we must always reparse the cond_string, independently
> +       * of the value of the reparse flag.

Reformat the comment to avoid the '*' at the beginning of each line
and put the final '*/' at the end of the last line, not on a new line:

      /* We just regenerated the list of breakpoint locations.
         The new location does not have its condition field set to anything
         and therefore, we must always reparse the cond_string, independently
         of the value of the reparse flag.  */

Thank you,
-- 
Joel

RE: [Patch] Watchpoint condition fix

[Marc Khouzam]

> Reformat the comment to avoid the '*' at the beginning of each line
> and put the final '*/' at the end of the last line, not on a new line:

Forgot about that.  Eclipse does the comments in its own way.
I fixed it.

I committed the following:

2008-04-17  Marc Khouzam  <marc.khouzam@ericsson.com>

       * breakpoint.c (update_watchpoint): Always reparse
       condition.


Index: gdb/breakpoint.c
===================================================================
RCS file: /cvs/src/src/gdb/breakpoint.c,v
retrieving revision 1.309
diff -u -r1.309 breakpoint.c
--- gdb/breakpoint.c    17 Apr 2008 22:43:17 -0000      1.309
+++ gdb/breakpoint.c    18 Apr 2008 00:35:22 -0000
@@ -994,14 +994,13 @@
            value_free (v);
        }
 
-      if (reparse && b->cond_string != NULL)
+      /* We just regenerated the list of breakpoint locations.
+         The new location does not have its condition field set to anything
+         and therefore, we must always reparse the cond_string, independently
+         of the value of the reparse flag.  */
+      if (b->cond_string != NULL)
        {
          char *s = b->cond_string;
-         if (b->loc->cond)
-           {
-             xfree (b->loc->cond);
-             b->loc->cond = NULL;
-           }
          b->loc->cond = parse_exp_1 (&s, b->exp_valid_block, 0);
        }
     }

Re: [Patch] Watchpoint condition fix

[Doug Evans]

On Thu, Apr 17, 2008 at 5:41 PM, Marc Khouzam <marc.khouzam@ericsson.com> wrote:
>
>  > Reformat the comment to avoid the '*' at the beginning of each line
>  > and put the final '*/' at the end of the last line, not on a new line:
>
>  Forgot about that.  Eclipse does the comments in its own way.
>  I fixed it.
>
>  I committed the following:
>
>  2008-04-17  Marc Khouzam  <marc.khouzam@ericsson.com>
>
>        * breakpoint.c (update_watchpoint): Always reparse
>        condition.

Is this something that should go in the 6.8 branch?

Re: [Patch] Watchpoint condition fix

[Joel Brobecker]

> >  2008-04-17  Marc Khouzam  <marc.khouzam@ericsson.com>
> >
> >        * breakpoint.c (update_watchpoint): Always reparse
> >        condition.
> 
> Is this something that should go in the 6.8 branch?

Looks like a good candidate to me. I'll put it in next week
unless someone objects.

-- 
Joel

Re: [Patch] Watchpoint condition fix

[Joel Brobecker]

> 2008-04-17  Marc Khouzam  <marc.khouzam@ericsson.com>
> 
>        * breakpoint.c (update_watchpoint): Always reparse
>        condition.

As suggested, I pushed this to the branch as well.

-- 
Joel