Re: [PATCHv3] Fix range end handling of inlined subroutines

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

* Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-03-21 21:31:07 +0100]:

> On 3/19/20 2:33 AM, Bernd Edlinger wrote:
> > On 3/17/20 11:27 PM, Andrew Burgess wrote:
> >>
> >> Are you arguing that we shouldn't use std::vector anywhere in GDB?  Or
> >> that this particular piece of code shouldn't use std::vector?
> >>
> > 
> > No, in general std::vector is just fine.
> > I think just here it is especially important to be below O(n*log(n)),
> > and don't depend on the underlying implementation.
> > 
> >> It was my understanding that we were moving GDB away from bespoke
> >> container management code unless there was a very compelling reason.
> >> I think as a minimum any new code that was written not using std::
> >> data structures (or some other gdb specific type) would need a comment
> >> explaining why, if nothing else to stop someone replacing the code
> >> with std:: types a few months down the line.
> >>
> > 
> > Right, good point.
> > I can add a comment here, so that will not happen:
> > 
> > index 46f985a..4f0d536 100644
> > --- a/gdb/buildsym.c
> > +++ b/gdb/buildsym.c
> > @@ -736,6 +736,10 @@ struct blockvector *
> >  void
> >  buildsym_compunit::record_inline_range_end (CORE_ADDR end)
> >  {
> > +  /* The performance of this function is very important,
> > +     it shall be O(n*log(n)) therefore we do not use std::vector
> > +     here since some compilers, e.g. visual studio, do not
> > +     guarantee that for vector::push_back.  */
> >    if (m_inline_end_vector == nullptr)
> >      {
> >        m_inline_end_vector_length = INITIAL_LINE_VECTOR_LENGTH;
> > 
> > 
> > Does that look good (also from the english)?
> > 
> 
> Hi Andrew,
> 
> how are you doing?
> 
> are you ok?

Sorry, I have not been ignoring this patch.  It is top of my review
stack.  It is being held up for a number of reasons, first Tom found a
number of issues with the original patch set, I've been trying to fix
these before reviewing your work in case the fixes had an impact.

I posted some patches for the easier of the issues, but I've not
solved the harder bug yet.  I'm pretty much decided to just go ahead
and review this patch anyway.

But then, I'm still struggling with the choice not to use
std::vector.  It's still not clear if there's actually a good reason
to avoid it in this case or not, so I feel I need to review other uses
of std::vector throughout GDB and see how your use compares.  Do you
maybe have some performance figures you could share to help make the
case for using custom container type?

Then the whole converting is_stmt entries to !is_stmt is still
concerning me.  I appreciate your expanded text in the patch
description, but I need to do more testing to see the real impact of
this change.  Most of my clients are driving GDB through some kind of
GUI, which means that break points are inevitably placed using
file:line sytnax.  One of the most common complaints I get is when a
breakpoint is placed on one line, but GDB stops on another.  Generally
my users can be quite forgiving of missing stack frames when inlining
is in play.  What this means is an argument of if you did break here
you'd have a frame missing, so I'm not going to let you break there
any more isn't that compelling to me.

That isn't a rejection of the patch, it's just me explaining why it's
taking me some time to review.

Thanks,
Andrew