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

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

[Carl Love]

Bruno:

Sorry for the slow response, I was out all of last week.

It looks like you committed the patch:

commit 49d7cd733a7f1b87aa1d40318b3d7c2b65aca5ac
Author: Bruno Larsen <blarsen@redhat.com>
Date:   Fri Aug 19 15:11:28 2022 +0200

    Change calculation of frame_id by amd64 epilogue unwinder

    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>

a week or so ago.  The test is generating hundreds of new errors on the
various PowerPC processors.  From the discussion above, it looks like
you were trying to fix an issue with the amd64 epilogue.  

The patch introduced a new patch to test the amd64 epilogue changes. 
Do you expect this new test to run on all other platforms?  In addition
to the 575 new errors on Power 10, I see a lots of warnings:

   WARNING: While evaluating expression in gdb_assert: missing operand at _@_
   in expression " _@_== 0x7fffffffc920"

where the hex value changes for each warning.  Still working on sorting
out where the messages come from.

The other thing I notice in the gdb/testsuite/gdb.base/unwind-on-each-
insn.exp file is that you set a break point in function foo with the
command:  gdb_breakpoint "*foo"

The use of breakpoint on *function is problematic on Power as discussed
earlier.  The use of *foo will set the breakpoint on the remote
breakpoint which will not get hit in this test on PowerPC.  I don't see
why you chose to use *foo instead of foo?  I don't see anything in the
test that relies on the prolog where you would need the address of the
function before the prolog.  I only see references to the epilog of the
function.  

Anyway, I am trying to figure out how to fix this test to work on
PowerPC.  Not sure if there needs to be some similar changes to the
rs6000-tdep.c file similar to the changes in amd64-tdep.c?  Any
thoughts you have on the test that would be helpful to getting it to
work on PowerPC, if that is appropriate, or having PowerPC skip this
test would be appreciated.  Thanks.

                        Carl 

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

[Bruno Larsen]

On 31/10/2022 21:16, Carl Love wrote:
> Bruno:
>
> Sorry for the slow response, I was out all of last week.
>
> It looks like you committed the patch:
>
> commit 49d7cd733a7f1b87aa1d40318b3d7c2b65aca5ac
> Author: Bruno Larsen <blarsen@redhat.com>
> Date:   Fri Aug 19 15:11:28 2022 +0200
>
>      Change calculation of frame_id by amd64 epilogue unwinder
>
>      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>
>
> a week or so ago.  The test is generating hundreds of new errors on the
> various PowerPC processors.  From the discussion above, it looks like
> you were trying to fix an issue with the amd64 epilogue.

Hi Carl,

The main point of the patch was fixing the issue with amd64 epilogue 
unwinding, but the issue itself was that frame IDs aren't consistent 
throughout the function. With my change in the following patch, if the 
frame ID is different  at any point, using "reverse next" might consume 
all the history and stop at the start of the recording, instead of going 
a single line. This would be applicable to all architectures.

>
> The patch introduced a new patch to test the amd64 epilogue changes.
> Do you expect this new test to run on all other platforms?

Yes, we did. See above rationale as to why it is important, but also all 
instructions in a function should have the same frame id, otherwise all 
sorts of internal GDB and user confusion can happen.

>    In addition
> to the 575 new errors on Power 10, I see a lots of warnings:
>
>     WARNING: While evaluating expression in gdb_assert: missing operand at _@_
>     in expression " _@_== 0x7fffffffc920"
>
> where the hex value changes for each warning.  Still working on sorting
> out where the messages come from.
I don't think this would be a regression from this patch. Either this 
was already a problem (just not being triggered) or it was another patch 
that regressed this. If I clear out my regression backlog, I'll try to 
help you figure out what is going on.
>
> The other thing I notice in the gdb/testsuite/gdb.base/unwind-on-each-
> insn.exp file is that you set a break point in function foo with the
> command:  gdb_breakpoint "*foo"
>
> The use of breakpoint on *function is problematic on Power as discussed
> earlier.  The use of *foo will set the breakpoint on the remote
> breakpoint which will not get hit in this test on PowerPC.  I don't see
> why you chose to use *foo instead of foo?  I don't see anything in the
> test that relies on the prolog where you would need the address of the
> function before the prolog.  I only see references to the epilog of the
> function.
Right, that is a problem with the test. We do want to test around the 
epilogue because all instructions have to show the same frame ID, but 
what could be done instead is breaking on line 23 and using stepi once.
>
> Anyway, I am trying to figure out how to fix this test to work on
> PowerPC.  Not sure if there needs to be some similar changes to the
> rs6000-tdep.c file similar to the changes in amd64-tdep.c?  Any
> thoughts you have on the test that would be helpful to getting it to
> work on PowerPC, if that is appropriate, or having PowerPC skip this
> test would be appreciated.  Thanks.

The easiest way to check if you need to change rs6000-tdep.c is to 
record the execution, next over foo and reverse-stepi once, then print 
the frame-id (maint print frame-id), reverse-step out of the epilogue 
and print the frame id again. If something doesn't match, you need to 
fix things.

Or you could use the modified test above and see if any printed frame 
ids is different, if the changes make things work on PPC.

I hope this clears things up :)

Cheers,
Bruno

>
>                          Carl
>
>
>
>

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

[Carl Love]

On Tue, 2022-11-01 at 11:20 +0100, Bruno Larsen wrote:
> On 31/10/2022 21:16, Carl Love wrote:
> > Bruno:
> > 
> > Sorry for the slow response, I was out all of last week.
> > 
> > It looks like you committed the patch:
> > 
> > commit 49d7cd733a7f1b87aa1d40318b3d7c2b65aca5ac
> > Author: Bruno Larsen <blarsen@redhat.com>
> > Date:   Fri Aug 19 15:11:28 2022 +0200
> > 
> >      Change calculation of frame_id by amd64 epilogue unwinder
> > 
> >      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>
> > 
> > a week or so ago.  The test is generating hundreds of new errors on
> > the
> > various PowerPC processors.  From the discussion above, it looks
> > like
> > you were trying to fix an issue with the amd64 epilogue.
> 
> Hi Carl,
> 
> The main point of the patch was fixing the issue with amd64 epilogue 
> unwinding, but the issue itself was that frame IDs aren't consistent 
> throughout the function. With my change in the following patch, if
> the 
> frame ID is different  at any point, using "reverse next" might
> consume 
> all the history and stop at the start of the recording, instead of
> going 
> a single line. This would be applicable to all architectures.
> 
> > The patch introduced a new patch to test the amd64 epilogue
> > changes.
> > Do you expect this new test to run on all other platforms?
> 
> Yes, we did. See above rationale as to why it is important, but also
> all 
> instructions in a function should have the same frame id, otherwise
> all 
> sorts of internal GDB and user confusion can happen.
> 
> >    In addition
> > to the 575 new errors on Power 10, I see a lots of warnings:
> > 
> >     WARNING: While evaluating expression in gdb_assert: missing
> > operand at _@_
> >     in expression " _@_== 0x7fffffffc920"
> > 
> > where the hex value changes for each warning.  Still working on
> > sorting
> > out where the messages come from.
> I don't think this would be a regression from this patch. Either
> this 
> was already a problem (just not being triggered) or it was another
> patch 
> that regressed this. If I clear out my regression backlog, I'll try
> to 
> help you figure out what is going on.
> > The other thing I notice in the gdb/testsuite/gdb.base/unwind-on-
> > each-
> > insn.exp file is that you set a break point in function foo with
> > the
> > command:  gdb_breakpoint "*foo"
> > 
> > The use of breakpoint on *function is problematic on Power as
> > discussed
> > earlier.  The use of *foo will set the breakpoint on the remote
> > breakpoint which will not get hit in this test on PowerPC.  I don't
> > see
> > why you chose to use *foo instead of foo?  I don't see anything in
> > the
> > test that relies on the prolog where you would need the address of
> > the
> > function before the prolog.  I only see references to the epilog of
> > the
> > function.
> Right, that is a problem with the test. We do want to test around
> the 
> epilogue because all instructions have to show the same frame ID,
> but 
> what could be done instead is breaking on line 23 and using stepi
> once.
> > Anyway, I am trying to figure out how to fix this test to work on
> > PowerPC.  Not sure if there needs to be some similar changes to the
> > rs6000-tdep.c file similar to the changes in amd64-tdep.c?  Any
> > thoughts you have on the test that would be helpful to getting it
> > to
> > work on PowerPC, if that is appropriate, or having PowerPC skip
> > this
> > test would be appreciated.  Thanks.

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.
---
 .../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" {
+	exp_continue
+    }
+
+    -re "^...($hex)\[^\r\n\]+\r\n" {
 	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*"]
+
 	if { $pc == $last_addr_in_foo } {
 	    break
 	}
-- 
2.37.2

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

[Bruno Larsen]

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
>   	}

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

[Carl Love]

Bruno:
> 
> 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.

I do not know of any architectures that have a dot at the beginning of
an instruction name.  So yes the pattern could be changed as suggested.
That will require changing the above comment as well to make it clear
we are skipping any "instruction" that starts with a dot, such as
.long.  
> 
> > +	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

My bad, that was not intentional. I was too focused on the above change
and didn't notice the accidental dings to the code.  Sorry.

> >   	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.
Ditto, unintentional change

I will undo the unintentional changes and update the needed
pattern/comment and post the patch for review.  Thanks for the help.

               Carl 

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

[Carl Love]

On Wed, 2022-11-02 at 08:36 -0700, Carl Love wrote:
> Bruno:
> > 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.
> 
> I do not know of any architectures that have a dot at the beginning
> of
> an instruction name.  So yes the pattern could be changed as
> suggested.
> That will require changing the above comment as well to make it clear
> we are skipping any "instruction" that starts with a dot, such as
> .long.  

Well, I guess I should have tried the change before I sent the email! 
Using just a dot in the pattern doesn't work as the pattern matches on
the period in the line "End of assembler dump." resulting in a failure
to find the statement for the end of the dump.  Argh!  I will leave the
pattern as it was to check for ".long".  

                    Carl