public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Improve forward progress test in gdb.python/python.exp
@ 2021-08-06 22:40 Carl Love
  2021-08-09  9:45 ` Andrew Burgess
  0 siblings, 1 reply; 6+ messages in thread
From: Carl Love @ 2021-08-06 22:40 UTC (permalink / raw)
  To: gdb-patches; +Cc: Carl Love, Rogerio Alves, Ulrich Weigand, Will Schmidt

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Improve forward progress test in gdb.python/python.exp
  2021-08-06 22:40 [PATCH] Improve forward progress test in gdb.python/python.exp Carl Love
@ 2021-08-09  9:45 ` Andrew Burgess
  2021-08-09 15:54   ` Carl Love
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Burgess @ 2021-08-09  9:45 UTC (permalink / raw)
  To: Carl Love; +Cc: gdb-patches, Rogerio Alves, Ulrich Weigand

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] Improve forward progress test in gdb.python/python.exp
  2021-08-09  9:45 ` Andrew Burgess
@ 2021-08-09 15:54   ` Carl Love
  2021-08-11 15:58     ` Andrew Burgess
  0 siblings, 1 reply; 6+ messages in thread
From: Carl Love @ 2021-08-09 15:54 UTC (permalink / raw)
  To: Andrew Burgess; +Cc: gdb-patches, Rogerio Alves, Ulrich Weigand


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

OK, I am fine with that it makes it more consistent
> 
> > +
> >  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?


The above test sequence adds the PC check but doesn't change the line
test.  So Powerpc will still fail the line test and thus we haven't
fixed the existing failure on Powerpc.

My proposed patch checks the line numbers.  If the line number check
"gdb.find_pc_line(pc_rtn).line >= line" is true the test passes.  If
the line test fails, as it does on Powerpc, the pc test is then done to
decide if the test ultimately passes or fails.   The result is that
even though the line check failed, the test passes on Powerpc because
we were able to show that the test made forward progress.  The failure
on Powerpc is fixed.

I agree that we should always expect the PC to be bigger, i.e. the test
made forward progress.  My first thought was to just remove the line
test and go with the new PC test instead.  My concern was I can't test
that change on all architectures.  So I figured it best to leave the
line test as that presumably is OK on all other architectures.  Just in
the case it fails (i.e. on Powerpc), do the additional PC check to make
a final determination if the test passes or fails.  

If everyone agrees that the PC test will work on all architectures, the
test could be changed to just do the PC check instead of the line
check.  This would make the whole test simpler.  Thoughts?

                      Carl 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Improve forward progress test in gdb.python/python.exp
  2021-08-09 15:54   ` Carl Love
@ 2021-08-11 15:58     ` Andrew Burgess
  2021-08-11 19:14       ` Carl Love
  0 siblings, 1 reply; 6+ messages in thread
From: Andrew Burgess @ 2021-08-11 15:58 UTC (permalink / raw)
  To: Carl Love; +Cc: gdb-patches, Rogerio Alves, Ulrich Weigand

* Carl Love <cel@us.ibm.com> [2021-08-09 08:54:16 -0700]:

> 
> > >  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.
> 
> OK, I am fine with that it makes it more consistent
> > 
> > > +
> > >  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?
> 
> 
> The above test sequence adds the PC check but doesn't change the line
> test.  So Powerpc will still fail the line test and thus we haven't
> fixed the existing failure on Powerpc.

It did change, previously the line check was '>', now I'm suggesting
we use '>='.

In your original email you wrote:

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

You suggest that the NOP is mapped to the _same_ source line, so I
would expect the new test to pass, even on ppc.

If the NOP isn't mapped to the same source line, then what is it
mapped too?  And you need to update your commit message.

Thanks,
Andrew

> 
> My proposed patch checks the line numbers.  If the line number check
> "gdb.find_pc_line(pc_rtn).line >= line" is true the test passes.  If
> the line test fails, as it does on Powerpc, the pc test is then done to
> decide if the test ultimately passes or fails.   The result is that
> even though the line check failed, the test passes on Powerpc because
> we were able to show that the test made forward progress.  The failure
> on Powerpc is fixed.
> 
> I agree that we should always expect the PC to be bigger, i.e. the test
> made forward progress.  My first thought was to just remove the line
> test and go with the new PC test instead.  My concern was I can't test
> that change on all architectures.  So I figured it best to leave the
> line test as that presumably is OK on all other architectures.  Just in
> the case it fails (i.e. on Powerpc), do the additional PC check to make
> a final determination if the test passes or fails.  
> 
> If everyone agrees that the PC test will work on all architectures, the
> test could be changed to just do the PC check instead of the line
> check.  This would make the whole test simpler.  Thoughts?
> 
>                       Carl 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] Improve forward progress test in gdb.python/python.exp
  2021-08-11 15:58     ` Andrew Burgess
@ 2021-08-11 19:14       ` Carl Love
  2021-08-12  9:08         ` Andrew Burgess
  0 siblings, 1 reply; 6+ messages in thread
From: Carl Love @ 2021-08-11 19:14 UTC (permalink / raw)
  To: Andrew Burgess, cel; +Cc: gdb-patches, Rogerio Alves, Ulrich Weigand

On Wed, 2021-08-11 at 16:58 +0100, Andrew Burgess wrote:
> 
> > > 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?
> > 
> > The above test sequence adds the PC check but doesn't change the
> > line
> > test.  So Powerpc will still fail the line test and thus we haven't
> > fixed the existing failure on Powerpc.
> 
> It did change, previously the line check was '>', now I'm suggesting
> we use '>='.

Ah, I missed the new equal sign in the test.  Note, I just read thru
the changes, I didn't actually run them.
> 
> In your original email you wrote:
> 
> > 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.
> 
> You suggest that the NOP is mapped to the _same_ source line, so I
> would expect the new test to pass, even on ppc.

Yes, the updated line test with the equal will pass on PPC64. As noted,
I missed the equal sign in your message. 

I have updated the patch with your changes and tested it on PPC64.  The
test now passes with no failures on PPC64.

                        Carl 

----------------------------------------------------------------
Improve forward progress test in python.exp

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.  The line test is changed from greater than to greater
than or equal.
---
 gdb/testsuite/gdb.python/python.exp | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/gdb.python/python.exp b/gdb/testsuite/gdb.python/python.exp
index d9fd60f3dd4..bc670ce25c1 100644
--- a/gdb/testsuite/gdb.python/python.exp
+++ b/gdb/testsuite/gdb.python/python.exp
@@ -496,12 +496,27 @@ 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 of func2 call site" 1
+
 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"
+# 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_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"
 
 gdb_test_no_output "set variable \$cvar1 = 23" "set convenience variable"
 gdb_test "python print(gdb.convenience_variable('cvar1'))" "23"
-- 
2.17.1




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Improve forward progress test in gdb.python/python.exp
  2021-08-11 19:14       ` Carl Love
@ 2021-08-12  9:08         ` Andrew Burgess
  0 siblings, 0 replies; 6+ messages in thread
From: Andrew Burgess @ 2021-08-12  9:08 UTC (permalink / raw)
  To: Carl Love; +Cc: gdb-patches, Rogerio Alves, Ulrich Weigand

* Carl Love <cel@us.ibm.com> [2021-08-11 12:14:32 -0700]:

> On Wed, 2021-08-11 at 16:58 +0100, Andrew Burgess wrote:
> > 
> > > > 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?
> > > 
> > > The above test sequence adds the PC check but doesn't change the
> > > line
> > > test.  So Powerpc will still fail the line test and thus we haven't
> > > fixed the existing failure on Powerpc.
> > 
> > It did change, previously the line check was '>', now I'm suggesting
> > we use '>='.
> 
> Ah, I missed the new equal sign in the test.  Note, I just read thru
> the changes, I didn't actually run them.
> > 
> > In your original email you wrote:
> > 
> > > 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.
> > 
> > You suggest that the NOP is mapped to the _same_ source line, so I
> > would expect the new test to pass, even on ppc.
> 
> Yes, the updated line test with the equal will pass on PPC64. As noted,
> I missed the equal sign in your message. 
> 
> I have updated the patch with your changes and tested it on PPC64.  The
> test now passes with no failures on PPC64.
> 
>                         Carl 
> 
> ----------------------------------------------------------------
> Improve forward progress test in python.exp
> 
> 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.  The line test is changed from greater than to greater
> than or equal.

LGTM.

Thanks,
Andrew


> ---
>  gdb/testsuite/gdb.python/python.exp | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/testsuite/gdb.python/python.exp b/gdb/testsuite/gdb.python/python.exp
> index d9fd60f3dd4..bc670ce25c1 100644
> --- a/gdb/testsuite/gdb.python/python.exp
> +++ b/gdb/testsuite/gdb.python/python.exp
> @@ -496,12 +496,27 @@ 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 of func2 call site" 1
> +
>  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"
> +# 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_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"
>  
>  gdb_test_no_output "set variable \$cvar1 = 23" "set convenience variable"
>  gdb_test "python print(gdb.convenience_variable('cvar1'))" "23"
> -- 
> 2.17.1
> 
> 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-12  9:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-06 22:40 [PATCH] Improve forward progress test in gdb.python/python.exp Carl Love
2021-08-09  9:45 ` Andrew Burgess
2021-08-09 15:54   ` Carl Love
2021-08-11 15:58     ` Andrew Burgess
2021-08-11 19:14       ` Carl Love
2021-08-12  9:08         ` Andrew Burgess

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).