gdb: Fix false match issue in skip_prologue_using_linetable

  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(-)
  

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
  

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

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
  

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
  

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
  

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
  

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
  

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