Re: [PATCH 1/2] gdb: Remove a VEC from gdbsupport/btrace-common.h

Simon Marchi
2019-09-24T02:09:57Z

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