* [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable
@ 2023-04-17 16:24 WANG Rui
2023-04-17 17:38 ` Keith Seitz
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: WANG Rui @ 2023-04-17 16:24 UTC (permalink / raw)
To: gdb-patches; +Cc: Lancelot SIX, 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable
2023-04-17 16:24 [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable WANG Rui
@ 2023-04-17 17:38 ` Keith Seitz
2023-04-18 2:26 ` hev
2023-04-17 21:12 ` Lancelot SIX
2023-04-22 8:36 ` Tom de Vries
2 siblings, 1 reply; 9+ messages in thread
From: Keith Seitz @ 2023-04-17 17:38 UTC (permalink / raw)
To: WANG Rui, gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable
2023-04-17 16:24 [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable WANG Rui
2023-04-17 17:38 ` Keith Seitz
@ 2023-04-17 21:12 ` Lancelot SIX
2023-04-18 2:26 ` hev
2023-04-22 8:36 ` Tom de Vries
2 siblings, 1 reply; 9+ messages in thread
From: Lancelot SIX @ 2023-04-17 21:12 UTC (permalink / raw)
To: WANG Rui, gdb-patches
>
> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable
2023-04-17 17:38 ` Keith Seitz
@ 2023-04-18 2:26 ` hev
2023-04-18 8:43 ` Tom de Vries
0 siblings, 1 reply; 9+ messages in thread
From: hev @ 2023-04-18 2:26 UTC (permalink / raw)
To: Keith Seitz; +Cc: gdb-patches
[-- 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable
2023-04-17 21:12 ` Lancelot SIX
@ 2023-04-18 2:26 ` hev
0 siblings, 0 replies; 9+ messages in thread
From: hev @ 2023-04-18 2:26 UTC (permalink / raw)
To: Lancelot SIX; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable
2023-04-18 2:26 ` hev
@ 2023-04-18 8:43 ` Tom de Vries
2023-04-18 9:59 ` hev
0 siblings, 1 reply; 9+ messages in thread
From: Tom de Vries @ 2023-04-18 8:43 UTC (permalink / raw)
To: hev, Keith Seitz; +Cc: gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable
2023-04-18 8:43 ` Tom de Vries
@ 2023-04-18 9:59 ` hev
0 siblings, 0 replies; 9+ messages in thread
From: hev @ 2023-04-18 9:59 UTC (permalink / raw)
To: Tom de Vries; +Cc: Keith Seitz, gdb-patches
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable
2023-04-17 16:24 [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable WANG Rui
2023-04-17 17:38 ` Keith Seitz
2023-04-17 21:12 ` Lancelot SIX
@ 2023-04-22 8:36 ` Tom de Vries
2023-04-23 1:24 ` hev
2 siblings, 1 reply; 9+ messages in thread
From: Tom de Vries @ 2023-04-22 8:36 UTC (permalink / raw)
To: WANG Rui, gdb-patches; +Cc: Lancelot SIX
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable
2023-04-22 8:36 ` Tom de Vries
@ 2023-04-23 1:24 ` hev
0 siblings, 0 replies; 9+ messages in thread
From: hev @ 2023-04-23 1:24 UTC (permalink / raw)
To: Tom de Vries; +Cc: gdb-patches, Lancelot SIX
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-04-23 1:25 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-17 16:24 [PATCH] gdb: Fix false match issue in skip_prologue_using_linetable WANG Rui
2023-04-17 17:38 ` Keith Seitz
2023-04-18 2:26 ` hev
2023-04-18 8:43 ` Tom de Vries
2023-04-18 9:59 ` hev
2023-04-17 21:12 ` Lancelot SIX
2023-04-18 2:26 ` hev
2023-04-22 8:36 ` Tom de Vries
2023-04-23 1:24 ` hev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).