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

[Tom de Vries <tdevries@suse.de>]

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