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

[Luis Machado via Gdb-patches <gdb-patches@sourceware.org>]

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