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

[Vladimir Prus <vladimir at codesourcery dot com>]

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