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

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

Hi Andrew,

On 4/6/20 7:44 PM, Andrew Burgess wrote:
> * Andrew Burgess <andrew.burgess@embecosm.com> [2020-04-05 00:03:27 +0100]:
> 

Hmm, I overlooked this mail in my inbox.  Sorry, I had a very
difficul discussion with Linus Torvalds at that weekend.  So I was not
able to look at all messages, and this one dropped. Sorry for that.

>> * Bernd Edlinger <bernd.edlinger@hotmail.de> [2020-03-27 04:50:29 +0100]:
>>
>>> 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.  */
>>
>> Out of interest I tried reverting this patch and don't see any
>> failures in gdb.cp/step-and-next-inline.exp.  Could you expand on
>> which tests specifically you expect to see fail, and maybe which
>> version of GCC you're using?  I'm on 9.3.1.  It'll be Monday before I
>> can try my other machine which has a wider selection of compiler
>> versions.
>>
>> I also don't understand what part of the previous behaviour was
>> undefined, could you help me to understand please.

Sorry, as I said, I overlooked this mail, I am rather under stress,
since I have to take precautions for my safety and my family all the time,
you know we have a corona pandemic out there.

It is decrementing e before the beginning of the array, that is undefined.

The code before, with the undefined behavior was this:

      e = subfile->line_vector->item + subfile->line_vector->nitems - 1;
      while (subfile->line_vector->nitems > 0 && e->pc == pc)
        {
          e--;
          subfile->line_vector->nitems--;
        }

So when you start e = pointer + nitems - 1;
decrement each time e and nitems by one.
and the exit condition is nitems > 0 or e -> pc == pc.
What happens when there is no element with pc == pc,
then the exit condition is when nitems = 0 and e = pointer - 1;
since e is now before the beginning of the array, gcc can
assume you guarantee that this undefined behavior is not happening
so you guarantee the e-> pc == pc is the only exit condition where
no undefined behavior happens.

so gcc can and will transform that loop to:

      e = subfile->line_vector->item + subfile->line_vector->nitems - 1;
      while (e->pc == pc)
        {
          e--;
          subfile->line_vector->nitems--;
        }

That has already happened, in the gmp test suite if I remember correctly.

> 
> I reverted this patch and tested with GCC versions:
> 

That part of the patch was a gratuitous fix, I have been testing it twice
before committing, but since Luis notified me of the regression in aarch64
I became aware that changing the is-stmt bits causes real problems and I
wanted to fix that with the other patch, but I can also make that an extra
patch, if that is necessary.

>   10.x - from 2020-02-05 - no regressions,
>   9.3.1 - (from previous) - no regressions,
>   9.2.0 - no regressions,
>   8.3.0 - no regressions,
>   7.4.0 - doesn't compile as the compiler doesn't support
>           '-gstatement-frontiers.'.
> 
> The 7.4.0 result seems to indicate that the test doesn't apply for
> compilers before then.
> 
> So, what exactly does this patch fix?  Or what new functionality does
> it bring to GDB?  Or what version of GCC should I use and expect to
> see a difference in the test results?
> 

Undefined behavior, and I need to really remove the lines at the
end of the function.  That behavior we need to restore.

I would like to not do the destructive step when the cu changes,
that is probably wrong, and causes the missing lines that your other
patch is trying to fix.


Thanks,
Bernd.


PS: what is T-J-Teru ?