Re: [PATCH] [gdb/testsuite] Fix gdb.base/inline-frame-cycle-unwind.exp for s390x (alternative)

[Andrew Burgess <aburgess@redhat.com>]

Tom de Vries <tdevries@suse.de> writes:

> With test-case gdb.base/inline-frame-cycle-unwind.exp on s390x-linux, I run
> into:
> ...
>  (gdb) bt^M
>  #0  inline_func () at inline-frame-cycle-unwind.c:49^M
>  #1  normal_func () at inline-frame-cycle-unwind.c:32^M
>  #2  0x000000000100065c in inline_func () at inline-frame-cycle-unwind.c:45^M
>  #3  normal_func () at inline-frame-cycle-unwind.c:32^M
>  Backtrace stopped: previous frame identical to this frame (corrupt stack?)^M
>  (gdb) FAIL: $exp: bt: cycle at level 5: backtrace when the unwind is broken \
>    at frame 5
> ...
>
> In contrast, on x86_64-linux, I get:
> ...
>  (gdb) bt^M
>  #0  inline_func () at inline-frame-cycle-unwind.c:49^M
>  #1  normal_func () at inline-frame-cycle-unwind.c:32^M
>  #2  0x0000000000401157 in inline_func () at inline-frame-cycle-unwind.c:45^M
>  #3  normal_func () at inline-frame-cycle-unwind.c:32^M
>  #4  0x0000000000401157 in inline_func () at inline-frame-cycle-unwind.c:45^M
>  #5  normal_func () at inline-frame-cycle-unwind.c:32^M
>  Backtrace stopped: previous frame identical to this frame (corrupt stack?)^M
>  (gdb) PASS: $exp: bt: cycle at level 5: backtrace when the unwind is broken \
>    at frame 5
> ...
>
> AFAIU, the mechanism of the test is as follows: the custom unwinder produces the
> frame-id for frame #5 at frame #4.  Consequently, when arriving at frame #5, a
> cycle is detected.

I don't believe this is how it works.  See below for what I think happens.

>
> [ It took me a while to understand this because of the following off-by-one
> confusion: for frame #0, we get pending_frame.level() == 1.  So when
> stop_at_level == 5, the custom unwinder calculates a frame-id for frame #4,
> not frame #5.  But the frame-id it calculates is the one for frame #5, so
> unwinding will stop at frame #5 because the frame-ids for frame #4 and
> frame #5 are identical. ]

I went back and looked at the unpatched test again, and I don't believe
this "off-by-one" issue is a thing, at least, I don't see one based on
your description.

It does appear that for frame #0 we get pending_frame.level() == 1, but
this isn't what's really happening.

Frame #0 is inline, so the Python frame unwinder is never run for this
frame.  The first frame for which the Python frame unwinder is run is
frame #1, hence pending_frame.level() == 1.

The frame-id calculated within TestUnwinder.__call__ is the frame-id for
the previous (outer, older) frame.  So, when pending_frame.level() == 5
we are calculating the frame-id for frame #6.  As frame #6 then appears
to be identical to frame #5, a cycle is detected and the backtrace ends.

> This relies on the test-case to calculate the offending frame-id, and the
> problem on s390x is that that calculation is incorrect.

Agreed.

>
> Fix this by using "maint print frame-id" to get all frame-ids, and using those
> instead.

I suspect this is the better approach.  I'm not sure 'maint print
frame-id' existed when I wrote this test (but I could be wrong).  Still,
it would be nice if we could understand exactly why this is going wrong
in this case.

My main question here is that we're only calculating a "fake" frame-id
for frame #6, so this should be the first frame that GDB doesn't
display.  That GDB is cutting off after #3 would seem to indicate that
#4, or maybe #5 (as the non-inline frame) is a duplicate of an earlier
frame, but I don't understand how that can be the case.

>
> Tested on x86_64-linux and s390x-linux.
> ---
>  .../gdb.base/inline-frame-cycle-unwind.exp          | 13 +++++++++++++
>  gdb/testsuite/gdb.base/inline-frame-cycle-unwind.py |  8 +++++---
>  2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.exp b/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.exp
> index 7fc47af624f..5c6504323ee 100644
> --- a/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.exp
> +++ b/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.exp
> @@ -72,6 +72,19 @@ gdb_continue_to_breakpoint "stop at test breakpoint"
>  gdb_test_no_output "source ${pyfile}"\
>      "import python scripts"
>  
> +foreach_with_prefix n { 0 1 2 3 4 5 6 } {
> +    set sp 0x0
> +    set pc 0x0
> +    gdb_test_multiple "maint print frame-id $n" "" {
> +	-re -wrap "frame-id for frame #$n: {stack=($hex),code=($hex),.*}" {
> +	    set sp $expect_out(1,string)
> +	    set pc $expect_out(2,string)
> +	    gdb_test_no_output "python frame_id_sp.append($sp)"
> +	    gdb_test_no_output "python frame_id_pc.append($pc)"
> +	}
> +    }
> +}
> +
>  # Test with and without filters.
>  foreach bt_cmd { "bt" "bt -no-filters" } {
>      with_test_prefix "$bt_cmd" {
> diff --git a/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.py b/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.py
> index 55dea989512..25a67b1a7c9 100644
> --- a/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.py
> +++ b/gdb/testsuite/gdb.base/inline-frame-cycle-unwind.py
> @@ -26,6 +26,9 @@ stop_at_level = None
>  # function called recursively.
>  stack_adjust = None
>  
> +frame_id_sp = []
> +frame_id_pc = []
> +
>  
>  class FrameId(object):
>      def __init__(self, sp, pc):
> @@ -55,9 +58,8 @@ class TestUnwinder(Unwinder):
>          if stop_at_level not in [1, 3, 5]:
>              raise gdb.GdbError("invalid stop_at_level")
>  
> -        sp_desc = pending_frame.architecture().registers().find("sp")
> -        sp = pending_frame.read_register(sp_desc) + stack_adjust
> -        pc = (gdb.lookup_symbol("normal_func"))[0].value().address
> +        sp = frame_id_sp[stop_at_level]
> +        pc = frame_id_pc[stop_at_level]
>          unwinder = pending_frame.create_unwind_info(FrameId(sp, pc))

I wonder if here you could:

        print(f"DEBUG: frame level: {pending_frame.level()}")
        sp_desc = pending_frame.architecture().registers().find("sp")
        prev_sp = int(pending_frame.read_register(sp_desc))
        print(f"DEBUG: previous-sp: {prev_sp:x}, adjustment: {int(stack_adjust):x}")
        adjusted_sp = int(prev_sp + stack_adjust)
        print(f"DEBUG: adjusted-sp: {adjusted_sp:x}")
        print(f"DEBUG: new-sp: {frame_id_sp[stop_at_level]:x}")
        addr = int((gdb.lookup_symbol("normal_func"))[0].value().address)
        print(f"DEBUG: previous-pc: {addr:x}")
        print(f"DEBUG: new-pc: {frame_id_pc[stop_at_level]:x}")

then we'd be able to see which part of the frame-id was being generated
incorrectly.

Thanks,
Andrew



>  
>          for reg in pending_frame.architecture().registers("general"):
>
> base-commit: 2271dee682787051c0628c869d7cdb220bdd0e67
> -- 
> 2.51.0