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

[Andrew Burgess <aburgess@redhat.com>]

Thanks for looking into this.  I have a few comments...

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.
>
> [ 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'm going to need to think about this some more, as still don't
understand this description of the problem.  For now I'm assuming this
is all correct, so I have a few comments on the changes below ...

>
> This relies on the test-case to calculate the offending frame-id, and the
> problem on s390x is that that calculation is incorrect.
>
> Fix this by using "maint print frame-id" to get all frame-ids, and using those
> instead.
>
> 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

Are these set to 0 needed?  We don't try to use these unless we match
the regexp, in which case they'll be set to the values that are matched,
right? 

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

I think you should delete STACK_ADJUST, and the code at the end of this
file that sets it up.

The comment at the end of the file which describes what the setup code
(at the end of the file) is doing should probably then be moved to the
start of the file, and ideally extended to explain that frame_id_sp and
frame_id_pc need to be filled in by the .exp script in order for the
test to work correctly.

The use of STACK_ADJUST within TestUnwinder.__call__ can then be
removed, by maybe replace it with a check, something like:

  if len(frame_id_sp) < stop_at_level or len(frame_id_pc) < stop_at_level:
    raise gdb.GdbError("invalid stack_adjust")

Thanks,
Andrew

>  
> +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))
>  
>          for reg in pending_frame.architecture().registers("general"):
>
> base-commit: 2271dee682787051c0628c869d7cdb220bdd0e67
> -- 
> 2.51.0