[RFA] Use std::vector for moribund_locations
Commit Message
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(-)
Comments
* 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
@@ -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
@@ -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