On 2019-09-23 6:09 p.m., Andrew Burgess wrote:
> Converts a VEC into a std::vector in gdbsupport/btrace-common.h. This
> commit just performs a mechanical conversion and doesn't do any
> refactoring. One consequence of this is that the std::vector must
> actually be a pointer to std::vector as it is placed within a union.
> It might be possible in future to refactor to a class hierarchy and
> remove the need for a union, but I'd rather have that be a separate
> change to make it easier to see the evolution of the code.
Hi Andrew,
I think this is a good approach for incremental progress.
I noted a few things I would change, but nothing looked wrong to me in the patch.
> @@ -1073,7 +1073,7 @@ btrace_compute_ftrace_bts (struct thread_info *tp,
>
> blk -= 1;
>
> - block = VEC_index (btrace_block_s, btrace->blocks, blk);
> + block = &btrace->blocks->at (blk);
Can you take the opportunity to make `block` a pointer to const? The `btrace`
object it comes from is const, so if we had proper accessors to get the data
out of it, the returned pointer would be const.
> @@ -1581,11 +1580,10 @@ btrace_add_pc (struct thread_info *tp)
> pc = regcache_read_pc (regcache);
>
> btrace.format = BTRACE_FORMAT_BTS;
> - btrace.variant.bts.blocks = NULL;
> + btrace.variant.bts.blocks = new std::vector <btrace_block_s>;
>
> - block = VEC_safe_push (btrace_block_s, btrace.variant.bts.blocks, NULL);
> - block->begin = pc;
> - block->end = pc;
> + btrace_block block (pc, pc);
> + btrace.variant.bts.blocks->push_back (block);
The impact here would be minimal since the size of btrace_block is very small,
but the idiomatic thing to do there would be to construct the object directly
in the vector using emplace_back:
> @@ -1724,9 +1722,9 @@ btrace_stitch_bts (struct btrace_data_bts *btrace, struct thread_info *tp)
> In the second case, the delta trace vector should contain exactly one
> entry for the partial block containing the current PC. Remove it. */
> if (first_new_block->end == last_insn.pc
> - && VEC_length (btrace_block_s, btrace->blocks) == 1)
> + && btrace->blocks->size () == 1)
This fits on a single line.
> @@ -2052,9 +2049,8 @@ parse_xml_btrace_block (struct gdb_xml_parser *parser,
> begin = (ULONGEST *) xml_find_attribute (attributes, "begin")->value.get ();
> end = (ULONGEST *) xml_find_attribute (attributes, "end")->value.get ();
>
> - block = VEC_safe_push (btrace_block_s, btrace->variant.bts.blocks, NULL);
> - block->begin = *begin;
> - block->end = *end;
> + btrace_block block (*begin, *end);
> + btrace->variant.bts.blocks->push_back (block);
I suggest using emplace_back here too.
> @@ -141,15 +141,12 @@ btrace_data_append (struct btrace_data *dst,
>
> /* We copy blocks in reverse order to have the oldest block at
> index zero. */
> - blk = VEC_length (btrace_block_s, src->variant.bts.blocks);
> + blk = src->variant.bts.blocks->size ();
> while (blk != 0)
> {
> - btrace_block_s *block;
> -
> - block = VEC_index (btrace_block_s, src->variant.bts.blocks,
> - --blk);
> -
> - VEC_safe_push (btrace_block_s, dst->variant.bts.blocks, block);
> + btrace_block_s block
> + = src->variant.bts.blocks->at (--blk);
This fits on a single line.
Also, I don't know if it would make a difference at all (or if it would even make things
worse, since btrace_data is such a small structure), but I would instinctively have made
`block` a const reference to avoid a copy.
> @@ -43,11 +41,22 @@ struct btrace_block
>
> /* The address of the first byte of the last instruction in the block. */
> CORE_ADDR end;
> +
> + /* Simple constructor. */
> + btrace_block (CORE_ADDR begin, CORE_ADDR end)
> + : begin (begin),
> + end (end)
> + {
> + /* Nothing. */
> + }
> +
> +private:
> + /* Don't create blocks without initialization. */
> + btrace_block () = delete;
Is this explicit deletion needed? When you have a user-defined constructor, the
compiler won't generate an implicit default contructor. It's the opposite, if we
wanted it, we would need to do:
btrace_block () = default;
Also, if you need to delete some otherwise implicitly generated constructor or
operator, you don't need to put it private, just the "= delete" is enough.
> };
>
> /* Define functions operating on a vector of branch trace blocks. */
> typedef struct btrace_block btrace_block_s;
> -DEF_VEC_O (btrace_block_s);
You can get rid of the typedef too, and just use `btrace_block` everywhere.
> diff --git a/gdb/nat/linux-btrace.c b/gdb/nat/linux-btrace.c
> index 8625291cce9..e1da4a1645b 100644
> --- a/gdb/nat/linux-btrace.c
> +++ b/gdb/nat/linux-btrace.c
> @@ -271,11 +271,11 @@ perf_event_sample_ok (const struct perf_event_sample *sample)
> In case the buffer overflows during sampling, one sample may have its lower
> part at the end and its upper part at the beginning of the buffer. */
>
> -static VEC (btrace_block_s) *
> +static std::vector <btrace_block_s> *
> perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
> const uint8_t *end, const uint8_t *start, size_t size)
> {
> - VEC (btrace_block_s) *btrace = NULL;
> + std::vector <btrace_block_s> *btrace = new std::vector <btrace_block_s> ();
You can omit the parenthesis. I just mention this because you have omitted them
elsewhere in the patch, so I thought, let's be consistent!
> @@ -785,7 +785,7 @@ linux_read_bts (struct btrace_data_bts *btrace,
> data_head = *pevent->data_head;
>
> /* Delete any leftover trace from the previous iteration. */
> - VEC_free (btrace_block_s, btrace->blocks);
> + delete (btrace->blocks);
You can remove the parenthesis here.
>
> if (type == BTRACE_READ_DELTA)
> {
> @@ -843,9 +843,9 @@ linux_read_bts (struct btrace_data_bts *btrace,
> /* Prune the incomplete last block (i.e. the first one of inferior execution)
> if we're not doing a delta read. There is no way of filling in its zeroed
> BEGIN element. */
> - if (!VEC_empty (btrace_block_s, btrace->blocks)
> + if (!btrace->blocks->empty ()
> && type != BTRACE_READ_DELTA)
This could fit one a single line.
Simon
@@ -1059,7 +1059,7 @@ btrace_compute_ftrace_bts (struct thread_info *tp,
gdbarch = target_gdbarch ();
btinfo = &tp->btrace;
- blk = VEC_length (btrace_block_s, btrace->blocks);
+ blk = btrace->blocks->size ();
if (btinfo->functions.empty ())
level = INT_MAX;
@@ -1073,7 +1073,7 @@ btrace_compute_ftrace_bts (struct thread_info *tp,
blk -= 1;
- block = VEC_index (btrace_block_s, btrace->blocks, blk);
+ block = &btrace->blocks->at (blk);
pc = block->begin;
for (;;)
@@ -1573,7 +1573,6 @@ static void
btrace_add_pc (struct thread_info *tp)
{
struct btrace_data btrace;
- struct btrace_block *block;
struct regcache *regcache;
CORE_ADDR pc;
@@ -1581,11 +1580,10 @@ btrace_add_pc (struct thread_info *tp)
pc = regcache_read_pc (regcache);
btrace.format = BTRACE_FORMAT_BTS;
- btrace.variant.bts.blocks = NULL;
+ btrace.variant.bts.blocks = new std::vector <btrace_block_s>;
- block = VEC_safe_push (btrace_block_s, btrace.variant.bts.blocks, NULL);
- block->begin = pc;
- block->end = pc;
+ btrace_block block (pc, pc);
+ btrace.variant.bts.blocks->push_back (block);
btrace_compute_ftrace (tp, &btrace, NULL);
}
@@ -1696,7 +1694,7 @@ btrace_stitch_bts (struct btrace_data_bts *btrace, struct thread_info *tp)
btinfo = &tp->btrace;
gdb_assert (!btinfo->functions.empty ());
- gdb_assert (!VEC_empty (btrace_block_s, btrace->blocks));
+ gdb_assert (!btrace->blocks->empty ());
last_bfun = &btinfo->functions.back ();
@@ -1705,14 +1703,14 @@ btrace_stitch_bts (struct btrace_data_bts *btrace, struct thread_info *tp)
of the new trace, though, since we can't fill in the start address.*/
if (last_bfun->insn.empty ())
{
- VEC_pop (btrace_block_s, btrace->blocks);
+ btrace->blocks->pop_back ();
return 0;
}
/* Beware that block trace starts with the most recent block, so the
chronologically first block in the new trace is the last block in
the new trace's block vector. */
- first_new_block = VEC_last (btrace_block_s, btrace->blocks);
+ first_new_block = &btrace->blocks->back ();
const btrace_insn &last_insn = last_bfun->insn.back ();
/* If the current PC at the end of the block is the same as in our current
@@ -1724,9 +1722,9 @@ btrace_stitch_bts (struct btrace_data_bts *btrace, struct thread_info *tp)
In the second case, the delta trace vector should contain exactly one
entry for the partial block containing the current PC. Remove it. */
if (first_new_block->end == last_insn.pc
- && VEC_length (btrace_block_s, btrace->blocks) == 1)
+ && btrace->blocks->size () == 1)
{
- VEC_pop (btrace_block_s, btrace->blocks);
+ btrace->blocks->pop_back ();
return 0;
}
@@ -2030,7 +2028,6 @@ parse_xml_btrace_block (struct gdb_xml_parser *parser,
std::vector<gdb_xml_value> &attributes)
{
struct btrace_data *btrace;
- struct btrace_block *block;
ULONGEST *begin, *end;
btrace = (struct btrace_data *) user_data;
@@ -2042,7 +2039,7 @@ parse_xml_btrace_block (struct gdb_xml_parser *parser,
case BTRACE_FORMAT_NONE:
btrace->format = BTRACE_FORMAT_BTS;
- btrace->variant.bts.blocks = NULL;
+ btrace->variant.bts.blocks = new std::vector <btrace_block_s>;
break;
default:
@@ -2052,9 +2049,8 @@ parse_xml_btrace_block (struct gdb_xml_parser *parser,
begin = (ULONGEST *) xml_find_attribute (attributes, "begin")->value.get ();
end = (ULONGEST *) xml_find_attribute (attributes, "end")->value.get ();
- block = VEC_safe_push (btrace_block_s, btrace->variant.bts.blocks, NULL);
- block->begin = *begin;
- block->end = *end;
+ btrace_block block (*begin, *end);
+ btrace->variant.bts.blocks->push_back (block);
}
/* Parse a "raw" xml record. */
@@ -3095,7 +3091,7 @@ btrace_maint_update_packets (struct btrace_thread_info *btinfo,
case BTRACE_FORMAT_BTS:
/* Nothing to do - we operate directly on BTINFO->DATA. */
*begin = 0;
- *end = VEC_length (btrace_block_s, btinfo->data.variant.bts.blocks);
+ *end = btinfo->data.variant.bts.blocks->size ();
*from = btinfo->maint.variant.bts.packet_history.begin;
*to = btinfo->maint.variant.bts.packet_history.end;
break;
@@ -3128,19 +3124,17 @@ btrace_maint_print_packets (struct btrace_thread_info *btinfo,
case BTRACE_FORMAT_BTS:
{
- VEC (btrace_block_s) *blocks;
+ std::vector <btrace_block_s> *blocks;
unsigned int blk;
blocks = btinfo->data.variant.bts.blocks;
for (blk = begin; blk < end; ++blk)
{
- const btrace_block_s *block;
-
- block = VEC_index (btrace_block_s, blocks, blk);
+ const btrace_block_s &block = blocks->at (blk);
printf_unfiltered ("%u\tbegin: %s, end: %s\n", blk,
- core_addr_to_string_nz (block->begin),
- core_addr_to_string_nz (block->end));
+ core_addr_to_string_nz (block.begin),
+ core_addr_to_string_nz (block.end));
}
btinfo->maint.variant.bts.packet_history.begin = begin;
@@ -3443,9 +3437,8 @@ maint_info_btrace_cmd (const char *args, int from_tty)
break;
case BTRACE_FORMAT_BTS:
- printf_unfiltered (_("Number of packets: %u.\n"),
- VEC_length (btrace_block_s,
- btinfo->data.variant.bts.blocks));
+ printf_unfiltered (_("Number of packets: %zu.\n"),
+ btinfo->data.variant.bts.blocks->size ());
break;
#if defined (HAVE_LIBIPT)
@@ -29,6 +29,7 @@
#include "gdbsupport/btrace-common.h"
#include "target/waitstatus.h" /* For enum target_stop_reason. */
#include "gdbsupport/enum-flags.h"
+#include "gdbsupport/vec.h"
#if defined (HAVE_LIBIPT)
# include <intel-pt.h>
@@ -7115,9 +7115,7 @@ linux_low_read_btrace (struct btrace_target_info *tinfo, struct buffer *buffer,
enum btrace_read_type type)
{
struct btrace_data btrace;
- struct btrace_block *block;
enum btrace_error err;
- int i;
err = linux_read_btrace (&btrace, tinfo, type);
if (err != BTRACE_ERR_NONE)
@@ -7140,11 +7138,9 @@ linux_low_read_btrace (struct btrace_target_info *tinfo, struct buffer *buffer,
buffer_grow_str (buffer, "<!DOCTYPE btrace SYSTEM \"btrace.dtd\">\n");
buffer_grow_str (buffer, "<btrace version=\"1.0\">\n");
- for (i = 0;
- VEC_iterate (btrace_block_s, btrace.variant.bts.blocks, i, block);
- i++)
+ for (const struct btrace_block &block : *btrace.variant.bts.blocks)
buffer_xml_printf (buffer, "<block begin=\"0x%s\" end=\"0x%s\"/>\n",
- paddress (block->begin), paddress (block->end));
+ paddress (block.begin), paddress (block.end));
buffer_grow_str0 (buffer, "</btrace>\n");
break;
@@ -73,7 +73,7 @@ btrace_data::fini ()
return;
case BTRACE_FORMAT_BTS:
- VEC_free (btrace_block_s, variant.bts.blocks);
+ delete variant.bts.blocks;
return;
case BTRACE_FORMAT_PT:
@@ -95,7 +95,7 @@ btrace_data::empty () const
return true;
case BTRACE_FORMAT_BTS:
- return VEC_empty (btrace_block_s, variant.bts.blocks);
+ return variant.bts.blocks->empty ();
case BTRACE_FORMAT_PT:
return (variant.pt.size == 0);
@@ -132,7 +132,7 @@ btrace_data_append (struct btrace_data *dst,
case BTRACE_FORMAT_NONE:
dst->format = BTRACE_FORMAT_BTS;
- dst->variant.bts.blocks = NULL;
+ dst->variant.bts.blocks = new std::vector <btrace_block_s>;
/* Fall-through. */
case BTRACE_FORMAT_BTS:
@@ -141,15 +141,12 @@ btrace_data_append (struct btrace_data *dst,
/* We copy blocks in reverse order to have the oldest block at
index zero. */
- blk = VEC_length (btrace_block_s, src->variant.bts.blocks);
+ blk = src->variant.bts.blocks->size ();
while (blk != 0)
{
- btrace_block_s *block;
-
- block = VEC_index (btrace_block_s, src->variant.bts.blocks,
- --blk);
-
- VEC_safe_push (btrace_block_s, dst->variant.bts.blocks, block);
+ btrace_block_s block
+ = src->variant.bts.blocks->at (--blk);
+ dst->variant.bts.blocks->push_back (block);
}
}
}
@@ -26,8 +26,6 @@
inferior. For presentation purposes, the branch trace is represented as a
list of sequential control-flow blocks, one such list per thread. */
-#include "vec.h"
-
/* A branch trace block.
This represents a block of sequential control-flow. Adjacent blocks will be
@@ -43,11 +41,22 @@ struct btrace_block
/* The address of the first byte of the last instruction in the block. */
CORE_ADDR end;
+
+ /* Simple constructor. */
+ btrace_block (CORE_ADDR begin, CORE_ADDR end)
+ : begin (begin),
+ end (end)
+ {
+ /* Nothing. */
+ }
+
+private:
+ /* Don't create blocks without initialization. */
+ btrace_block () = delete;
};
/* Define functions operating on a vector of branch trace blocks. */
typedef struct btrace_block btrace_block_s;
-DEF_VEC_O (btrace_block_s);
/* Enumeration of btrace formats. */
@@ -137,8 +146,9 @@ struct btrace_config
struct btrace_data_bts
{
/* Branch trace is represented as a vector of branch trace blocks starting
- with the most recent block. */
- VEC (btrace_block_s) *blocks;
+ with the most recent block. This needs to be a pointer as we place
+ btrace_data_bts into a union. */
+ std::vector <btrace_block_s> *blocks;
};
/* Configuration information to go with the trace data. */
@@ -271,11 +271,11 @@ perf_event_sample_ok (const struct perf_event_sample *sample)
In case the buffer overflows during sampling, one sample may have its lower
part at the end and its upper part at the beginning of the buffer. */
-static VEC (btrace_block_s) *
+static std::vector <btrace_block_s> *
perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
const uint8_t *end, const uint8_t *start, size_t size)
{
- VEC (btrace_block_s) *btrace = NULL;
+ std::vector <btrace_block_s> *btrace = new std::vector <btrace_block_s> ();
struct perf_event_sample sample;
size_t read = 0;
struct btrace_block block = { 0, 0 };
@@ -343,7 +343,7 @@ perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
/* We found a valid sample, so we can complete the current block. */
block.begin = psample->bts.to;
- VEC_safe_push (btrace_block_s, btrace, &block);
+ btrace->push_back (block);
/* Start the next block. */
block.end = psample->bts.from;
@@ -354,7 +354,7 @@ perf_event_read_bts (struct btrace_target_info* tinfo, const uint8_t *begin,
reading delta trace, we can fill in the start address later on.
Otherwise we will prune it. */
block.begin = 0;
- VEC_safe_push (btrace_block_s, btrace, &block);
+ btrace->push_back (block);
return btrace;
}
@@ -785,7 +785,7 @@ linux_read_bts (struct btrace_data_bts *btrace,
data_head = *pevent->data_head;
/* Delete any leftover trace from the previous iteration. */
- VEC_free (btrace_block_s, btrace->blocks);
+ delete (btrace->blocks);
if (type == BTRACE_READ_DELTA)
{
@@ -843,9 +843,9 @@ linux_read_bts (struct btrace_data_bts *btrace,
/* Prune the incomplete last block (i.e. the first one of inferior execution)
if we're not doing a delta read. There is no way of filling in its zeroed
BEGIN element. */
- if (!VEC_empty (btrace_block_s, btrace->blocks)
+ if (!btrace->blocks->empty ()
&& type != BTRACE_READ_DELTA)
- VEC_pop (btrace_block_s, btrace->blocks);
+ btrace->blocks->pop_back ();
return BTRACE_ERR_NONE;
}