Re: [PATCHv2] Fix an undefined behavior in record_line

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

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.