[PATCH] Fix an undefined behavior in record_line

[Andrew Burgess]

* 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