RE: [PATCHv4 1/3] gdb: Remove a VEC from gdbsupport/btrace-common.h

["Metzger, Markus T" <markus.t.metzger@intel.com>]

Hello Andrew,

> @@ -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;

We should be able to guarantee that BTRACE->BLOCKS != nullptr.  We only get
here if the btrace format is set to BTRACE_FORMAT_BTS.  The blocks vector must
be allocated whenever we set the format to BTRACE_FORMAT_BTS.


> @@ -3128,19 +3119,17 @@ btrace_maint_print_packets (struct
> btrace_thread_info *btinfo,
> 
>      case BTRACE_FORMAT_BTS:
>        {
> -	VEC (btrace_block_s) *blocks;
> +	const std::vector <btrace_block> &blocks
> +	  = *btinfo->data.variant.bts.blocks;

Would that fit onto a single line?


> diff --git a/gdb/btrace.h b/gdb/btrace.h
> index ba8d27c879c..d09c424804b 100644
> --- a/gdb/btrace.h
> +++ b/gdb/btrace.h
> @@ -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"

Didn't you want to remove those includes?


> @@ -141,15 +142,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);
> +		const btrace_block &block
> +		  = src->variant.bts.blocks->at (--blk);

Would this fit onto one line?

 
> @@ -137,8 +139,28 @@ 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> *blocks;
> +
> +  /* The BLOCKS vector used to be implemented using a C vector library.  A
> +     property of this library was that the size and empty functions could
> +     be called with a nullptr (to the vector) and still have a sensible
> +     answer return.
> +
> +     When converting to C++ std::vector this method was added here to avoid
> +     introducing errors if we ever did try to call size or empty on a
> +     nullptr.  */
> +  size_t blocks_size () const
> +  {
> +    return (blocks == nullptr) ? 0 : blocks->size ();
> +  }
> +
> +  /* See the comment for BLOCKS_SIZE above.  */
> +  bool blocks_empty () const
> +  {
> +    return (blocks == nullptr) ? true : blocks->empty ();
> +  }

These helpers shouldn't be necessary.  If the format is set to BTRACE_FORMAT_BTS,
the blocks vector must have been allocated.  And it must be freed when the format
is set to any other value - most likely BTRACE_FORMAT_NONE.

Same for BTRACE_FORMAT_PT.  Without trace, the format should be left at
BTRACE_FORMAT_NONE.


Regards,
Markus.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Gary Kershaw
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928