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

[Jim Blandy <jimb at codesourcery dot com>]

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.