Re: [PATCH] improve python finish breakpoint for exceptions/longjmp case.

Andrew Burgess
2012-10-10T21:08:02Z

*ping* *again*

In Summary,

- python finish breakpoint, documentation looks good.
- implementation does not match documentation in exception/longjmp case
- test case does not match documentation
- broken test relies on target implementation of exceptions
- this patch fixes some, but not all of the issues
- as far as I know I introduce no new issues
- will raise a bug for one issue I know about before I commit this patch, and fill in bug id.

Anyone interested?

Thanks,
Andrew

On 01/10/2012 5:30 PM, Andrew Burgess wrote:
> *ping*
>
> I did spot that the patch I attached was not the version with the c++
> streams -> cstdio change in. I changed my mind about including this
> change but forgot to delete the line from my email.
>
> The rest should still be true.
>
> Thanks,
>
> Andrew
>
>
> On 21/09/2012 3:57 PM, Andrew Burgess wrote:
>> The documentation for the python FinishBreakpoints suggests that when a longjmp or c++ exception is used to terminate a function the out_of_scope method will be called. A quick look inside gdb/python/py-finishbreakpoint.c shows that we take no action to ensure that we will break on exceptions or longjmps (for example using breakpoint.c:set_longjmp_breakpoint), instead if we leave a function using for longjmp/exception we rely on hitting some other stop point to trigger the call to the out_of_scope method.
>>
>> A further issue is that the testing for FinishBreakpoints, in gdb/testsuite/gdb.python/py-finish-breakpoint2.exp, the test action titled "check FinishBreakpoint in catch()" expects the "stop" method to fire rather than the "out_of_scope" method, this is due to the generated code (on x86 and maybe other targets), the first breakpoint we hit after throwing the exception happens to be the finish breakpoint, however this is not guaranteed, and means that (a) the test does not match the documentation, and (b) the test is platform specific.
>>
>> I have a patch below that improves and extends the testing to cover more cases, and a patch for py-finishbreakpoint.c that goes some way to fixing some of the problems.
>>
>> There are a couple of comments to go with the patch,
>>
>> 1. I've changed the c++ test program from using c++ streams to using cstdio. I have to confess self interest here, on my local target the c++ streams are too large to build in, so any test that uses them will not work for me. I'm not the only target for which this is the case, see the comments in gdb/testsuite/lib/gdb.exp. Though we _obviously_ have to have good test coverage for gdb with c++ streams, I think it would be a shame if some important behaviours are only tested on targets that support c++ streams. However, if this is a problem then I can change the test back to using c++ streams.
>>
>> 2. The patch does not solve all the problems with FinishBreakpoints. I use set_longjmp_breakpoint to create the breakpoints for the lonjmp/exceptions, however, between creating an instance of FinishBreakpoint, when the breakpoints are created, and hitting these breakpoints, we are free to issue any commands we like, including things like step/finish/etc all of which use set_longjmp_breakpoint themselves. As these commands (step/finish/etc) complete, they remove the longjmp breakpoints, preventing the FinishBreakpoint from stopping correctly. This behaviour is tested in my updated test and currently kfails; before committing this patch I'll raise a defect and fill in the new bug id.
>>
>> I have some ideas about how to fix #2 but I'm open to suggestions. As the gdb is no worse than it was in case #2, and is significantly better in other cases, I'd like to push this patch now (given that I'll raise a bug for broken case).
>>
>> Let me know what you think, OK to commit?
>>
>> Thanks,
>> Andrew
>>
>> gdb/ChangeLog
>>
>> 2012-09-21 Andrew Burgess <aburgess@broadcom.com>
>>
>> * python/py-finishbreakpoint.c (struct finish_breakpoint_object)
>> <initiating_frame>: New field.
>> (bpfinishpy_post_stop_hook): Disable the longjmp breakpoints when
>> we stop at a finish breakpoint. Have the finish breakpoint
>> deleted at the next stop, wherever that might be.
>> (bpfinishpy_init): Set longjmp breakpoints. Remember frame we're
>> in when creating the finish breakpoint.
>> (bpfinishpy_detect_out_scope_cb): Look for frame we are hoping to
>> finish when deciding if we're out of scope, not frame of parent.
>>
>> diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
>> index 56ab775..a2cd980 100644
>> --- a/gdb/python/py-finishbreakpoint.c
>> +++ b/gdb/python/py-finishbreakpoint.c
>> @@ -53,6 +53,9 @@ struct finish_breakpoint_object
>> the function; Py_None if the value is not computable; NULL if GDB is
>> not stopped at a FinishBreakpoint. */
>> PyObject *return_value;
>> + /* The initiating frame for this operation, used to decide when we have
>> + left this frame. */
>> + struct frame_id initiating_frame;
>> };
>>
>> /* Python function to get the 'return_value' attribute of
>> @@ -141,6 +144,10 @@ bpfinishpy_post_stop_hook (struct breakpoint_object *bp_obj)
>> /* Can't delete it here, but it will be removed at the next stop. */
>> disable_breakpoint (bp_obj->bp);
>> gdb_assert (bp_obj->bp->disposition == disp_del);
>> + bp_obj->bp->disposition = disp_del_at_next_stop;
>> +
>> + /* Disable all the longjmp breakpoints too. */
>> + delete_longjmp_breakpoint_at_next_stop (inferior_thread ()->num);
>> }
>> if (except.reason < 0)
>> {
>> @@ -295,11 +302,13 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>> AUTO_BOOLEAN_TRUE,
>> &bkpt_breakpoint_ops,
>> 0, 1, internal_bp, 0);
>> + set_longjmp_breakpoint (inferior_thread (), null_frame_id);
>> }
>> GDB_PY_SET_HANDLE_EXCEPTION (except);
>>
>> self_bpfinish->py_bp.bp->frame_id = frame_id;
>> self_bpfinish->py_bp.is_finish_bp = 1;
>> + self_bpfinish->initiating_frame = get_frame_id (frame);
>>
>> /* Bind the breakpoint with the current program space. */
>> self_bpfinish->py_bp.bp->pspace = current_program_space;
>> @@ -329,6 +338,7 @@ bpfinishpy_out_of_scope (struct finish_breakpoint_object *bpfinish_obj)
>> gdbpy_print_stack ();
>> }
>>
>> + delete_longjmp_breakpoint_at_next_stop (inferior_thread ()->num);
>> delete_breakpoint (bpfinish_obj->py_bp.bp);
>> }
>>
>> @@ -355,9 +365,11 @@ bpfinishpy_detect_out_scope_cb (struct breakpoint *b, void *args)
>> {
>> TRY_CATCH (except, RETURN_MASK_ALL)
>> {
>> + struct frame_id fid = finish_bp->initiating_frame;
>> +
>> if (b->pspace == current_inferior ()->pspace
>> && (!target_has_registers
>> - || frame_find_by_id (b->frame_id) == NULL))
>> + || frame_find_by_id (fid) == NULL))
>> bpfinishpy_out_of_scope (finish_bp);
>> }
>> if (except.reason < 0)
>>
>> 2012-09-21 Andrew Burgess <aburgess@broadcom.com>
>>
>> * gdb.python/py-finish-breakpoint2.cc: Add extra levels of nesting
>> to allow more testing opportunities.
>> * gdb.python/py-finish-breakpoint2.exp: Additional test cases.
>>
>> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc b/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc
>> index 8cc756f..930e6bc 100644
>> --- a/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc
>> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint2.cc
>> @@ -22,7 +22,8 @@
>> void
>> throw_exception_1 (int e)
>> {
>> - throw new int (e);
>> + if (e > 5)
>> + throw new int (e);
>> }
>>
>> void
>> @@ -32,28 +33,79 @@ throw_exception (int e)
>> }
>>
>> int
>> -main (void)
>> +do_test (int e)
>> {
>> - int i;
>> + int i = 0;
>> try
>> {
>> - throw_exception_1 (10);
>> + throw_exception_1 (e);
>> +
>> + i += 1;
>> }
>> catch (const int *e)
>> {
>> std::cerr << "Exception #" << *e << std::endl;
>> }
>> - i += 1; /* Break after exception 1. */
>> + i += 1;
>>
>> try
>> {
>> - throw_exception (10);
>> + throw_exception (e);
>> +
>> + i += 1;
>> }
>> catch (const int *e)
>> {
>> std::cerr << "Exception #" << *e << std::endl;
>> }
>> - i += 1; /* Break after exception 2. */
>> + i += 1;
>> +
>> + return i;
>> +}
>> +
>> +int
>> +call_do_test (int e)
>> +{
>> + int i;
>> +
>> + i = do_test (e);
>> +
>> + throw_exception_1 (e);
>> +
>> + return i;
>> +}
>> +
>> +int
>> +do_nested_test (int e)
>> +{
>> + int i = 0;
>> +
>> + try
>> + {
>> + call_do_test (e);
>> +
>> + i += 1;
>> + }
>> + catch (const int *e)
>> + {
>> + std::cerr << "Exception #" << *e << std::endl;
>> + }
>> + i += 1;
>>
>> return i;
>> }
>> +
>> +
>> +int
>> +main (void)
>> +{
>> + int i = 0;
>> +
>> + i += do_test (10);
>> +
>> + i += do_test (4);
>> +
>> + i += do_nested_test (10);
>> +
>> + return i; /* Additional breakpoint */
>> +}
>> diff --git a/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp b/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
>> index 3b08ef8..f1403d9 100644
>> --- a/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
>> +++ b/gdb/testsuite/gdb.python/py-finish-breakpoint2.exp
>> @@ -37,7 +37,7 @@ if ![runto_main] then {
>> # Check FinishBreakpoints against C++ exceptions
>> #
>>
>> -gdb_breakpoint [gdb_get_line_number "Break after exception 2"]
>> +gdb_breakpoint [gdb_get_line_number "Additional breakpoint"]
>>
>> gdb_test "source $pyfile" ".*Python script imported.*" \
>> "import python scripts"
>> @@ -47,9 +47,43 @@ gdb_test "continue" "Breakpoint .*throw_exception_1.*" "run to exception 1"
>>
>> gdb_test "python print len(gdb.breakpoints())" "3" "check BP count"
>> gdb_test "python ExceptionFinishBreakpoint(gdb.newest_frame())" "init ExceptionFinishBreakpoint" "set FinishBP after the exception"
>> -gdb_test "continue" ".*stopped at ExceptionFinishBreakpoint.*" "check FinishBreakpoint in catch()"
>> +gdb_test "continue" ".*do_test \\(e=10\\).*catch \\(const int \\*e\\).*exception did not finish.*" "FinishBreakpoint with exception thrown caught in parent"
>> gdb_test "python print len(gdb.breakpoints())" "3" "check finish BP removal"
>>
>> gdb_test "continue" ".*Breakpoint.* throw_exception_1.*" "continue to second exception"
>> gdb_test "python ExceptionFinishBreakpoint(gdb.newest_frame())" "init ExceptionFinishBreakpoint" "set FinishBP after the exception"
>> -gdb_test "continue" ".*exception did not finish.*" "FinishBreakpoint with exception thrown not caught"
>> +gdb_test "continue" ".*do_test \\(e=10\\).*catch \\(const int \\*e\\).*exception did not finish.*" "FinishBreakpoint with exception thrown caught in grandparent"
>> +gdb_test "python print len(gdb.breakpoints())" "3" "check finish BP removal"
>> +
>> +gdb_test "continue" ".*Breakpoint.* throw_exception_1.*" "continue to first no throw test"
>> +gdb_test "python ExceptionFinishBreakpoint(gdb.newest_frame())" "init ExceptionFinishBreakpoint" "set FinishBP after the exception"
>> +gdb_test "continue" ".*stopped at ExceptionFinishBreakpoint.*" "FinishBreakpoint with exception not thrown"
>> +gdb_test "python print len(gdb.breakpoints())" "3" "check finish BP removal"
>> +
>> +gdb_test "continue" ".*Breakpoint.* throw_exception_1.*" "continue to second no throw test"
>> +gdb_test "python ExceptionFinishBreakpoint(gdb.newest_frame())" "init ExceptionFinishBreakpoint" "set FinishBP after the exception"
>> +gdb_test "continue" ".*stopped at ExceptionFinishBreakpoint.*" "FinishBreakpoint with exception not thrown"
>> +gdb_test "python print len(gdb.breakpoints())" "3" "check finish BP removal"
>> +
>> +# Now exercies the nested test example, we're creating an
>> +# ExceptionFinishBreakpoint inside a frame, then going to continue into
>> +# further child frames before using the "finish" command, finally, we'll
>> +# continue, and look for the original ExceptionFinishBreakpoint frame to
>> +# finish.
>> +
>> +gdb_breakpoint "call_do_test"
>> +gdb_test "continue" ".*Breakpoint.* call_do_test.*" "continue to nested test."
>> +gdb_test "python print len(gdb.breakpoints())" "4" "check BP count before nested test."
>> +gdb_test "python ExceptionFinishBreakpoint(gdb.newest_frame())" "init ExceptionFinishBreakpoint" "set FinishBP after the exception"
>> +gdb_test "continue" ".*Breakpoint.* throw_exception_1.*" "continue to second no throw test"
>> +gdb_test "finish" ".*do_test \\(e=10\\).*catch \\(const int \\*e\\).*" "finish with exception being thrown, caught in parent"
>> +
>> +gdb_test "continue" ".*Breakpoint.* throw_exception_1.*" "continue to second no throw test"
>> +gdb_test "finish" ".*do_test \\(e=10\\).*catch \\(const int \\*e\\).*" "finish with exception being thrown, caught in grand-parent"
>> +
>> +gdb_test "continue" ".*Breakpoint.* throw_exception_1.*" "continue to second no throw test"
>> +
>> +setup_kfail "BUG-ID" "*-*-*"
>> +gdb_test "continue" ".*catch \\(const int \\*e\\).*exception did not finish.*" "FinishBreakpoint nested with exception thrown caught in parent"
>> +
>> +gdb_test "python print len(gdb.breakpoints())" "4" "check finish BP removal"
>>
>>
>>
>>
>
>
>
>
>