Re: [PATCHv2] Fix an undefined behavior in record_line

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

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.