Re: [PATCH 07/12] gdb: link breakpoint locations with intrusive_list

[Simon Marchi <simon.marchi@efficios.com>]

>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>> index e9b6574bba77..6ec0f6a11e88 100644
>> --- a/gdb/breakpoint.c
>> +++ b/gdb/breakpoint.c
>> @@ -1049,13 +1049,18 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
>>  	     the error and the condition string will be rejected.
>>  	     This two-pass approach is taken to avoid setting the
>>  	     state of locations in case of a reject.  */
>> -	  for (bp_location *loc : b->locations ())
>> +	  auto bp_loc_range = b->locations ();
>> +	  for (auto bp_loc_it = bp_loc_range.begin ();
>> +	       bp_loc_it != bp_loc_range.end ();
>> +	       ++bp_loc_it)
>>  	    {
>> +	      bp_location &loc = **bp_loc_it;
>> +
> 
> You could switch this to:
> 
>   for (const bp_location *loc : b->locations ())
> 
> if we could also do .... (continued below)
> 
>>  	      try
>>  		{
>>  		  const char *arg = exp;
>> -		  parse_exp_1 (&arg, loc->address,
>> -			       block_for_pc (loc->address), 0);
>> +		  parse_exp_1 (&arg, loc.address,
>> +			       block_for_pc (loc.address), 0);
>>  		  if (*arg != 0)
>>  		    error (_("Junk at end of expression"));
>>  		  break;
>> @@ -1065,7 +1070,7 @@ set_breakpoint_condition (struct breakpoint *b, const char *exp,
>>  		  /* Condition string is invalid.  If this happens to
>>  		     be the last loc, abandon (if not forced) or continue
>>  		     (if forced).  */
>> -		  if (loc->next == nullptr && !force)
>> +		  if (std::next (bp_loc_it) == bp_loc_range.end () && !force)
> 
> ... this:
> 
>   if (loc == b->last_loc ())
>     throw
> 
> This requires adding 'breakpoint::last_loc ()' but we already have
> 'breakpoint::first_loc ()' so that doesn't seem crazy, and I think the
> resulting code would be easier to read maybe?
Yeah, that's a good idea.  It makes the code read just like the comment
does (if the loc is the last loc), which is a plus for readability.

Note that I used &b->last_loc (), as I made last_loc return a reference
(like first_loc).

>> @@ -4518,28 +4510,23 @@ bpstat_locno (const bpstat *bs)
>>    const struct breakpoint *b = bs->breakpoint_at;
>>    const struct bp_location *bl = bs->bp_location_at.get ();
>>  
>> -  int locno = 0;
>> -
>>    if (b != nullptr && b->has_multiple_locations ())
>>      {
>> -      const bp_location *bl_i;
>> -
>> -      for (bl_i = b->loc;
>> -	   bl_i != bl && bl_i->next != nullptr;
>> -	   bl_i = bl_i->next)
>> -	locno++;
>> +      int locno = 1;
>>  
>> -      if (bl_i == bl)
>> -	locno++;
>> -      else
>> +      for (bp_location *loc : b->locations ())
>>  	{
>> -	  warning (_("location number not found for breakpoint %d address %s."),
>> -		   b->number, paddress (bl->gdbarch, bl->address));
>> -	  locno = 0;
>> +	  if (bl == loc)
>> +	    return locno;
>> +
>> +	  ++locno;
>>  	}
>> +
>> +      warning (_("location number not found for breakpoint %d address %s."),
>> +	       b->number, paddress (bl->gdbarch, bl->address));
>>      }
> 
> I might be missing something here, but could this code be simplified by
> using std::distance in some way?  We already use std::distance later in
> this patch to count the total number of locations, so it feels like this
> should work.
> 
> I know that the current code allows for the possibility that the
> location (from the bp_stat) might not be in the location list of the
> breakpoint (from bp_stat), but, surely that can't happen, right?  Surely
> the location was originally found by looking through the locations of
> the breakpoint... so it should be certain that we find the location.
> 
> I guess maybe we could remove the breakpoint_at from bp_stat, and just
> use the owner of the bp_location_at variable, that way we could be
> certain about all this...
> 
> Either way, there's nothing wrong with your code above, just wondering
> if we can make it tighter...

I'm not sure about whether we are sure that the location is in its
parent location list, there is perhaps some corner cases about handling
a breakpoint hit event on a location that has been removed since.
However, I see that before putting a location in the moribund_locations
vector, we set its owner to NULL.  So if the owner field is non-NULL, it
should be safe to assume the location is in the list.

I don't think it's easy to use std::distance right now though, because
with the reference_to_pointer_iterator wrapper, we can't go from item to
iterator in O(1).  We can if we had access to the intrusive_list
directly, since it has an "iterator_to" method.

In any case, I will not make such a change in this patch to avoid mixing
things together.  I tried to make this patch a straightforward
translation without any unnecessary algorithmic changes.  I'll check if
it becomes possible at the end of the series though.

>> @@ -8312,12 +8299,8 @@ code_breakpoint::add_location (const symtab_and_line &sal)
>>  						sal.pspace);
>>  
>>    /* Sort the locations by their ADDRESS.  */
>> -  new_loc = allocate_location ();
>> -  for (tmp = &(loc); *tmp != NULL && (*tmp)->address <= adjusted_address;
>> -       tmp = &((*tmp)->next))
>> -    ;
>> -  new_loc->next = *tmp;
>> -  *tmp = new_loc;
>> +  bp_location *new_loc = this->allocate_location ();
>> +  breakpoint::add_location (*new_loc, adjusted_address);
> 
> I notice here the code basically does:
> 
>   1. Create bp_location object,
>   2. Add bp_location object to list,
>   3. Initialise bp_location object.
> 
> We have to pass adjusted_address through because the address of new_loc
> has not yet been initialised.
> 
> Could we reorder things so we:
> 
>   1. Create bp_location object,
>   2. Initialise bp_location object,
>   3. Add bp_location object to list.
> 
> The benefit I think would be that we can remove these two functions:
> 
>   void breakpoint::add_location (bp_location &loc);
>   void breakpoint::add_location (bp_location &loc, CORE_ADDR address);
> 
> And we replace them with one function:
> 
>   void breakpoint::add_location (bp_location &loc, bool sorted = false);
> 
> The implementation would be free to use 'loc.address' for the sorting.

Good idea, I will do that.  That seems like a minor / low-risk change,
so I'll do it in this patch.

For consistency, I moved the "add_location" call after setting
loc->address in update_watchpoint as well, even though it wouldn't
matter in that case.

>> @@ -633,30 +630,71 @@ struct breakpoint
>>    /* Allocate a location for this breakpoint.  */
>>    virtual struct bp_location *allocate_location ();
>>  
>> +  /* Return a range of this breakpoint's locations.  */
>> +  bp_location_range locations () const;
>> +
>> +  /* Add LOC at the end of the location list of this breakpoint.
>> +
>> +     LOC must have this breakpoint as its owner.  LOC must not already be linked
>> +     in a location list.  */
>> +  void add_location (bp_location &loc);
>> +
>> +  /* Add LOC in the location list of this breakpoint, sorted by address.
>> +
>> +     LOC must have this breakpoint as its owner.  LOC must not already be linked
>> +     in a location list.  */
>> +  void add_location (bp_location &loc, CORE_ADDR address);
> 
> It would be nice for the comment to mention what ADDRESS means here.
> However, my earlier comments for where this function is used might mean
> this is no longer needed in this form.

Yes, this gets removed.

Thanks,

Simon