[RFA] Use std::vector for moribund_locations

  This changes moribund_locations to use a std::vector rather than VEC.
I also noticed that moribund_locations is only used in breakpoint.c,
so now it is static as well.

It might be possible to make this code a bit simpler by using a
ref_ptr in moribund_locations; however, I have not done this.

Tested by the buildbot.

gdb/ChangeLog
2018-06-05  Tom Tromey  <tom@tromey.com>

	* breakpoint.c (moribund_locations): Now static and a
	std::vector.
	(breakpoint_init_inferior, moribund_breakpoint_here_p)
	(build_bpstat_chain, update_global_location_list)
	(breakpoint_retire_moribund): Update.
	* breakpoint.h (bp_location_p): Remove typedef.  Don't declare
	VEC.
---
 gdb/ChangeLog    | 10 ++++++++++
 gdb/breakpoint.c | 42 ++++++++++++++++++------------------------
 gdb/breakpoint.h |  3 ---
 3 files changed, 28 insertions(+), 27 deletions(-)
  

* Tom Tromey <tom@tromey.com> [2018-06-05 13:23:46 -0600]:

> This changes moribund_locations to use a std::vector rather than VEC.
> I also noticed that moribund_locations is only used in breakpoint.c,
> so now it is static as well.
> 
> It might be possible to make this code a bit simpler by using a
> ref_ptr in moribund_locations; however, I have not done this.
> 
> Tested by the buildbot.
> 
> gdb/ChangeLog
> 2018-06-05  Tom Tromey  <tom@tromey.com>
> 
> 	* breakpoint.c (moribund_locations): Now static and a
> 	std::vector.
> 	(breakpoint_init_inferior, moribund_breakpoint_here_p)
> 	(build_bpstat_chain, update_global_location_list)
> 	(breakpoint_retire_moribund): Update.
> 	* breakpoint.h (bp_location_p): Remove typedef.  Don't declare
> 	VEC.

FWIW this looks fine to me.

Thanks,
Andrew


> ---
>  gdb/ChangeLog    | 10 ++++++++++
>  gdb/breakpoint.c | 42 ++++++++++++++++++------------------------
>  gdb/breakpoint.h |  3 ---
>  3 files changed, 28 insertions(+), 27 deletions(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 00c538e989e..36d1a6735e6 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -560,7 +560,7 @@ static CORE_ADDR bp_locations_shadow_len_after_address_max;
>  /* The locations that no longer correspond to any breakpoint, unlinked
>     from the bp_locations array, but for which a hit may still be
>     reported by a target.  */
> -VEC(bp_location_p) *moribund_locations = NULL;
> +static std::vector<bp_location *> moribund_locations;
>  
>  /* Number of last breakpoint made.  */
>  
> @@ -3860,8 +3860,6 @@ void
>  breakpoint_init_inferior (enum inf_context context)
>  {
>    struct breakpoint *b, *b_tmp;
> -  struct bp_location *bl;
> -  int ix;
>    struct program_space *pspace = current_program_space;
>  
>    /* If breakpoint locations are shared across processes, then there's
> @@ -3951,9 +3949,9 @@ breakpoint_init_inferior (enum inf_context context)
>    }
>  
>    /* Get rid of the moribund locations.  */
> -  for (ix = 0; VEC_iterate (bp_location_p, moribund_locations, ix, bl); ++ix)
> +  for (bp_location *bl : moribund_locations)
>      decref_bp_location (&bl);
> -  VEC_free (bp_location_p, moribund_locations);
> +  moribund_locations.clear ();
>  }
>  
>  /* These functions concern about actual breakpoints inserted in the
> @@ -4041,10 +4039,7 @@ breakpoint_in_range_p (const address_space *aspace,
>  int
>  moribund_breakpoint_here_p (const address_space *aspace, CORE_ADDR pc)
>  {
> -  struct bp_location *loc;
> -  int ix;
> -
> -  for (ix = 0; VEC_iterate (bp_location_p, moribund_locations, ix, loc); ++ix)
> +  for (bp_location *loc : moribund_locations)
>      if (breakpoint_location_address_match (loc, aspace, pc))
>        return 1;
>  
> @@ -5362,10 +5357,7 @@ build_bpstat_chain (const address_space *aspace, CORE_ADDR bp_addr,
>    if (!target_supports_stopped_by_sw_breakpoint ()
>        || !target_supports_stopped_by_hw_breakpoint ())
>      {
> -      bp_location *loc;
> -
> -      for (int ix = 0;
> -	   VEC_iterate (bp_location_p, moribund_locations, ix, loc); ++ix)
> +      for (bp_location *loc : moribund_locations)
>  	{
>  	  if (breakpoint_location_address_match (loc, aspace, bp_addr)
>  	      && need_moribund_for_location_type (loc))
> @@ -12009,7 +12001,7 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>  	      old_loc->events_till_retirement = 3 * (thread_count () + 1);
>  	      old_loc->owner = NULL;
>  
> -	      VEC_safe_push (bp_location_p, moribund_locations, old_loc);
> +	      moribund_locations.push_back (old_loc);
>  	    }
>  	  else
>  	    {
> @@ -12112,16 +12104,18 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>  void
>  breakpoint_retire_moribund (void)
>  {
> -  struct bp_location *loc;
> -  int ix;
> -
> -  for (ix = 0; VEC_iterate (bp_location_p, moribund_locations, ix, loc); ++ix)
> -    if (--(loc->events_till_retirement) == 0)
> -      {
> -	decref_bp_location (&loc);
> -	VEC_unordered_remove (bp_location_p, moribund_locations, ix);
> -	--ix;
> -      }
> +  auto it = std::remove_if (moribund_locations.begin (),
> +			    moribund_locations.end (),
> +			    [] (bp_location *loc)
> +			    {
> +			      if (--(loc->events_till_retirement) == 0)
> +				{
> +				  decref_bp_location (&loc);
> +				  return true;
> +				}
> +			      return false;
> +			    });
> +  moribund_locations.erase (it, moribund_locations.end ());
>  }
>  
>  static void
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 4223158fbc0..6c9326fb8a9 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -666,9 +666,6 @@ enum watchpoint_triggered
>    watch_triggered_yes  
>  };
>  
> -typedef struct bp_location *bp_location_p;
> -DEF_VEC_P(bp_location_p);
> -
>  /* Some targets (e.g., embedded PowerPC) need two debug registers to set
>     a watchpoint over a memory region.  If this flag is true, GDB will use
>     only one register per watchpoint, thus assuming that all acesses that
> -- 
> 2.13.6
>
  

On 2018-06-05 03:23 PM, Tom Tromey wrote:
> @@ -12112,16 +12104,18 @@ update_global_location_list (enum ugll_insert_mode insert_mode)
>  void
>  breakpoint_retire_moribund (void)
>  {
> -  struct bp_location *loc;
> -  int ix;
> -
> -  for (ix = 0; VEC_iterate (bp_location_p, moribund_locations, ix, loc); ++ix)
> -    if (--(loc->events_till_retirement) == 0)
> -      {
> -	decref_bp_location (&loc);
> -	VEC_unordered_remove (bp_location_p, moribund_locations, ix);
> -	--ix;
> -      }
> +  auto it = std::remove_if (moribund_locations.begin (),
> +			    moribund_locations.end (),
> +			    [] (bp_location *loc)
> +			    {
> +			      if (--(loc->events_till_retirement) == 0)
> +				{
> +				  decref_bp_location (&loc);
> +				  return true;
> +				}
> +			      return false;
> +			    });
> +  moribund_locations.erase (it, moribund_locations.end ());
>  }

Just a note that this changes an "unordered remove" to an "ordered remove".
If we don't need to keep the relative order of the remaining elements, it
might be good performance-wise to keep the original behavior.  This could
be done by keeping the original code structure (iterating by index) and
calling undordered_remove.

Simon
  

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> Just a note that this changes an "unordered remove" to an "ordered remove".
Simon> If we don't need to keep the relative order of the remaining elements, it
Simon> might be good performance-wise to keep the original behavior.  This could
Simon> be done by keeping the original code structure (iterating by index) and
Simon> calling undordered_remove.

Somewhere else, Pedro pointed out the unordered_remove template function
in common/gdb_vecs.h.

Tom
  

On 2018-07-04 00:55, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:
> 
> Simon> Just a note that this changes an "unordered remove" to an
> "ordered remove".
> Simon> If we don't need to keep the relative order of the remaining 
> elements, it
> Simon> might be good performance-wise to keep the original behavior.  
> This could
> Simon> be done by keeping the original code structure (iterating by 
> index) and
> Simon> calling undordered_remove.
> 
> Somewhere else, Pedro pointed out the unordered_remove template 
> function
> in common/gdb_vecs.h.

Yes, this is what I'm referring to.

Simon