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

[Carl Love <cel@us.ibm.com>]

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