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

[Bruno Larsen <blarsen@redhat.com>]

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