This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA] Try 3: Use multiple locations for hardware watchpoints.
- From: Vladimir Prus <vladimir at codesourcery dot com>
- To: Jim Blandy <jimb at codesourcery dot com>
- Cc: gdb-patches at sources dot redhat dot com
- Date: Thu, 17 Jan 2008 17:02:46 +0300
- Subject: Re: [RFA] Try 3: Use multiple locations for hardware watchpoints.
- References: <200712011412.34207.vladimir@codesourcery.com> <m3wsq9yzwg.fsf@codesourcery.com>
On Thursday 17 January 2008 05:00:31 Jim Blandy wrote:
>
> Vladimir Prus <vladimir at codesourcery.com> writes:
> > I've noticed that my previous patch to use multiple locations
> > for watchpoint causes (non-deterministic) regression on watchthreads.exp,
> > caused by the fact that we always updated the 'val' field on
> > watchpoint, and it's possible that if two threads are stopped on
> > different watchpoints, we'd update the val field of the second watchpoint
> > while processing the hit of the first one, and then when we get to
> > the second watchpoint, we'd decide that the value has not changed.
> > I think this revision if really final. OK?
> >
> > - Volodya
> >
> > This eliminates the need to traverse value chain, doing
> > various checks, in three different places.
> >
> > * breakpoint.h (struct bp_location): New fields
> > lengths and watchpoint_type.
> > (struct breakpoint): Remove the val_chain field.
> > * breakpoint.c (is_hardware_watchpoint): New.
> > (free_valchain): Remove.
> > (update_watchpoint): New.
> > (insert_bp_location): For hardware watchpoint, just
> > directly insert it.
> > (insert_breakpoints): Call update_watchpoint_locations
> > on all watchpoints. If we have failed to insert
> > any location of a hardware watchpoint, remove all inserted
> > locations.
> > (remove_breakpoint): For hardware watchpoints, directly
> > remove location.
> > (watchpoints_triggered): Iterate over locations.
> > (bpstat_stop_status): Use only first location of
> > a resource watchpoint.
> > (delete_breakpoint): Don't call free_valchain.
> > (print_one_breakpoint): Don't print all
> > locations for watchpoints.
> > (breakpoint_re_set_one): Use update_watchpoint for
> > watchpoints.
> >
> > testsuite/
> > * gdb.base/watchpoint-solib.exp: New.
> > * gdb.base/watchpoint-solib.c: New.
> > * gdb.base/watchpoint-solib-shr.c: New.
>
> I have a question about this patch:
>
> insert_breakpoints calls update_watchpoint, which calls
> frame_find_by_id, which only searches the currently selected thread's
> stack. But it seems to me that insert_breakpoints gets called from
> many different contexts, and there's no reason to assume that the
> thread that was selected when the watchpoint was set is still
> current. If it's not, then within_current_scope will be (incorrectly)
> set to zero, and the watchpoint will be deleted as invalid.
>
> Is this a pre-existing problem? (I could try this myself, but I have
> to stop work in a bit, so I wanted to just get the question out
> there.) I imagine the breakpoint should record the ptid along with
> watchpoint_frame, and then update_watchpoint should use something like
> make_cleanup_restore_current_thread to save and restore the thread and
> frame while re-evaluating the expression.
I think it's the pre-existing problem. We appear to have exact same
problem with variable object -- where a variable object is associated
with a specific frame, it should also be associated with specific thread,
as frames are only specific to a given thread, and might not even exist
until we've got backtrace for a thread.
- Volodya