[RFA] Don't reset watchpoint block on solib load.

[RFA] Don't reset watchpoint block on solib load.

[Vladimir Prus]

There's code inside breakpoint_re_set_one to refresh watchpoints, 
which seems suspicious to me. 

First problem with that code is that it always resets watchpoint's
block to NULL. So, if we have a local watchpoint, and you do 
dlopen (without exiting the scope where watchpoint is valid), 
then the watchpoint's block will be reset to NULL, and 
watchpoint's expression will be reparsed in global block -- 
which will surely break the watchpoint. 

Second problem is that this code reevalautes the expression,
and given that insert_breakpoints does that too, we can just
reset breakpoints value to NULL, and have insert_breakpoints to the work. 

Finally, this code reevaluates condition. While this is probably 
correct way to handle case where meaning of condition changes due to 
loading of shared library, there's no code to match for the 
case when a shared library is unloaded. I think a more robust 
approach if to reevaluate condition inside insert_bp_location.

This patch is prompted by the following problem:

    void some_function() {

	g = 10;
	....
	dlopen("whatever", ...);
	....
	g = 15;
    }

If you set watchpoint on 'g', and continue over dlopen, the watchpoint is never hit.
The exact mode of failure differs. I actually have a testcase for this, and it
passes for me locally, and I would have liked to provide it, but there are two
issues for which I don't have yet a complete solution:

    - if we have no debug information for ld.so, then when we stop in 
    ld.so, we cannot find the frame associated with watchpoint, and delete
    watchpoint.
    - if we have debug information for ld.so, then when we stop in
     ld.so, gdb tries to reevaluate 'g'. Unfortunately, it does that in
    wrong block, does not find 'g', and dies with internal error.

I have a hack for the second problem, that allows the test to work, but causes
regressions elsewhere. And I don't have yet an approach for the first problem,
so complete solution will take more time. For the time being, I think the
below patch makes sense independently as logic cleanup. OK?

- Volodya

	* breakpoint.c (insert_bp_location): For watchpoints,
	recompute condition.
	(breakpoint_re_set_one): Instead of recomputing value
	and condition for watchpoints, just reset value and
	let insert_breakpoints/insert_bp_location recompute it.
---
 gdb/breakpoint.c |   81 +++++++++++++++++++++++------------------------------
 1 files changed, 35 insertions(+), 46 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2f4bd8b..32f4dd1 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1111,6 +1111,18 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 		    }
 		}
 	    }
+
+	  if (bpt->owner->cond_string != NULL)
+	    {
+	      char *s = bpt->owner->cond_string;
+	      if (bpt->cond)
+		{
+		  xfree (bpt->cond);
+		  bpt->cond = NULL;
+		}
+	      bpt->cond = parse_exp_1 (&s, bpt->owner->exp_valid_block, 0);
+	    }
+	      
 	  /* Failure to insert a watchpoint on any memory value in the
 	     value chain brings us here.  */
 	  if (!bpt->inserted)
@@ -7534,53 +7546,30 @@ breakpoint_re_set_one (void *bint)
     case bp_hardware_watchpoint:
     case bp_read_watchpoint:
     case bp_access_watchpoint:
-      innermost_block = NULL;
-      /* The issue arises of what context to evaluate this in.  The
-         same one as when it was set, but what does that mean when
-         symbols have been re-read?  We could save the filename and
-         functionname, but if the context is more local than that, the
-         best we could do would be something like how many levels deep
-         and which index at that particular level, but that's going to
-         be less stable than filenames or function names.  */
-
-      /* So for now, just use a global context.  */
+       /* Loading of new shared library can lead to symbols
+        used by watchpoint expression to become available.
+        Reparse expression.  */
       if (b->exp)
-	{
-	  xfree (b->exp);
-	  /* Avoid re-freeing b->exp if an error during the call to
-             parse_expression.  */
-	  b->exp = NULL;
-	}
-      b->exp = parse_expression (b->exp_string);
-      b->exp_valid_block = innermost_block;
-      mark = value_mark ();
-      if (b->val)
-	{
-	  value_free (b->val);
-	  /* Avoid re-freeing b->val if an error during the call to
-             evaluate_expression.  */
-	  b->val = NULL;
-	}
-      b->val = evaluate_expression (b->exp);
-      release_value (b->val);
-      if (value_lazy (b->val) && breakpoint_enabled (b))
-	value_fetch_lazy (b->val);
-
-      if (b->cond_string != NULL)
-	{
-	  s = b->cond_string;
-	  if (b->loc->cond)
-	    {
-	      xfree (b->loc->cond);
-	      /* Avoid re-freeing b->exp if an error during the call
-		 to parse_exp_1.  */
-	      b->loc->cond = NULL;
-	    }
-	  b->loc->cond = parse_exp_1 (&s, (struct block *) 0, 0);
-	}
-      if (breakpoint_enabled (b))
-	mention (b);
-      value_free_to_mark (mark);
+       {
+         xfree (b->exp);
+         b->exp = NULL;
+       }
+      s = b->exp_string;
+      b->exp = parse_exp_1 (&s, b->exp_valid_block, 0);
+
+      /* Since we reparsed expression, we need to update the
+        value.  In fact, I'm not aware of any way how
+        reparsing an expression after loading of a shared library
+        can change a valid expression into another valid expression
+        but with different meaning, but better be safe.  We set
+        value to NULL, and insert_breakpoints will update the value.  */
+       if (b->val)
+        value_free (b->val);
+       b->val = NULL;
+
+      /* Loading of new shared library change the meaning of
+        watchpoint condition.  However, insert_bp_location will
+        recompute watchpoint condition anyway, nothing to do here.  */
       break;
     case bp_catch_catch:
     case bp_catch_throw:
-- 
1.5.3.5

Re: [RFA] Don't reset watchpoint block on solib load.

[Jim Blandy]

Vladimir Prus <vladimir at codesourcery.com> writes:
> There's code inside breakpoint_re_set_one to refresh watchpoints, 
> which seems suspicious to me. 
>
> First problem with that code is that it always resets watchpoint's
> block to NULL. So, if we have a local watchpoint, and you do 
> dlopen (without exiting the scope where watchpoint is valid), 
> then the watchpoint's block will be reset to NULL, and 
> watchpoint's expression will be reparsed in global block -- 
> which will surely break the watchpoint. 

Is that right?  We set innermost_block to NULL, but then we call
parse_expression, which should set innermost_block to the innermost
block containing a symbol actually used by the expression.

We also call breakpoint_re_set_one when we've unloaded a shared
library.  At that point, b->exp_valid_block could be a dangling
pointer; we can't use it to re-parse the expression.

I think the bug is that we re-parse the expression with
parse_expression, which leaves the scope unspecified, defaulting to
the currently selected frame.  We should:

1) Verify that the frame given by b->watchpoint_frame is still valid,
   and delete the watchpoint if it isn't.
   
2) Call get_frame_block (b->watchpoint_frame) to see if we have a
   block for the frame's location, and deleting the watchpoint if we
   don't (saying we don't have the symbolic info available to update
   it), and

3) Call parse_exp_1 (..., watchpoint frame's block, ...) to reparse
   the watchpoint's expression in the proper block.

> Second problem is that this code reevalautes the expression,
> and given that insert_breakpoints does that too, we can just
> reset breakpoints value to NULL, and have insert_breakpoints to the
> work.

I think it's an invariant that b->val may be NULL only when we have
just started the inferior, and know that insert_breakpoints will be
called.  In other contexts, we don't always call insert_breakpoints
before letting the program run.  Wouldn't leaving the value NULL cause
a problem in that case?

> Finally, this code reevaluates condition.

Re-parses, you mean?

> While this is probably 
> correct way to handle case where meaning of condition changes due to 
> loading of shared library, there's no code to match for the 
> case when a shared library is unloaded. I think a more robust 
> approach if to reevaluate condition inside insert_bp_location.

I agree.

> This patch is prompted by the following problem:
>
>     void some_function() {
>
> 	g = 10;
> 	....
> 	dlopen("whatever", ...);
> 	....
> 	g = 15;
>     }
>
> If you set watchpoint on 'g', and continue over dlopen, the watchpoint is never hit.
> The exact mode of failure differs. I actually have a testcase for this, and it
> passes for me locally, and I would have liked to provide it, but there are two
> issues for which I don't have yet a complete solution:
>
>     - if we have no debug information for ld.so, then when we stop in 
>     ld.so, we cannot find the frame associated with watchpoint, and delete
>     watchpoint.

Does this case arise in normal usage?  I'm not saying it doesn't; I'm
just not sure how to work around it either, so I'm wondering how
serious a problem it is.

>     - if we have debug information for ld.so, then when we stop in
>      ld.so, gdb tries to reevaluate 'g'. Unfortunately, it does that in
>     wrong block, does not find 'g', and dies with internal error.

My suggestion above should avoid this.

Re: [RFA] Don't reset watchpoint block on solib load.

[Vladimir Prus]

On Wednesday 28 November 2007 02:00:15 Jim Blandy wrote:
> 
> Vladimir Prus <vladimir at codesourcery.com> writes:
> > There's code inside breakpoint_re_set_one to refresh watchpoints, 
> > which seems suspicious to me. 
> >
> > First problem with that code is that it always resets watchpoint's
> > block to NULL. So, if we have a local watchpoint, and you do 
> > dlopen (without exiting the scope where watchpoint is valid), 
> > then the watchpoint's block will be reset to NULL, and 
> > watchpoint's expression will be reparsed in global block -- 
> > which will surely break the watchpoint. 
> 
> Is that right?  We set innermost_block to NULL, but then we call
> parse_expression, which should set innermost_block to the innermost
> block containing a symbol actually used by the expression.

Except that parse_expression is called while we're inside shared lib
machinery code -- so neither original block nor original frame is
used, and parse_expression is likely to fail.

> We also call breakpoint_re_set_one when we've unloaded a shared
> library.  At that point, b->exp_valid_block could be a dangling
> pointer; we can't use it to re-parse the expression.
> 
> I think the bug is that we re-parse the expression with
> parse_expression, which leaves the scope unspecified, defaulting to
> the currently selected frame.  We should:
> 
> 1) Verify that the frame given by b->watchpoint_frame is still valid,
>    and delete the watchpoint if it isn't.
>    
> 2) Call get_frame_block (b->watchpoint_frame) to see if we have a
>    block for the frame's location, and deleting the watchpoint if we
>    don't (saying we don't have the symbolic info available to update
>    it), and
> 
> 3) Call parse_exp_1 (..., watchpoint frame's block, ...) to reparse
>    the watchpoint's expression in the proper block.

Let me try thinking out loud.

1. Suppose I have a watchpoint on a global variable. Such a watchpoint
can reasonably survive program restart, and given that it can be set on 
a global variable (including static variables of C++ classes) defined
in a shared library, it makes sense recompute such watchpoint
when a shared library is loaded, or unloaded. 

I'm not 100% sure it works now, since we use evaluate_expression in few places,
so if a watchpoint uses a variable defined in not-yet-loaded shared library,
evaluate_expression will throw. I believe Dan's recent patch will fix this,
however.

2. Suppose I have a watchpoint on a local variable. GDB does a fairly
good job of removing the breakpoint when we leave the scope -- either
using regular return, or longjmp or C++ exception -- and in any way
the first watchpoint_check call will remove a watchpoint if we've left
the scope. The only exception is disabled local watchpoint -- but
it can't be re-enabled until we're inside watchpoint frame again.

What this means is:

2.1 Assume we catch solib load/unload after exiting watchpoint scope. Then,
we should not do anything -- if watchpoint should be deleted, it will
be deleted by other code. And if it's disabled we can't reparse it,
but should not delete it either (at least if we want to preserve the existing
behaviour).

2.2 Assume we catch solib load/unload in a frame called from the frame where
watchpoint is defined. Technically speaking, it's possible that either watchpoint
expression, or watchpoint condition makes use of a global variable, and so
can change. Not that watchpoint expression and condition are evaluated 
when watchpoint is created, and should be valid for watchpoint to exist. So, 
the only way for a watchpoint to change meaning if when a shared library 
is unloaded, and brings some symbols used by watchpoint with it. Then, 
the library can be re-loaded, and make the watchpoint valid again. It seems 
to me that while loading or unloading of a shared library from within 
frame where watchpoint is defined seems valid use-case, the case where 
a given library is both unloaded, and the reloaded, and it has a variable
that watchpoint uses, is rather arcane. But we might as well still recompute
watchpoint expression, since it's easy.

The final conclusion is that we need to wrap the updating code in:

	if (b->exp_valid_block == NULL || 
	    find_frame_by_id (b->watchpoint_frame) != NULL)
	{
	}

We don't need to delete watchpoint, here, if frame is no longer valid
as other code will do that anyway. You've also suggested to use get_frame_block.
Can you offer a case when the watchpoint frame is still valid, but the block
is no longer valid?

> > Second problem is that this code reevalautes the expression,
> > and given that insert_breakpoints does that too, we can just
> > reset breakpoints value to NULL, and have insert_breakpoints to the
> > work.
> 
> I think it's an invariant that b->val may be NULL only when we have
> just started the inferior,  and know that insert_breakpoints will be 
> called.  In other contexts, we don't always call insert_breakpoints
> before letting the program run.  Wouldn't leaving the value NULL cause
> a problem in that case?

Presently, insert_breakpoints will be always called before resuming
target -- look at 'proceed' and 'keep_going'. Note that this is
independent of any bits of async support now present. 
Should we need to revise this, we'll revise this. 

> > Finally, this code reevaluates condition.
> 
> Re-parses, you mean?

Yes.
 
> > While this is probably 
> > correct way to handle case where meaning of condition changes due to 
> > loading of shared library, there's no code to match for the 
> > case when a shared library is unloaded. I think a more robust 
> > approach if to reevaluate condition inside insert_bp_location.
> 
> I agree.
> 
> > This patch is prompted by the following problem:
> >
> >     void some_function() {
> >
> > 	g = 10;
> > 	....
> > 	dlopen("whatever", ...);
> > 	....
> > 	g = 15;
> >     }
> >
> > If you set watchpoint on 'g', and continue over dlopen, the watchpoint is never hit.
> > The exact mode of failure differs. I actually have a testcase for this, and it
> > passes for me locally, and I would have liked to provide it, but there are two
> > issues for which I don't have yet a complete solution:
> >
> >     - if we have no debug information for ld.so, then when we stop in 
> >     ld.so, we cannot find the frame associated with watchpoint, and delete
> >     watchpoint.
> 
> Does this case arise in normal usage?  I'm not saying it doesn't; I'm
> just not sure how to work around it either, so I'm wondering how
> serious a problem it is.

This problem does not require any extraordinary setup. On my Kubuntu system,
ld.so does not have debug symbols by default. So, to reproduce the problem
you only need a local watchpoint in a function that calls, directly or indirectly,
dlopen. This is not everybody have daily, of course, but still possible to
have.

> >     - if we have debug information for ld.so, then when we stop in
> >      ld.so, gdb tries to reevaluate 'g'. Unfortunately, it does that in
> >     wrong block, does not find 'g', and dies with internal error.
> 
> My suggestion above should avoid this.

Oops! I failed to mention that 'g' is a  global variable. So, your suggestion
won't help -- as both block and frame id will be NULL for watchpoint on g. I don't
fully understand the problem -- it appears that the innermost_block is set to NULL
by c-exp.y to indicate that access to 'g' does not need any specific frame. However,
'g' is in static block for it's library, and is not visible everywhere. I've hacked this around like this:

diff --git a/gdb/c-exp.y b/gdb/c-exp.y
index 40dbc93..783ea77 100644
--- a/gdb/c-exp.y
+++ b/gdb/c-exp.y
@@ -697,7 +697,7 @@ variable:   name_not_typename
                              /* We want to use the selected frame, not
                                 another more inner frame which happens to
                                 be in the same block.  */
-                             write_exp_elt_block (NULL);
+                             write_exp_elt_block (block_found);
                              write_exp_elt_sym (sym);
                              write_exp_elt_opcode (OP_VAR_VALUE);
                            }

but this patch causes other problems.

I attach the revised patch with that extra if, does it look OK?

- Volodya

        * breakpoint.c (insert_bp_location): For watchpoints,
        recompute condition.
        (breakpoint_re_set_one): Instead of recomputing value
        and condition for watchpoints, just reset value and
        let insert_breakpoints/insert_bp_location recompute it.
        Don't do anything about disabled watchpoint.
---
 gdb/breakpoint.c |  107 +++++++++++++++++++++++++++++++++---------------------
 1 files changed, 65 insertions(+), 42 deletions(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index 2203f6e..6d06d33 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -1109,6 +1109,18 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
                    }
                }
            }
+
+         if (bpt->owner->cond_string != NULL)
+           {
+             char *s = bpt->owner->cond_string;
+             if (bpt->cond)
+               {
+                 xfree (bpt->cond);
+                 bpt->cond = NULL;
+               }
+             bpt->cond = parse_exp_1 (&s, bpt->owner->exp_valid_block, 0);
+           }
+             
          /* Failure to insert a watchpoint on any memory value in the
             value chain brings us here.  */
          if (!bpt->inserted)
@@ -7527,53 +7539,64 @@ breakpoint_re_set_one (void *bint)
     case bp_hardware_watchpoint:
     case bp_read_watchpoint:
     case bp_access_watchpoint:
-      innermost_block = NULL;
-      /* The issue arises of what context to evaluate this in.  The
-         same one as when it was set, but what does that mean when
-         symbols have been re-read?  We could save the filename and
-         functionname, but if the context is more local than that, the
-         best we could do would be something like how many levels deep
-         and which index at that particular level, but that's going to
-         be less stable than filenames or function names.  */
-
-      /* So for now, just use a global context.  */
-      if (b->exp)
-       {
-         xfree (b->exp);
-         /* Avoid re-freeing b->exp if an error during the call to
-             parse_expression.  */
-         b->exp = NULL;
-       }
-      b->exp = parse_expression (b->exp_string);
-      b->exp_valid_block = innermost_block;
-      mark = value_mark ();
-      if (b->val)
-       {
-         value_free (b->val);
-         /* Avoid re-freeing b->val if an error during the call to
-             evaluate_expression.  */
-         b->val = NULL;
-       }
-      b->val = evaluate_expression (b->exp);
-      release_value (b->val);
-      if (value_lazy (b->val) && breakpoint_enabled (b))
-       value_fetch_lazy (b->val);
+      /* Watchpoint can be either on expression using entirely global variables,
+        or it can be on local variables.
+
+        Watchpoints of the first kind are never auto-deleted, and even persist
+        across program restarts. Since they can use variables from shared 
+        libraries, we need to reparse expression as libraries are loaded
+        and unloaded.
+
+        Watchpoints on local variables can also change meaning as result
+        of solib event. For example, if a watchpoint uses both a local and
+        a global variables in expression, it's a local watchpoint, but
+        unloading of a shared library will make the expression invalid.
+        This is not a very common use case, but we still re-evaluate
+        expression, to avoid surprises to the user. 
+
+        Note that for local watchpoints, we re-evaluate it only if
+        watchpoints frame id is still valid.  If it's not, it means
+        the watchpoint is out of scope and will be deleted soon. In fact,
+        I'm not sure we'll ever be called in this case.  
+
+        If a local watchpoint's frame id is still valid, then
+        b->exp_valid_block is likewise valid, and we can safely use it.  
+        
+        Don't do anything about disabled watchpoints, since they will
+        be reevaluated again when enabled.  */
 
-      if (b->cond_string != NULL)
+      if (!breakpoint_enabled (b))
+       break;
+
+      if (b->exp_valid_block == NULL
+         || frame_find_by_id (b->watchpoint_frame) != NULL)
        {
-         s = b->cond_string;
-         if (b->loc->cond)
+         if (b->exp)
            {
-             xfree (b->loc->cond);
-             /* Avoid re-freeing b->exp if an error during the call
-                to parse_exp_1.  */
-             b->loc->cond = NULL;
+             xfree (b->exp);
+             b->exp = NULL;
            }
-         b->loc->cond = parse_exp_1 (&s, (struct block *) 0, 0);
+         s = b->exp_string;
+         b->exp = parse_exp_1 (&s, b->exp_valid_block, 0);
+
+         /* Since we reparsed expression, we need to update the
+            value.  I'm not aware of any way a single solib load or unload
+            can change a valid value into different valid value, but:
+            - even if the value is no longer valid, we have to record
+            this fact, so that when it becomes valid we reports this
+            as value change
+            - unloaded followed by load can change the value for sure.
+
+            We set value to NULL, and insert_breakpoints will 
+            update the value.  */
+         if (b->val)
+           value_free (b->val);
+         b->val = NULL;
+
+         /* Loading of new shared library change the meaning of
+            watchpoint condition.  However, insert_bp_location will
+            recompute watchpoint condition anyway, nothing to do here.  */
        }
-      if (breakpoint_enabled (b))
-       mention (b);
-      value_free_to_mark (mark);
       break;
     case bp_catch_catch:
     case bp_catch_throw:
-- 
1.5.3.5

Re: [RFA] Don't reset watchpoint block on solib load.

[Vladimir Prus]

Vladimir Prus wrote:

> I attach the revised patch with that extra if, does it look OK?

In fact, you might want to hold off reviewing this patch until:

1. I post revised patch to make watchpoints use multiple locations,
which will also introduce a single function to update watchpoint
that will be used in breakpoint_re_set_one.

2. I figure out why current unmodified GDB does not actually handle
watchpoints on variables in shared libraries that get loaded, and
segfaults. 

- Volodya

Re: [RFA] Don't reset watchpoint block on solib load.

[Jim Blandy]

Vladimir Prus <vladimir at codesourcery.com> writes:
> On Wednesday 28 November 2007 02:00:15 Jim Blandy wrote:
>> 
>> Vladimir Prus <vladimir at codesourcery.com> writes:
>> > There's code inside breakpoint_re_set_one to refresh watchpoints, 
>> > which seems suspicious to me. 
>> >
>> > First problem with that code is that it always resets watchpoint's
>> > block to NULL. So, if we have a local watchpoint, and you do 
>> > dlopen (without exiting the scope where watchpoint is valid), 
>> > then the watchpoint's block will be reset to NULL, and 
>> > watchpoint's expression will be reparsed in global block -- 
>> > which will surely break the watchpoint. 
>> 
>> Is that right?  We set innermost_block to NULL, but then we call
>> parse_expression, which should set innermost_block to the innermost
>> block containing a symbol actually used by the expression.
>
> Except that parse_expression is called while we're inside shared lib
> machinery code -- so neither original block nor original frame is
> used, and parse_expression is likely to fail.
>
>> We also call breakpoint_re_set_one when we've unloaded a shared
>> library.  At that point, b->exp_valid_block could be a dangling
>> pointer; we can't use it to re-parse the expression.
>> 
>> I think the bug is that we re-parse the expression with
>> parse_expression, which leaves the scope unspecified, defaulting to
>> the currently selected frame.  We should:
>> 
>> 1) Verify that the frame given by b->watchpoint_frame is still valid,
>>    and delete the watchpoint if it isn't.
>>    
>> 2) Call get_frame_block (b->watchpoint_frame) to see if we have a
>>    block for the frame's location, and deleting the watchpoint if we
>>    don't (saying we don't have the symbolic info available to update
>>    it), and
>> 
>> 3) Call parse_exp_1 (..., watchpoint frame's block, ...) to reparse
>>    the watchpoint's expression in the proper block.
>
> Let me try thinking out loud.
>
> 1. Suppose I have a watchpoint on a global variable. Such a watchpoint
> can reasonably survive program restart, and given that it can be set on 
> a global variable (including static variables of C++ classes) defined
> in a shared library, it makes sense recompute such watchpoint
> when a shared library is loaded, or unloaded. 

The steps I wrote don't address the case of watchpoints that aren't
frame-relative.  I wonder how we should be dealing with those.

If we have a watchpoint on a static variable local to some file or
block, then I don't honestly see how we can possibly re-set it
correctly after a symbol reload with the data we have now.  We can't
tell whether b->exp_valid_block is a dangling pointer or not, and
b->watchpoint_frame will be null on such watchpoints.

At the moment, a watchpoint has two interesting fields:

- b->exp_valid_block, which is set only when the watchpoint refers to
  frame-local variables.  Frustratingly, it's NULL if the watchpoint
  refers to block-scoped static variables, even though we have no way
  of finding those variables again if we re-parse the expression.

- b->watchpoint_frame, which establishes which instances of the local
  variables the watchpoint refers to.

The interesting thing is that exp_valid_block is almost always used as
a binary value.  The only exception is when we use it as a sanity
check on a frame we've found that matches b->watchpoint_frame.

Should we instead be saving the PC in whose scope we parsed the
expression?  That's something we could legitimately look up in
breakpoint_re_set_one after a symbol table change.  The test in
watchpoint_check could look up the PC's function and compare it to the
frame's function.

(There's also some terminology that's bugging me: the way the code
calls watchpoints "in scope" or "out of scope" is misleading: it's not
about scope, it's about what the ISO C standard calls "lifetime".  For
example, a static variable local to some block is "in scope" from the
point of its declaration to the end of its block, but the variable's
lifetime is the execution of the entire program (or until the shared
library that contains the function is dlclosed).  A watchpoint should
be deleted when the lifetime of any of the objects it refers to ends,
regardless of whether they are in scope or not.)

(I need to eat lunch, but I'll get to the rest when I get back.)

Re: [RFA] Don't reset watchpoint block on solib load.

[Vladimir Prus]

On Wednesday 28 November 2007 22:45:58 you wrote:
> 
> Vladimir Prus <vladimir at codesourcery.com> writes:
> > On Wednesday 28 November 2007 02:00:15 Jim Blandy wrote:
> >> 
> >> Vladimir Prus <vladimir at codesourcery.com> writes:
> >> > There's code inside breakpoint_re_set_one to refresh watchpoints, 
> >> > which seems suspicious to me. 
> >> >
> >> > First problem with that code is that it always resets watchpoint's
> >> > block to NULL. So, if we have a local watchpoint, and you do 
> >> > dlopen (without exiting the scope where watchpoint is valid), 
> >> > then the watchpoint's block will be reset to NULL, and 
> >> > watchpoint's expression will be reparsed in global block -- 
> >> > which will surely break the watchpoint. 
> >> 
> >> Is that right?  We set innermost_block to NULL, but then we call
> >> parse_expression, which should set innermost_block to the innermost
> >> block containing a symbol actually used by the expression.
> >
> > Except that parse_expression is called while we're inside shared lib
> > machinery code -- so neither original block nor original frame is
> > used, and parse_expression is likely to fail.
> >
> >> We also call breakpoint_re_set_one when we've unloaded a shared
> >> library.  At that point, b->exp_valid_block could be a dangling
> >> pointer; we can't use it to re-parse the expression.
> >> 
> >> I think the bug is that we re-parse the expression with
> >> parse_expression, which leaves the scope unspecified, defaulting to
> >> the currently selected frame.  We should:
> >> 
> >> 1) Verify that the frame given by b->watchpoint_frame is still valid,
> >>    and delete the watchpoint if it isn't.
> >>    
> >> 2) Call get_frame_block (b->watchpoint_frame) to see if we have a
> >>    block for the frame's location, and deleting the watchpoint if we
> >>    don't (saying we don't have the symbolic info available to update
> >>    it), and
> >> 
> >> 3) Call parse_exp_1 (..., watchpoint frame's block, ...) to reparse
> >>    the watchpoint's expression in the proper block.
> >
> > Let me try thinking out loud.
> >
> > 1. Suppose I have a watchpoint on a global variable. Such a watchpoint
> > can reasonably survive program restart, and given that it can be set on 
> > a global variable (including static variables of C++ classes) defined
> > in a shared library, it makes sense recompute such watchpoint
> > when a shared library is loaded, or unloaded. 
> 
> The steps I wrote don't address the case of watchpoints that aren't
> frame-relative.  I wonder how we should be dealing with those.
> 
> If we have a watchpoint on a static variable local to some file or
> block, then I don't honestly see how we can possibly re-set it
> correctly after a symbol reload with the data we have now.  We can't
> tell whether b->exp_valid_block is a dangling pointer or not, and
> b->watchpoint_frame will be null on such watchpoints.

I haven't run into any case when b->exp_valid_block is not-NULL,
but b->watchpoint_frame is NULL.

In fact, we don't need to care about blocks for global watchpoints --
just like ordinary breakpoint on 'foo' does not care which shared library
'foo' comes from, global watchpoint on 'important_data_structure' need
not care about where that variable comes from. If we re-parse watchpoint
expression on each solib load, things are fine.

> At the moment, a watchpoint has two interesting fields:
> 
> - b->exp_valid_block, which is set only when the watchpoint refers to
>   frame-local variables.  Frustratingly, it's NULL if the watchpoint
>   refers to block-scoped static variables, even though we have no way
>   of finding those variables again if we re-parse the expression.

Right, that's rather nasty.

> - b->watchpoint_frame, which establishes which instances of the local
>   variables the watchpoint refers to.
> 
> The interesting thing is that exp_valid_block is almost always used as
> a binary value.  The only exception is when we use it as a sanity
> check on a frame we've found that matches b->watchpoint_frame.

Yes, I noticed that too.

> Should we instead be saving the PC in whose scope we parsed the
> expression?  That's something we could legitimately look up in
> breakpoint_re_set_one after a symbol table change.  The test in
> watchpoint_check could look up the PC's function and compare it to the
> frame's function.

Well, it does not work with address randomization. And I think that for
global watchpoints, just re-parsing the expression works good enough.

> (There's also some terminology that's bugging me: the way the code
> calls watchpoints "in scope" or "out of scope" is misleading: it's not
> about scope, it's about what the ISO C standard calls "lifetime".  For
> example, a static variable local to some block is "in scope" from the
> point of its declaration to the end of its block, but the variable's
> lifetime is the execution of the entire program (or until the shared
> library that contains the function is dlclosed).  A watchpoint should
> be deleted when the lifetime of any of the objects it refers to ends,
> regardless of whether they are in scope or not.)

Similar problem exists in MI varobjs -- where 'in scope' means something
unclear.

- Volodya

Re: [RFA] Don't reset watchpoint block on solib load.

[Jim Blandy]

Vladimir Prus <vladimir at codesourcery.com> writes:
>> The steps I wrote don't address the case of watchpoints that aren't
>> frame-relative.  I wonder how we should be dealing with those.
>> 
>> If we have a watchpoint on a static variable local to some file or
>> block, then I don't honestly see how we can possibly re-set it
>> correctly after a symbol reload with the data we have now.  We can't
>> tell whether b->exp_valid_block is a dangling pointer or not, and
>> b->watchpoint_frame will be null on such watchpoints.
>
> I haven't run into any case when b->exp_valid_block is not-NULL,
> but b->watchpoint_frame is NULL.

By 'dangling pointer', I mean that it's referring to a block that was
freed when we freed the objfile it belongs to.  My point was just that
neither of those fields would be useful in addressing the problem I
described.

> In fact, we don't need to care about blocks for global watchpoints --
> just like ordinary breakpoint on 'foo' does not care which shared library
> 'foo' comes from, global watchpoint on 'important_data_structure' need
> not care about where that variable comes from. If we re-parse watchpoint
> expression on each solib load, things are fine.

Our messages crossed paths: I think we should worry about re-parsing
global watchpoints in the proper scope.

For example, we do take care to remember which function breakpoints
refer to when there is more than one possible referent.  In the
transcript below, note that the breakpoint set on the static 'foo' in
t2.c, as opposed to the global 'foo' in t1.c, remains there even when
we re-read the main executable.

Watchpoints should behave the same way.

  $ cat t1.c
  #include <stdio.h>

  void bar (void);

  void
  foo (void)
  {
    puts ("t1.c: foo");
  }

  int
  main (int argc, char **argv)
  {
    foo ();
    bar ();

    return 0;
  }

  $ cat t2.c
  #include <stdio.h>

  static void 
  foo (void)
  {
    puts ("t2.c: foo");
  }


  void
  bar (void)
  {
    foo ();
  }
  $ gcc -g t1.c t2.c -o t
  $ ~/gdb/pub/bin/gdb t
  GNU gdb 6.7.50.20071127-cvs
  Copyright (C) 2007 Free Software Foundation, Inc.
  License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
  This is free software: you are free to change and redistribute it.
  There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
  and "show warranty" for details.
  This GDB was configured as "i686-pc-linux-gnu"...
  (gdb) break bar
  Breakpoint 1 at 0x80483de: file t2.c, line 13.
  (gdb) run
  Starting program: /home/jimb/play/t 
  t1.c: foo

  Breakpoint 1, bar () at t2.c:13
  13        foo ();
  (gdb) break foo
  Breakpoint 2 at 0x80483ca: file t2.c, line 6.
  (gdb) i br
  Num     Type           Disp Enb  Address    What
  1       breakpoint     keep y    0x080483de in bar at t2.c:13
          breakpoint already hit 1 time
  2       breakpoint     keep y    0x080483ca in foo at t2.c:6
  (gdb) c
  Continuing.

  Breakpoint 2, foo () at t2.c:6
  6         puts ("t2.c: foo");
  (gdb) shell touch t
  (gdb) run
  The program being debugged has been started already.
  Start it from the beginning? (y or n) y
  `/home/jimb/play/t' has changed; re-reading symbols.
  Starting program: /home/jimb/play/t 
  t1.c: foo

  Breakpoint 1, bar () at t2.c:13
  13        foo ();
  (gdb) info break
  Num     Type           Disp Enb  Address    What
  1       breakpoint     keep y    0x080483de in bar at t2.c:13
          breakpoint already hit 1 time
  2       breakpoint     keep y    0x080483ca in foo at t2.c:6
  (gdb) c
  Continuing.

  Breakpoint 2, foo () at t2.c:6
  6         puts ("t2.c: foo");
  (gdb) 

Re: [RFA] Don't reset watchpoint block on solib load.

[Vladimir Prus]

On Thursday 29 November 2007 01:36:35 Jim Blandy wrote:
> 
> Vladimir Prus <vladimir at codesourcery.com> writes:
> >> The steps I wrote don't address the case of watchpoints that aren't
> >> frame-relative.  I wonder how we should be dealing with those.
> >> 
> >> If we have a watchpoint on a static variable local to some file or
> >> block, then I don't honestly see how we can possibly re-set it
> >> correctly after a symbol reload with the data we have now.  We can't
> >> tell whether b->exp_valid_block is a dangling pointer or not, and
> >> b->watchpoint_frame will be null on such watchpoints.
> >
> > I haven't run into any case when b->exp_valid_block is not-NULL,
> > but b->watchpoint_frame is NULL.
> 
> By 'dangling pointer', I mean that it's referring to a block that was
> freed when we freed the objfile it belongs to.  My point was just that
> neither of those fields would be useful in addressing the problem I
> described.

Yes, the 'dangling pointer' is a potential problem; I even
have a half-finished patch for that. Except, that it's not easy to
get such dangling pointer. For global watchpoint, exp_valid_block is NULL.
For watchpoint on local var, to get dangling pointer you need to unloaded
shlib from within shlib code, which is likely to crash before we get back.

> 
> > In fact, we don't need to care about blocks for global watchpoints --
> > just like ordinary breakpoint on 'foo' does not care which shared library
> > 'foo' comes from, global watchpoint on 'important_data_structure' need
> > not care about where that variable comes from. If we re-parse watchpoint
> > expression on each solib load, things are fine.
> 
> Our messages crossed paths: I think we should worry about re-parsing
> global watchpoints in the proper scope.
> 
> For example, we do take care to remember which function breakpoints
> refer to when there is more than one possible referent.  In the
> transcript below, note that the breakpoint set on the static 'foo' in
> t2.c, as opposed to the global 'foo' in t1.c, remains there even when
> we re-read the main executable.
> 
> Watchpoints should behave the same way.

Not that breakpoints in shared libraries do not behave this way,
exactly because shared library can be loaded at random address.

> This isn't what GDB does now, but I think this behavior is not hard to
> obtain.  When we set a watchpoint, instead of b->exp_valid_block, we
> should record the PC in whose scope we parsed the expression.  When we
> check a watchpoint's condition, we should look up the PC's block: if
> we can't find one, then we've unloaded symbolic information for that
> watchpoint, and we should delete it.
> 
> The tricky part is deciding what to do when the main executable file
> is modified.  It's hard to see what exactly the watchpoint should
> stick to in this case, since the PC isn't a reliable identifier across
> such changes.  Perhaps it would be enough to also record the name of
> the function and source file the PC falls in, re-parse if they have
> remained stable, and delete the watchpoint if they have changed.

The question is how useful this is. Program changes all the time, and PC
can easily end up in a different function. And I think a change can also
cause PC to end up in the same function and source file, but different block.
And we don't have any way to persistently identify blocks.

Anyway, the point of my two patches was to change watchpoints to use
multiple locations internally, without breaking behaviour that GDB *now*
tries to implement. Now, gdb tries to reparse global watchpoints, my
patches keep that, except that now GDB does not crash doing so.

You'd note my second patch has 'update_watchpoint' function, which is
the single place to update watchpoint if something changes. Should
we come up with a smarter approach of refreshing breakpoints, we can just
rename the 'reparse' parameter to that function into 'symbols_changed',
and let that function have any smart logic we'll come up. There will
be no need to redo anything else of my patches.

- Volodya

Re: [RFA] Don't reset watchpoint block on solib load.

[Jim Blandy]

Vladimir Prus <vladimir at codesourcery.com> writes:
>> Should we instead be saving the PC in whose scope we parsed the
>> expression?  That's something we could legitimately look up in
>> breakpoint_re_set_one after a symbol table change.  The test in
>> watchpoint_check could look up the PC's function and compare it to the
>> frame's function.
>
> Well, it does not work with address randomization. And I think that for
> global watchpoints, just re-parsing the expression works good
> enough.

Address randomization is a good point.  I think we should still be
able to get the right behavior somehow, though.

Re: [RFA] Don't reset watchpoint block on solib load.

[Jim Blandy]

Actually, let me withdraw the three steps I suggested above.  I
understand that I'm straying from commenting on your patch ---
apologies --- but that's because as I tried to think about it, I
became confused about what behavior we actually want to get out of
GDB.


Here's the behavior I think we actually want:

A watchpoint should 'stick' to the scope it was originally set in.
For example, suppose I have a shared library libx.so with a source
file x.c, and that x.c has a static variable S.  And suppose there's
also a global variable named S defined in the main program.  While
control is stopped somewhere in x.c, I set a watchpoint on S.
Clearly, that watchpoint refers to x.c's static S, not the global S.

Now, if I later unload libx.so, the watchpoint should delete itself
with an appropriate message, just as a watchpoint on a stack variable
does when its frame is popped --- when a shared library is unloaded,
that ends the lifetimes of the variables it defines, just as exiting a
block ends the lifetimes of the variables defined in the block.

But specifically, the watchpoint should *not* transform into a
watchpoint on the global S.  That's an entirely different variable; it
could even have a different type.  This is what I mean by a watchpoint
'sticking' to the scope it was set in.

So, opening and closing shared libraries should never cause us to
re-parse a watchpoint's expression.  Loading a shared library should
never re-parse a watchpoint expression, because the watchpoint should
stick to its original scope, which is still there.  Unloading a shared
library should never re-parse the watchpoint expression, but it should
delete the watchpoint if it refers to variables from shared library,
since they're gone now.

This isn't what GDB does now, but I think this behavior is not hard to
obtain.  When we set a watchpoint, instead of b->exp_valid_block, we
should record the PC in whose scope we parsed the expression.  When we
check a watchpoint's condition, we should look up the PC's block: if
we can't find one, then we've unloaded symbolic information for that
watchpoint, and we should delete it.

The tricky part is deciding what to do when the main executable file
is modified.  It's hard to see what exactly the watchpoint should
stick to in this case, since the PC isn't a reliable identifier across
such changes.  Perhaps it would be enough to also record the name of
the function and source file the PC falls in, re-parse if they have
remained stable, and delete the watchpoint if they have changed.

Re: [RFA] Don't reset watchpoint block on solib load.

[Eli Zaretskii]

> Cc: gdb-patches@sources.redhat.com
> From: Jim Blandy <jimb@codesourcery.com>
> Date: Wed, 28 Nov 2007 14:18:06 -0800
> 
> Now, if I later unload libx.so, the watchpoint should delete itself
> with an appropriate message, just as a watchpoint on a stack variable
> does when its frame is popped --- when a shared library is unloaded,
> that ends the lifetimes of the variables it defines, just as exiting a
> block ends the lifetimes of the variables defined in the block.

Actually, a more useful behavior would be to disable the watchpoint in
this case, and reenable it (and, possibly, re-parse the expression) if
the library gets loaded again.

Similarly when a watchpoint on a static variable, or an automatic
variable in the `main' function, goes out of scope, because the
program exits: it would be useful, at least as an option, to have the
watchpoint re-enabled when the program is restarted.

Re: [RFA] Don't reset watchpoint block on solib load.

[Vladimir Prus]

On Thursday 29 November 2007 07:24:36 Eli Zaretskii wrote:
> > Cc: gdb-patches@sources.redhat.com
> > From: Jim Blandy <jimb@codesourcery.com>
> > Date: Wed, 28 Nov 2007 14:18:06 -0800
> > 
> > Now, if I later unload libx.so, the watchpoint should delete itself
> > with an appropriate message, just as a watchpoint on a stack variable
> > does when its frame is popped --- when a shared library is unloaded,
> > that ends the lifetimes of the variables it defines, just as exiting a
> > block ends the lifetimes of the variables defined in the block.
> 
> Actually, a more useful behavior would be to disable the watchpoint in
> this case, and reenable it (and, possibly, re-parse the expression) if
> the library gets loaded again.

This is probably good behaviour, indeed. Or maybe we should not
disable watchpoint, but mark it as pending, in the same sense of
"user wanted it to be enabled, but it won't trigger until a shared
lib is loaded" that is used for ordinary watchpoints.

But before we even get to that, we need to fix all the existing
bugs with watchpoints/solib interaction. For example, currently
a watchpoint on a global variable in shared library wedges gdb
if that shared library is unloaded:

	Old value = 1
	New value = 2
	foo (i=2) at helper.cpp:7
	7       }
	(gdb) n
	main () at main.cpp:14
	14          dlclose(h);
	(gdb) n
	Address of symbol "g" is unknown.
	(gdb) n
	Single stepping until exit from function _dl_debug_state,
	which has no line number information.
	Address of symbol "g" is unknown.

Here, all further 'next' commands will tell you the same. This is problem
present before my patch, and I don't try to fix it. Then, there's problem
whereby local watchpoint gets deleted if a completely unrelated shared library
is loaded. Probably, there's much more. With that done, we can think about
more useful general behaviour.

- Volodya



> 
> Similarly when a watchpoint on a static variable, or an automatic
> variable in the `main' function, goes out of scope, because the
> program exits: it would be useful, at least as an option, to have the
> watchpoint re-enabled when the program is restarted.
> 

Re: [RFA] Don't reset watchpoint block on solib load.

[Jim Blandy]

Vladimir Prus <vladimir at codesourcery.com> writes:
> On Thursday 29 November 2007 07:24:36 Eli Zaretskii wrote:
>> > Cc: gdb-patches@sources.redhat.com
>> > From: Jim Blandy <jimb@codesourcery.com>
>> > Date: Wed, 28 Nov 2007 14:18:06 -0800
>> > 
>> > Now, if I later unload libx.so, the watchpoint should delete itself
>> > with an appropriate message, just as a watchpoint on a stack variable
>> > does when its frame is popped --- when a shared library is unloaded,
>> > that ends the lifetimes of the variables it defines, just as exiting a
>> > block ends the lifetimes of the variables defined in the block.
>> 
>> Actually, a more useful behavior would be to disable the watchpoint in
>> this case, and reenable it (and, possibly, re-parse the expression) if
>> the library gets loaded again.
>
> This is probably good behaviour, indeed. Or maybe we should not
> disable watchpoint, but mark it as pending, in the same sense of
> "user wanted it to be enabled, but it won't trigger until a shared
> lib is loaded" that is used for ordinary watchpoints.

I think so, too.  I guess the key observation is that, while it's not
meaningful to talk about a particular local variable "coming back
alive", since each function call creates a distinct set of local
variables, and you can have recursion, etc., it is meaningful to talk
about a shared library being reloaded, and it's intuitive to identify
the 'X' from the first loading with the 'X' in the second loading,
even if they're at different addresses.

Variable identification (Was: [RFA] Don't reset watchpoint block on solib load.)

[Vladimir Prus]

Jim Blandy wrote:

>> This is probably good behaviour, indeed. Or maybe we should not
>> disable watchpoint, but mark it as pending, in the same sense of
>> "user wanted it to be enabled, but it won't trigger until a shared
>> lib is loaded" that is used for ordinary watchpoints.
> 
> I think so, too.  I guess the key observation is that, while it's not
> meaningful to talk about a particular local variable "coming back
> alive", since each function call creates a distinct set of local
> variables, and you can have recursion, etc., it is meaningful to talk
> about a shared library being reloaded, and it's intuitive to identify
> the 'X' from the first loading with the 'X' in the second loading,
> even if they're at different addresses.

Yes. I now recall this is more general problem with identification of
variables in GDB. Say, you're in function, and you have local variable
'foo'. In GUI, you do something with 'foo' -- set display format to
hex, expand it, and so on. It's highly desirable to keep this
information for the next run of program, or even next run of the GUI --
even if variable is local, it's not likely that the display properties
user wants depend on frame.

Unfortunately, there's no way to do that.

- Volodya

Re: Variable identification (Was: [RFA] Don't reset watchpoint block on solib load.)

[Michael Snyder]

On Thu, 2007-11-29 at 22:02 +0300, Vladimir Prus wrote:
> Jim Blandy wrote:
> 
> >> This is probably good behaviour, indeed. Or maybe we should not
> >> disable watchpoint, but mark it as pending, in the same sense of
> >> "user wanted it to be enabled, but it won't trigger until a shared
> >> lib is loaded" that is used for ordinary watchpoints.
> > 
> > I think so, too.  I guess the key observation is that, while it's not
> > meaningful to talk about a particular local variable "coming back
> > alive", since each function call creates a distinct set of local
> > variables, and you can have recursion, etc., it is meaningful to talk
> > about a shared library being reloaded, and it's intuitive to identify
> > the 'X' from the first loading with the 'X' in the second loading,
> > even if they're at different addresses.
> 
> Yes. I now recall this is more general problem with identification of
> variables in GDB. Say, you're in function, and you have local variable
> 'foo'. In GUI, you do something with 'foo' -- set display format to
> hex, expand it, and so on. It's highly desirable to keep this
> information for the next run of program, or even next run of the GUI --
> even if variable is local, it's not likely that the display properties
> user wants depend on frame.
> 
> Unfortunately, there's no way to do that.

I haven't followed the discussion closely, but
shouldn't it be up to the GUI to keep such persistant
info?  It's nothing to do with gdb, really.  It's the
GUI's state.

Re: Variable identification (Was: [RFA] Don't reset watchpoint block on solib load.)

[Vladimir Prus]

On Friday 30 November 2007 04:09:46 Michael Snyder wrote:
> On Thu, 2007-11-29 at 22:02 +0300, Vladimir Prus wrote:
> > Jim Blandy wrote:
> > >> This is probably good behaviour, indeed. Or maybe we should not
> > >> disable watchpoint, but mark it as pending, in the same sense of
> > >> "user wanted it to be enabled, but it won't trigger until a shared
> > >> lib is loaded" that is used for ordinary watchpoints.
> > >
> > > I think so, too.  I guess the key observation is that, while it's not
> > > meaningful to talk about a particular local variable "coming back
> > > alive", since each function call creates a distinct set of local
> > > variables, and you can have recursion, etc., it is meaningful to talk
> > > about a shared library being reloaded, and it's intuitive to identify
> > > the 'X' from the first loading with the 'X' in the second loading,
> > > even if they're at different addresses.
> >
> > Yes. I now recall this is more general problem with identification of
> > variables in GDB. Say, you're in function, and you have local variable
> > 'foo'. In GUI, you do something with 'foo' -- set display format to
> > hex, expand it, and so on. It's highly desirable to keep this
> > information for the next run of program, or even next run of the GUI --
> > even if variable is local, it's not likely that the display properties
> > user wants depend on frame.
> >
> > Unfortunately, there's no way to do that.
>
> I haven't followed the discussion closely, but
> shouldn't it be up to the GUI to keep such persistant
> info?  It's nothing to do with gdb, really.  It's the
> GUI's state.

It's GUI state, true. But to keep it, GUI needs some identifier for variables, 
that is is persistent across program restarts, preferably even if program 
changes. Presently, there's no such identifier provided by GDB.

- Volodya

Re: Variable identification (Was: [RFA] Don't reset watchpoint block on solib load.)

[Eli Zaretskii]

> From: Michael Snyder <msnyder@specifix.com>
> Cc: gdb-patches@sources.redhat.com
> Date: Thu, 29 Nov 2007 17:09:46 -0800
> 
> I haven't followed the discussion closely, but
> shouldn't it be up to the GUI to keep such persistant
> info?  It's nothing to do with gdb, really.  It's the
> GUI's state.

I'm not sure what you are talking about.  This discussion mentioned
two different aspects of watchpoints: I wanted the watchpoints on
automatic variables defined in `main' to not be deleted when the
program exits, and Vlad was talking about automatic variables in
general (which hits the problem with recursive invocations of the same
function).  I submit that the former is how GDB should behave, and
that the fact it doesn't today is a misfeature, unrelated to the GUI
state.  To a programmer, automatic variables in `main' are almost
indistinguishable from static variables, so watchpoints on both kinds
should behave the same.

Re: Variable identification

[Jim Blandy]

Michael Snyder <msnyder at specifix.com> writes:
> On Thu, 2007-11-29 at 22:02 +0300, Vladimir Prus wrote:
>> Jim Blandy wrote:
>> 
>> >> This is probably good behaviour, indeed. Or maybe we should not
>> >> disable watchpoint, but mark it as pending, in the same sense of
>> >> "user wanted it to be enabled, but it won't trigger until a shared
>> >> lib is loaded" that is used for ordinary watchpoints.
>> > 
>> > I think so, too.  I guess the key observation is that, while it's not
>> > meaningful to talk about a particular local variable "coming back
>> > alive", since each function call creates a distinct set of local
>> > variables, and you can have recursion, etc., it is meaningful to talk
>> > about a shared library being reloaded, and it's intuitive to identify
>> > the 'X' from the first loading with the 'X' in the second loading,
>> > even if they're at different addresses.
>> 
>> Yes. I now recall this is more general problem with identification of
>> variables in GDB. Say, you're in function, and you have local variable
>> 'foo'. In GUI, you do something with 'foo' -- set display format to
>> hex, expand it, and so on. It's highly desirable to keep this
>> information for the next run of program, or even next run of the GUI --
>> even if variable is local, it's not likely that the display properties
>> user wants depend on frame.
>> 
>> Unfortunately, there's no way to do that.
>
> I haven't followed the discussion closely, but
> shouldn't it be up to the GUI to keep such persistant
> info?  It's nothing to do with gdb, really.  It's the
> GUI's state.

These questions all affect how watchpoints behave in the CLI as well.

Re: Variable identification

[Michael Snyder]

On Fri, 2007-11-30 at 17:38 -0800, Jim Blandy wrote:
> Michael Snyder <msnyder at specifix.com> writes:
> > On Thu, 2007-11-29 at 22:02 +0300, Vladimir Prus wrote:
> >> Jim Blandy wrote:
> >> 
> >> >> This is probably good behaviour, indeed. Or maybe we should not
> >> >> disable watchpoint, but mark it as pending, in the same sense of
> >> >> "user wanted it to be enabled, but it won't trigger until a shared
> >> >> lib is loaded" that is used for ordinary watchpoints.
> >> > 
> >> > I think so, too.  I guess the key observation is that, while it's not
> >> > meaningful to talk about a particular local variable "coming back
> >> > alive", since each function call creates a distinct set of local
> >> > variables, and you can have recursion, etc., it is meaningful to talk
> >> > about a shared library being reloaded, and it's intuitive to identify
> >> > the 'X' from the first loading with the 'X' in the second loading,
> >> > even if they're at different addresses.
> >> 
> >> Yes. I now recall this is more general problem with identification of
> >> variables in GDB. Say, you're in function, and you have local variable
> >> 'foo'. In GUI, you do something with 'foo' -- set display format to
> >> hex, expand it, and so on. It's highly desirable to keep this
> >> information for the next run of program, or even next run of the GUI --
> >> even if variable is local, it's not likely that the display properties
> >> user wants depend on frame.
> >> 
> >> Unfortunately, there's no way to do that.
> >
> > I haven't followed the discussion closely, but
> > shouldn't it be up to the GUI to keep such persistant
> > info?  It's nothing to do with gdb, really.  It's the
> > GUI's state.
> 
> These questions all affect how watchpoints behave in the CLI as well.

Right, with the CLI being a special case of "the GUI" -- one
in which we (gdb maintainers) have control of everything.

GDB can keep track of the CLI "state", but other GUI 
should keep track of their own state.

Re: [RFA] Don't reset watchpoint block on solib load.

[Jim Blandy]

(Since it took me a bit to re-discover what this patch was about, I
figure I should recap...)

breakpoint_re_set_one gets called when the symbolic information
available to GDB has changed: we've re-run or attached and the
executable file has changed since we last read it, or the program has
loaded or unloaded a shared library.  The breakpoint_re_set_one
function is responsible for updating all references to symbol table
contents in breakpoints, watchpoints, etc.

A watchpoint can refer to local variables that were in scope when it
was set, so it may be associated with a particular frame.  However,
breakpoint_re_set_one re-parses watchpoint expressions using
parse_expression, which parses in the scope of whatever frame happens
to selected when it is called.  This is very wrong: GDB can call
breakpoint_re_set_one in many different circumstances, and the frame
selected at that time has no necessary relationship to the frame in
which the user set the watchpoint.

In particular, GDB calls breakpoint_re_set_one when it stops for a
shared library load or unload.  At this point, the selected frame is
the youngest frame, in the dynamic linker's debug breakpoint function.
Since watchpoints' local variables are almost certainly not in scope
in that function, GDB fails to re-parse watchpoint expressions that
refer to local variables whenever we do a dlopen or dlclose.  (And
then fails to handle the error and dies.)  For example:

    $ cat w1.c
    #include <dlfcn.h>

    int
    main (int argc, char **argv)
    {
      volatile int g = 1;

      g = 2;

      void *lib = dlopen ("libm.so", RTLD_NOW);

      g = 3;

      return g;
    }
    $ gcc -g w1.c -ldl -o w1
    $ ~/gdb/pub/nat/gdb/gdb w1
    GNU gdb 6.7.50.20080111-cvs
    Copyright (C) 2008 Free Software Foundation, Inc.
    License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
    This is free software: you are free to change and redistribute it.
    There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
    and "show warranty" for details.
    This GDB was configured as "i686-pc-linux-gnu"...
    (gdb) b 10
    Breakpoint 1 at 0x8048403: file w1.c, line 10.
    (gdb) run
    Starting program: /home/jimb/play/w1 

    Breakpoint 1, main () at w1.c:10
    10        void *lib = dlopen ("libm.so", RTLD_NOW);
    (gdb) watch g
    Hardware watchpoint 2: g
    (gdb) next
    Error in re-setting breakpoint 2: No symbol "g" in current context.
    Segmentation fault (core dumped)
    $ 

I believe Vlad's most recent patch is this one:

http://sourceware.org/ml/gdb-patches/2007-11/msg00522.html

The patch has two effects:

First, in breakpoint_re_set_one (w), if W->exp_valid_block is NULL
(meaning that the watchpoint refers to no local variables), or if it
is set and GDB can find W->watchpoint_frame on the stack, then it
tries to re-parse the expression in exp_valid_block.

Second, it re-parses watchpoint condition expressions (the Y in 'watch
X if Y') when it inserts breakpoints.

I have two questions about this patch.

- If we dlclose a shared library while there are still frames in that
  library's code on the stack, couldn't exp_valid_block be pointing to
  garbage (i.e. a 'struct block' in the shared library whose debug
  info has now been freed), even though frame_find_by_id can find the
  frame?

  Granted, the user's program is unhappy, because it's going to return
  to code that isn't there any more, but GDB shouldn't crash --- after
  all, debuggers are meant for use on unhappy programs.

  If we can find the frame, and we can find the frame's block, could
  we use that instead?  In the perverse case I outlined, I assume we'd
  be able to find the frame, but not the block.

  Certainly, for watchpoints that cite no local variables, we should
  just always try to re-parse.  The prior conversation identified some
  ugly issues there, too, but that's an existing bug; let's take it
  one step at a time.

- Why shouldn't we re-parse the condition at the same time we re-parse
  the expression, in the same block?  Why should we re-parse the
  condition when we insert breakpoints?

  Although I agreed to reparsing the condition when we insert
  breakpoints earlier in the thread, I can't see now why it's a good
  idea.  It seems wasteful, since we remove and insert breakpoints
  frequently, and since we do remove and re-insert breakpoints on
  dlopen/dlclose, it doesn't seem to be a significantly more
  auspicious time to do the parse.

Re: [RFA] Don't reset watchpoint block on solib load.

[Vladimir Prus]

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

On Wednesday 16 January 2008 04:28:32 Jim Blandy wrote:
> 
> (Since it took me a bit to re-discover what this patch was about, I
> figure I should recap...)
> 
> breakpoint_re_set_one gets called when the symbolic information
> available to GDB has changed: we've re-run or attached and the
> executable file has changed since we last read it, or the program has
> loaded or unloaded a shared library.  

I think we've established that right now, breakpoint_re_set_one is
not called when a library is unloaded.

> The breakpoint_re_set_one 
> function is responsible for updating all references to symbol table
> contents in breakpoints, watchpoints, etc.
> 
> A watchpoint can refer to local variables that were in scope when it
> was set, so it may be associated with a particular frame.  However,
> breakpoint_re_set_one re-parses watchpoint expressions using
> parse_expression, which parses in the scope of whatever frame happens
> to selected when it is called.  This is very wrong: GDB can call
> breakpoint_re_set_one in many different circumstances, and the frame
> selected at that time has no necessary relationship to the frame in
> which the user set the watchpoint.
> 
> In particular, GDB calls breakpoint_re_set_one when it stops for a
> shared library load or unload.  At this point, the selected frame is
> the youngest frame, in the dynamic linker's debug breakpoint function.
> Since watchpoints' local variables are almost certainly not in scope
> in that function, GDB fails to re-parse watchpoint expressions that
> refer to local variables whenever we do a dlopen or dlclose.  (And
> then fails to handle the error and dies.)  For example:
> 
>     $ cat w1.c
>     #include <dlfcn.h>
> 
>     int
>     main (int argc, char **argv)
>     {
>       volatile int g = 1;
> 
>       g = 2;
> 
>       void *lib = dlopen ("libm.so", RTLD_NOW);
> 
>       g = 3;
> 
>       return g;
>     }
>     $ gcc -g w1.c -ldl -o w1
>     $ ~/gdb/pub/nat/gdb/gdb w1
>     GNU gdb 6.7.50.20080111-cvs
>     Copyright (C) 2008 Free Software Foundation, Inc.
>     License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
>     This is free software: you are free to change and redistribute it.
>     There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
>     and "show warranty" for details.
>     This GDB was configured as "i686-pc-linux-gnu"...
>     (gdb) b 10
>     Breakpoint 1 at 0x8048403: file w1.c, line 10.
>     (gdb) run
>     Starting program: /home/jimb/play/w1 
> 
>     Breakpoint 1, main () at w1.c:10
>     10        void *lib = dlopen ("libm.so", RTLD_NOW);
>     (gdb) watch g
>     Hardware watchpoint 2: g
>     (gdb) next
>     Error in re-setting breakpoint 2: No symbol "g" in current context.
>     Segmentation fault (core dumped)
>     $ 
> 
> I believe Vlad's most recent patch is this one:
> 
> http://sourceware.org/ml/gdb-patches/2007-11/msg00522.html
> 
> The patch has two effects:
> 
> First, in breakpoint_re_set_one (w), if W->exp_valid_block is NULL
> (meaning that the watchpoint refers to no local variables), or if it
> is set and GDB can find W->watchpoint_frame on the stack, then it
> tries to re-parse the expression in exp_valid_block.
> 
> Second, it re-parses watchpoint condition expressions (the Y in 'watch
> X if Y') when it inserts breakpoints.
> 
> I have two questions about this patch.
> 
> - If we dlclose a shared library while there are still frames in that
>   library's code on the stack, couldn't exp_valid_block be pointing to
>   garbage (i.e. a 'struct block' in the shared library whose debug
>   info has now been freed), even though frame_find_by_id can find the
>   frame?

It's possible. And it's pre-existing problem ;-) Basically, now both
dlopen and dlclose mess up watchpoints, and the patch is a step that
brings us closer to dlopen working. 

>   Granted, the user's program is unhappy, because it's going to return
>   to code that isn't there any more, but GDB shouldn't crash --- after
>   all, debuggers are meant for use on unhappy programs.
> 
>   If we can find the frame, and we can find the frame's block, could
>   we use that instead?  

Did you mean "if we can find the same, and *can't* find the block"? 
Basically, if we have a watchpoint associated with frame id, and we can't
find that frame id, there's nothing we can do -- because we can't access
local variables relatively to frame base we don't know. However,
it's possible that frame is found, but in fact, the block is gone -- in
which case we should not try to use the gone block.

As I mention, this is pre-existing problem. We need a mechanism to get
notified when a block is gone. Note that it's strictly speaking
different from shared library unloaded, since syntab is not necessary
removed at this point. So, ideally, we'd need a new observer that's called
when symtab is removed. We'll plug into that observer and disable
all watchpoints which conditions use the gone blocks. In fact, I'm
not sure that this problem is only relevant to watchpoints. Ordinary
breakpoint might well have a condition that uses variables from shared
libraries, and such condition will become invalid when shared library
is removed.

So, I think it's a global existing problem, and we should postpone it.

>   In the perverse case I outlined, I assume we'd 
>   be able to find the frame, but not the block.
> 
>   Certainly, for watchpoints that cite no local variables, we should
>   just always try to re-parse.  The prior conversation identified some
>   ugly issues there, too, but that's an existing bug; let's take it
>   one step at a time.
> 
> - Why shouldn't we re-parse the condition at the same time we re-parse
>   the expression, in the same block?  Why should we re-parse the
>   condition when we insert breakpoints?
> 
>   Although I agreed to reparsing the condition when we insert
>   breakpoints earlier in the thread, I can't see now why it's a good
>   idea.  It seems wasteful, since we remove and insert breakpoints
>   frequently, and since we do remove and re-insert breakpoints on
>   dlopen/dlclose, it doesn't seem to be a significantly more
>   auspicious time to do the parse.

Well, we reparse the entire watchpoint expression on each 
insert already, so reparsing a condition is not a big deal. However,
you're right it's not needed, strictly speaking.

Honestly, the point of moving it for this patch was to place all
code for updating watchpoint in one place.

The next patch, that switches watchpoint to using multiple locations,
introduced update_watchpoint function which is a single place for
all watchpoint updates. Looking at the patch, however, I see that
I always reparse condition anyway. I've changed that patch to
reparse condition only when update_watchpoint is called with reparse=1,
which happens only from breakpoint_re_set_one, and I don't get any
regression as result.

So, I guess the solutions are either:

1. Omit the move of condition parsing from this patch, or
2. Keep condition parsing in this patch, and then commit
the use-multiple-locations-for-watchpoint patch which will
immediately make condition only reparsed when breakpoint_re_set_one
is called.

Given that use-multiple-locations-for-watchpoint patch is
approved except for the bit that makes reparsing of condition
optional, it seems that (2) is less work.

Jim, does this look good for you?

For convenience, I attach the current version of 
use-multiple-locations-for-watchpoint patch. The difference
to the previous revision is as follows:


@@ -966,7 +966,7 @@ update_watchpoint (struct breakpoint *b, int reparse)
            value_free (v);
        }

-      if (b->cond_string != NULL)
+      if (reparse && b->cond_string != NULL)
        {
          char *s = b->cond_string;
          if (b->loc->cond)


Dan, is the patch still OK to commit?

- Volodya

[-- Attachment #2: 0003-Use-multiple-locations-for-hardware-watchpoints.patch --]
[-- Type: text/x-diff, Size: 29515 bytes --]

From 536a6f0019cc9915ed52c8169af88f4c5c12f057 Mon Sep 17 00:00:00 2001
From: Vladimir Prus <vladimir@codesourcery.com>
Date: Tue, 20 Nov 2007 20:10:54 +0300
Subject: [RFA] Use multiple locations for hardware watchpoints.
To: gdb-patches@sources.redhat.com
X-KMail-Transport: CodeSourcery
X-KMail-Identity: 901867920

This eliminates the need to traverse value chain, doing
various checks, in three different places.

	* breakpoint.h (struct bp_location): New fields
	lengths and watchpoint_type.
	(struct breakpoint): Remove the val_chain field.
	* breakpoint.c (is_hardware_watchpoint): New.
	(free_valchain): Remove.
	(update_watchpoint): New.
	(insert_bp_location): For hardware watchpoint, just
	directly insert it.
	(insert_breakpoints): Call update_watchpoint_locations
	on all watchpoints.  If we have failed to insert
	any location of a hardware watchpoint, remove all inserted
	locations.
	(remove_breakpoint): For hardware watchpoints, directly
	remove location.
	(watchpoints_triggered): Iterate over locations.
	(bpstat_stop_status): Use only first location of
	a resource watchpoint.
	(delete_breakpoint): Don't call free_valchain.
	(print_one_breakpoint): Don't print all
	locations for watchpoints.
	(breakpoint_re_set_one): Use update_watchpoint for
	watchpoints.

	testsuite/
	* gdb.base/watchpoint-solib.exp: New.
	* gdb.base/watchpoint-solib.c: New.
	* gdb.base/watchpoint-solib-shr.c: New.
---
 gdb/breakpoint.c                              |  502 +++++++++++++------------
 gdb/breakpoint.h                              |    9 +-
 gdb/testsuite/gdb.base/watchpoint-solib-shr.c |   25 ++
 gdb/testsuite/gdb.base/watchpoint-solib.c     |   68 ++++
 gdb/testsuite/gdb.base/watchpoint-solib.exp   |   89 +++++
 5 files changed, 441 insertions(+), 252 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/watchpoint-solib-shr.c
 create mode 100644 gdb/testsuite/gdb.base/watchpoint-solib.c
 create mode 100644 gdb/testsuite/gdb.base/watchpoint-solib.exp

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ecc2478..0bed4ef 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -200,6 +200,15 @@ static void free_bp_location (struct bp_location *loc);
 
 static void mark_breakpoints_out (void);
 
+static struct bp_location *
+allocate_bp_location (struct breakpoint *bpt, enum bptype bp_type);
+
+static void
+unlink_locations_from_global_list (struct breakpoint *bpt);
+
+static int
+is_hardware_watchpoint (struct breakpoint *bpt);
+
 /* Prototypes for exported functions. */
 
 /* If FALSE, gdb will not use hardware support for watchpoints, even
@@ -808,24 +817,182 @@ insert_catchpoint (struct ui_out *uo, void *args)
     }
 }
 
-/* Helper routine: free the value chain for a breakpoint (watchpoint).  */
+static int
+is_hardware_watchpoint (struct breakpoint *bpt)
+{
+  return (bpt->type == bp_hardware_watchpoint
+	  || bpt->type == bp_read_watchpoint
+	  || bpt->type == bp_access_watchpoint);
+}
 
+/* Assuming that B is a hardware breakpoint:
+   - Reparse watchpoint expression, is REPARSE is non-zero
+   - Evaluate expression and store the result in B->val
+   - Update the list of values that must be watched in B->loc.
+
+   If the watchpoint is disabled, do nothing.  If this is
+   local watchpoint that is out of scope, delete it.  */
 static void
-free_valchain (struct bp_location *b)
+update_watchpoint (struct breakpoint *b, int reparse)
 {
-  struct value *v;
-  struct value *n;
+  int within_current_scope;
+  struct value *mark = value_mark ();
+  struct frame_id saved_frame_id;
+  struct bp_location *loc;
+  bpstat bs;
+
+  unlink_locations_from_global_list (b);
+  for (loc = b->loc; loc;)
+    {
+      struct bp_location *loc_next = loc->next;
+      remove_breakpoint (loc, mark_uninserted);
+      xfree (loc);
+      loc = loc_next;
+    }
+  b->loc = NULL;
 
-  /* Free the saved value chain.  We will construct a new one
-     the next time the watchpoint is inserted.  */
-  for (v = b->owner->val_chain; v; v = n)
+  if (b->disposition == disp_del_at_next_stop)
+    return;
+ 
+  /* Save the current frame's ID so we can restore it after
+     evaluating the watchpoint expression on its own frame.  */
+  /* FIXME drow/2003-09-09: It would be nice if evaluate_expression
+     took a frame parameter, so that we didn't have to change the
+     selected frame.  */
+  saved_frame_id = get_frame_id (get_selected_frame (NULL));
+
+  /* Determine if the watchpoint is within scope.  */
+  if (b->exp_valid_block == NULL)
+    within_current_scope = 1;
+  else
     {
-      n = value_next (v);
-      value_free (v);
+      struct frame_info *fi;
+      fi = frame_find_by_id (b->watchpoint_frame);
+      within_current_scope = (fi != NULL);
+      if (within_current_scope)
+	select_frame (fi);
+    }
+
+  if (within_current_scope && reparse)
+    {
+      char *s;
+      if (b->exp)
+	{
+	  xfree (b->exp);
+	  b->exp = NULL;
+	}
+      s = b->exp_string;
+      b->exp = parse_exp_1 (&s, b->exp_valid_block, 0);
+      /* If the meaning of expression itself changed, the old value is
+	 no longer relevant.  We don't want to report a watchpoint hit
+	 to the user when the old value and the new value may actually
+	 be completely different objects.  */
+      value_free (b->val);
+      b->val = NULL;      
     }
-  b->owner->val_chain = NULL;
+  
+
+  /* If we failed to parse the expression, for example because
+     it refers to a global variable in a not-yet-loaded shared library,
+     don't try to insert watchpoint.  We don't automatically delete
+     such watchpoint, though, since failure to parse expression
+     is different from out-of-scope watchpoint.  */
+  if (within_current_scope && b->exp)
+    {
+      struct value *v, *next;
+
+      /* Evaluate the expression and make sure it's not lazy, so that
+	 after target stops again, we have a non-lazy previous value
+	 to compare with. Also, making the value non-lazy will fetch
+	 intermediate values as needed, which we use to decide which
+	 addresses to watch.
+
+	 The value returned by evaluate_expression is stored in b->val.
+	 In addition, we look at all values which were created
+	 during evaluation, and set watchoints at addresses as needed.
+	 Those values are explicitly deleted here.  */
+      v = evaluate_expression (b->exp);
+      /* Avoid setting b->val if it's already set.  The meaning of
+	 b->val is 'the last value' user saw, and we should update
+	 it only if we reported that last value to user.  As it
+	 happens, the code that reports it updates b->val directly.  */
+      if (b->val == NULL)
+	b->val = v;
+      value_contents (v);
+      value_release_to_mark (mark);
+
+      /* Look at each value on the value chain.  */
+      for (; v; v = next)
+	{
+	  /* If it's a memory location, and GDB actually needed
+	     its contents to evaluate the expression, then we
+	     must watch it.  */
+	  if (VALUE_LVAL (v) == lval_memory
+	      && ! value_lazy (v))
+	    {
+	      struct type *vtype = check_typedef (value_type (v));
+
+	      /* We only watch structs and arrays if user asked
+		 for it explicitly, never if they just happen to
+		 appear in the middle of some value chain.  */
+	      if (v == b->val
+		  || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
+		      && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
+		{
+		  CORE_ADDR addr;
+		  int len, type;
+		  struct bp_location *loc, **tmp;
+
+		  addr = VALUE_ADDRESS (v) + value_offset (v);
+		  len = TYPE_LENGTH (value_type (v));
+		  type = hw_write;
+		  if (b->type == bp_read_watchpoint)
+		    type = hw_read;
+		  else if (b->type == bp_access_watchpoint)
+		    type = hw_access;
+		  
+		  loc = allocate_bp_location (b, bp_hardware_watchpoint);
+		  for (tmp = &(b->loc); *tmp != NULL; tmp = &((*tmp)->next))
+		    ;
+		  *tmp = loc;
+		  loc->address = addr;
+		  loc->length = len;
+		  loc->watchpoint_type = type;
+		}
+	    }
+
+	  next = value_next (v);
+	  if (v != b->val)
+	    value_free (v);
+	}
+
+      if (reparse && 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);
+	}
+    }
+  else if (!within_current_scope)
+    {
+      printf_filtered (_("\
+Hardware watchpoint %d deleted because the program has left the block \n\
+in which its expression is valid.\n"),
+		       b->number);
+      if (b->related_breakpoint)
+	b->related_breakpoint->disposition = disp_del_at_next_stop;
+      b->disposition = disp_del_at_next_stop;
+    }
+
+  /* Restore the selected frame.  */
+  select_frame (frame_find_by_id (saved_frame_id));
 }
 
+
 /* Insert a low-level "breakpoint" of some type.  BPT is the breakpoint.
    Any error messages are printed to TMP_ERROR_STREAM; and DISABLED_BREAKS,
    PROCESS_WARNING, and HW_BREAKPOINT_ERROR are used to report problems.
@@ -1016,136 +1183,10 @@ Note: automatically using hardware breakpoints for read-only addresses.\n"));
 	      watchpoints.  It's not clear that it's necessary... */
 	   && bpt->owner->disposition != disp_del_at_next_stop)
     {
-      /* FIXME drow/2003-09-08: This code sets multiple hardware watchpoints
-	 based on the expression.  Ideally this should happen at a higher level,
-	 and there should be one bp_location for each computed address we
-	 must watch.  As soon as a many-to-one mapping is available I'll
-	 convert this.  */
-
-      int within_current_scope;
-      struct value *mark = value_mark ();
-      struct value *v;
-      struct frame_id saved_frame_id;
-
-      /* Save the current frame's ID so we can restore it after
-	 evaluating the watchpoint expression on its own frame.  */
-      /* FIXME drow/2003-09-09: It would be nice if evaluate_expression
-	 took a frame parameter, so that we didn't have to change the
-	 selected frame.  */
-      saved_frame_id = get_frame_id (get_selected_frame (NULL));
-
-      /* Determine if the watchpoint is within scope.  */
-      if (bpt->owner->exp_valid_block == NULL)
-	within_current_scope = 1;
-      else
-	{
-	  struct frame_info *fi;
-	  fi = frame_find_by_id (bpt->owner->watchpoint_frame);
-	  within_current_scope = (fi != NULL);
-	  if (within_current_scope)
-	    select_frame (fi);
-	}
-
-      if (within_current_scope)
-	{
-	  free_valchain (bpt);
-
-	  /* Evaluate the expression and cut the chain of values
-	     produced off from the value chain.
-
-	     Make sure the value returned isn't lazy; we use
-	     laziness to determine what memory GDB actually needed
-	     in order to compute the value of the expression.  */
-	  v = evaluate_expression (bpt->owner->exp);
-	  value_contents (v);
-	  value_release_to_mark (mark);
-
-	  bpt->owner->val_chain = v;
-	  bpt->inserted = 1;
-
-	  /* Look at each value on the value chain.  */
-	  for (; v; v = value_next (v))
-	    {
-	      /* If it's a memory location, and GDB actually needed
-		 its contents to evaluate the expression, then we
-		 must watch it.  */
-	      if (VALUE_LVAL (v) == lval_memory
-		  && ! value_lazy (v))
-		{
-		  struct type *vtype = check_typedef (value_type (v));
-
-		  /* We only watch structs and arrays if user asked
-		     for it explicitly, never if they just happen to
-		     appear in the middle of some value chain.  */
-		  if (v == bpt->owner->val_chain
-		      || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
-			  && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
-		    {
-		      CORE_ADDR addr;
-		      int len, type;
-
-		      addr = VALUE_ADDRESS (v) + value_offset (v);
-		      len = TYPE_LENGTH (value_type (v));
-		      type = hw_write;
-		      if (bpt->owner->type == bp_read_watchpoint)
-			type = hw_read;
-		      else if (bpt->owner->type == bp_access_watchpoint)
-			type = hw_access;
-
-		      val = target_insert_watchpoint (addr, len, type);
-		      if (val == -1)
-			{
-			  /* Don't exit the loop, try to insert
-			     every value on the value chain.  That's
-			     because we will be removing all the
-			     watches below, and removing a
-			     watchpoint we didn't insert could have
-			     adverse effects.  */
-			  bpt->inserted = 0;
-			}
-		      val = 0;
-		    }
-		}
-	    }
-
-	  if (bpt->owner->cond_string != NULL)
-	    {
-	      char *s = bpt->owner->cond_string;
-	      if (bpt->cond)
-		{
-		  xfree (bpt->cond);
-		  bpt->cond = NULL;
-		}
-	      bpt->cond = parse_exp_1 (&s, bpt->owner->exp_valid_block, 0);
-	    }
-	      
-	  /* Failure to insert a watchpoint on any memory value in the
-	     value chain brings us here.  */
-	  if (!bpt->inserted)
-	    {
-	      remove_breakpoint (bpt, mark_uninserted);
-	      *hw_breakpoint_error = 1;
-	      fprintf_unfiltered (tmp_error_stream,
-				  "Could not insert hardware watchpoint %d.\n", 
-				  bpt->owner->number);
-	      val = -1;
-	    }               
-	}
-      else
-	{
-	  printf_filtered (_("\
-Hardware watchpoint %d deleted because the program has left the block \n\
-in which its expression is valid.\n"),
-			   bpt->owner->number);
-	  if (bpt->owner->related_breakpoint)
-	    bpt->owner->related_breakpoint->disposition = disp_del_at_next_stop;
-	  bpt->owner->disposition = disp_del_at_next_stop;
-	}
-
-      /* Restore the selected frame.  */
-      select_frame (frame_find_by_id (saved_frame_id));
-
-      return val;
+      val = target_insert_watchpoint (bpt->address, 
+				      bpt->length,
+				      bpt->watchpoint_type);
+      bpt->inserted = (val != -1);
     }
 
   else if (bpt->owner->type == bp_catch_fork
@@ -1178,6 +1219,7 @@ in which its expression is valid.\n"),
 void
 insert_breakpoints (void)
 {
+  struct breakpoint *bpt;
   struct bp_location *b, *temp;
   int error = 0;
   int val = 0;
@@ -1192,6 +1234,10 @@ insert_breakpoints (void)
      there was an error.  */
   fprintf_unfiltered (tmp_error_stream, "Warning:\n");
 
+  ALL_BREAKPOINTS (bpt)
+    if (is_hardware_watchpoint (bpt))
+      update_watchpoint (bpt, 0 /* don't reparse */);      
+	
   ALL_BP_LOCATIONS_SAFE (b, temp)
     {
       if (!breakpoint_enabled (b->owner))
@@ -1203,19 +1249,6 @@ insert_breakpoints (void)
 	  && !valid_thread_id (b->owner->thread))
 	continue;
 
-      /* FIXME drow/2003-10-07: This code should be pushed elsewhere when
-	 hardware watchpoints are split into multiple loc breakpoints.  */
-      if ((b->loc_type == bp_loc_hardware_watchpoint
-	   || b->owner->type == bp_watchpoint) && !b->owner->val)
-	{
-	  struct value *val;
-	  val = evaluate_expression (b->owner->exp);
-	  release_value (val);
-	  if (value_lazy (val))
-	    value_fetch_lazy (val);
-	  b->owner->val = val;
-	}
-
       val = insert_bp_location (b, tmp_error_stream,
 				    &disabled_breaks, &process_warning,
 				    &hw_breakpoint_error);
@@ -1223,6 +1256,39 @@ insert_breakpoints (void)
 	error = val;
     }
 
+  /* If we failed to insert all locations of a watchpoint,
+     remove them, as half-inserted watchpoint is of limited use.  */
+  ALL_BREAKPOINTS (bpt)  
+    {
+      int some_failed = 0;
+      struct bp_location *loc;
+
+      if (!is_hardware_watchpoint (bpt))
+	continue;
+
+      if (bpt->enable_state != bp_enabled)
+	continue;
+      
+      for (loc = bpt->loc; loc; loc = loc->next)
+	if (!loc->inserted)
+	  {
+	    some_failed = 1;
+	    break;
+	  }
+      if (some_failed)
+	{
+	  for (loc = bpt->loc; loc; loc = loc->next)
+	    if (loc->inserted)
+	      remove_breakpoint (loc, mark_uninserted);
+
+	  hw_breakpoint_error = 1;
+	  fprintf_unfiltered (tmp_error_stream,
+			      "Could not insert hardware watchpoint %d.\n", 
+			      bpt->number);
+	  error = -1;
+	}
+    }
+
   if (error)
     {
       /* If a hardware breakpoint or watchpoint was inserted, add a
@@ -1508,46 +1574,15 @@ remove_breakpoint (struct bp_location *b, insertion_state_t is)
 	return val;
       b->inserted = (is == mark_inserted);
     }
-  else if (b->loc_type == bp_loc_hardware_watchpoint
-	   && breakpoint_enabled (b->owner)
-	   && !b->duplicate)
+  else if (b->loc_type == bp_loc_hardware_watchpoint)
     {
       struct value *v;
       struct value *n;
 
       b->inserted = (is == mark_inserted);
-      /* Walk down the saved value chain.  */
-      for (v = b->owner->val_chain; v; v = value_next (v))
-	{
-	  /* For each memory reference remove the watchpoint
-	     at that address.  */
-	  if (VALUE_LVAL (v) == lval_memory
-	      && ! value_lazy (v))
-	    {
-	      struct type *vtype = check_typedef (value_type (v));
-
-	      if (v == b->owner->val_chain
-		  || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
-		      && TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
-		{
-		  CORE_ADDR addr;
-		  int len, type;
+      val = target_remove_watchpoint (b->address, b->length, 
+				      b->watchpoint_type);
 
-		  addr = VALUE_ADDRESS (v) + value_offset (v);
-		  len = TYPE_LENGTH (value_type (v));
-		  type   = hw_write;
-		  if (b->owner->type == bp_read_watchpoint)
-		    type = hw_read;
-		  else if (b->owner->type == bp_access_watchpoint)
-		    type = hw_access;
-
-		  val = target_remove_watchpoint (addr, len, type);
-		  if (val == -1)
-		    b->inserted = 1;
-		  val = 0;
-		}
-	    }
-	}
       /* Failure to remove any of the hardware watchpoints comes here.  */
       if ((is == mark_uninserted) && (b->inserted))
 	warning (_("Could not remove hardware watchpoint %d."),
@@ -2451,33 +2486,19 @@ watchpoints_triggered (struct target_waitstatus *ws)
 	|| b->type == bp_read_watchpoint
 	|| b->type == bp_access_watchpoint)
       {
+	struct bp_location *loc;
 	struct value *v;
 
 	b->watchpoint_triggered = watch_triggered_no;
-	for (v = b->val_chain; v; v = value_next (v))
-	  {
-	    if (VALUE_LVAL (v) == lval_memory && ! value_lazy (v))
-	      {
-		struct type *vtype = check_typedef (value_type (v));
-
-		if (v == b->val_chain
-		    || (TYPE_CODE (vtype) != TYPE_CODE_STRUCT
-			&& TYPE_CODE (vtype) != TYPE_CODE_ARRAY))
-		  {
-		    CORE_ADDR vaddr;
-
-		    vaddr = VALUE_ADDRESS (v) + value_offset (v);
-		    /* Exact match not required.  Within range is
-		       sufficient.  */
-		    if (addr >= vaddr
-			&& addr < vaddr + TYPE_LENGTH (value_type (v)))
-		      {
-			b->watchpoint_triggered = watch_triggered_yes;
-			break;
-		      }
-		  }
-	      }
-	  }
+	for (loc = b->loc; loc; loc = loc->next)
+	  /* Exact match not required.  Within range is
+	     sufficient.  */
+	  if (addr >= loc->address
+	      && addr < loc->address + loc->length)
+	    {
+	      b->watchpoint_triggered = watch_triggered_yes;
+	      break;
+	    }
       }
 
   return 1;
@@ -2716,6 +2737,15 @@ bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid)
 	&& !inferior_has_execd (PIDGET (inferior_ptid), &b->exec_pathname))
       continue;
 
+    /* For hardware watchpoints, we look only at the first location.
+       The watchpoint_check function will work on entire expression,
+       not the individual locations.  For read watchopints, the
+       watchpoints_triggered function have checked all locations
+       alrea
+     */
+    if (b->type == bp_hardware_watchpoint && bl != b->loc)
+      continue;
+
     /* Come here if it's a watchpoint, or if the break address matches */
 
     bs = bpstat_alloc (bl, bs);	/* Alloc a bpstat to explain stop */
@@ -2909,6 +2939,10 @@ bpstat_stop_status (CORE_ADDR bp_addr, ptid_t ptid)
 	      || bs->breakpoint_at->owner->type == bp_read_watchpoint
 	      || bs->breakpoint_at->owner->type == bp_access_watchpoint))
 	{
+	  /* remove/insert can invalidate bs->breakpoint_at, if this
+	     location is no longer used by the watchpoint.  Prevent
+	     further code from trying to use it.  */
+	  bs->breakpoint_at = NULL;
 	  remove_breakpoints ();
 	  insert_breakpoints ();
 	  break;
@@ -3629,10 +3663,14 @@ print_one_breakpoint (struct breakpoint *b,
 	 disabled, we print it as if it had
 	 several locations, since otherwise it's hard to
 	 represent "breakpoint enabled, location disabled"
-	 situation.  */	 
+	 situation.  
+	 Note that while hardware watchpoints have
+	 several locations internally, that's no a property
+	 exposed to user.  */
       if (b->loc 
+	  && !is_hardware_watchpoint (b)
 	  && (b->loc->next || !b->loc->enabled)
-	  && !ui_out_is_mi_like_p (uiout))
+	  && !ui_out_is_mi_like_p (uiout)) 
 	{
 	  struct bp_location *loc;
 	  int n = 1;
@@ -6829,9 +6867,7 @@ delete_breakpoint (struct breakpoint *bpt)
     {
       if (loc->inserted)
 	remove_breakpoint (loc, mark_inserted);
-
-      free_valchain (loc);
-
+      
       if (loc->cond)
 	xfree (loc->cond);
 
@@ -7265,39 +7301,7 @@ breakpoint_re_set_one (void *bint)
 	 
 	 Don't do anything about disabled watchpoints, since they will
 	 be reevaluated again when enabled.  */
-
-      if (!breakpoint_enabled (b))
-	break;
-
-      if (b->exp_valid_block == NULL
-	  || frame_find_by_id (b->watchpoint_frame) != NULL)
-	{
-	  if (b->exp)
-	    {
-	      xfree (b->exp);
-	      b->exp = NULL;
-	    }
-	  s = b->exp_string;
-	  b->exp = parse_exp_1 (&s, b->exp_valid_block, 0);
-
-	  /* Since we reparsed expression, we need to update the
-	     value.  I'm not aware of any way a single solib load or unload
-	     can change a valid value into different valid value, but:
-	     - even if the value is no longer valid, we have to record
-	     this fact, so that when it becomes valid we reports this
-	     as value change
-	     - unloaded followed by load can change the value for sure.
-
-   	     We set value to NULL, and insert_breakpoints will 
-	     update the value.  */
-	  if (b->val)
-	    value_free (b->val);
-	  b->val = NULL;
-
-	  /* Loading of new shared library change the meaning of
-	     watchpoint condition.  However, insert_bp_location will
-	     recompute watchpoint condition anyway, nothing to do here.  */
-	}
+      update_watchpoint (b, 1 /* reparse */);
       break;
       /* We needn't really do anything to reset these, since the mask
          that requests them is unaffected by e.g., new libraries being
diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
index 91667ab..7b72e19 100644
--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -273,6 +273,12 @@ struct bp_location
      bp_loc_other.  */
   CORE_ADDR address;
 
+  /* For hardware watchpoints, the size of data ad ADDRESS being watches.  */
+  int length;
+
+  /* Type of hardware watchpoint. */
+  enum target_hw_bp_type watchpoint_type;
+
   /* For any breakpoint type with an address, this is the BFD section
      associated with the address.  Used primarily for overlay debugging.  */
   asection *section;
@@ -388,9 +394,6 @@ struct breakpoint
     /* Value of the watchpoint the last time we checked it.  */
     struct value *val;
 
-    /* Holds the value chain for a hardware watchpoint expression.  */
-    struct value *val_chain;
-
     /* Holds the address of the related watchpoint_scope breakpoint
        when using watchpoints on local variables (might the concept
        of a related breakpoint be useful elsewhere, if not just call
diff --git a/gdb/testsuite/gdb.base/watchpoint-solib-shr.c b/gdb/testsuite/gdb.base/watchpoint-solib-shr.c
new file mode 100644
index 0000000..699f559
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-solib-shr.c
@@ -0,0 +1,25 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2004, 2007 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+
+int g = 0;
+
+void foo (int i)
+{
+  g = i;
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint-solib.c b/gdb/testsuite/gdb.base/watchpoint-solib.c
new file mode 100644
index 0000000..b316024
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-solib.c
@@ -0,0 +1,68 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2004, 2007 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>. */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+#ifdef __WIN32__
+#include <windows.h>
+#define dlopen(name, mode) LoadLibrary (TEXT (name))
+#ifdef _WIN32_WCE
+# define dlsym(handle, func) GetProcAddress (handle, TEXT (func))
+#else
+# define dlsym(handle, func) GetProcAddress (handle, func)
+#endif
+#define dlclose(handle) FreeLibrary (handle)
+#define dlerror() "error %d occurred", GetLastError ()
+#else
+#include <dlfcn.h>
+#endif
+
+
+void open_shlib ()
+{
+  void *handle;
+  void (*foo) (int);
+
+  handle = dlopen (SHLIB_NAME, RTLD_LAZY);
+  
+  if (!handle)
+    {
+      fprintf (stderr, dlerror ());
+      exit (1);
+    }
+
+  foo = (void (*)(int))dlsym (handle, "foo");
+
+  if (!foo)
+    {
+      fprintf (stderr, dlerror ());
+      exit (1);
+    }
+
+  foo (1); // call to foo
+  foo (2);
+
+  dlclose (handle);
+}
+
+
+int main()
+{
+  open_shlib ();
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.base/watchpoint-solib.exp b/gdb/testsuite/gdb.base/watchpoint-solib.exp
new file mode 100644
index 0000000..fb5d886
--- /dev/null
+++ b/gdb/testsuite/gdb.base/watchpoint-solib.exp
@@ -0,0 +1,89 @@
+#   Copyright 2007 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if $tracelevel then {
+    strace $tracelevel
+}
+
+#
+# test running programs
+#
+set prms_id 0
+set bug_id 0
+
+if {[skip_shlib_tests]} {
+    return 0
+}
+
+# TODO: Use LoadLibrary on this target instead of dlopen.
+if {[istarget arm*-*-symbianelf*]} {
+    return 0
+}
+
+set testfile "watchpoint-solib"
+set libfile "watchpoint-solib-shr"
+set libname "${libfile}.sl"
+set libsrcfile ${libfile}.c
+set srcfile $srcdir/$subdir/$testfile.c
+set binfile $objdir/$subdir/$testfile
+set shlibdir ${objdir}/${subdir}
+set libsrc  $srcdir/$subdir/$libfile.c
+set lib_sl  $objdir/$subdir/$libname
+
+if [get_compiler_info ${binfile}] {
+    return -1
+}
+
+set lib_opts debug
+set exec_opts [list debug shlib_load additional_flags=-DSHLIB_NAME\=\"${libname}\"]
+
+if { [gdb_compile_shlib $libsrc $lib_sl $lib_opts] != ""
+     || [gdb_compile $srcfile $binfile executable $exec_opts] != ""} {
+    untested "Couldn't compile $libsrc or $srcfile."
+    return -1
+}
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+gdb_load_shlibs $lib_sl
+
+if [target_info exists gdb_stub] {
+    gdb_step_for_stub;
+}
+
+runto_main
+
+
+gdb_test_multiple "break foo" "set pending breakpoint" {
+     -re ".*Make breakpoint pending.*y or \\\[n\\\]. $" {
+	    gdb_test "y" "Breakpoint.*foo.*pending." "set pending breakpoint"
+     }
+}
+
+gdb_test "continue" ".*Breakpoint 2.*foo.*" "continue to foo"
+gdb_test "watch g" "Hardware watchpoint 3: g" "set watchpoint on g"
+gdb_test "continue" ".*New value = 1.*" "continue to watchpoint hit"
+rerun_to_main
+gdb_test "continue" ".*Breakpoint 2.*foo.*" "continue to foo again"
+gdb_test "continue" ".*New value = 1.*" "continue to watchpoint hit again"
+
+
+
+
+
-- 
1.5.3.5

Re: [RFA] Don't reset watchpoint block on solib load.

[Daniel Jacobowitz]

On Wed, Jan 23, 2008 at 12:57:27PM +0300, Vladimir Prus wrote:
> So, I guess the solutions are either:
> 
> 1. Omit the move of condition parsing from this patch, or
> 2. Keep condition parsing in this patch, and then commit
> the use-multiple-locations-for-watchpoint patch which will
> immediately make condition only reparsed when breakpoint_re_set_one
> is called.
> 
> Given that use-multiple-locations-for-watchpoint patch is
> approved except for the bit that makes reparsing of condition
> optional, it seems that (2) is less work.

All this sounds fine to me.  I'm OK with this patch, which I believe
is the one here:
  http://sourceware.org/ml/gdb-patches/2007-11/msg00522.html

> For convenience, I attach the current version of 
> use-multiple-locations-for-watchpoint patch. The difference
> to the previous revision is as follows:
> 
> 
> @@ -966,7 +966,7 @@ update_watchpoint (struct breakpoint *b, int reparse)
>             value_free (v);
>         }
> 
> -      if (b->cond_string != NULL)
> +      if (reparse && b->cond_string != NULL)
>         {
>           char *s = b->cond_string;
>           if (b->loc->cond)
> 
> 
> Dan, is the patch still OK to commit?

Yes, it is.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Don't reset watchpoint block on solib load.

[Vladimir Prus]

On Wednesday 28 November 2007 02:00:15 Jim Blandy wrote:

> We also call breakpoint_re_set_one when we've unloaded a shared
> library.  

Can you point me at a codepath that would cause breakpoint_re_set_one
to be called when a shared library is *unloaded*? I've poked at
this for a while, and can't see it.

- Volodya

Re: [RFA] Don't reset watchpoint block on solib load.

[Jim Blandy]

Vladimir Prus <vladimir at codesourcery.com> writes:
> On Wednesday 28 November 2007 02:00:15 Jim Blandy wrote:
>
>> We also call breakpoint_re_set_one when we've unloaded a shared
>> library.  
>
> Can you point me at a codepath that would cause breakpoint_re_set_one
> to be called when a shared library is *unloaded*? I've poked at
> this for a while, and can't see it.

I beg your pardon.  It does not.  This is a pretty serious bug.  A
dlclose will call free_objfile:

handle_inferior_event
-> solib_add
   -> update_solib_list
      -> free_objfile

but it takes no steps to clear references to the freed shared
library's symbols.  I had assumed that it must be calling
clear_symtab_users.  We need to call clear_symtab_users whenever any
objfile is freed.

Now, where to do this...

Re: [RFA] Don't reset watchpoint block on solib load.

[Vladimir Prus]

On Thursday 29 November 2007 21:39:22 you wrote:
> 
> Vladimir Prus <vladimir at codesourcery.com> writes:
> > On Wednesday 28 November 2007 02:00:15 Jim Blandy wrote:
> >
> >> We also call breakpoint_re_set_one when we've unloaded a shared
> >> library.  
> >
> > Can you point me at a codepath that would cause breakpoint_re_set_one
> > to be called when a shared library is *unloaded*? I've poked at
> > this for a while, and can't see it.
> 
> I beg your pardon.  It does not.  

Excellent, at least I did not miss anything.

> This is a pretty serious bug.  A 
> dlclose will call free_objfile:
> 
> handle_inferior_event
> -> solib_add
>    -> update_solib_list
>       -> free_objfile
> 
> but it takes no steps to clear references to the freed shared
> library's symbols.  I had assumed that it must be calling
> clear_symtab_users.  We need to call clear_symtab_users whenever any
> objfile is freed.
> 
> Now, where to do this...

The precedent now is varobj_invalidate, called from clear_symbtab_users.
The other precedent is preserve_values, called from free_objfile.

I actually have a patch in works, that would make free_objfile call a function
in breakpoints.c that will handle breakpoint referring to the unloaded solib.
But I'd rather not mix this thing into an already big and scary patch ;-)

- Volodya