Hello Andrew,
I can help you run the gdb.btrace suite. Do you have a user branch with these patches?
> @@ -1068,13 +1069,12 @@ btrace_compute_ftrace_bts (struct thread_info *tp,
>
> while (blk != 0)
> {
> - btrace_block_s *block;
> CORE_ADDR pc;
>
> blk -= 1;
>
> - block = VEC_index (btrace_block_s, btrace->blocks, blk);
> - pc = block->begin;
> + const btrace_block &block = btrace->blocks->at (blk);
> + pc = block.begin;
We could also turn BTRACE->BLOCKS into a const & outside the loop.
> @@ -1581,11 +1580,9 @@ 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>;
A stack-allocated vector should do. Looks like we leaked the one-entry
vector before.
> @@ -2052,9 +2048,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;
> + gdb_assert (btrace->variant.bts.blocks != nullptr);
We don't check most pointers in GDB. Besides, we just allocated the vector.
The invariant is that if BTRACE->FORMAT == BTRACE_FORMAT_BTS, the vector
is non-null.
> @@ -3095,7 +3090,8 @@ 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);
> + gdb_assert (btinfo->data.variant.bts.blocks != nullptr);
See above. More below.
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
* Metzger, Markus T <markus.t.metzger@intel.com> [2019-09-26 08:47:23 +0000]:
> Hello Andrew,
>
> I can help you run the gdb.btrace suite. Do you have a user branch
> with these patches?
You can find this branch here:
https://github.com/T-J-Teru/binutils-gdb/tree/gdb-remove-vec
Any additional testing is always welcome, though from Simon's feedback
I believe that I should be testing this OK - I'm running on a
post-Broadwell Intel CPU, and I am linking GDB against libipt, and see
no regressions over the entire testsuite.
>
>
> > @@ -1068,13 +1069,12 @@ btrace_compute_ftrace_bts (struct thread_info *tp,
> >
> > while (blk != 0)
> > {
> > - btrace_block_s *block;
> > CORE_ADDR pc;
> >
> > blk -= 1;
> >
> > - block = VEC_index (btrace_block_s, btrace->blocks, blk);
> > - pc = block->begin;
> > + const btrace_block &block = btrace->blocks->at (blk);
> > + pc = block.begin;
>
> We could also turn BTRACE->BLOCKS into a const & outside the loop.
I was worried about the case where we get here and btrace->blocks ==
nullptr. I don't know if such a path is possible, I had a little
through the code and my conclusion was that the answer is not
obvious. So ideally I'd leave the code as close to the original as
possible to avoid introducing bugs.
>
>
> > @@ -1581,11 +1580,9 @@ 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>;
>
> A stack-allocated vector should do. Looks like we leaked the one-entry
> vector before.
Nope the btrace object is stack local, and its destructor deletes the
vector, so its easier if this just stays being new'd for now.
>
>
> > @@ -2052,9 +2048,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;
> > + gdb_assert (btrace->variant.bts.blocks != nullptr);
>
> We don't check most pointers in GDB. Besides, we just allocated the vector.
> The invariant is that if BTRACE->FORMAT == BTRACE_FORMAT_BTS, the vector
> is non-null.
You are correct. Thanks. I've removed this assertion.
>
>
> > @@ -3095,7 +3090,8 @@ 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);
> > + gdb_assert (btinfo->data.variant.bts.blocks != nullptr);
>
> See above. More below.
I think you've convinced me that having all of these assertions around
isn't great. So I've taken a slightly different approach. I'll post
another iteration soon.
Thanks,
Andrew
@@ -1059,7 +1059,8 @@ btrace_compute_ftrace_bts (struct thread_info *tp,
gdbarch = target_gdbarch ();
btinfo = &tp->btrace;
- blk = VEC_length (btrace_block_s, btrace->blocks);
+ gdb_assert (btrace->blocks != nullptr);
+ blk = btrace->blocks->size ();
if (btinfo->functions.empty ())
level = INT_MAX;
@@ -1068,13 +1069,12 @@ btrace_compute_ftrace_bts (struct thread_info *tp,
while (blk != 0)
{
- btrace_block_s *block;
CORE_ADDR pc;
blk -= 1;
- block = VEC_index (btrace_block_s, btrace->blocks, blk);
- pc = block->begin;
+ const btrace_block &block = btrace->blocks->at (blk);
+ pc = block.begin;
for (;;)
{
@@ -1083,7 +1083,7 @@ btrace_compute_ftrace_bts (struct thread_info *tp,
int size;
/* We should hit the end of the block. Warn if we went too far. */
- if (block->end < pc)
+ if (block.end < pc)
{
/* Indicate the gap in the trace. */
bfun = ftrace_new_gap (btinfo, BDE_BTS_OVERFLOW, gaps);
@@ -1119,7 +1119,7 @@ btrace_compute_ftrace_bts (struct thread_info *tp,
ftrace_update_insns (bfun, insn);
/* We're done once we pushed the instruction at the end. */
- if (block->end == pc)
+ if (block.end == pc)
break;
/* We can't continue if we fail to compute the size. */
@@ -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,9 @@ 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>;
- block = VEC_safe_push (btrace_block_s, btrace.variant.bts.blocks, NULL);
- block->begin = pc;
- block->end = pc;
+ btrace.variant.bts.blocks->emplace_back (pc, pc);
btrace_compute_ftrace (tp, &btrace, NULL);
}
@@ -1692,11 +1689,12 @@ btrace_stitch_bts (struct btrace_data_bts *btrace, struct thread_info *tp)
{
struct btrace_thread_info *btinfo;
struct btrace_function *last_bfun;
- btrace_block_s *first_new_block;
+ btrace_block *first_new_block;
btinfo = &tp->btrace;
gdb_assert (!btinfo->functions.empty ());
- gdb_assert (!VEC_empty (btrace_block_s, btrace->blocks));
+ gdb_assert (btrace->blocks != nullptr);
+ 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
@@ -1723,10 +1721,9 @@ btrace_stitch_bts (struct btrace_data_bts *btrace, struct thread_info *tp)
entries.
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)
+ if (first_new_block->end == last_insn.pc && btrace->blocks->size () == 1)
{
- VEC_pop (btrace_block_s, btrace->blocks);
+ btrace->blocks->pop_back ();
return 0;
}
@@ -2030,7 +2027,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 +2038,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>;
break;
default:
@@ -2052,9 +2048,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;
+ gdb_assert (btrace->variant.bts.blocks != nullptr);
+ btrace->variant.bts.blocks->emplace_back (*begin, *end);
}
/* Parse a "raw" xml record. */
@@ -3095,7 +3090,8 @@ 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);
+ gdb_assert (btinfo->data.variant.bts.blocks != nullptr);
+ *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> *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 &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 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,8 @@ btrace_data::fini ()
return;
case BTRACE_FORMAT_BTS:
- VEC_free (btrace_block_s, variant.bts.blocks);
+ delete variant.bts.blocks;
+ variant.bts.blocks = nullptr;
return;
case BTRACE_FORMAT_PT:
@@ -95,7 +96,8 @@ btrace_data::empty () const
return true;
case BTRACE_FORMAT_BTS:
- return VEC_empty (btrace_block_s, variant.bts.blocks);
+ gdb_assert (variant.bts.blocks != nullptr);
+ return variant.bts.blocks->empty ();
case BTRACE_FORMAT_PT:
return (variant.pt.size == 0);
@@ -132,7 +134,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>;
/* Fall-through. */
case BTRACE_FORMAT_BTS:
@@ -141,15 +143,13 @@ 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);
+ gdb_assert (src->variant.bts.blocks != nullptr);
+ 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);
+ 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,15 @@ struct btrace_block
/* The address of the first byte of the last instruction in the block. */
CORE_ADDR end;
-};
-/* Define functions operating on a vector of branch trace blocks. */
-typedef struct btrace_block btrace_block_s;
-DEF_VEC_O (btrace_block_s);
+ /* Simple constructor. */
+ btrace_block (CORE_ADDR begin, CORE_ADDR end)
+ : begin (begin),
+ end (end)
+ {
+ /* Nothing. */
+ }
+};
/* Enumeration of btrace formats. */
@@ -137,8 +139,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> *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> *
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> *btrace = new std::vector <btrace_block>;
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,8 @@ 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;
+ btrace->blocks = nullptr;
if (type == BTRACE_READ_DELTA)
{
@@ -843,9 +844,8 @@ 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)
- && type != BTRACE_READ_DELTA)
- VEC_pop (btrace_block_s, btrace->blocks);
+ if (!btrace->blocks->empty () && type != BTRACE_READ_DELTA)
+ btrace->blocks->pop_back ();
return BTRACE_ERR_NONE;
}