Re: Question: [PATCH] Change calculation of frame_id by amd64 epilogue unwinder

[Bruno Larsen <blarsen@redhat.com>]

On 01/11/2022 20:51, Carl Love wrote:
> Bruno:
>
> I found the issue after chasing down a few dead ends...
>
> Basically, the test disassembles function foo and then scans thru the
> disassembly looking for the address of the last instruction which is
> assumed to be right before the line "End of assembler dump".  The
> PowerPC disassembly has three addresses containing a .long after the
> last instruction in the function.  So, the test gets the wrong final
> address for function foo.  Basicaly, the test works fine until you walk
> past the end of the function.  I added a condition to the test
> gdb_test_multiple "disassemble foo" to ignore the lines with the .long
> entries so the correct address for the last instruction is recorded.
> This fixes all of the errors and warnings.
>
> The break foo* does not seem to be a problem on this test as I was
> concerned that it would be.
>
> I have tested the patch on PowerPC and Intel.  Please take a look and
> see what you think.  If it looks OK to you, I will formally post it to
> the mailing list for review and approval.  Thanks.
>
>                                    Carl
> -----------------------------------
>  From 764f4476f16fa222e95ab2a7087355dfae818502 Mon Sep 17 00:00:00 2001
> From: Carl Love<cel@us.ibm.com>
> Date: Tue, 1 Nov 2022 15:33:17 -0400
> Subject: [PATCH] Powerpc fix for gdb.base/unwind-on-each-insn.exp
>
> The test disassembles function foo and searches for the line
> "End of assembler dump" to determing the last address in the function.  The
> assumption is the last instruction will be given right before the line
> "End of assembler dump".  This assumption fails on PowerPC.
>
> The PowerPC disassembly of the function foo looks like:
>   Dump of assembler code for function foo:
> #  => 0x00000000100006dc <+0>:     std     r31,-8(r1)
> #     0x00000000100006e0 <+4>:     stdu    r1,-48(r1)
> #     0x00000000100006e4 <+8>:     mr      r31,r1
> #     0x00000000100006e8 <+12>:    nop
> #     0x00000000100006ec <+16>:    addi    r1,r31,48
> #     0x00000000100006f0 <+20>:    ld      r31,-8(r1)
> #     0x00000000100006f4 <+24>:    blr
> #     0x00000000100006f8 <+28>:    .long 0x0
> #     0x00000000100006fc <+32>:    .long 0x0
> #     0x0000000010000700 <+36>:    .long 0x1000180
> #     End of assembler dump.
>
> The blr instruction is the last instruction in function foo.  The lines
> with .long follwing the blr instruction need to be ignored.
>
> This patch adds a new condition to the gdb_test_multiple "disassemble foo"
> test to ignore the lines with the .long.
>
> The patch has been tested on PowerPC and Intel X86-64.

Hi Carl,

The idea of the patch looks fine, I just have a few style nits inlined.

> ---
>   .../gdb.base/unwind-on-each-insn.exp          | 25 +++++++++++++++++--
>   1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> index 3b48805cff8..c908a4b838e 100644
> --- a/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> +++ b/gdb/testsuite/gdb.base/unwind-on-each-insn.exp
> @@ -41,7 +41,6 @@ if ![runto_main] then {
>   proc get_sp_and_fba { testname } {
>       with_test_prefix "get \$sp and frame base $testname" {
>   	set sp [get_hexadecimal_valueof "\$sp" "*UNKNOWN*"]
> -
>   	set fba ""
>   	gdb_test_multiple "info frame" "" {
>   	    -re -wrap ".*Stack level ${::decimal}, frame at ($::hex):.*" {
> @@ -75,6 +74,23 @@ gdb_continue_to_breakpoint "enter foo"
>   
>   # Figure out the range of addresses covered by this function.
>   set last_addr_in_foo ""
> +
> +# The disassembly of foo on PowerPC looks like:
> +#     Dump of assembler code for function foo:
> +#  => 0x00000000100006dc <+0>:     std     r31,-8(r1)
> +#     0x00000000100006e0 <+4>:     stdu    r1,-48(r1)
> +#     0x00000000100006e4 <+8>:     mr      r31,r1
> +#     0x00000000100006e8 <+12>:    nop
> +#     0x00000000100006ec <+16>:    addi    r1,r31,48
> +#     0x00000000100006f0 <+20>:    ld      r31,-8(r1)
> +#     0x00000000100006f4 <+24>:    blr
> +#     0x00000000100006f8 <+28>:    .long 0x0
> +#     0x00000000100006fc <+32>:    .long 0x0
> +#     0x0000000010000700 <+36>:    .long 0x1000180
> +#     End of assembler dump.
> +#
> +# The last instruction in function foo is blr.  Ignore the .long
> +# entries following the blr instruction.
>   gdb_test_multiple "disassemble foo" "" {
>       -re "^disassemble foo\r\n" {
>   	exp_continue
> @@ -84,7 +100,11 @@ gdb_test_multiple "disassemble foo" "" {
>   	exp_continue
>       }
>   
> -    -re "^...($hex) \[^\r\n\]+\r\n" {
> +    -re "^...($hex) \[<>+0-9:\s\t\]*\.long\[\s\t\]*\[^\r\n\]*\r\n" {

I wonder if this pattern is unnecessarily strict. Correct me if I'm 
wrong, but I think that no architecture has an instruction that starts 
with a .  so this pattern could probably be simplified to

^...($hex) \[<>+0-9:\s\t\]*\.[^\r\n\]*\r\n

And no other architectures would have a similar problem in the future.

I'm not very knowledgeable on assembly so you may know better in this area.

> +	exp_continue
> +    }
> +
> +    -re "^...($hex)\[^\r\n\]+\r\n" {
You removed a space here, which makes no difference in the pattern and 
makes the diff more confusing
>   	set last_addr_in_foo $expect_out(1,string)
>   	exp_continue
>       }
> @@ -137,6 +157,7 @@ for { set i_count 1 } { true } { incr i_count } {
>   	gdb_test "frame 0" ".*"
>   
>   	set pc [get_hexadecimal_valueof "\$pc" "*UNKNOWN*"]
> +
Unnecessary new line here.

Cheers,
Bruno

>   	if { $pc == $last_addr_in_foo } {
>   	    break
>   	}