Re: [PATCH] Fix an undefined behavior in record_line

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

Bernd,

I apologise for replying to this old email.

For some reason my mail client brought this to the top of my list - I
think I tagged it as 'todo' long ago and never untagged it.  I'm not
really sure :-/

Anyway, I should have double checked the date before replying.  I
didn't, and I apologise.

Thanks,

Andrew


* Andrew Burgess <andrew.burgess@embecosm.com> [2020-06-16 09:08:15 +0100]:

> * Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-03-13 12:15:35 +0100]:
> 
> > Additionally do not completely remove symbols
> > at the same PC than the end marker, instead
> > make them non-is-stmt breakpoints.
> 
> You need to be careful, symbols means something completely different
> within GDB, you're talking about line table entries here.
> 
> Also, we generally split different functionality into different
> patches, so I would expect to see at least two patches here, one
> fixing the undefined behaviour and resize issue, and another for the
> change in behaviour deleting line markers vs setting them to
> non-is-stmt.
> 
> The patch for changing from deleting empty lines to making them
> non-is-stmt would need some additional justification text I think, why
> is this important?  Also, it would ideally have some tests attached,
> or, if this change fixes some existing test, would reference that test
> in the commit message.
> 
> I do remember looking at a version of this patch before, and I worked
> through the code and did manage to figure out what the undefined
> behaviour was that you were fixing.  But I can't remember now what the
> problem was.
> 
> Please could you expand the commit message to explain what the
> undefined behaviour actually is that your fixing.
> 
> > 
> > Also fix the condition when the line table need to be resized,
> > that was wasting one element.
> > 
> > 2020-03-10  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> > 	* buildsym.c (record_line): Fix ub and preserve lines at eof.
> 
> Please expand 'ub' to 'undefined behaviour' or at the very least
> capitalise to UB, it's not a commonly used acronym and it just looks
> like a type (to me).
> 
> Thanks,
> Andrew
> 
> > ---
> >  gdb/buildsym.c | 28 +++++++++++-----------------
> >  1 file changed, 11 insertions(+), 17 deletions(-)
> > 
> > diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> > index 7155db3..e090fdb 100644
> > --- a/gdb/buildsym.c
> > +++ b/gdb/buildsym.c
> > @@ -695,7 +695,7 @@ struct blockvector *
> >  	}
> >      }
> >  
> > -  if (subfile->line_vector->nitems + 1 >= subfile->line_vector_length)
> > +  if (subfile->line_vector->nitems >= subfile->line_vector_length)
> >      {
> >        subfile->line_vector_length *= 2;
> >        subfile->line_vector = (struct linetable *)
> > @@ -705,27 +705,21 @@ struct blockvector *
> >  		      * sizeof (struct linetable_entry))));
> >      }
> >  
> > -  /* Normally, we treat lines as unsorted.  But the end of sequence
> > -     marker is special.  We sort line markers at the same PC by line
> > -     number, so end of sequence markers (which have line == 0) appear
> > -     first.  This is right if the marker ends the previous function,
> > -     and there is no padding before the next function.  But it is
> > -     wrong if the previous line was empty and we are now marking a
> > -     switch to a different subfile.  We must leave the end of sequence
> > -     marker at the end of this group of lines, not sort the empty line
> > -     to after the marker.  The easiest way to accomplish this is to
> > -     delete any empty lines from our table, if they are followed by
> > -     end of sequence markers.  All we lose is the ability to set
> > -     breakpoints at some lines which contain no instructions
> > -     anyway.  */
> > +  /* The end of sequence marker is special.  We need to reset the
> > +     is_stmt flag on previous lines at the same PC, otherwise these
> > +     lines may cause problems.  All we lose is the ability to set
> > +     breakpoints at some lines which contain no instructions anyway.  */
> >    if (line == 0 && subfile->line_vector->nitems > 0)
> >      {
> > -      e = subfile->line_vector->item + subfile->line_vector->nitems - 1;
> > -      while (subfile->line_vector->nitems > 0 && e->pc == pc)
> > +      e = subfile->line_vector->item + subfile->line_vector->nitems;
> > +      do
> >  	{
> >  	  e--;
> > -	  subfile->line_vector->nitems--;
> > +	  if (e->pc != pc)
> > +	    break;
> > +	  e->is_stmt = 0;
> >  	}
> > +      while (e > subfile->line_vector->item);
> >      }
> >  
> >    e = subfile->line_vector->item + subfile->line_vector->nitems++;
> > -- 
> > 1.9.1