Re: RFC: next/finish/etc -vs- exceptions

Jan Kratochvil
2010-11-25T07:59:12Z

On Thu, 07 Oct 2010 03:37:38 +0200, Tom Tromey wrote:
> * breakpoint.h (enum bptype) <bp_exception, bp_exception_resume,
> bp_exception_master>: New constants.

Therefore it is a precent new `bptype's are permitted instead of using
breakpoint_ops (which may need some extensions, not sure).

It may be explained also by the Joel's difficulties to do so:
http://sourceware.org/ml/gdb-patches/2009-06/msg00298.html

BTW the testcase does not work on neither ppc32 nor on ppc64. ppc32 due to
some unexpected next/step stop lines. I see two places in the code using
CORE_ADDR where is probably gdbarch_convert_from_func_ptr_addr appropriate for
the ppc64 case.

> @@ -8524,6 +8588,10 @@ until_break_command (char *arg, int from_tty, int anywhere)
> frame_unwind_caller_id (frame),
> bp_until);
> make_cleanup_delete_breakpoint (breakpoint2);
> +
> + set_longjmp_breakpoint (thread);
> + tp->initiating_frame = frame_unwind_caller_id (frame);

tp->initiating_frame should be initialized from set_longjmp_breakpoint, as it
is required for that operation.

It probably should not be placed in TP. If we are stepping/until-ing/etc.
some code and execute some breakpoint's command list trying to step/next/etc.
again already from a different frame it won't work. But this is a problem for
most of the TP variables already so that's OK for this patch. It should be
carried over from the set-breakpoint to resume-breakpoint otherwise.

> +static void
> +until_next_continuation (void *arg)
> +{
> + struct thread_info *tp = arg;

Missing empty line. :-)

> + delete_longjmp_breakpoint (tp->num);
> +}

> @@ -1270,7 +1284,19 @@ until_next_command (int from_tty)
>
> tp->step_multi = 0; /* Only one call to proceed */
>
> + set_longjmp_breakpoint (thread);
> + tp->initiating_frame = get_frame_id (frame);
> + old_chain = make_cleanup (delete_longjmp_breakpoint_cleanup, &thread);
> +
> proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 1);
> +
> + if (target_can_async_p () && is_running (inferior_ptid))
> + {
> + discard_cleanups (old_chain);
> + add_continuation (tp, until_next_continuation, tp, NULL);

continuation_free_args is NULL here but I think the breakpoint should get
deleted even if there is some premature thread deletion. But maybe just all
the breakpoints specific for that thread (clear_thread_inferior_resources)
should be deleted which would also solve this problem?

> +static void
> +insert_exception_resume_breakpoint (struct thread_info *tp,
> + struct block *b,
> + struct frame_info *frame,
> + struct symbol *sym)
> +{
> + struct gdb_exception e;
> +
> + /* We want to ignore errors here. */
> + TRY_CATCH (e, RETURN_MASK_ALL)

Shouldn't it be RETURN_MASK_ERROR then? Otherwise it should rethrow QUIT (I
hope it works that way) but no such cleanups are probably needed here.

> +static void
> +check_exception_resume (struct execution_control_state *ecs,
> + struct frame_info *frame, struct symbol *func)
> +{
> + struct gdb_exception e;
> +
> + TRY_CATCH (e, RETURN_MASK_ALL)

again RETURN_MASK_ERROR probably.

> diff --git a/gdb/testsuite/gdb.cp/gdb9593.exp b/gdb/testsuite/gdb.cp/gdb9593.exp
> new file mode 100644
> index 0000000..04e9c82
> --- /dev/null
> +++ b/gdb/testsuite/gdb.cp/gdb9593.exp

Please do not use numeric names for testcases. When dealing with them during
regression testing / rebasing etc. it makes more difficult to keep track of
all the numbers and what they were testing.

> +if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug c++}] != "" } {
> + untested gdb9593.exp
> + return -1
> +}

maybe prepare_for_testing?
(nitpick)

> +gdb_exit
> +gdb_start
> +gdb_reinitialize_dir $srcdir/$subdir
> +gdb_load ${binfile}

prepare_for_testing / clean_restart
(nitpick)

> +if {!$ok} {
> + untested gdb9593.exp

rather unsupported? Maybe not, just a hint.

> + return -1
> +}
> +
> +# See http://sourceware.org/bugzilla/show_bug.cgi?id=9593
> +
> +gdb_test "next" \
> + ".*catch (...).*" \
> + "next over a throw 1"

These all have redundant leading `.*'. `(...)' is not backslashed.

> --- /dev/null
> +++ b/gdb/testsuite/gdb.java/jnpe.exp
[...]
> +set testfile "jnpe"
> +set srcfile ${testfile}.java
> +set binfile ${objdir}/${subdir}/${testfile}
> +if { [compile_java_from_source ${srcdir}/$subdir/${srcfile} ${binfile} "-g"] != "" } {
> + untested "Couldn't compile ${srcdir}/$subdir/${srcfile}"
> + return -1
> +}

maybe prepare_for_testing?
(nitpick)

> +# The line where we stop differ according to gcj; check just we did not already
> +# execute the catch point.
> +
> +gdb_test "next" \
> + "" \

Here should be ".*" (such sanity checking is missing now in lib/gdb.exp).

> --- /dev/null
> +++ b/gdb/testsuite/gdb.java/jnpe.java
> @@ -0,0 +1,38 @@
> +// Test next-over-NPE.
> +/* This testcase is part of GDB, the GNU debugger.
> +
> + Copyright 2009 Free Software Foundation, Inc.

+2010

> + public static void main (String[] args)
> + {
> + try
> + {
> + System.out.println (npe ()); // break here
> + }
> + catch (NullPointerException n)
> + {
> + System.out.println ("success"); // catch point

BTW the testcase fives on Fedora 14 x86_64 and i686:
+FAIL: gdb.java/jnpe.exp: continue to breakpoint: catch point

but this is due to -O0 -g .debug_line wrong in this case.

Breakpoint for line 35 is at 0x400cb3:
0x0000000000400cb3 <+112>: mov $0x601560,%edi
0x0000000000400cb8 <+117>: callq 0x400a18 <_Jv_InitClass@plt>
0x0000000000400cbd <+122>: mov $0x1,%ebx

But the execution goes:
(gdb) stepi
0x0000000000400d0d 33 catch (NullPointerException n)
2: x/i $pc
=> 0x400d0d <jnpe.main(java.lang.String[])void+202>: mov (%rax),%rax
(gdb) stepi
35 System.out.println ("success"); // catch point
2: x/i $pc
=> 0x400d10 <jnpe.main(java.lang.String[])void+205>: test %bl,%bl
(gdb) stepi
0x0000000000400d12 35 System.out.println ("success"); // catch point
2: x/i $pc
=> 0x400d12 <jnpe.main(java.lang.String[])void+207>:
jne 0x400cbd <jnpe.main(java.lang.String[])void+122>
(gdb) stepi
0x0000000000400cbd 35 System.out.println ("success"); // catch point
2: x/i $pc
=> 0x400cbd <jnpe.main(java.lang.String[])void+122>: mov $0x1,%ebx

> + }
> + }
> +}

Thanks,
Jan