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

[Vladimir Prus <vladimir at codesourcery dot com>]

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