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

[Jim Blandy <jimb@codesourcery.com>]

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.