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

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

[Tom de Vries]

From: WANG Rui <r@hev.cc>

[ Changes in v2:
  - rebase on trunk
  Changes in v3:
  - add test-case ]

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

Tested on x86_64-linux.

Co-Authored-By: WANG Rui <r@hev.cc> (fix, tiny change [1])
Co-Authored-By: Tom de Vries <tdevries@suse.de> (test-case)

[1] https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html
---
 gdb/symtab.c                                  |   2 +-
 gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.c |  30 +++++
 .../gdb.dwarf2/dw2-prologue-end-2.exp         | 118 ++++++++++++++++++
 3 files changed, 149 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.c
 create mode 100644 gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 9e9798676cb..a789512d60b 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -3705,7 +3705,7 @@ skip_prologue_using_linetable (CORE_ADDR func_addr)
 
       for (;
 	   (it < linetable->item + linetable->nitems
-	    && it->raw_pc () <= unrel_end);
+	    && it->raw_pc () < unrel_end);
 	   it++)
 	if (it->prologue_end)
 	  return {it->pc (objfile)};
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.c b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.c
new file mode 100644
index 00000000000..2f29652b633
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.c
@@ -0,0 +1,30 @@
+/* Copyright 2023 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+int
+main (void)
+{
+  int main = 1; /* Main body.  */
+
+  asm ("foo_label: .global foo_label");
+  int foo = 2; /* Foo body.  */
+  asm ("foo_end: .global foo_end");
+
+  asm ("bar_label: .global bar_label");
+  int bar = 3; /* Bar body.  */
+  asm ("bar_end: .global bar_end");
+
+  return main + foo + bar;
+}
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
new file mode 100644
index 00000000000..488f85f9674
--- /dev/null
+++ b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
@@ -0,0 +1,118 @@
+# Copyright 2023 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# Check that prologue analysis in foo is correct in the presence of a
+# prologue_end marker in immediately following function bar.
+
+load_lib dwarf.exp
+
+# This test can only be run on targets which support DWARF-2 and use gas.
+require dwarf2_support
+
+standard_testfile .c .S
+
+set asm_file [standard_output_file $srcfile2]
+Dwarf::assemble $asm_file {
+    global srcdir subdir srcfile srcfile2
+    declare_labels lines_label
+
+    cu {} {
+	compile_unit {
+	    {language @DW_LANG_C}
+	    {name dw2-prologue-end.c}
+	    {stmt_list ${lines_label} DW_FORM_sec_offset}
+	} {
+	    subprogram {
+		{external 1 flag}
+		{name foo}
+		{low_pc foo_label addr}
+		{high_pc foo_end addr}
+	    }
+	    subprogram {
+		{external 1 flag}
+		{name bar}
+		{low_pc bar_label addr}
+		{high_pc bar_end addr}
+	    }
+	}
+    }
+
+    lines {version 5} lines_label {
+	set diridx [include_dir "${srcdir}/${subdir}"]
+	file_name "$srcfile" $diridx
+
+	program {
+	    DW_LNS_set_file $diridx
+
+	    DW_LNE_set_address foo_label
+	    line [gdb_get_line_number "Foo body"]
+	    DW_LNS_copy
+
+	    DW_LNE_set_address bar_label
+	    DW_LNS_set_prologue_end
+	    line [gdb_get_line_number "Bar body"]
+	    DW_LNS_copy
+
+	    DW_LNE_set_address bar_end
+	    DW_LNE_end_sequence
+	}
+    }
+}
+
+if { [prepare_for_testing "failed to prepare" ${testfile} \
+	  [list $srcfile $asm_file] {nodebug}] } {
+    return -1
+}
+
+# Don't runto main here, otherwise the following doesn't
+# function as regression test for PR30369.
+
+# Get the "break foo" address.
+
+set break_addr ""
+gdb_test_multiple "break foo" "" {
+    -re -wrap "at ($hex):\[^\r\n\]*" {
+	set break_addr $expect_out(1,string)
+	pass $gdb_test_name
+    }
+}
+
+if { $break_addr == "" } {
+    return
+}
+
+# Get the "foo_label" address.
+
+set foo_label_addr ""
+gdb_test_multiple "print /x &foo_label" "" {
+    -re -wrap "= ($hex)" {
+	set foo_label_addr $expect_out(1,string)
+	pass $gdb_test_name
+    }
+}
+
+if { $foo_label_addr == "" } {
+    return
+}
+
+# Check that bar immediately follows foo.  Otherwise the following doesn't
+# function as regression test for PR30369.
+
+gdb_test "print &foo_end == &bar_label" " = 1"
+
+# Check that the breakpoint is set at the expected address.  Regression test
+# for PR30369.
+
+gdb_assert { $break_addr == $foo_label_addr }

base-commit: c2f60ac565f1d369fde98146a16f1d3ef79e1000
-- 
2.35.3

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

[Tom de Vries]

On 4/18/23 14:09, Tom de Vries via Gdb-patches wrote:
> Co-Authored-By: WANG Rui <r@hev.cc> (fix, tiny change [1])
> Co-Authored-By: Tom de Vries <tdevries@suse.de> (test-case)
> 
> [1] https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html

Hi,

I'm not used to deal with these matters, so I'd appreciate some 
review/approval on this.  Is my copyright status assessment correct, and 
did I write it up correctly?

Thanks,
- Tom

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

[hev]

Hello Tom,

On Tue, Apr 18, 2023 at 8:15 PM Tom de Vries <tdevries@suse.de> wrote:
>
> On 4/18/23 14:09, Tom de Vries via Gdb-patches wrote:
> > Co-Authored-By: WANG Rui <r@hev.cc> (fix, tiny change [1])
> > Co-Authored-By: Tom de Vries <tdevries@suse.de> (test-case)
> >
> > [1] https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html
>
> Hi,
>
> I'm not used to deal with these matters, so I'd appreciate some
> review/approval on this.  Is my copyright status assessment correct, and
> did I write it up correctly?

Wow! LGTM. Thanks for your awesome test case. :)

--
Rui

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

[Kevin Buettner]

Hi Tom,

On Tue, 18 Apr 2023 14:15:06 +0200
Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> wrote:

> On 4/18/23 14:09, Tom de Vries via Gdb-patches wrote:
> > Co-Authored-By: WANG Rui <r@hev.cc> (fix, tiny change [1])
> > Co-Authored-By: Tom de Vries <tdevries@suse.de> (test-case)
> > 
> > [1] https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html  
> 
> I'm not used to deal with these matters, so I'd appreciate some 
> review/approval on this.  Is my copyright status assessment correct, and 
> did I write it up correctly?

I refreshed my memory via the link you provided above.  Based on what
is written there, I conclude that Wang Rui's change is not legally
signficant for copyright purposes.

Also, I've looked over the Rui's patch as well as your test case, and
it looks good to me.  So...

Approved-by: Kevin Buettner <kevinb@redhat.com>

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

[Tom de Vries]

On 4/21/23 20:03, Kevin Buettner wrote:
> Hi Tom,
> 
> On Tue, 18 Apr 2023 14:15:06 +0200
> Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> wrote:
> 
>> On 4/18/23 14:09, Tom de Vries via Gdb-patches wrote:
>>> Co-Authored-By: WANG Rui <r@hev.cc> (fix, tiny change [1])
>>> Co-Authored-By: Tom de Vries <tdevries@suse.de> (test-case)
>>>
>>> [1] https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html
>>
>> I'm not used to deal with these matters, so I'd appreciate some
>> review/approval on this.  Is my copyright status assessment correct, and
>> did I write it up correctly?
> 
> I refreshed my memory via the link you provided above.  Based on what
> is written there, I conclude that Wang Rui's change is not legally
> signficant for copyright purposes.
> 
> Also, I've looked over the Rui's patch as well as your test case, and
> it looks good to me.  So...
> 
> Approved-by: Kevin Buettner <kevinb@redhat.com>
> 

Hi Kevin,

Thanks for review.

Committed and also backported to gdb-13-branch, because it was a 12 -> 
13 regression.

Thanks,
- Tom

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

[Luis Machado]

On 4/22/23 09:01, Tom de Vries via Gdb-patches wrote:
> On 4/21/23 20:03, Kevin Buettner wrote:
>> Hi Tom,
>>
>> On Tue, 18 Apr 2023 14:15:06 +0200
>> Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> wrote:
>>
>>> On 4/18/23 14:09, Tom de Vries via Gdb-patches wrote:
>>>> Co-Authored-By: WANG Rui <r@hev.cc> (fix, tiny change [1])
>>>> Co-Authored-By: Tom de Vries <tdevries@suse.de> (test-case)
>>>>
>>>> [1] https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html
>>>
>>> I'm not used to deal with these matters, so I'd appreciate some
>>> review/approval on this.  Is my copyright status assessment correct, and
>>> did I write it up correctly?
>>
>> I refreshed my memory via the link you provided above.  Based on what
>> is written there, I conclude that Wang Rui's change is not legally
>> signficant for copyright purposes.
>>
>> Also, I've looked over the Rui's patch as well as your test case, and
>> it looks good to me.  So...
>>
>> Approved-by: Kevin Buettner <kevinb@redhat.com>
>>
> 
> Hi Kevin,
> 
> Thanks for review.
> 
> Committed and also backported to gdb-13-branch, because it was a 12 -> 13 regression.
> 
> Thanks,
> - Tom
> 

For some reason aarch64 is grumpy with this test, and it FAIL's the last comparison.

Maybe aarch64 is broken in this regard?

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

[Tom de Vries]

On 4/24/23 14:53, Luis Machado wrote:
> On 4/22/23 09:01, Tom de Vries via Gdb-patches wrote:
>> On 4/21/23 20:03, Kevin Buettner wrote:
>>> Hi Tom,
>>>
>>> On Tue, 18 Apr 2023 14:15:06 +0200
>>> Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> wrote:
>>>
>>>> On 4/18/23 14:09, Tom de Vries via Gdb-patches wrote:
>>>>> Co-Authored-By: WANG Rui <r@hev.cc> (fix, tiny change [1])
>>>>> Co-Authored-By: Tom de Vries <tdevries@suse.de> (test-case)
>>>>>
>>>>> [1] 
>>>>> https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html
>>>>
>>>> I'm not used to deal with these matters, so I'd appreciate some
>>>> review/approval on this.  Is my copyright status assessment correct, 
>>>> and
>>>> did I write it up correctly?
>>>
>>> I refreshed my memory via the link you provided above.  Based on what
>>> is written there, I conclude that Wang Rui's change is not legally
>>> signficant for copyright purposes.
>>>
>>> Also, I've looked over the Rui's patch as well as your test case, and
>>> it looks good to me.  So...
>>>
>>> Approved-by: Kevin Buettner <kevinb@redhat.com>
>>>
>>
>> Hi Kevin,
>>
>> Thanks for review.
>>
>> Committed and also backported to gdb-13-branch, because it was a 12 -> 
>> 13 regression.
>>
>> Thanks,
>> - Tom
>>
> 
> For some reason aarch64 is grumpy with this test, and it FAIL's the last 
> comparison.
> 
> Maybe aarch64 is broken in this regard?

Hi Luis,

thanks for reporting this.

I could reproduce it on openSUSE Leap 15.4.

I think there are two independent problems:
- the aarch64 prologue analyzer walks past the end of the function
- the test-case assumes that the prologue analyzer will return the first
   insn in foo, rather that some insn in foo.

The WIP patch below addresses both issues, and allows the test-case to 
pass for me.

[ FWIW, alternatively using some "maint set skip-prologue" value from 
this RFC ( 
https://sourceware.org/pipermail/gdb-patches/2022-August/191343.html ) 
could also suffice to ignore the first problem. ]

Thanks,
- Tom

...
diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
index ec0e51bdaf7..d974595e48f 100644
--- a/gdb/aarch64-tdep.c
+++ b/gdb/aarch64-tdep.c
@@ -917,12 +917,13 @@ aarch64_analyze_prologue_test (void)
  static CORE_ADDR
  aarch64_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
  {
-  CORE_ADDR func_addr, limit_pc;
+  CORE_ADDR func_addr, func_end_addr, limit_pc;

    /* See if we can determine the end of the prologue via the symbol
       table.  If so, then return either PC, or the PC after the
       prologue, whichever is greater.  */
-  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
+  bool func_addr_found = find_pc_partial_function (pc, NULL, 
&func_addr, &func_end_addr);
+  if (func_addr_found)
      {
        CORE_ADDR post_prologue_pc
         = skip_prologue_using_sal (gdbarch, func_addr);
@@ -941,7 +942,8 @@ aarch64_skip_prologue (struct gdbarch *gdbarch, 
CORE_ADDR pc)
    limit_pc = skip_prologue_using_sal (gdbarch, pc);
    if (limit_pc == 0)
      limit_pc = pc + 128;       /* Magic.  */
-
+  limit_pc = std::min (limit_pc, func_end_addr - 4);
+
    /* Try disassembling prologue.  */
    return aarch64_analyze_prologue (gdbarch, pc, limit_pc, NULL);
  }
diff --git a/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp 
b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
index 488f85f9674..c506cfd55cc 100644
--- a/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
+++ b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
@@ -95,15 +95,15 @@ if { $break_addr == "" } {

  # Get the "foo_label" address.

-set foo_label_addr ""
-gdb_test_multiple "print /x &foo_label" "" {
+set bar_label_addr ""
+gdb_test_multiple "print /x &bar_label" "" {
      -re -wrap "= ($hex)" {
-       set foo_label_addr $expect_out(1,string)
+       set bar_label_addr $expect_out(1,string)
         pass $gdb_test_name
      }
  }

-if { $foo_label_addr == "" } {
+if { $bar_label_addr == "" } {
      return
  }

@@ -115,4 +115,4 @@ gdb_test "print &foo_end == &bar_label" " = 1"
  # Check that the breakpoint is set at the expected address. 
Regression test
  # for PR30369.

-gdb_assert { $break_addr == $foo_label_addr }
+gdb_assert { $break_addr < $bar_label_addr }
...

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

[Luis Machado]

On 4/24/23 15:15, Tom de Vries wrote:
> On 4/24/23 14:53, Luis Machado wrote:
>> On 4/22/23 09:01, Tom de Vries via Gdb-patches wrote:
>>> On 4/21/23 20:03, Kevin Buettner wrote:
>>>> Hi Tom,
>>>>
>>>> On Tue, 18 Apr 2023 14:15:06 +0200
>>>> Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> wrote:
>>>>
>>>>> On 4/18/23 14:09, Tom de Vries via Gdb-patches wrote:
>>>>>> Co-Authored-By: WANG Rui <r@hev.cc> (fix, tiny change [1])
>>>>>> Co-Authored-By: Tom de Vries <tdevries@suse.de> (test-case)
>>>>>>
>>>>>> [1] https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html
>>>>>
>>>>> I'm not used to deal with these matters, so I'd appreciate some
>>>>> review/approval on this.  Is my copyright status assessment correct, and
>>>>> did I write it up correctly?
>>>>
>>>> I refreshed my memory via the link you provided above.  Based on what
>>>> is written there, I conclude that Wang Rui's change is not legally
>>>> signficant for copyright purposes.
>>>>
>>>> Also, I've looked over the Rui's patch as well as your test case, and
>>>> it looks good to me.  So...
>>>>
>>>> Approved-by: Kevin Buettner <kevinb@redhat.com>
>>>>
>>>
>>> Hi Kevin,
>>>
>>> Thanks for review.
>>>
>>> Committed and also backported to gdb-13-branch, because it was a 12 -> 13 regression.
>>>
>>> Thanks,
>>> - Tom
>>>
>>
>> For some reason aarch64 is grumpy with this test, and it FAIL's the last comparison.
>>
>> Maybe aarch64 is broken in this regard?
> 
> Hi Luis,
> 
> thanks for reporting this.
> 
> I could reproduce it on openSUSE Leap 15.4.
> 
> I think there are two independent problems:
> - the aarch64 prologue analyzer walks past the end of the function
> - the test-case assumes that the prologue analyzer will return the first
>    insn in foo, rather that some insn in foo.
> 
> The WIP patch below addresses both issues, and allows the test-case to pass for me.
> 
> [ FWIW, alternatively using some "maint set skip-prologue" value from this RFC ( https://sourceware.org/pipermail/gdb-patches/2022-August/191343.html ) could also suffice to ignore the first problem. ]
> 
> Thanks,
> - Tom
> 
> ...
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index ec0e51bdaf7..d974595e48f 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -917,12 +917,13 @@ aarch64_analyze_prologue_test (void)
>   static CORE_ADDR
>   aarch64_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>   {
> -  CORE_ADDR func_addr, limit_pc;
> +  CORE_ADDR func_addr, func_end_addr, limit_pc;
> 
>     /* See if we can determine the end of the prologue via the symbol
>        table.  If so, then return either PC, or the PC after the
>        prologue, whichever is greater.  */
> -  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
> +  bool func_addr_found = find_pc_partial_function (pc, NULL, &func_addr, &func_end_addr);
> +  if (func_addr_found)
>       {
>         CORE_ADDR post_prologue_pc
>          = skip_prologue_using_sal (gdbarch, func_addr);
> @@ -941,7 +942,8 @@ aarch64_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>     limit_pc = skip_prologue_using_sal (gdbarch, pc);
>     if (limit_pc == 0)
>       limit_pc = pc + 128;       /* Magic.  */
> -
> +  limit_pc = std::min (limit_pc, func_end_addr - 4);
> +
>     /* Try disassembling prologue.  */
>     return aarch64_analyze_prologue (gdbarch, pc, limit_pc, NULL);
>   }
> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
> index 488f85f9674..c506cfd55cc 100644
> --- a/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
> +++ b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
> @@ -95,15 +95,15 @@ if { $break_addr == "" } {
> 
>   # Get the "foo_label" address.
> 
> -set foo_label_addr ""
> -gdb_test_multiple "print /x &foo_label" "" {
> +set bar_label_addr ""
> +gdb_test_multiple "print /x &bar_label" "" {
>       -re -wrap "= ($hex)" {
> -       set foo_label_addr $expect_out(1,string)
> +       set bar_label_addr $expect_out(1,string)
>          pass $gdb_test_name
>       }
>   }
> 
> -if { $foo_label_addr == "" } {
> +if { $bar_label_addr == "" } {
>       return
>   }
> 
> @@ -115,4 +115,4 @@ gdb_test "print &foo_end == &bar_label" " = 1"
>   # Check that the breakpoint is set at the expected address. Regression test
>   # for PR30369.
> 
> -gdb_assert { $break_addr == $foo_label_addr }
> +gdb_assert { $break_addr < $bar_label_addr }
> ...

Sorry, I thought I had replied to this thread. Indeed the above patch addresses this problem with the aarch64 prologue skipper,
and it also fixes things for arm. For arm I suspect we might need the same fix to the prologue skipper that the patch addresses for aarch64.

I can pick it up, refresh and submit if you're happy with it as well.

Regards,
Luis

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

[Tom de Vries]

On 5/16/23 16:19, Luis Machado wrote:
> On 4/24/23 15:15, Tom de Vries wrote:
>> On 4/24/23 14:53, Luis Machado wrote:
>>> On 4/22/23 09:01, Tom de Vries via Gdb-patches wrote:
>>>> On 4/21/23 20:03, Kevin Buettner wrote:
>>>>> Hi Tom,
>>>>>
>>>>> On Tue, 18 Apr 2023 14:15:06 +0200
>>>>> Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> wrote:
>>>>>
>>>>>> On 4/18/23 14:09, Tom de Vries via Gdb-patches wrote:
>>>>>>> Co-Authored-By: WANG Rui <r@hev.cc> (fix, tiny change [1])
>>>>>>> Co-Authored-By: Tom de Vries <tdevries@suse.de> (test-case)
>>>>>>>
>>>>>>> [1] 
>>>>>>> https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html
>>>>>>
>>>>>> I'm not used to deal with these matters, so I'd appreciate some
>>>>>> review/approval on this.  Is my copyright status assessment 
>>>>>> correct, and
>>>>>> did I write it up correctly?
>>>>>
>>>>> I refreshed my memory via the link you provided above.  Based on what
>>>>> is written there, I conclude that Wang Rui's change is not legally
>>>>> signficant for copyright purposes.
>>>>>
>>>>> Also, I've looked over the Rui's patch as well as your test case, and
>>>>> it looks good to me.  So...
>>>>>
>>>>> Approved-by: Kevin Buettner <kevinb@redhat.com>
>>>>>
>>>>
>>>> Hi Kevin,
>>>>
>>>> Thanks for review.
>>>>
>>>> Committed and also backported to gdb-13-branch, because it was a 12 
>>>> -> 13 regression.
>>>>
>>>> Thanks,
>>>> - Tom
>>>>
>>>
>>> For some reason aarch64 is grumpy with this test, and it FAIL's the 
>>> last comparison.
>>>
>>> Maybe aarch64 is broken in this regard?
>>
>> Hi Luis,
>>
>> thanks for reporting this.
>>
>> I could reproduce it on openSUSE Leap 15.4.
>>
>> I think there are two independent problems:
>> - the aarch64 prologue analyzer walks past the end of the function
>> - the test-case assumes that the prologue analyzer will return the first
>>    insn in foo, rather that some insn in foo.
>>
>> The WIP patch below addresses both issues, and allows the test-case to 
>> pass for me.
>>
>> [ FWIW, alternatively using some "maint set skip-prologue" value from 
>> this RFC ( 
>> https://sourceware.org/pipermail/gdb-patches/2022-August/191343.html ) 
>> could also suffice to ignore the first problem. ]
>>
>> Thanks,
>> - Tom
>>
>> ...
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index ec0e51bdaf7..d974595e48f 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -917,12 +917,13 @@ aarch64_analyze_prologue_test (void)
>>   static CORE_ADDR
>>   aarch64_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR pc)
>>   {
>> -  CORE_ADDR func_addr, limit_pc;
>> +  CORE_ADDR func_addr, func_end_addr, limit_pc;
>>
>>     /* See if we can determine the end of the prologue via the symbol
>>        table.  If so, then return either PC, or the PC after the
>>        prologue, whichever is greater.  */
>> -  if (find_pc_partial_function (pc, NULL, &func_addr, NULL))
>> +  bool func_addr_found = find_pc_partial_function (pc, NULL, 
>> &func_addr, &func_end_addr);
>> +  if (func_addr_found)
>>       {
>>         CORE_ADDR post_prologue_pc
>>          = skip_prologue_using_sal (gdbarch, func_addr);
>> @@ -941,7 +942,8 @@ aarch64_skip_prologue (struct gdbarch *gdbarch, 
>> CORE_ADDR pc)
>>     limit_pc = skip_prologue_using_sal (gdbarch, pc);
>>     if (limit_pc == 0)
>>       limit_pc = pc + 128;       /* Magic.  */
>> -
>> +  limit_pc = std::min (limit_pc, func_end_addr - 4);
>> +
>>     /* Try disassembling prologue.  */
>>     return aarch64_analyze_prologue (gdbarch, pc, limit_pc, NULL);
>>   }
>> diff --git a/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp 
>> b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
>> index 488f85f9674..c506cfd55cc 100644
>> --- a/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
>> +++ b/gdb/testsuite/gdb.dwarf2/dw2-prologue-end-2.exp
>> @@ -95,15 +95,15 @@ if { $break_addr == "" } {
>>
>>   # Get the "foo_label" address.
>>
>> -set foo_label_addr ""
>> -gdb_test_multiple "print /x &foo_label" "" {
>> +set bar_label_addr ""
>> +gdb_test_multiple "print /x &bar_label" "" {
>>       -re -wrap "= ($hex)" {
>> -       set foo_label_addr $expect_out(1,string)
>> +       set bar_label_addr $expect_out(1,string)
>>          pass $gdb_test_name
>>       }
>>   }
>>
>> -if { $foo_label_addr == "" } {
>> +if { $bar_label_addr == "" } {
>>       return
>>   }
>>
>> @@ -115,4 +115,4 @@ gdb_test "print &foo_end == &bar_label" " = 1"
>>   # Check that the breakpoint is set at the expected address. 
>> Regression test
>>   # for PR30369.
>>
>> -gdb_assert { $break_addr == $foo_label_addr }
>> +gdb_assert { $break_addr < $bar_label_addr }
>> ...
> 
> Sorry, I thought I had replied to this thread. Indeed the above patch 
> addresses this problem with the aarch64 prologue skipper,
> and it also fixes things for arm. For arm I suspect we might need the 
> same fix to the prologue skipper that the patch addresses for aarch64.
> 
> I can pick it up, refresh and submit if you're happy with it as well.

Hi Luis,

thanks for confirming.

If you want to pick this up, great, thanks.

- Tom