Re: [PATCH] Partially revert my UB fix in record_line

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

* Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-04-09 03:35:17 +0200]:

> This reverts the following commit partially:
> 
> commit 64dc2d4bd24ff7119c913fff91184414f09b8042
> Author: Bernd Edlinger <bernd.edlinger@hotmail.de>
> Date:   Thu Mar 12 11:52:34 2020 +0100
> 
>     Fix an undefined behavior in record_line
> 
>     Additionally do not completely remove symbols
>     at the same PC than the end marker, instead
>     make them non-is-stmt breakpoints.
> 
> We keep the undefined behavoir fix,
> but have to restore the original behavior
> regarding deletion of the line entries.
> 
> 2020-04-09  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
> 	revert partially:
> 	2020-04-01  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
>         * buildsym.c (record_line): Fix undefined behavior and preserve
>         lines at eof.

Looks good.  Approved.

Thanks,

Andrew


> ---
>  gdb/buildsym.c | 37 ++++++++++++++++++-------------------
>  1 file changed, 18 insertions(+), 19 deletions(-)
> 
> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> index fe07103..c08c476 100644
> --- a/gdb/buildsym.c
> +++ b/gdb/buildsym.c
> @@ -691,29 +691,28 @@ struct blockvector *
>  		      * sizeof (struct linetable_entry))));
>      }
>  
> -  /* 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 since they might be at the same address
> -     as the following function.  For instance suppose a function calls
> -     abort there is no reason to emit a ret after that point (no joke).
> -     So the label may be at the same address where the following
> -     function begins.  A similar problem appears if a label is at the
> -     same address where an inline function ends we cannot reliably tell
> -     if this is considered part of the inline function or the calling
> -     program or even the next inline function, so stack traces may
> -     give surprising results.  Expect gdb.cp/step-and-next-inline.exp
> -     to fail if these lines are not modified here.  */
> -  if (line == 0 && subfile->line_vector->nitems > 0)
> +  /* 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.  */
> +  if (line == 0)
>      {
> -      e = subfile->line_vector->item + subfile->line_vector->nitems;
> -      do
> +      while (subfile->line_vector->nitems > 0)
>  	{
> -	  e--;
> -	  if (e->pc != pc || e->line == 0)
> +	  e = subfile->line_vector->item + subfile->line_vector->nitems - 1;
> +	  if (e->pc != pc)
>  	    break;
> -	  e->is_stmt = 0;
> +	  subfile->line_vector->nitems--;
>  	}
> -      while (e > subfile->line_vector->item);
>      }
>  
>    e = subfile->line_vector->item + subfile->line_vector->nitems++;
> -- 
> 1.9.1