RFC: finish_command_continuation and errors

RFC: finish_command_continuation and errors

[Tom Tromey]

I'd appreciate comments on this patch, particularly from Pedro.

While debugging a gcj-compiled program yesterday, I got this error while
trying to re-run:

Cannot insert breakpoint 0.
Error accessing memory address 0x3e1dc5: Input/output error.

Looking at "maint info b" showed:

(gdb) maint info b
Num     Type           Disp Enb Address    What
0       finish         keep y   0x003ea395 ../../../../../trunk/libjava/classpath/tools/gnu/classpath/tools/javah/Main.java:414 inf 1 thread 1
	stop only in thread 1


This can happen if gdb calls error while printing the return value.
In this case, the finish breakpoint is never deleted.

This patch fixes the problem by wrapping the call to print_return_value
in a TRY_CATCH.  Other fixes are possible, but I chose this one because
it is small and occurs at the point in the code that must be
exception-safe.

Built and regtested on x86-64 (compile farm).
I also tried it again on my test case, provoked the error, and used
"maint info b" to verify that the finish breakpoint was deleted.

I also glanced at the other continuation functions in infcmd.c, but none
of them appeared to have a similar problem.

Tom

2010-03-18  Tom Tromey  <tromey@redhat.com>

	* infcmd.c (finish_command_continuation): Wrap print_return_value
	in TRY_CATCH.

Index: infcmd.c
===================================================================
RCS file: /cvs/src/src/gdb/infcmd.c,v
retrieving revision 1.259
diff -u -r1.259 infcmd.c
--- infcmd.c	16 Feb 2010 21:18:46 -0000	1.259
+++ infcmd.c	18 Mar 2010 17:00:51 -0000
@@ -1420,7 +1420,19 @@
 			_("finish_command: function has no target type"));
 
       if (TYPE_CODE (value_type) != TYPE_CODE_VOID)
-	print_return_value (SYMBOL_TYPE (a->function), value_type);
+	{
+	  volatile struct gdb_exception ex;
+
+	  TRY_CATCH (ex, RETURN_MASK_ALL)
+	    {
+	      /* print_return_value can throw an exception in some
+		 circumstances.  We need to catch this so that we still
+		 delete the breakpoint.  */
+	      print_return_value (SYMBOL_TYPE (a->function), value_type);
+	    }
+	  if (ex.reason < 0)
+	    exception_print (gdb_stdout, ex);
+	}
     }
 
   /* We suppress normal call of normal_stop observer and do it here so

Re: RFC: finish_command_continuation and errors

[Pedro Alves]

On Thursday 18 March 2010 17:07:06, Tom Tromey wrote:

> This can happen if gdb calls error while printing the return value.

For the record, can you show us which error was that?

> In this case, the finish breakpoint is never deleted.
> 
> This patch fixes the problem by wrapping the call to print_return_value
> in a TRY_CATCH.  Other fixes are possible, but I chose this one because
> it is small and occurs at the point in the code that must be
> exception-safe.
> 
> Built and regtested on x86-64 (compile farm).
> I also tried it again on my test case, provoked the error, and used
> "maint info b" to verify that the finish breakpoint was deleted.

This probably also fixes frontends: the normal_stop observers notification
call just a bit below was skipped too, which means the MI *stopped
notification must have gone missing; a frontend was being left with no
idea the thread had stopped.  Could you check with your test, but running
with -i=mi, that mi_on_normal_stop also isn't throwing an exception too
in your case?  I suspect not, but just in case.  You can issue the normal
CLI commands while in -i=mi to test this.

> 2010-03-18  Tom Tromey  <tromey@redhat.com>
> 
> 	* infcmd.c (finish_command_continuation): Wrap print_return_value
> 	in TRY_CATCH.

Okay.  Thanks.

-- 
Pedro Alves

Re: RFC: finish_command_continuation and errors

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

>> This can happen if gdb calls error while printing the return value.

Pedro> For the record, can you show us which error was that?

In this case it was:

Value returned is $9 = warning: RTTI symbol not found for class 'java::lang::String'
Cannot access memory at address 0x0

Pedro> This probably also fixes frontends: the normal_stop observers
Pedro> notification call just a bit below was skipped too, which means
Pedro> the MI *stopped notification must have gone missing; a frontend
Pedro> was being left with no idea the thread had stopped.  Could you
Pedro> check with your test, but running with -i=mi, that
Pedro> mi_on_normal_stop also isn't throwing an exception too in your
Pedro> case?  I suspect not, but just in case.  You can issue the normal
Pedro> CLI commands while in -i=mi to test this.

I tried this and it worked fine after my patch.  From what I can see,
mi_on_normal_stop doesn't try to print the return value.

It seems to me that, abstractly, observers should not be allowed to
throw exceptions.  Perhaps the observer machinery itself ought to catch
them.  My reasoning is that letting an observer throw an exception will
make the observer mechanism apparently unreliable: a given observer
might or might not be called, depending on whether some earlier observer
encountered an error.

Tom

Re: RFC: finish_command_continuation and errors

[Joel Brobecker]

> It seems to me that, abstractly, observers should not be allowed to
> throw exceptions.  Perhaps the observer machinery itself ought to catch
> them.  My reasoning is that letting an observer throw an exception will
> make the observer mechanism apparently unreliable: a given observer
> might or might not be called, depending on whether some earlier observer
> encountered an error.

I think this makes sense.

-- 
Joel

Re: RFC: finish_command_continuation and errors

[Pedro Alves]

On Thursday 18 March 2010 17:59:02, Tom Tromey wrote:
> Pedro> For the record, can you show us which error was that?
> 
> In this case it was:
> 
> Value returned is $9 = warning: RTTI symbol not found for class 'java::lang::String'
> Cannot access memory at address 0x0

Thanks.  Looks strange that GDB found something missing, and still
tried to use address 0x0 though.  Is the 0 GDB working with a NULL
type somewhere?  Or is that unrelated?

> Pedro> This probably also fixes frontends: the normal_stop observers
> Pedro> notification call just a bit below was skipped too, which means
> Pedro> the MI *stopped notification must have gone missing; a frontend
> Pedro> was being left with no idea the thread had stopped.  Could you
> Pedro> check with your test, but running with -i=mi, that
> Pedro> mi_on_normal_stop also isn't throwing an exception too in your
> Pedro> case?  I suspect not, but just in case.  You can issue the normal
> Pedro> CLI commands while in -i=mi to test this.
> 
> I tried this and it worked fine after my patch.  From what I can see,
> mi_on_normal_stop doesn't try to print the return value.

Yes.  Thanks.  I didn't know what exactly was failing before -- I
was wondering if frame printing would also be failing.

> It seems to me that, abstractly, observers should not be allowed to
> throw exceptions.  Perhaps the observer machinery itself ought to catch
> them.  My reasoning is that letting an observer throw an exception will
> make the observer mechanism apparently unreliable: a given observer
> might or might not be called, depending on whether some earlier observer
> encountered an error.

Yes, agreed in general.  I think some classes of errors still
need escaping though, like internal errors, and Quits.

-- 
Pedro Alves

Re: RFC: finish_command_continuation and errors

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:

>> Value returned is $9 = warning: RTTI symbol not found for class 'java::lang::String'
>> Cannot access memory at address 0x0

Pedro> Thanks.  Looks strange that GDB found something missing, and still
Pedro> tried to use address 0x0 though.  Is the 0 GDB working with a NULL
Pedro> type somewhere?  Or is that unrelated?

I haven't investigated this yet.

The RTTI thing is just a warning, so I assume that higher layers don't
know that it happened.  It is strange that this is even attempted for a
Java type, though, because Java doesn't emit C++ RTTI info at all.

There are other problems Java too, I'm working on a separate patch to
fix some crashes.

Tom