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

Metzger, Markus T
2019-09-26T13:40:23Z

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