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

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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