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

[Vladimir Prus <vladimir@codesourcery.com>]

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