This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] Try 3: Use multiple locations for hardware watchpoints.


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]