From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25455 invoked by alias); 3 Aug 2011 18:11:06 -0000 Received: (qmail 25443 invoked by uid 22791); 3 Aug 2011 18:11:03 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 03 Aug 2011 18:10:46 +0000 Received: (qmail 22019 invoked from network); 3 Aug 2011 18:10:45 -0000 Received: from unknown (HELO scottsdale.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 3 Aug 2011 18:10:45 -0000 From: Pedro Alves To: gdb-patches@sourceware.org Subject: Don't preserve `uiout' across TRY_CATCH. Date: Wed, 03 Aug 2011 18:11:00 -0000 User-Agent: KMail/1.13.6 (Linux/2.6.38-8-generic; KDE/4.6.2; x86_64; ; ) MIME-Version: 1.0 Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Message-Id: <201108031910.42133.pedro@codesourcery.com> X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2011-08/txt/msg00054.txt.bz2 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 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);