[PATCH] Improve forward progress test in gdb.python/python.exp
Carl Love
cel@us.ibm.com
Mon Aug 9 15:54:16 GMT 2021
> > 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
More information about the Gdb-patches
mailing list