[PATCH] Fix an undefined behavior in record_line

[PATCH] Fix an undefined behavior in record_line

[Bernd Edlinger]

Additionally do not completely remove symbols
at the same PC than the end marker, instead
make them non-is-stmt breakpoints.

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.
---
 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

Re: [PATCH] Fix an undefined behavior in record_line

[Bernd Edlinger]

On 3/13/20 12:15 PM, Bernd Edlinger wrote:
> Additionally do not completely remove symbols
> at the same PC than the end marker, instead
> make them non-is-stmt breakpoints.
> 
> 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.
> ---
>  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)

When I think about it, I would leave any previous-end-of-sequence makers
as is_stmt, so change that to if (p->pc != pc or p->line == 0)
as line = 0 should imply is_stmt = 1


Bernd.
> +	    break;
> +	  e->is_stmt = 0;
>  	}
> +      while (e > subfile->line_vector->item);
>      }
>  
>    e = subfile->line_vector->item + subfile->line_vector->nitems++;
> 

Re: [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

Re: [PATCH] Fix an undefined behavior in record_line

[Andrew Burgess]

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