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

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

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