Re: [PATCH v3 2/2] Fix an undefined behavior in record_line

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

On 4/4/20 12:53 AM, Luis Machado wrote:
> Hi,
> 
> This seems to have caused a few regressions for aarch64-linux. I'm seeing the following:
> 
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: lo-cold: step-test-3: step into foo from main
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: lo-cold: step-test-3: step into bar from foo
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: lo-cold: step-test-3: step out of bar to foo
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: lo-cold: step-test-3: step into foo_cold from foo
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: lo-cold: step-test-3: step into baz from foo_cold
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: lo-cold: step-test-3: step out of baz to foo_cold
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: lo-cold: step-test-3: step out of foo_cold to foo
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: lo-cold: step-test-3: step out of foo to main
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: hi-cold: step-test-3: step into foo from main
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: hi-cold: step-test-3: step into bar from foo
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: hi-cold: step-test-3: step out of bar to foo
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: hi-cold: step-test-3: step into foo_cold from foo
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: hi-cold: step-test-3: step into baz from foo_cold
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: hi-cold: step-test-3: step out of baz to foo_cold
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: hi-cold: step-test-3: step out of foo_cold to foo
> FAIL: gdb.dwarf2/dw2-ranges-func.exp: hi-cold: step-test-3: step out of foo to main
> 
> git bisect pointed at this commit:
> 

Oh, dear.

Andrew, please watch out,

your other patch is also about to
change something in this area.

I tested on x86_64 where everything looked good,
(at least for me, but sime test cases are always faling
or are unstable ...)

It could be that your patch

PATCH 2/2] gdb: Preserve is-stmt lines when switch between files

I just saw in my inbox is also trying to address the same issue.

I was not aware that you were working on the same issue.


Thanks
Bernd.

> ---
> 
> 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.
> 
>     2020-04-01  Bernd Edlinger  <bernd.edlinger@hotmail.de>
> 
>             * buildsym.c (record_line): Fix undefined behavior and preserve lines at eof.
> 
> ---
> 
> What i see in the log is stepping through lines not working as expected.
> 
> 
> On 3/27/20 12:50 AM, Bernd Edlinger wrote:
>> Additionally do not completely remove symbols
>> at the same PC than the end marker, instead
>> make them non-is-stmt breakpoints.
>>
>> 2020-03-27  Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>     * buildsym.c (record_line): Fix undefined behavior and preserve
>>     lines at eof.
>> ---
>>   gdb/buildsym.c | 34 ++++++++++++++++++----------------
>>   1 file changed, 18 insertions(+), 16 deletions(-)
>>
>> diff --git a/gdb/buildsym.c b/gdb/buildsym.c
>> index 2d1e441..46c5bb1 100644
>> --- a/gdb/buildsym.c
>> +++ b/gdb/buildsym.c
>> @@ -705,27 +705,29 @@ 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 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)
>>       {
>> -      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_vector->nitems++;
>>