Re: [PATCH v4 1/2] Change calculation of frame_id by amd64 epilogue unwinder

[Bruno Larsen <blarsen@redhat.com>]

On 25/10/2022 15:59, Simon Marchi wrote:
> On 10/25/22 09:44, Simon Marchi wrote:
>> On 10/5/22 06:38, Bruno Larsen via Gdb-patches wrote:
>>> When GDB is stopped at a ret instruction and no debug information is
>>> available for unwinding, GDB defaults to the amd64 epilogue unwinder, to
>>> be able to generate a decent backtrace. However, when calculating the
>>> frame id, the epilogue unwinder generates information as if the return
>>> instruction was the whole frame.
>>>
>>> This was an issue especially when attempting to reverse debug, as GDB
>>> would place a step_resume_breakpoint from the epilogue of a function if
>>> we were to attempt to skip that function, and this breakpoint should
>>> ideally have the current function's frame_id to avoid other problems
>>> such as PR record/16678.
>>>
>>> This commit changes the frame_id calculation for the amd64 epilogue,
>>> so that it is always the same as the dwarf2 unwinder's frame_id.
>>>
>>> It also adds a test to confirm that the frame_id will be the same,
>>> regardless of using the epilogue unwinder or not, thanks to Andrew
>>> Burgess.
>>>
>>> Co-Authored-By: Andrew Burgess <aburgess@redhat.com>
>>> ---
>>>   gdb/amd64-tdep.c                              |  10 +-
>>>   .../gdb.base/unwind-on-each-insn-foo.c        |  22 +++
>>>   gdb/testsuite/gdb.base/unwind-on-each-insn.c  |  25 +++
>>>   .../gdb.base/unwind-on-each-insn.exp          | 154 ++++++++++++++++++/usr/lib/x86_64-linux-gnu/libasan.so.6
>>>   4 files changed, 206 insertions(+), 5 deletions(-)
>>>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn-foo.c
>>>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.c
>>>   create mode 100644 gdb/testsuite/gdb.base/unwind-on-each-insn.exp
>> Hi Bruno,
>>
>> On Ubuntu 22.04, I can get this new test to fail quite reliably with:
>>
>>      $ taskset -c 1 make check TESTS="gdb.base/unwind-on-each-insn.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>>
>> Can you give it a try?
>>
>> Simon
> Actually, I took the time to look into it, it turns out the problem is
> simple.  Here's a patch below.
Ah great! I didn't even have time to finish making an Ubuntu VM lol. I 
just noticed one typo:
>
>
>  From 7090bf701b2f1cca89985ea1b45b0a2e3859e19e Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@efficios.com>
> Date: Tue, 25 Oct 2022 09:50:56 -0400
> Subject: [PATCH] gdb/testsuite: make sure to consume the prompt in
>   gdb.base/unwind-on-each-insn.exp
>
> This test fails quite reliably for me when ran as:
>
>      $ taskset -c 1 make check TESTS="gdb.base/unwind-on-each-insn.exp" RUNTESTFLAGS="--target_board=native-gdbserver"
>
> or more simply:
>
>      $ make check-read1 TESTS="gdb.base/unwind-on-each-insn.exp"
>
> The problem is that the that grabs the frame id from "maint print

Missing word here. "The problem is that the __proc__" ?


With this fixed, LGTM.

Reviewed-By: Bruno Larsen <blarsen@redhat.com>

Cheers,
Bruno


> frame-id" does not consume the prompt.  Well, it does sometimes due to
> the trailing .*, but not always.  If the prompt is not consumed, the
> following tests get confused:
>
>      FAIL: gdb.base/unwind-on-each-insn.exp: gdb_breakpoint: set breakpoint at *foo
>      FAIL: gdb.base/unwind-on-each-insn.exp: disassemble foo
>      FAIL: gdb.base/unwind-on-each-insn.exp: get $sp and frame base in foo: get hexadecimal valueof "$sp"
>
> Use -wrap to make gdb_test_multiple consume the prompt.
>
> While at it, remove the bit that consumes the command name and do
> exp_continue, it's not really necessary.  And for consistency, do the
> same changes to the gdb_test_multiple that consumes the stack address,
> although that one was fine, it did consume the prompt explicitly.
>
> Change-Id: I2b7328c8844c7e98921ea494c4c05107162619fc
> ---
>   gdb/testsuite/gdb.base/unwind-on-each-insn.exp | 12 ++----------
>   1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> index faa6a1a3f064..3b48805cff83 100644
> --- a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> @@ -44,11 +44,7 @@ proc get_sp_and_fba { testname } {
>
>   	set fba ""
>   	gdb_test_multiple "info frame" "" {
> -	    -re "^info frame\r\n" {
> -		exp_continue
> -	    }
> -
> -	    -re "^Stack level ${::decimal}, frame at ($::hex):\r\n.*$::gdb_prompt $" {
> +	    -re -wrap ".*Stack level ${::decimal}, frame at ($::hex):.*" {
>   		set fba $expect_out(1,string)
>   	    }
>   	}
> @@ -62,11 +58,7 @@ proc get_sp_and_fba { testname } {
>   proc get_fid { } {
>       set fid ""
>       gdb_test_multiple "maint print frame-id" "" {
> -	-re "^maint print frame-id\r\n" {
> -	    exp_continue
> -	}
> -
> -	-re "^frame-id for frame #${::decimal}: (\[^\r\n\]+).*" {
> +	-re -wrap ".*frame-id for frame #${::decimal}: (.*)" {
>   	    set fid $expect_out(1,string)
>   	}
>       }