Re: [PATCH] gdb/unwinders: better support for $pc not saved

[Kevin Buettner <kevinb@redhat.com>]

On Sat, 27 Jan 2024 15:26:32 +0000
Andrew Burgess <aburgess@redhat.com> wrote:

> This started with a Red Hat bug report which can be seen here:
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1850710
> 
> The problem reported here was using GDB on GNU/Linux for S390, the
> user stepped into JIT generated code.  As they enter the JIT code GDB
> would report 'PC not saved', and this same message would be reported
> after each step/stepi.
> 
> Additionally, the user had 'set disassemble-next-line on', and once
> they entered the JIT code this output was not displayed, nor were any
> 'display' directives displayed.
> 
> The user is not making use of the JIT plugin API to provide debug
> information.  But that's OK, they aren't expecting any source level
> debug here, they are happy to use 'stepi', but the missing 'display'
> directives are a problem, as is the constant 'PC not saved' (error)
> message.
> 
> What is happening here is that as GDB is failing to find any debug
> information for the JIT generated code, it is falling back on to the
> S390 prologue unwinder to try and unwind frame #0.  Unfortunately,
> without being able to identify the function boundaries, the S390
> prologue scanner can't help much, in fact, it doesn't even suggest an
> arbitrary previous $pc value (some targets that use a link-register
> will, by default, assume the link-register contains the previous $pc),
> instead the S390 will just say, "sorry, I have no previous $pc value".
> 
> The result of this is that when GDB tries to find frame #1 we end
> throwing an error from frame_unwind_pc (the 'PC not saved' error).
> This error is not caught anywhere except at the top-level interpreter
> loop, and so we end up skipping all the 'display' directive handling.
> 
> While thinking about this, I wondered, could I trigger the same error
> using the Python Unwinder API?  What happens if a Python unwinder
> claims a frame, but then fails to provide a previous $pc value?
> 
> Turns out that exactly the same thing happens, which is great, as that
> means we now have a way to reproduce this bug on any target.  And so
> the test included with this patch does just this.  I have a Python
> unwinder that claims a frame, but doesn't provide any previous
> register values.
> 
> I then do two tests, first I stop in the claimed frame (i.e. frame #0
> is the frame that can't be unwound), I perform a few steps, and check
> the backtrace.  And second, I stop in a child of the problem
> frame (i.e. frame #1 is the frame that can't be unwound), and from
> here I check the backtrace.
> 
> While all this is going on I have a 'display' directive in place, and
> each time GDB stops I check that the display directive triggers.
> 
> Additionally, when checking the backtrace, I am checking that the
> backtrace finishes with the message 'Backtrace stopped: frame did not
> save the PC'.
> 
> As for the fix I chose to add a call to frame_unwind_pc directly to
> get_prev_frame_always_1.  Calling frame_unwind_pc will cache the
> unwound $pc value, so this doesn't add much additional work as
> immediately after the new frame_unwind_pc call, we call
> get_prev_frame_maybe_check_cycle, which actually generates the
> previous frame, which will always (I think) require a call to
> frame_unwind_pc anyway.
> 
> The reason for adding the frame_unwind_pc call into
> get_prev_frame_always_1, is that if the frame_unwind_pc call fails we
> want to set the frames 'stop_reason', and get_prev_frame_always_1
> seems to be the place where this is done, so I wanted to keep the new
> stop_reason setting code next to all the existing stop_reason setting
> code.
> 
> Additionally, once we enter get_prev_frame_maybe_check_cycle we
> actually create the previous frame, then, if it turns out that the
> previous frame can't be created we need to remove the frame .. this
> seemed more complex than just making the check in
> get_prev_frame_always_1.
> 
> With this fix in place the original S390 bug is fixed, and also the
> test added in this commit, that uses the Python API, is also fixed.

Thanks for the detailed commit log!

This approach looks reasonable to me.

Reviewed-by: Kevin Buettner <kevinb@redhat.com>