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

Metzger, Markus T
2019-09-26T13:07:44Z

Hello Andrew,

> 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.

That should suffice. I wasn't sure if you had access to a suitable system.

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

We wouldn't get into this loop if BTRACE->BLOCKS == nullptr. BLK is initialized to
BTRACE->BLOCKS->size () (or zero with your latest version) so we'd skip this if we
ever had BTRACE->BLOCKS == nullptr.

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

Thanks. We could clear it again but it's probably not worth the added complexity.

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