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

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Bernd Edlinger-2
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  <[hidden email]>
        * 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++;
--
1.9.1
Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
>>>>> "Bernd" == Bernd Edlinger <[hidden email]> writes:

Bernd> Additionally do not completely remove symbols
Bernd> at the same PC than the end marker, instead
Bernd> make them non-is-stmt breakpoints.

Bernd> 2020-03-27  Bernd Edlinger  <[hidden email]>
Bernd> * buildsym.c (record_line): Fix undefined behavior and preserve
Bernd> lines at eof.

IIUC this fixes:

Bernd> +     give surprising results.  Expect gdb.cp/step-and-next-inline.exp
Bernd> +     to fail if these lines are not modified here.  */

... but doesn't regress anything else?

In that case I think it's fine.  Thank you.

Tom
Reply | Threaded
Open this post in threaded view
|

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

Bernd Edlinger-2
On 4/1/20 6:23 PM, Tom Tromey wrote:

>>>>>> "Bernd" == Bernd Edlinger <[hidden email]> writes:
>
> Bernd> Additionally do not completely remove symbols
> Bernd> at the same PC than the end marker, instead
> Bernd> make them non-is-stmt breakpoints.
>
> Bernd> 2020-03-27  Bernd Edlinger  <[hidden email]>
> Bernd> * buildsym.c (record_line): Fix undefined behavior and preserve
> Bernd> lines at eof.
>
> IIUC this fixes:
>
> Bernd> +     give surprising results.  Expect gdb.cp/step-and-next-inline.exp
> Bernd> +     to fail if these lines are not modified here.  */
>
> ... but doesn't regress anything else?
>

I did my best to compare the test results with and without this
patch, but I have plenty of time, and can repeat that test before
I commit, just to make sure that it is still the case.

> In that case I think it's fine.  Thank you.
>
> Tom
>

Thanks
Bernd.
Reply | Threaded
Open this post in threaded view
|

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

Bernd Edlinger-2
On 4/1/20 6:52 PM, Bernd Edlinger wrote:

> On 4/1/20 6:23 PM, Tom Tromey wrote:
>>>>>>> "Bernd" == Bernd Edlinger <[hidden email]> writes:
>>
>> Bernd> Additionally do not completely remove symbols
>> Bernd> at the same PC than the end marker, instead
>> Bernd> make them non-is-stmt breakpoints.
>>
>> Bernd> 2020-03-27  Bernd Edlinger  <[hidden email]>
>> Bernd> * buildsym.c (record_line): Fix undefined behavior and preserve
>> Bernd> lines at eof.
>>
>> IIUC this fixes:
>>
>> Bernd> +     give surprising results.  Expect gdb.cp/step-and-next-inline.exp
>> Bernd> +     to fail if these lines are not modified here.  */
>>
>> ... but doesn't regress anything else?
>>
>
> I did my best to compare the test results with and without this
> patch, but I have plenty of time, and can repeat that test before
> I commit, just to make sure that it is still the case.
>
>> In that case I think it's fine.  Thank you.
>>
>> Tom
>>
>
> Thanks
> Bernd.
>

One last question, does this approval also include
part 1/2 "Fix the resizing condition of the line table" ?


Thanks
Bernd.
Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
>>>>> "Bernd" == Bernd Edlinger <[hidden email]> writes:

Bernd> One last question, does this approval also include
Bernd> part 1/2 "Fix the resizing condition of the line table" ?

Yeah...  I sent a separate note about that.

https://sourceware.org/pipermail/gdb-patches/2020-April/167214.html

Anyway, no worries, please go ahead :)

Tom
Reply | Threaded
Open this post in threaded view
|

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

Bernd Edlinger-2
On 4/1/20 8:53 PM, Tom Tromey wrote:

>>>>>> "Bernd" == Bernd Edlinger <[hidden email]> writes:
>
> Bernd> One last question, does this approval also include
> Bernd> part 1/2 "Fix the resizing condition of the line table" ?
>
> Yeah...  I sent a separate note about that.
>
> https://sourceware.org/pipermail/gdb-patches/2020-April/167214.html
>
> Anyway, no worries, please go ahead :)
>
> Tom
>

Ah, interesting, not in my inbox and not in my spam folder either,
I think I did recently also not receive a response to one of my linux
patches, which I also saw only on the mailing list archives, but
I was wondering why I did not receive it in the first place.
Did this message bounce, or is it grey-listed, I don't think hotmail.de
does this, but you never know.


Thanks
Bernd.
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
In reply to this post by Bernd Edlinger-2
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:

---

commit 64dc2d4bd24ff7119c913fff91184414f09b8042
Author: Bernd Edlinger <[hidden email]>
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  <[hidden email]>

             * 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  <[hidden email]>
> * 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++;
>
Reply | Threaded
Open this post in threaded view
|

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

Bernd Edlinger-2


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 <[hidden email]>
> 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  <[hidden email]>
>
>             * 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  <[hidden email]>
>>     * 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++;
>>
Reply | Threaded
Open this post in threaded view
|

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

Bernd Edlinger-2
On 4/4/20 6:21 AM, Bernd Edlinger wrote:

>
>
> 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:
>>

Louis,

So, I cannot do much, to debug aarch64 here.
I would however like to know what the output of the test result is,
when it fails, that is where the step does stop when it is not where it should.

And how the line table looks in the test case when it is compiled on aarch64,
btw. which gcc version do you use?

Thanks
Bernd.

>
> 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 <[hidden email]>
>> 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  <[hidden email]>
>>
>>             * 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  <[hidden email]>
>>>     * 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++;
>>>
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
Hi Bernd,

On 4/4/20 4:06 AM, Bernd Edlinger wrote:

> On 4/4/20 6:21 AM, Bernd Edlinger wrote:
>>
>>
>> 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:
>>>
>
> Louis,
>
> So, I cannot do much, to debug aarch64 here.
> I would however like to know what the output of the test result is,
> when it fails, that is where the step does stop when it is not where it should.
On a quick look, we're stopping at the wrong location during an attempt
to break at "main". I think things just derail from there.

For now, please find attached a couple logs, one without regressions and
one with the failing tests.

Also, I've attached the decoded/raw line information for both binaries
from this testcase. I can play with it further to get more information
if you need. Hopefully this data is useful for now.

Let me know otherwise.

>
> And how the line table looks in the test case when it is compiled on aarch64,
> btw. which gcc version do you use?

The compiler version is gcc version 7.4.0 (Ubuntu/Linaro
7.4.0-1ubuntu1~18.04.1).

>
> Thanks
> Bernd.
>
>>
>> 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 <[hidden email]>
>>> 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  <[hidden email]>
>>>
>>>              * 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  <[hidden email]>
>>>>      * 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++;
>>>>

dw2-ranges-func-hi-cold.l (3K) Download Attachment
dw2-ranges-func-hi-cold.L (1K) Download Attachment
dw2-ranges-func-lo-cold.l (3K) Download Attachment
dw2-ranges-func-lo-cold.L (1K) Download Attachment
dw_bad.log (97K) Download Attachment
dw_good.log (97K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Bernd Edlinger-2
Hi Luis,

the attachments seem to be twice the lo-cold.c linetables,
I wonder if the hi-cold.c linetable are missing, to somehow
make the picture complete?

Thanks
Bernd.

On 4/4/20 3:56 PM, Luis Machado wrote:

> Hi Bernd,
>
> On 4/4/20 4:06 AM, Bernd Edlinger wrote:
>> On 4/4/20 6:21 AM, Bernd Edlinger wrote:
>>>
>>>
>>> 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:
>>>>
>>
>> Louis,
>>
>> So, I cannot do much, to debug aarch64 here.
>> I would however like to know what the output of the test result is,
>> when it fails, that is where the step does stop when it is not where it should.
>
> On a quick look, we're stopping at the wrong location during an attempt to break at "main". I think things just derail from there.
>
> For now, please find attached a couple logs, one without regressions and one with the failing tests.
>
> Also, I've attached the decoded/raw line information for both binaries from this testcase. I can play with it further to get more information if you need. Hopefully this data is useful for now.
>
> Let me know otherwise.
>
>>
>> And how the line table looks in the test case when it is compiled on aarch64,
>> btw. which gcc version do you use?
>
> The compiler version is gcc version 7.4.0 (Ubuntu/Linaro 7.4.0-1ubuntu1~18.04.1).
>
>>
>> Thanks
>> Bernd.
>>
>>>
>>> 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 <[hidden email]>
>>>> 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  <[hidden email]>
>>>>
>>>>              * 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  <[hidden email]>
>>>>>      * 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++;
>>>>>
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
Hi Bernd,

I messed it up, sorry. Here's the line table information again.

Regards,
Luis

On 4/4/20 1:06 PM, Bernd Edlinger wrote:

> Hi Luis,
>
> the attachments seem to be twice the lo-cold.c linetables,
> I wonder if the hi-cold.c linetable are missing, to somehow
> make the picture complete?
>
> Thanks
> Bernd.
>
> On 4/4/20 3:56 PM, Luis Machado wrote:
>> Hi Bernd,
>>
>> On 4/4/20 4:06 AM, Bernd Edlinger wrote:
>>> On 4/4/20 6:21 AM, Bernd Edlinger wrote:
>>>>
>>>>
>>>> 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:
>>>>>
>>>
>>> Louis,
>>>
>>> So, I cannot do much, to debug aarch64 here.
>>> I would however like to know what the output of the test result is,
>>> when it fails, that is where the step does stop when it is not where it should.
>>
>> On a quick look, we're stopping at the wrong location during an attempt to break at "main". I think things just derail from there.
>>
>> For now, please find attached a couple logs, one without regressions and one with the failing tests.
>>
>> Also, I've attached the decoded/raw line information for both binaries from this testcase. I can play with it further to get more information if you need. Hopefully this data is useful for now.
>>
>> Let me know otherwise.
>>
>>>
>>> And how the line table looks in the test case when it is compiled on aarch64,
>>> btw. which gcc version do you use?
>>
>> The compiler version is gcc version 7.4.0 (Ubuntu/Linaro 7.4.0-1ubuntu1~18.04.1).
>>
>>>
>>> Thanks
>>> Bernd.
>>>
>>>>
>>>> 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 <[hidden email]>
>>>>> 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  <[hidden email]>
>>>>>
>>>>>               * 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  <[hidden email]>
>>>>>>       * 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++;
>>>>>>

dw2-ranges-func-hi-cold.l (3K) Download Attachment
dw2-ranges-func-hi-cold.L (1K) Download Attachment
dw2-ranges-func-lo-cold.l (3K) Download Attachment
dw2-ranges-func-lo-cold.L (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Bernd Edlinger-2
Okay, I think I see what is wrong.

 Line Number Statements:
  [0x000000b4]  Extended opcode 2: set Address to 0x73c
  [0x000000bf]  Advance Line by 75 to 76
  [0x000000c2]  Copy

....

  [0x00000129]  Extended opcode 2: set Address to 0x73c
  [0x00000134]  Advance Line by 1 to 73
  [0x00000136]  Copy
  [0x00000137]  Extended opcode 1: End of Sequence

so the second line was previously deleted,
but now it is a non-is-stmt line.
The address is the same,

There was previously a discussion that two consecutive line-entries are a
rogue method to notify the debugger of where the end of header is.
I don't recall in the moment where this code is located.
But if someone could point out the place to me, I could
probably add code to ignore non-is-stmt lines,

Andrew, are you already on that target, with one of your patches from
yesterday?


Thanks
Bernd.
Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
* Bernd Edlinger <[hidden email]> [2020-04-04 18:34:07 +0200]:

> Okay, I think I see what is wrong.
>
>  Line Number Statements:
>   [0x000000b4]  Extended opcode 2: set Address to 0x73c
>   [0x000000bf]  Advance Line by 75 to 76
>   [0x000000c2]  Copy
>
> ....
>
>   [0x00000129]  Extended opcode 2: set Address to 0x73c
>   [0x00000134]  Advance Line by 1 to 73
>   [0x00000136]  Copy
>   [0x00000137]  Extended opcode 1: End of Sequence
>
> so the second line was previously deleted,
> but now it is a non-is-stmt line.
> The address is the same,
>
> There was previously a discussion that two consecutive line-entries are a
> rogue method to notify the debugger of where the end of header is.
> I don't recall in the moment where this code is located.
> But if someone could point out the place to me, I could
> probably add code to ignore non-is-stmt lines,

It's in symtab.c:skip_prologue_using_sal.

>
> Andrew, are you already on that target, with one of your patches from
> yesterday?

Not really.  The other aarch64 issue only required me to get a target
started, so I was using the gdbsim.  But this is known to be pretty
iffy, you'd probably have more luck building QEMU and running against
that.

Thanks,
Andrew

>
>
> Thanks
> Bernd.
Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
In reply to this post by Bernd Edlinger-2
* Bernd Edlinger <[hidden email]> [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  <[hidden email]>
> * 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.

Thanks,
Andrew



>    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++;
> --
> 1.9.1
Reply | Threaded
Open this post in threaded view
|

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

Bernd Edlinger-2
In reply to this post by Andrew Burgess


On 4/5/20 12:55 AM, Andrew Burgess wrote:

> * Bernd Edlinger <[hidden email]> [2020-04-04 18:34:07 +0200]:
>
>> Okay, I think I see what is wrong.
>>
>>  Line Number Statements:
>>   [0x000000b4]  Extended opcode 2: set Address to 0x73c
>>   [0x000000bf]  Advance Line by 75 to 76
>>   [0x000000c2]  Copy
>>
>> ....
>>
>>   [0x00000129]  Extended opcode 2: set Address to 0x73c
>>   [0x00000134]  Advance Line by 1 to 73
>>   [0x00000136]  Copy
>>   [0x00000137]  Extended opcode 1: End of Sequence
>>
>> so the second line was previously deleted,
>> but now it is a non-is-stmt line.
>> The address is the same,
>>
>> There was previously a discussion that two consecutive line-entries are a
>> rogue method to notify the debugger of where the end of header is.
>> I don't recall in the moment where this code is located.
>> But if someone could point out the place to me, I could
>> probably add code to ignore non-is-stmt lines,
>
> It's in symtab.c:skip_prologue_using_sal.
>
>>
>> Andrew, are you already on that target, with one of your patches from
>> yesterday?
>
> Not really.  The other aarch64 issue only required me to get a target
> started, so I was using the gdbsim.  But this is known to be pretty
> iffy, you'd probably have more luck building QEMU and running against
> that.
>

Yeah, that is on my wish list since a while.

I have no idea how much work it is, I would also be
happy to have an aarch64 QEMU, for testing my openssl
patches, I heard they emulate the full instruction set.


Maybe not today, but what are the necessary steps?


Thanks
Bernd.

> Thanks,
> Andrew
>
>>
>>
>> Thanks
>> Bernd.
Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
In reply to this post by Andrew Burgess
* Andrew Burgess <[hidden email]> [2020-04-05 00:03:27 +0100]:

> * Bernd Edlinger <[hidden email]> [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  <[hidden email]>
> > * 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.

I reverted this patch and tested with GCC versions:

  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?

Thanks,
Andrew




>
> Thanks,
> Andrew
>
>
>
> >    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++;
> > --
> > 1.9.1
Reply | Threaded
Open this post in threaded view
|

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

Bernd Edlinger-2
Hi Andrew,

On 4/6/20 7:44 PM, Andrew Burgess wrote:
> * Andrew Burgess <[hidden email]> [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 <[hidden email]> [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  <[hidden email]>
>>> * 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 ?