Re: [patch 3/2] Do not bpstat_clear_actions on throw_exception - async fixes

Re: [patch 3/2] Do not bpstat_clear_actions on throw_exception - async fixes

[Pedro Alves]

[Hi Jan, adding the list back, it looks like you just forgot it]

On Tuesday 23 August 2011 21:35:20, Jan Kratochvil wrote:
> Hi,
> 
> while trying to fix the async mode from:
> 	Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3
> 	http://sourceware.org/ml/gdb-patches/2011-08/msg00424.html
> 
> I have found some errors are now silent - suppressed - in the async mode.
> Fix it here.
> 
> Not going to check in that `gdb_test_no_output "set target-async on"' part as
> I guess one should run the whole testsuite in async mode as Pedro did for:
> 	[WIP/FYI] fix remaining problems with target async
> 	http://sourceware.org/ml/gdb-patches/2011-06/msg00158.html
> 
> 
> Thanks,
> Jan
> 
> 
> gdb/
> 2011-08-23  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 
> 	* inf-loop.c (fetch_inferior_event_wrapper): Remove declaration.
> 	(fetch_inferior_event_error_cleanup): New function, with code from ...
> 	(inferior_event_handler): ... here of INF_REG_EVENT, drop the
> 	bpstat_clear_actions call, register it by make_cleanup instead, remove
> 	catch_errors.  Add exception_print in INF_EXEC_COMPLETE.

Hmm, I don't understand the "drop the bpstat_clear_actions call"
part?  Isn't that undoing your last change to the previous patch?


> 	(fetch_inferior_event_wrapper): Remove.
> 
> --- a/gdb/inf-loop.c
> +++ b/gdb/inf-loop.c
> @@ -30,7 +30,16 @@
>  #include "gdbthread.h"
>  #include "continuations.h"
>  
> -static int fetch_inferior_event_wrapper (gdb_client_data client_data);
> +/* Executed only if fetch_inferior_event throws an exception.  */
> +
> +static void
> +fetch_inferior_event_error_cleanup (void *unused)
> +{
> +  do_all_intermediate_continuations (1);
> +  do_all_continuations (1);
> +  async_enable_stdin ();
> +  display_gdb_prompt (0);
> +}
>  
>  /* General function to handle events in the inferior.  So far it just
>     takes care of detecting errors reported by select() or poll(),
> @@ -47,19 +56,14 @@ inferior_event_handler (enum inferior_event_type event_type,
>    switch (event_type)
>      {
>      case INF_REG_EVENT:
> -      /* Use catch errors for now, until the inner layers of
> -	 fetch_inferior_event (i.e. readchar) can return meaningful
> -	 error status.  If an error occurs while getting an event from
> -	 the target, just cancel the current command.  */
> -      if (!catch_errors (fetch_inferior_event_wrapper, 
> -			 client_data, "", RETURN_MASK_ALL))
> -	{
> -	  bpstat_clear_actions ();
> -	  do_all_intermediate_continuations (1);
> -	  do_all_continuations (1);
> -	  async_enable_stdin ();
> -	  display_gdb_prompt (0);
> -	}
> +      /* Catch exceptions for now by pushing the error case to
> +	 cleanup_if_error, until the inner layers of fetch_inferior_event (i.e.
> +	 readchar) can return meaningful error status.  If an error occurs
> +	 while getting an event from the target, just cancel the current
> +	 command.  */
> +
> +      make_cleanup (fetch_inferior_event_error_cleanup, NULL);
> +      fetch_inferior_event (client_data);

This now lets exceptions go all the way back to start_event_loop,
correct?  I think we'll call async_enable_stdin and display_gdb_prompt
twice then, possibly printing a double prompt.  I guess 
removing those calls from fetch_inferior_event_error_cleanup would
fix it.  But I don't understand the desire to mix this change with ...

>        break;
>  
>      case INF_EXEC_COMPLETE:
> @@ -121,6 +125,7 @@ inferior_event_handler (enum inferior_event_type event_type,
>  	{
>  	  bpstat_do_actions ();
>  	}
> +      exception_print (gdb_stderr, e);

... this one though.  Isn't this bit the only thing missing, and
the rest would be considered just a cleanup?  What am I missing?

>  
>        if (!was_sync
>  	  && exec_done_display_p
> @@ -147,10 +152,3 @@ inferior_event_handler (enum inferior_event_type event_type,
>  
>    discard_cleanups (cleanup_if_error);
>  }
> -
> -static int 
> -fetch_inferior_event_wrapper (gdb_client_data client_data)
> -{
> -  fetch_inferior_event (client_data);
> -  return 1;
> -}
> --- a/gdb/testsuite/gdb.base/commands.exp
> +++ b/gdb/testsuite/gdb.base/commands.exp
> @@ -740,6 +740,7 @@ proc error_clears_commands_left {} {
>      }
>      gdb_test_no_output "end" "main commands 2c"
>  
> +    gdb_test_no_output "set target-async on"
>      gdb_run_cmd
>      gdb_test "" "\r\nhook-stop1\r\n.*\r\ncmd1\r\nUndefined command: \"errorcommandxy\"\\.  Try \"help\"\\." "cmd1 error"
>  
> 

-- 
Pedro Alves

Re: [patch 3/2] Do not bpstat_clear_actions on throw_exception - async fixes

[Jan Kratochvil]

On Wed, 24 Aug 2011 12:33:46 +0200, Pedro Alves wrote:
> Hmm, I don't understand the "drop the bpstat_clear_actions call"
> part?  Isn't that undoing your last change to the previous patch?

I agree, it was bogus.


> > +      exception_print (gdb_stderr, e);
> 
> ... this one though.  Isn't this bit the only thing missing, and
> the rest would be considered just a cleanup?  What am I missing?

Yes.

It looks like pre-approved, I will check it in with the [patch 2/2].
(Without the gdb.base/commands.exp part.)


Thanks,
Jan


gdb/
2011-08-26  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* inf-loop.c (inferior_event_handler): Add exception_print in
	INF_EXEC_COMPLETE.

--- a/gdb/inf-loop.c
+++ b/gdb/inf-loop.c
@@ -121,6 +121,7 @@ inferior_event_handler (enum inferior_event_type event_type,
 	{
 	  bpstat_do_actions ();
 	}
+      exception_print (gdb_stderr, e);
 
       if (!was_sync
 	  && exec_done_display_p
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -740,6 +740,7 @@ proc error_clears_commands_left {} {
     }
     gdb_test_no_output "end" "main commands 2c"
 
+    gdb_test_no_output "set target-async on"
     gdb_run_cmd
     gdb_test "" "\r\nhook-stop1\r\n.*\r\ncmd1\r\nUndefined command: \"errorcommandxy\"\\.  Try \"help\"\\." "cmd1 error"
 

Re: [patch 3/2] Do not bpstat_clear_actions on throw_exception - async fixes

[Pedro Alves]

On Friday 26 August 2011 22:07:06, Jan Kratochvil wrote:

> It looks like pre-approved, I will check it in with the [patch 2/2].
> (Without the gdb.base/commands.exp part.)

Thanks.

-- 
Pedro Alves

Re: [patch 3/2] Do not bpstat_clear_actions on throw_exception - async fixes

[Jan Kratochvil]

On Fri, 26 Aug 2011 23:38:15 +0200, Pedro Alves wrote:
> Thanks.

Checked in.


Thanks,
Jan


http://sourceware.org/ml/gdb-cvs/2011-08/msg00121.html

--- src/gdb/ChangeLog	2011/08/26 21:45:21	1.13285
+++ src/gdb/ChangeLog	2011/08/26 21:47:21	1.13286
@@ -1,5 +1,10 @@
 2011-08-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
 
+	* inf-loop.c (inferior_event_handler): Add exception_print in
+	INF_EXEC_COMPLETE.
+
+2011-08-26  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
 	* breakpoint.c (bpstat_do_actions): New variable cleanup_if_error, call
 	make_bpstat_clear_actions_cleanup and discard_cleanups for it.
 	* defs.h (make_bpstat_clear_actions_cleanup): New declaration.
--- src/gdb/inf-loop.c	2011/08/26 21:45:22	1.35
+++ src/gdb/inf-loop.c	2011/08/26 21:47:21	1.36
@@ -121,6 +121,7 @@
 	{
 	  bpstat_do_actions ();
 	}
+      exception_print (gdb_stderr, e);
 
       if (!was_sync
 	  && exec_done_display_p

[patch 3/2] Do not bpstat_clear_actions on throw_exception - async fixes

[Jan Kratochvil]

Hi,

while trying to fix the async mode from:
	Re: [patch 2/2] Do not bpstat_clear_actions on throw_exception #3
	http://sourceware.org/ml/gdb-patches/2011-08/msg00424.html

I have found some errors are now silent - suppressed - in the async mode.
Fix it here.

Not going to check in that `gdb_test_no_output "set target-async on"' part as
I guess one should run the whole testsuite in async mode as Pedro did for:
	[WIP/FYI] fix remaining problems with target async
	http://sourceware.org/ml/gdb-patches/2011-06/msg00158.html


Thanks,
Jan


gdb/
2011-08-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* inf-loop.c (fetch_inferior_event_wrapper): Remove declaration.
	(fetch_inferior_event_error_cleanup): New function, with code from ...
	(inferior_event_handler): ... here of INF_REG_EVENT, drop the
	bpstat_clear_actions call, register it by make_cleanup instead, remove
	catch_errors.  Add exception_print in INF_EXEC_COMPLETE.
	(fetch_inferior_event_wrapper): Remove.

--- a/gdb/inf-loop.c
+++ b/gdb/inf-loop.c
@@ -30,7 +30,16 @@
 #include "gdbthread.h"
 #include "continuations.h"
 
-static int fetch_inferior_event_wrapper (gdb_client_data client_data);
+/* Executed only if fetch_inferior_event throws an exception.  */
+
+static void
+fetch_inferior_event_error_cleanup (void *unused)
+{
+  do_all_intermediate_continuations (1);
+  do_all_continuations (1);
+  async_enable_stdin ();
+  display_gdb_prompt (0);
+}
 
 /* General function to handle events in the inferior.  So far it just
    takes care of detecting errors reported by select() or poll(),
@@ -47,19 +56,14 @@ inferior_event_handler (enum inferior_event_type event_type,
   switch (event_type)
     {
     case INF_REG_EVENT:
-      /* Use catch errors for now, until the inner layers of
-	 fetch_inferior_event (i.e. readchar) can return meaningful
-	 error status.  If an error occurs while getting an event from
-	 the target, just cancel the current command.  */
-      if (!catch_errors (fetch_inferior_event_wrapper, 
-			 client_data, "", RETURN_MASK_ALL))
-	{
-	  bpstat_clear_actions ();
-	  do_all_intermediate_continuations (1);
-	  do_all_continuations (1);
-	  async_enable_stdin ();
-	  display_gdb_prompt (0);
-	}
+      /* Catch exceptions for now by pushing the error case to
+	 cleanup_if_error, until the inner layers of fetch_inferior_event (i.e.
+	 readchar) can return meaningful error status.  If an error occurs
+	 while getting an event from the target, just cancel the current
+	 command.  */
+
+      make_cleanup (fetch_inferior_event_error_cleanup, NULL);
+      fetch_inferior_event (client_data);
       break;
 
     case INF_EXEC_COMPLETE:
@@ -121,6 +125,7 @@ inferior_event_handler (enum inferior_event_type event_type,
 	{
 	  bpstat_do_actions ();
 	}
+      exception_print (gdb_stderr, e);
 
       if (!was_sync
 	  && exec_done_display_p
@@ -147,10 +152,3 @@ inferior_event_handler (enum inferior_event_type event_type,
 
   discard_cleanups (cleanup_if_error);
 }
-
-static int 
-fetch_inferior_event_wrapper (gdb_client_data client_data)
-{
-  fetch_inferior_event (client_data);
-  return 1;
-}
--- a/gdb/testsuite/gdb.base/commands.exp
+++ b/gdb/testsuite/gdb.base/commands.exp
@@ -740,6 +740,7 @@ proc error_clears_commands_left {} {
     }
     gdb_test_no_output "end" "main commands 2c"
 
+    gdb_test_no_output "set target-async on"
     gdb_run_cmd
     gdb_test "" "\r\nhook-stop1\r\n.*\r\ncmd1\r\nUndefined command: \"errorcommandxy\"\\.  Try \"help\"\\." "cmd1 error"