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

[Vladimir Prus <vladimir@codesourcery.com>]

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