Re: [patch 1/3] Clear stale specific locs, not whole bpts [rediff]

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Sun, 23 May 2010 23:40:17 +0200, Pedro Alves wrote:
> On Monday 17 May 2010 22:22:43, Jan Kratochvil wrote:
> >  static void free_bp_location (struct bp_location *loc)
> >  {
> > +  iterate_over_threads (bpstat_remove_bp_location_callback, loc);
> 
> I think this isn't the best place for this, considering moribund
> breakpoint locations --- it's too late.  When you delete a
> breakpoint in non-stop/always-inserted modes, you'll delete
> the breakpoint, but deleting the bp_location itself is deferred.
> This means that in that case, the thread's bpstat chains may still
> contain references to bp_locations that are now in the moribund list
> (those will be still valid objects, not xfree'd yet), but their owner
> breakpoint is gone, meaning bpstat walks can still dereference those
> now invalid bp_location->owner pointers.
      ^^^^^^^
Not "invalid" but they are explicitly NULL:
static void
update_global_location_list (int should_insert)
              old_loc->owner = NULL;
              VEC_safe_push (bp_location_p, moribund_locations, old_loc);

And the code expects such state with valid bp_location but NULL its ->owner.
struct bpstat_what
bpstat_what (bpstat bs)
      if (bs->breakpoint_at->owner == NULL)
        bs_class = bp_nostop;

Other bs->breakpoint_at->owner dereferencing code either checks it is NULL or
such code cannot meet bpstat referencing moribund bp_location.


> Instead, can you move this to within in the `if (!found_object)' branch
> of update_global_location_list?  That is, move it close to where we
> detected we had unlinked the bp_location from the global
> location list?

While I can do the simple move in next mail do you still request it?

I try to follow the paradigm to always remove references exactly at the point
one of the two peers dies.  That makes the code both simple and foolproof for
future modifications even by such programmers like me.

Therefore if something is referencing bp_location then the reference should
die at the point bp_location dies and that's all.


Thanks,
Jan


2010-06-04  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* breakpoint.h (owner): Extend the comment.

--- a/gdb/breakpoint.h
+++ b/gdb/breakpoint.h
@@ -244,7 +244,8 @@ struct bp_location
 
   /* Each breakpoint location must belong to exactly one higher-level
      breakpoint.  This and the DUPLICATE flag are more straightforward
-     than reference counting.  */
+     than reference counting.  This pointer is NULL iff this bp_location is in
+     (and therefore only in) moribund_locations.  */
   struct breakpoint *owner;
 
   /* Conditional.  Break only if this expression's value is nonzero.