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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* 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