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

[Andrew Burgess <aburgess@redhat.com>]

Simon Marchi <simon.marchi@efficios.com> writes:

>>> 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.

Not a problem, I did wonder if there was going to be some reason why
this wasn't a helpful suggestion :)

Thanks for looking into it though.

Andrew

>
>>> @@ -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