This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Don't reset watchpoint block on solib load.
- From: Jim Blandy <jimb at codesourcery dot com>
- To: Vladimir Prus <vladimir at codesourcery dot com>
- Cc: gdb-patches at sources dot redhat dot com
- Date: Tue, 27 Nov 2007 15:00:15 -0800
- Subject: Re: [RFA] Don't reset watchpoint block on solib load.
- References: <200711202013.47537.vladimir@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.