Don't preserve `uiout' across TRY_CATCH.

Don't preserve `uiout' across TRY_CATCH.

[Pedro Alves]

Looking at the differences between tui/tui-interp.c:tui_command_loop
and event-top.c:cli_event_loop,
thinking of getting rid of tui_command_loop, the only difference is
that tui_command_loop inlines event-loop.c:start_event_loop, and
adds this bit:

+      /* Update gdb output according to TUI mode.  Since catch_errors
+         preserves the uiout from changing, this must be done at top
+         level of event loop.  */
+      if (tui_active)
+        current_uiout = tui_out;
+      else
+        current_uiout = tui_old_uiout;

I thought, "why don't we just use TRY_CATCH instead of
catch_errors then?".  Turns out we can't as is, because it is
TRY_CATCH itself that preserves the uiout from changing...
I think that's wrong for TRY_CATCH to do - TRY_CATCH should be a
low level exceptions framework, and it should be catch_errors and
whatever other callers that want to preserve uiout that should
guarantee uiout is preserved.  That's what the patch does.  If
it turns out there are places that flip the uiout temporarily and
we inadvertently end up at the top command loop with it still
flipped, then that calls for a missing cleanup instead, at
the place we temporarily swapped the uiout global.

I then found functions that look like:

int
catch_exceptions_with_msg (struct ui_out *uiout, ...
{
  ...
  TRY_CATCH (exception, mask)
   {
     // do stuff assuming `uiout' is the current builder
   }

Notice the subtlety.  We're relying on masking the
global `uiout' with the function's parameter.  If we
rename the parameter, TRY_CATCH would then run the
TRY_CATCH body with the global uiout instead of
temporarily switching to the incoming argument uiout...

`uiout' is not a great name for grepping for uses.  I'll post a
followup that renames it to `current_uiout'.

Tested on x86_64-linux.  Anyone see a problem with this?

-- 
Pedro Alves

2011-08-03  Pedro Alves  <pedro@codesourcery.com>

	gdb/
	* exceptions.c (struct catcher): Remove saved_uiout field.
	(exceptions_state_mc_init): Remove the `func_uiout' parameter, and
	no longer save/resvore the global ui_out builder.
	(catch_exceptions_with_msg): Save/override/restore the global
	ui_out builder manually instead of relying on TRY_CATCH to do it.
	(catch_errors): Save/restore the global ui_out builder manually
	instead of relying on TRY_CATCH to do it.
	* exceptions.h (exceptions_state_mc_init): Remove the `func_uiout'
	parameter.
	(TRY_CATCH): Adjust.
	* cli/cli-interp.c (safe_execute_command): Save/override/restore
	the global ui_out builder manually instead of relying on TRY_CATCH
	to do it.

---
 gdb/cli/cli-interp.c |   11 ++++++++++-
 gdb/exceptions.c     |   47 +++++++++++++++++++++++++++++++++++------------
 gdb/exceptions.h     |    5 ++---
 3 files changed, 47 insertions(+), 16 deletions(-)

Index: src/gdb/exceptions.c
===================================================================
--- src.orig/gdb/exceptions.c	2011-08-03 17:53:31.000000000 +0100
+++ src/gdb/exceptions.c	2011-08-03 18:08:28.876634279 +0100
@@ -60,7 +60,6 @@ struct catcher
   volatile struct gdb_exception *exception;
   /* Saved/current state.  */
   int mask;
-  struct ui_out *saved_uiout;
   struct cleanup *saved_cleanup_chain;
   /* Back link.  */
   struct catcher *prev;
@@ -70,8 +69,7 @@ struct catcher
 static struct catcher *current_catcher;
 
 EXCEPTIONS_SIGJMP_BUF *
-exceptions_state_mc_init (struct ui_out *func_uiout,
-			  volatile struct gdb_exception *exception,
+exceptions_state_mc_init (volatile struct gdb_exception *exception,
 			  return_mask mask)
 {
   struct catcher *new_catcher = XZALLOC (struct catcher);
@@ -84,10 +82,6 @@ exceptions_state_mc_init (struct ui_out
 
   new_catcher->mask = mask;
 
-  /* Override the global ``struct ui_out'' builder.  */
-  new_catcher->saved_uiout = uiout;
-  uiout = func_uiout;
-
   /* Prevent error/quit during FUNC from calling cleanups established
      prior to here.  */
   new_catcher->saved_cleanup_chain = save_cleanups ();
@@ -112,8 +106,6 @@ catcher_pop (void)
 
   restore_cleanups (old_catcher->saved_cleanup_chain);
 
-  uiout = old_catcher->saved_uiout;
-
   xfree (old_catcher);
 }
 
@@ -459,7 +451,7 @@ catch_exceptions (struct ui_out *uiout,
 }
 
 int
-catch_exceptions_with_msg (struct ui_out *uiout,
+catch_exceptions_with_msg (struct ui_out *func_uiout,
 		  	   catch_exceptions_ftype *func,
 		  	   void *func_args,
 			   char **gdberrmsg,
@@ -467,11 +459,27 @@ catch_exceptions_with_msg (struct ui_out
 {
   volatile struct gdb_exception exception;
   volatile int val = 0;
+  struct ui_out *saved_uiout;
 
-  TRY_CATCH (exception, mask)
+  /* Save and override the global ``struct ui_out'' builder.  */
+  saved_uiout = uiout;
+  uiout = func_uiout;
+
+  TRY_CATCH (exception, RETURN_MASK_ALL)
     {
       val = (*func) (uiout, func_args);
     }
+
+  /* Restore the global builder.  */
+  uiout = saved_uiout;
+
+  if (exception.reason < 0 && (mask & RETURN_MASK (exception.reason)) == 0)
+    {
+      /* The caller didn't request that the event be caught.
+	 Rethrow.  */
+      throw_exception (exception);
+    }
+
   print_any_exception (gdb_stderr, NULL, exception);
   gdb_assert (val >= 0);
   gdb_assert (exception.reason <= 0);
@@ -500,11 +508,26 @@ catch_errors (catch_errors_ftype *func,
 {
   volatile int val = 0;
   volatile struct gdb_exception exception;
+  struct ui_out *saved_uiout;
 
-  TRY_CATCH (exception, mask)
+  /* Save the global ``struct ui_out'' builder.  */
+  saved_uiout = uiout;
+
+  TRY_CATCH (exception, RETURN_MASK_ALL)
     {
       val = func (func_args);
     }
+
+  /* Restore the global builder.  */
+  uiout = saved_uiout;
+
+  if (exception.reason < 0 && (mask & RETURN_MASK (exception.reason)) == 0)
+    {
+      /* The caller didn't request that the event be caught.
+	 Rethrow.  */
+      throw_exception (exception);
+    }
+
   print_any_exception (gdb_stderr, errstring, exception);
   if (exception.reason != 0)
     return 0;
Index: src/gdb/exceptions.h
===================================================================
--- src.orig/gdb/exceptions.h	2011-08-03 17:53:31.000000000 +0100
+++ src/gdb/exceptions.h	2011-08-03 17:58:30.946634175 +0100
@@ -114,8 +114,7 @@ extern const struct gdb_exception except
 
 /* Functions to drive the exceptions state m/c (internal to
    exceptions).  */
-EXCEPTIONS_SIGJMP_BUF *exceptions_state_mc_init (struct ui_out *func_uiout,
-						 volatile struct
+EXCEPTIONS_SIGJMP_BUF *exceptions_state_mc_init (volatile struct
 						 gdb_exception *exception,
 						 return_mask mask);
 int exceptions_state_mc_action_iter (void);
@@ -146,7 +145,7 @@ int exceptions_state_mc_action_iter_1 (v
 #define TRY_CATCH(EXCEPTION,MASK) \
      { \
        EXCEPTIONS_SIGJMP_BUF *buf = \
-	 exceptions_state_mc_init (uiout, &(EXCEPTION), (MASK)); \
+	 exceptions_state_mc_init (&(EXCEPTION), (MASK)); \
        EXCEPTIONS_SIGSETJMP (*buf); \
      } \
      while (exceptions_state_mc_action_iter ()) \
Index: src/gdb/cli/cli-interp.c
===================================================================
--- src.orig/gdb/cli/cli-interp.c	2011-08-03 17:53:31.000000000 +0100
+++ src/gdb/cli/cli-interp.c	2011-08-03 18:08:15.186634277 +0100
@@ -112,14 +112,23 @@ cli_interpreter_exec (void *data, const
 }
 
 static struct gdb_exception
-safe_execute_command (struct ui_out *uiout, char *command, int from_tty)
+safe_execute_command (struct ui_out *command_uiout, char *command, int from_tty)
 {
   volatile struct gdb_exception e;
+  struct ui_out *saved_uiout;
+
+  /* Override the global ``struct ui_out'' builder.  */
+  saved_uiout = uiout;
+  uiout = command_uiout;
 
   TRY_CATCH (e, RETURN_MASK_ALL)
     {
       execute_command (command, from_tty);
     }
+
+  /* Restore the global builder.  */
+  uiout = saved_uiout;
+
   /* FIXME: cagney/2005-01-13: This shouldn't be needed.  Instead the
      caller should print the exception.  */
   exception_print (gdb_stderr, e);

Re: Don't preserve `uiout' across TRY_CATCH.

[Tom Tromey]

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

Pedro> I think that's wrong for TRY_CATCH to do - TRY_CATCH should be a
Pedro> low level exceptions framework, and it should be catch_errors and
Pedro> whatever other callers that want to preserve uiout that should
Pedro> guarantee uiout is preserved.

I completely agree.  Thanks for doing this.

Want to see something (sort of) related and gross?  From
throw_exception:

  /* Perhaps it would be cleaner to do this via the cleanup chain (not sure
     I can think of a reason why that is vital, though).  */
  if (tp != NULL)
    {
      /* Clear queued breakpoint commands.  */
      bpstat_clear_actions (tp->control.stop_bpstat);
    }

  disable_current_display ();


Pedro> `uiout' is not a great name for grepping for uses.  I'll post a
Pedro> followup that renames it to `current_uiout'.

Thanks, that sounds great.  One step closer to -Wshadow :-)

Pedro> Tested on x86_64-linux.  Anyone see a problem with this?

It is hard to know whether it will uncover a problem somewhere, but TBH
I would rather put it in and find out.  I did spot-check assignments to
uiout (there aren't many according to my quick grep) and didn't see
anything.

Tom

Re: Don't preserve `uiout' across TRY_CATCH.

[Pedro Alves]

On Wednesday 03 August 2011 20:27:10, Tom Tromey wrote:
> >>>>> "Pedro" == Pedro Alves <pedro@codesourcery.com> writes:
> 
> Pedro> I think that's wrong for TRY_CATCH to do - TRY_CATCH should be a
> Pedro> low level exceptions framework, and it should be catch_errors and
> Pedro> whatever other callers that want to preserve uiout that should
> Pedro> guarantee uiout is preserved.
> 
> I completely agree.  Thanks for doing this.
> 
> Want to see something (sort of) related and gross?  From
> throw_exception:
> 
>   /* Perhaps it would be cleaner to do this via the cleanup chain (not sure
>      I can think of a reason why that is vital, though).  */
>   if (tp != NULL)
>     {
>       /* Clear queued breakpoint commands.  */
>       bpstat_clear_actions (tp->control.stop_bpstat);
>     }
> 
>   disable_current_display ();

Yeah, that so needs to go away!  Any inner exception that is
caught and handled potentially breaks breakpoints and displays...

> Pedro> `uiout' is not a great name for grepping for uses.  I'll post a
> Pedro> followup that renames it to `current_uiout'.
> 
> Thanks, that sounds great.  One step closer to -Wshadow :-)

Coming up as soon as I manage to write the ChangeLog.  :-P

> Pedro> Tested on x86_64-linux.  Anyone see a problem with this?
> 
> It is hard to know whether it will uncover a problem somewhere, but TBH
> I would rather put it in and find out.  I did spot-check assignments to
> uiout (there aren't many according to my quick grep) and didn't see
> anything.

Thanks.  I applied it.

-- 
Pedro Alves

Re: Don't preserve `uiout' across TRY_CATCH.

[Pedro Alves]

On Thursday 04 August 2011 19:26:16, Pedro Alves wrote:
> One step closer to -Wshadow :-)

Reminds me that now could be a good time for
-Wmissing-prototypes and perhaps -Wstrict-prototypes ?

-- 
Pedro Alves

Re: Don't preserve `uiout' across TRY_CATCH.

[Tom Tromey]

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

Pedro> On Thursday 04 August 2011 19:26:16, Pedro Alves wrote:
>> One step closer to -Wshadow :-)

Pedro> Reminds me that now could be a good time for
Pedro> -Wmissing-prototypes and perhaps -Wstrict-prototypes ?

Sounds good to me.

Tom