Re: [PATCH] Improve forward progress test in gdb.python/python.exp

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Carl Love via Gdb-patches <gdb-patches@sourceware.org> [2021-08-06 15:40:58 -0700]:

> GDB maintainers:
> 
> The current gdb.python/python.exp test fails on powerpc.  The test
> checks to see if the source code line for the function call is less
> than the line number after the function call.  The issue on powerpc is
> the assembly instructions for the branch and the NOP following the
> branch map to the same source code line.  
> 
> An additional check of the PC before and after the function call is
> done if the line number check fails.  This additional check takes care
> of the case where the call and return locations map to the same source
> code line.
> 
> The patch has been run on Powerpc with no regressions.
> 
> Please let me know if the patch is acceptable.  Thanks.
> 
>                           Carl Love
> 
> 
> --------------------------------------------------------
> 
> The test steps into func2 and than does an up to get back to the previous
> frame. The test checks that the line number you are at after the up command
> is greater than the line where the function was called from. The
> assembly/codegen for the powerpc target includes a NOP after the
> branch-link.
> 
> func2 (); /* Break at func2 call site. /
> 10000694: 59 00 00 48 bl 100006ec
> 10000698: 00 00 00 60 nop
> return 0; / Break to end. */
> 1000069c: 00 00 20 39 li r9,0
> 
> The PC at the instruction following the branch-link is 0x10000698 which
> GDB.find_pc_line() maps to the same line number as the bl instruction.
> GDB did move past the branch-link location thus making forward progress.
> 
> The following proposed fix adds an additional PC check to see if forward
> progress was made in the case where the original line test fails.
> ---
>  gdb/testsuite/gdb.python/python.exp | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/testsuite/gdb.python/python.exp b/gdb/testsuite/gdb.python/python.exp
> index d9fd60f3dd4..4472ed0994f 100644
> --- a/gdb/testsuite/gdb.python/python.exp
> +++ b/gdb/testsuite/gdb.python/python.exp
> @@ -496,12 +496,32 @@ if ![runto_main] then {
>  runto [gdb_get_line_number "Break at func2 call site."]
>  
>  gdb_py_test_silent_cmd "python line = gdb.selected_frame().find_sal().line" "Get line number of func2 call site" 1
> +
> +gdb_py_test_silent_cmd "python pc_call = gdb.selected_frame().pc()" "Get pc at return from func2 location" 1

I find the name for this test a little strange.  I'd have expected
"Get pc of func2 call site" in order to match the previous test.

> +
>  gdb_test "python print (gdb.find_pc_line(gdb.selected_frame().pc()).line == line)" "True" "test find_pc_line at func2 call site"
>  
>  gdb_py_test_silent_cmd "step" "Step into func2" 1
>  gdb_py_test_silent_cmd "up" "Step out of func2" 1
>  
> -gdb_test "python print (gdb.find_pc_line(gdb.selected_frame().pc()).line > line)" "True" "test find_pc_line with resume address"
> +gdb_py_test_silent_cmd "python pc_rtn = gdb.selected_frame().pc()" "Get pc at func2 return site" 1

I wonder if we can simplify the logic here?  We always expect the pc
to be bigger, right?  So we can just test that in all cases.  Once we
know that is being tested, we can make the line number test simpler,
like this:

  gdb_py_test_silent_cmd "python pc_rtn = gdb.selected_frame().pc()" \
      "Get pc at func2 return site" 1

  gdb_test "python print (pc_rtn > pc_call)" "True" \
      "test resume address greater then call address"

  gdb_test "python print (gdb.find_pc_line(pc_rtn).line >= line)" "True" \
      "test find_pc_line with resume address"

I wrapped the lines to keep them under 80 characters, which is the
correct style where possible.

What do you think?

Thanks,
Andrew


> +
> +# The point of the following test is to see if gdb has advanced past the
> +# location where the branch to a function was made.
> +set test_name "test find_pc_line with resume address"
> +
> +gdb_test_multiple "python print (gdb.find_pc_line(gdb.selected_frame().pc()).line > line)" $test_name {
> +    -re "True.*" {
> +	pass $test_name
> +    }
> +    -re "False.*" {
> +	# The instruction at the call-return site may also be associated with
> +	# the same line that contained the branch. For example, PPC64 puts a
> +	# NOP after the branch to function.  Check if the frame PC has
> +	# advanced past the call location.
> +	gdb_test "python print (pc_rtn > pc_call)" "True" "test pc_rtn greater then pc_call"
> +    }
> +}
>  
>  gdb_test_no_output "set variable \$cvar1 = 23" "set convenience variable"
>  gdb_test "python print(gdb.convenience_variable('cvar1'))" "23"
> -- 
> 2.17.1
> 
>