[PATCHv2] Fix an undefined behavior in record_line

[PATCHv2] 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..960a36c 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 || e->line == 0)
+	    break;
+	  e->is_stmt = 0;
 	}
+      while (e > subfile->line_vector->item);
     }
 
   e = subfile->line_vector->item + subfile->line_vectoms++;
-- 
1.9.1

Re: [PATCHv2] Fix an undefined behavior in record_line

[Bernd Edlinger]

On 3/13/20 12:55 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..960a36c 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 || e->line == 0)
> +	    break;
> +	  e->is_stmt = 0;
>  	}
> +      while (e > subfile->line_vector->item);
>      }
>  
>    e = subfile->line_vector->item + subfile->line_vectoms++;
> 

Hi everyone,

I'd say this should be a no-brainer, fixind undefined
behavior, and not removing data when not necessary.

Is it OK for trunk?


Thanks
Bernd.

Re: [PATCHv2] Fix an undefined behavior in record_line

[Bernd Edlinger]

On 3/22/20 4:25 AM, Bernd Edlinger wrote:
> On 3/13/20 12:55 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..960a36c 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 || e->line == 0)
>> +	    break;
>> +	  e->is_stmt = 0;
>>  	}
>> +      while (e > subfile->line_vector->item);
>>      }
>>  
>>    e = subfile->line_vector->item + subfile->line_vectoms++;
>>

Andrew, this is the place where currently the is-stmt entries
are deleted.  With your is-stmt patch this code is executed in more
cases than before.  Therefore I would suggest to convert them
to !is_stmt lines for now, but maybe in the long run add a new flag
that allows them to be used in the file:line case, but make these
lines behave differently when stepping, I am only trying to fix
the case where you step out of the subroutine.


Bernd.

Re: [PATCHv2] Fix an undefined behavior in record_line

[Andrew Burgess]

* Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-03-23 22:25:42 +0100]:

> On 3/22/20 4:25 AM, Bernd Edlinger wrote:
> > On 3/13/20 12:55 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.

I suspect this commit message has evolved overtime - having the first
word be "additionally" seems a little strange.

> >>
> >> 2020-03-10  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> >> 	* buildsym.c (record_line): Fix ub and preserve lines at eof.

Typo: ub -> up

> >> ---
> >>  gdb/buildsym.c | 28 +++++++++++-----------------
> >>  1 file changed, 11 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> >> index 7155db3..960a36c 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))));
> >>      }

This part seems separate to what comes below I think.  This should be
a separate commit.

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

You need to expand on what "problems" means here.  Someone coming back
to this code in the future will have no idea why we're making this
change, and with no tests for this commit they can't even try to
figure out the "problems" by looking at a test.

> >>    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 || e->line == 0)
> >> +	    break;
> >> +	  e->is_stmt = 0;
> >>  	}
> >> +      while (e > subfile->line_vector->item);
> >>      }
> >>  
> >>    e = subfile->line_vector->item + subfile->line_vectoms++;
> >>
> 
> Andrew, this is the place where currently the is-stmt entries
> are deleted.  With your is-stmt patch this code is executed in more
> cases than before.  Therefore I would suggest to convert them
> to !is_stmt lines for now, but maybe in the long run add a new flag
> that allows them to be used in the file:line case, but make these
> lines behave differently when stepping, I am only trying to fix
> the case where you step out of the subroutine.

I'm super uncomfortable with any code that changes is-stmt to
!is-stmt, as I worry about what we might be giving up.  You say "All
we lose is the ability to set breakpoints at some lines which contain
no instructions anyway.", but I'll need to work through some examples
to see what this actually means in practice before I can be happy with
this change.

Thanks,
Andrew

Re: [PATCHv2] Fix an undefined behavior in record_line

[Bernd Edlinger]

On 3/24/20 10:10 AM, Andrew Burgess wrote:
> * Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-03-23 22:25:42 +0100]:
> 
>> On 3/22/20 4:25 AM, Bernd Edlinger wrote:
>>> On 3/13/20 12:55 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.
> 
> I suspect this commit message has evolved overtime - having the first
> word be "additionally" seems a little strange.
> 

I'll re-think the commit message, thanks.

>>>>
>>>> 2020-03-10  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>> 	* buildsym.c (record_line): Fix ub and preserve lines at eof.
> 
> Typo: ub -> up
> 
>>>> ---
>>>>  gdb/buildsym.c | 28 +++++++++++-----------------
>>>>  1 file changed, 11 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
>>>> index 7155db3..960a36c 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))));
>>>>      }
> 
> This part seems separate to what comes below I think.  This should be
> a separate commit.
> 

Okay, good point.  That should be easy.

>>>>  
>>>> -  /* 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.  */
> 
> You need to expand on what "problems" means here.  Someone coming back
> to this code in the future will have no idea why we're making this
> change, and with no tests for this commit they can't even try to
> figure out the "problems" by looking at a test.
> 

I will try to explain that better, yes.

>>>>    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 || e->line == 0)
>>>> +	    break;
>>>> +	  e->is_stmt = 0;
>>>>  	}
>>>> +      while (e > subfile->line_vector->item);
>>>>      }
>>>>  
>>>>    e = subfile->line_vector->item + subfile->line_vectoms++;
>>>>
>>
>> Andrew, this is the place where currently the is-stmt entries
>> are deleted.  With your is-stmt patch this code is executed in more
>> cases than before.  Therefore I would suggest to convert them
>> to !is_stmt lines for now, but maybe in the long run add a new flag
>> that allows them to be used in the file:line case, but make these
>> lines behave differently when stepping, I am only trying to fix
>> the case where you step out of the subroutine.
> 
> I'm super uncomfortable with any code that changes is-stmt to
> !is-stmt, as I worry about what we might be giving up.  You say "All
> we lose is the ability to set breakpoints at some lines which contain
> no instructions anyway.", but I'll need to work through some examples
> to see what this actually means in practice before I can be happy with
> this change.
> 

There is no pressure from my side to do anything about it.
I am just saying is-stmt -> !is-stmt is better than removing
is-stmt lines that are at the same PC by chance.

I will come up with an updated patch, eventually, but will need
to spend more time on the openssl project now, to meet the schedule for the
next release.


Bernd.

Re: [PATCHv2] Fix an undefined behavior in record_line

[Andrew Burgess]

* Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-03-24 11:20:25 +0100]:

> 
> 
> On 3/24/20 10:10 AM, Andrew Burgess wrote:
> > * Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-03-23 22:25:42 +0100]:
> > 
> >> On 3/22/20 4:25 AM, Bernd Edlinger wrote:
> >>> On 3/13/20 12:55 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.
> > 
> > I suspect this commit message has evolved overtime - having the first
> > word be "additionally" seems a little strange.
> > 
> 
> I'll re-think the commit message, thanks.
> 
> >>>>
> >>>> 2020-03-10  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> >>>> 	* buildsym.c (record_line): Fix ub and preserve lines at eof.
> > 
> > Typo: ub -> up
> > 
> >>>> ---
> >>>>  gdb/buildsym.c | 28 +++++++++++-----------------
> >>>>  1 file changed, 11 insertions(+), 17 deletions(-)
> >>>>
> >>>> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
> >>>> index 7155db3..960a36c 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))));
> >>>>      }
> > 
> > This part seems separate to what comes below I think.  This should be
> > a separate commit.
> > 
> 
> Okay, good point.  That should be easy.
> 
> >>>>  
> >>>> -  /* 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.  */
> > 
> > You need to expand on what "problems" means here.  Someone coming back
> > to this code in the future will have no idea why we're making this
> > change, and with no tests for this commit they can't even try to
> > figure out the "problems" by looking at a test.
> > 
> 
> I will try to explain that better, yes.
> 
> >>>>    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 || e->line == 0)
> >>>> +	    break;
> >>>> +	  e->is_stmt = 0;
> >>>>  	}
> >>>> +      while (e > subfile->line_vector->item);
> >>>>      }
> >>>>  
> >>>>    e = subfile->line_vector->item + subfile->line_vectoms++;
> >>>>
> >>
> >> Andrew, this is the place where currently the is-stmt entries
> >> are deleted.  With your is-stmt patch this code is executed in more
> >> cases than before.  Therefore I would suggest to convert them
> >> to !is_stmt lines for now, but maybe in the long run add a new flag
> >> that allows them to be used in the file:line case, but make these
> >> lines behave differently when stepping, I am only trying to fix
> >> the case where you step out of the subroutine.
> > 
> > I'm super uncomfortable with any code that changes is-stmt to
> > !is-stmt, as I worry about what we might be giving up.  You say "All
> > we lose is the ability to set breakpoints at some lines which contain
> > no instructions anyway.", but I'll need to work through some examples
> > to see what this actually means in practice before I can be happy with
> > this change.
> > 
> 
> There is no pressure from my side to do anything about it.
> I am just saying is-stmt -> !is-stmt is better than removing
> is-stmt lines that are at the same PC by chance.

You're absolutely right, I miss-understood what was going on here. I
think if you split the two parts of the patch, and could expand on the
description a bit then this should be fine.

My understanding of the "problem" here is that lines appear
within one subfile at the same address that we switch to some other
subfile.  As such I think, the address will be attributed to the
second subfile, and we shouldn't be reporting lines for the first
subfile.

Hopefully you can expand that more with your understanding.

Thanks,
Andrew



> 
> I will come up with an updated patch, eventually, but will need
> to spend more time on the openssl project now, to meet the schedule for the
> next release.
> 
> 
> Bernd.

Re: [PATCHv2] Fix an undefined behavior in record_line

[Bernd Edlinger]

On 3/25/20 12:08 PM, Andrew Burgess wrote:
> * Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-03-24 11:20:25 +0100]:
> 
>>
>>
>> On 3/24/20 10:10 AM, Andrew Burgess wrote:
>>> * Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-03-23 22:25:42 +0100]:
>>>
>>>> On 3/22/20 4:25 AM, Bernd Edlinger wrote:
>>>>> On 3/13/20 12:55 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.
>>>
>>> I suspect this commit message has evolved overtime - having the first
>>> word be "additionally" seems a little strange.
>>>
>>
>> I'll re-think the commit message, thanks.
>>
>>>>>>
>>>>>> 2020-03-10  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>>>> 	* buildsym.c (record_line): Fix ub and preserve lines at eof.
>>>
>>> Typo: ub -> up
>>>
>>>>>> ---
>>>>>>  gdb/buildsym.c | 28 +++++++++++-----------------
>>>>>>  1 file changed, 11 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
>>>>>> index 7155db3..960a36c 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))));
>>>>>>      }
>>>
>>> This part seems separate to what comes below I think.  This should be
>>> a separate commit.
>>>
>>
>> Okay, good point.  That should be easy.
>>
>>>>>>  
>>>>>> -  /* 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.  */
>>>
>>> You need to expand on what "problems" means here.  Someone coming back
>>> to this code in the future will have no idea why we're making this
>>> change, and with no tests for this commit they can't even try to
>>> figure out the "problems" by looking at a test.
>>>
>>
>> I will try to explain that better, yes.
>>
>>>>>>    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 || e->line == 0)
>>>>>> +	    break;
>>>>>> +	  e->is_stmt = 0;
>>>>>>  	}
>>>>>> +      while (e > subfile->line_vector->item);
>>>>>>      }
>>>>>>  
>>>>>>    e = subfile->line_vector->item + subfile->line_vectoms++;
>>>>>>
>>>>
>>>> Andrew, this is the place where currently the is-stmt entries
>>>> are deleted.  With your is-stmt patch this code is executed in more
>>>> cases than before.  Therefore I would suggest to convert them
>>>> to !is_stmt lines for now, but maybe in the long run add a new flag
>>>> that allows them to be used in the file:line case, but make these
>>>> lines behave differently when stepping, I am only trying to fix
>>>> the case where you step out of the subroutine.
>>>
>>> I'm super uncomfortable with any code that changes is-stmt to
>>> !is-stmt, as I worry about what we might be giving up.  You say "All
>>> we lose is the ability to set breakpoints at some lines which contain
>>> no instructions anyway.", but I'll need to work through some examples
>>> to see what this actually means in practice before I can be happy with
>>> this change.
>>>
>>
>> There is no pressure from my side to do anything about it.
>> I am just saying is-stmt -> !is-stmt is better than removing
>> is-stmt lines that are at the same PC by chance.
> 
> You're absolutely right, I miss-understood what was going on here. I
> think if you split the two parts of the patch, and could expand on the
> description a bit then this should be fine.
> 
> My understanding of the "problem" here is that lines appear
> within one subfile at the same address that we switch to some other
> subfile.  As such I think, the address will be attributed to the
> second subfile, and we shouldn't be reporting lines for the first
> subfile.
> 
> Hopefully you can expand that more with your understanding.
> 

I am just a bit tired in the moment :)
as I did not get much sleep lately.
I think just be patient with me, it is on
my TODO list, but I better take the time I need.


Thanks
Bernd.

> Thanks,
> Andrew
> 
> 
> 
>>
>> I will come up with an updated patch, eventually, but will need
>> to spend more time on the openssl project now, to meet the schedule for the
>> next release.
>>
>>
>> Bernd.

Re: [PATCHv2] Fix an undefined behavior in record_line

[Bernd Edlinger]

Am 24.03.20 um 10:10 schrieb Andrew Burgess:
> * Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-03-23 22:25:42 +0100]:
> 
>> On 3/22/20 4:25 AM, Bernd Edlinger wrote:
>>> On 3/13/20 12:55 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.
> 
> I suspect this commit message has evolved overtime - having the first
> word be "additionally" seems a little strange.
> 
>>>>
>>>> 2020-03-10  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>>> 	* buildsym.c (record_line): Fix ub and preserve lines at eof.
> 
> Typo: ub -> up
> 

Ah, I just realized this is a GCC-slang word for undefined behavior
we always just say "ub" when we mean the code has undefined behavior.
Will change to "undefined behavior" and have two lines in commit message instead.

>>>> ---
>>>>  gdb/buildsym.c | 28 +++++++++++-----------------
>>>>  1 file changed, 11 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
>>>> index 7155db3..960a36c 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))));
>>>>      }
> 
> This part seems separate to what comes below I think.  This should be
> a separate commit.
> 
>>>>  
>>>> -  /* 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.  */
> 
> You need to expand on what "problems" means here.  Someone coming back
> to this code in the future will have no idea why we're making this
> change, and with no tests for this commit they can't even try to
> figure out the "problems" by looking at a test.
> 
>>>>    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 || e->line == 0)
>>>> +	    break;
>>>> +	  e->is_stmt = 0;
>>>>  	}
>>>> +      while (e > subfile->line_vector->item);
>>>>      }
>>>>  
>>>>    e = subfile->line_vector->item + subfile->line_vectoms++;
>>>>
>>
>> Andrew, this is the place where currently the is-stmt entries
>> are deleted.  With your is-stmt patch this code is executed in more
>> cases than before.  Therefore I would suggest to convert them
>> to !is_stmt lines for now, but maybe in the long run add a new flag
>> that allows them to be used in the file:line case, but make these
>> lines behave differently when stepping, I am only trying to fix
>> the case where you step out of the subroutine.
> 
> I'm super uncomfortable with any code that changes is-stmt to
> !is-stmt, as I worry about what we might be giving up.  You say "All
> we lose is the ability to set breakpoints at some lines which contain
> no instructions anyway.", but I'll need to work through some examples
> to see what this actually means in practice before I can be happy with
> this change.
> 
> Thanks,
> Andrew
>