[PATCH] gdb: Fix false match issue in skip_prologue_using_linetable

[PATCH] gdb: Fix false match issue in skip_prologue_using_linetable

[WANG Rui]

We should exclude matches to the ending PC to prevent false matches with the
next function, as prologue_end is located at the end PC.

  <fun1>:
    0x00: ... <-- start_pc
    0x04: ...
    0x08: ... <-- breakpoint
    0x0c: ret
  <fun2>:
    0x10: ret <-- end_pc | prologue_end of fun2
---
 gdb/symtab.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/symtab.c b/gdb/symtab.c
index f2b1a14e006..a662d7d1869 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3735,7 +3735,7 @@ skip_prologue_using_linetable (CORE_ADDR func_addr)
 	 });
 
       for (;
-	   it < linetable->item + linetable->nitems && it->pc <= end_pc;
+	   it < linetable->item + linetable->nitems && it->pc < end_pc;
 	   it++)
 	if (it->prologue_end)
 	  return {it->pc};
-- 
2.40.0

Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable

[Keith Seitz]

On 4/17/23 09:24, WANG Rui wrote:
> We should exclude matches to the ending PC to prevent false matches with the
> next function, as prologue_end is located at the end PC.
> 
>    <fun1>:
>      0x00: ... <-- start_pc
>      0x04: ...
>      0x08: ... <-- breakpoint
>      0x0c: ret
>    <fun2>:
>      0x10: ret <-- end_pc | prologue_end of fun2

Thank you for the patch. Indeed, my recollection is that we always
record/search for pc's in [start, end). find_pc_partial_function seems to
concur.

> ---
>   gdb/symtab.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index f2b1a14e006..a662d7d1869 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -3735,7 +3735,7 @@ skip_prologue_using_linetable (CORE_ADDR func_addr)
>   	 });
>   
>         for (;
> -	   it < linetable->item + linetable->nitems && it->pc <= end_pc;
> +	   it < linetable->item + linetable->nitems && it->pc < end_pc;
>   	   it++)
>   	if (it->prologue_end)
>   	  return {it->pc};

This appears to be against gdb 13 and will need to be rebased.

I have regression tested this on x86_64 and found nothing of concern.
[The patch which introduced this function contained a test case,
gdb.dwarf2/dw2-prologue-end.exp, and that test also shows no regressions.]

I have to ask, though, is there a way to write a test case for this? Maybe
by using dw2-prologue-end.exp as an example?

Keith

Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable

[Lancelot SIX]

> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index f2b1a14e006..a662d7d1869 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -3735,7 +3735,7 @@ skip_prologue_using_linetable (CORE_ADDR func_addr)
>           });
> 
>         for (;
> -          it < linetable->item + linetable->nitems && it->pc <= end_pc;
> +          it < linetable->item + linetable->nitems && it->pc < end_pc;
>             it++)
>          if (it->prologue_end)
>            return {it->pc};

Hi Rui, thanks for spotting this.

I am not a maintainer, so I can only comment.

I do not think this patch applies cleanly to the master branch, but the 
change should be trivial.  That being said, it is true that 
find_pc_partial_function returns the first address past the end of the 
function, so the change looks good to me.  Thanks for spotting this!

Best,
Lancelot.

Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable

[hev]

[-- Attachment #1: Type: text/plain, Size: 2281 bytes --]

Hello Keith, Thanks for your comments.

On Tue, Apr 18, 2023 at 1:38 AM Keith Seitz <keiths@redhat.com> wrote:
>
> On 4/17/23 09:24, WANG Rui wrote:
> > We should exclude matches to the ending PC to prevent false matches
with the
> > next function, as prologue_end is located at the end PC.
> >
> >    <fun1>:
> >      0x00: ... <-- start_pc
> >      0x04: ...
> >      0x08: ... <-- breakpoint
> >      0x0c: ret
> >    <fun2>:
> >      0x10: ret <-- end_pc | prologue_end of fun2
>
> Thank you for the patch. Indeed, my recollection is that we always
> record/search for pc's in [start, end). find_pc_partial_function seems to
> concur.
>
> > ---
> >   gdb/symtab.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > index f2b1a14e006..a662d7d1869 100644
> > --- a/gdb/symtab.c
> > +++ b/gdb/symtab.c
> > @@ -3735,7 +3735,7 @@ skip_prologue_using_linetable (CORE_ADDR
func_addr)
> >        });
> >
> >         for (;
> > -        it < linetable->item + linetable->nitems && it->pc <= end_pc;
> > +        it < linetable->item + linetable->nitems && it->pc < end_pc;
> >          it++)
> >       if (it->prologue_end)
> >         return {it->pc};
>
> This appears to be against gdb 13 and will need to be rebased.
>
> I have regression tested this on x86_64 and found nothing of concern.
> [The patch which introduced this function contained a test case,
> gdb.dwarf2/dw2-prologue-end.exp, and that test also shows no regressions.]
>
> I have to ask, though, is there a way to write a test case for this? Maybe
> by using dw2-prologue-end.exp as an example?

I attempted to write a test case, but it did not work. I discovered this
issue while running the Rust debuginfo test[1] on LoongArch. As the
function entry alignment is 4-byte, which is the size of an instruction,
there is no padding between the two functions. This creates a possibility
of matching the start address of the next function. This is unlike x86,
which is why this problem does not occur on x86. I sincerely hope that this
information proves to be beneficial to you.

[1]
https://github.com/rust-lang/rust/blob/7908a1d65496b88626e4b7c193c81d777005d6f3/tests/debuginfo/box.rs

--
Rui

Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable

[hev]

Hello Lancelot, Thanks for your comments.

On Tue, Apr 18, 2023 at 5:13 AM Lancelot SIX <Lancelot.Six@amd.com> wrote:
>
> >
> > diff --git a/gdb/symtab.c b/gdb/symtab.c
> > index f2b1a14e006..a662d7d1869 100644
> > --- a/gdb/symtab.c
> > +++ b/gdb/symtab.c
> > @@ -3735,7 +3735,7 @@ skip_prologue_using_linetable (CORE_ADDR func_addr)
> >           });
> >
> >         for (;
> > -          it < linetable->item + linetable->nitems && it->pc <= end_pc;
> > +          it < linetable->item + linetable->nitems && it->pc < end_pc;
> >             it++)
> >          if (it->prologue_end)
> >            return {it->pc};
>
> Hi Rui, thanks for spotting this.
>
> I am not a maintainer, so I can only comment.
>
> I do not think this patch applies cleanly to the master branch, but the
> change should be trivial.  That being said, it is true that
> find_pc_partial_function returns the first address past the end of the
> function, so the change looks good to me.  Thanks for spotting this!

I realized that I made a mistake. I have been focusing so much on
debugging Rust issues that I forgot to work on the 13 branch. I will
work on the v2 patch. Thank you!

--
Rui

Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable

[Tom de Vries]

On 4/18/23 04:26, hev wrote:
>> I have to ask, though, is there a way to write a test case for this? Maybe
>> by using dw2-prologue-end.exp as an example?

> I attempted to write a test case, but it did not work. I discovered this
> issue while running the Rust debuginfo test[1] on LoongArch. As the
> function entry alignment is 4-byte, which is the size of an instruction,
> there is no padding between the two functions. This creates a possibility
> of matching the start address of the next function. This is unlike x86,
> which is why this problem does not occur on x86. I sincerely hope that this
> information proves to be beneficial to you.

Using this information I managed to write a regression test for this, 
I've attached it to a PR I opened for this issue ( 
https://sourceware.org/bugzilla/show_bug.cgi?id=30369 ).

Thanks,
- Tom

Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable

[hev]

On Tue, Apr 18, 2023 at 4:43 PM Tom de Vries <tdevries@suse.de> wrote:
>
> On 4/18/23 04:26, hev wrote:
> >> I have to ask, though, is there a way to write a test case for this? Maybe
> >> by using dw2-prologue-end.exp as an example?
>
> > I attempted to write a test case, but it did not work. I discovered this
> > issue while running the Rust debuginfo test[1] on LoongArch. As the
> > function entry alignment is 4-byte, which is the size of an instruction,
> > there is no padding between the two functions. This creates a possibility
> > of matching the start address of the next function. This is unlike x86,
> > which is why this problem does not occur on x86. I sincerely hope that this
> > information proves to be beneficial to you.
>
> Using this information I managed to write a regression test for this,
> I've attached it to a PR I opened for this issue (
> https://sourceware.org/bugzilla/show_bug.cgi?id=30369 ).

Awesome! Thank you.

--
Rui

Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable

[Tom de Vries]

On 4/17/23 18:24, WANG Rui wrote:
> We should exclude matches to the ending PC to prevent false matches with the
> next function, as prologue_end is located at the end PC.

Hi Rui,

thanks for the bug-report-and-fix, much appreciated.

If you might make more or larger contributions in the future, please 
consider filing a copyright assignment ( 
https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment 
).

Thanks,
- Tom

Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable

[hev]

Hi Tom,

On Sat, Apr 22, 2023 at 4:36 PM Tom de Vries <tdevries@suse.de> wrote:
>
> On 4/17/23 18:24, WANG Rui wrote:
> > We should exclude matches to the ending PC to prevent false matches with the
> > next function, as prologue_end is located at the end PC.
>
> Hi Rui,
>
> thanks for the bug-report-and-fix, much appreciated.
>
> If you might make more or larger contributions in the future, please
> consider filing a copyright assignment (
> https://sourceware.org/gdb/wiki/ContributionChecklist#FSF_copyright_Assignment
> ).

Thank you for suggesting that I consider filing a copyright assignment
with the FSF if I plan on making more contributions in the future. I
appreciate your guidance and will definitely keep it in mind. :)

Thanks
Rui