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

[Simon Marchi <simark@simark.ca>]

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