From patchwork Wed Sep 27 22:37:18 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 23182 Received: (qmail 101840 invoked by alias); 27 Sep 2017 22:37:24 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 101825 invoked by uid 89); 27 Sep 2017 22:37:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_STOCKGEN, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=restores, 6897 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 27 Sep 2017 22:37:21 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F2A2E776DF; Wed, 27 Sep 2017 22:37:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com F2A2E776DF Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves@redhat.com Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id D67BD8BE30; Wed, 27 Sep 2017 22:37:18 +0000 (UTC) Subject: Re: [RFA] Change exceptions.h functions to use gdb::function_view To: Tom Tromey References: <20170927165302.30177-1-tom@tromey.com> <4098e86f-d084-fb3b-ea9b-0d5565ddd930@redhat.com> <87o9pvyjlb.fsf@pokyo> Cc: gdb-patches@sourceware.org From: Pedro Alves Message-ID: <7262baed-d54f-4c7c-abe3-b2bd90ff9562@redhat.com> Date: Wed, 27 Sep 2017 23:37:18 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <87o9pvyjlb.fsf@pokyo> On 09/27/2017 09:25 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves writes: > > Pedro> I'm borderline about this. I have to say that I question the value of > Pedro> catch_exceptions&co, over just using TRY/CATCH + a scoped_restore(current_uiout) > Pedro> in the try scope + printing the exception. A TRY/CATCH is likely to be > Pedro> easier to understand and debug, I think. > [...] > Pedro> Did you consider this? > > Nope. This was actually just a preparatory patch for another const change. > > I can look into the change though. I agree it would be better. I took a stab at this. I converted catch_errors to see what it looks like. See below a(n untested) patch on top of yours. This exposed further cleanup opportunities around no longer having to follow catch_errors callback type, and also removes a few cleanups. I didn't do anything to save/restore current_uiout because it feels to me like that should be the responsibility of the code that changes current_uiout in the first place. For the the other catch_foo functions, we may need to write scoped restores. > >>> + { >>> + return do_captured_thread_select (inner_uiout, tidstr); >>> + }, > > Pedro> Note the patch has several cases of tabs vs spaces like above. > > Sorry about that. > Was there ever a decision about not using tabs? > Nope. I push this to the users/palves/catch_exceptions branch too. From bea0a5aeece991b2f4f20a746ce655283607d9a6 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 27 Sep 2017 22:43:26 +0100 Subject: [PATCH] zap catch_errors --- gdb/breakpoint.c | 155 +++++++++++++++++++++++++----------------------------- gdb/exceptions.c | 39 -------------- gdb/exceptions.h | 32 +++-------- gdb/infrun.c | 41 +++++++++------ gdb/main.c | 10 +++- gdb/objc-lang.c | 26 ++++----- gdb/record-full.c | 23 ++++---- gdb/solib.c | 19 ++++--- gdb/symmisc.c | 37 ++++++------- 9 files changed, 167 insertions(+), 215 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index d4ae985..02e028c 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -103,8 +103,6 @@ static void map_breakpoint_numbers (const char *, static void ignore_command (char *, int); -static int breakpoint_re_set_one (void *); - static void breakpoint_re_set_default (struct breakpoint *); static void @@ -178,8 +176,6 @@ static void info_breakpoints_command (char *, int); static void info_watchpoints_command (char *, int); -static int breakpoint_cond_eval (void *); - static void cleanup_executing_breakpoints (void *); static void commands_command (char *, int); @@ -191,8 +187,6 @@ static int remove_breakpoint_1 (struct bp_location *, enum remove_bp_reason); static enum print_stop_action print_bp_stop_message (bpstat bs); -static int watchpoint_check (void *); - static int hw_breakpoint_used_count (void); static int hw_watchpoint_use_count (struct breakpoint *); @@ -4842,21 +4836,16 @@ bpstat_print (bpstat bs, int kind) return PRINT_UNKNOWN; } -/* Evaluate the expression EXP and return 1 if value is zero. - This returns the inverse of the condition because it is called - from catch_errors which returns 0 if an exception happened, and if an - exception happens we want execution to stop. - The argument is a "struct expression *" that has been cast to a - "void *" to make it pass through catch_errors. */ +/* Evaluate the boolean expression EXP and return the result. */ -static int -breakpoint_cond_eval (void *exp) +static bool +breakpoint_cond_eval (expression *exp) { struct value *mark = value_mark (); - int i = !value_true (evaluate_expression ((struct expression *) exp)); + bool res = value_true (evaluate_expression (exp)); value_free_to_mark (mark); - return i; + return res; } /* Allocate a new bpstat. Link it to the FIFO list by BS_LINK_POINTER. */ @@ -4966,30 +4955,28 @@ watchpoints_triggered (struct target_waitstatus *ws) return 1; } -/* Possible return values for watchpoint_check (this can't be an enum - because of check_errors). */ -/* The watchpoint has been deleted. */ -#define WP_DELETED 1 -/* The value has changed. */ -#define WP_VALUE_CHANGED 2 -/* The value has not changed. */ -#define WP_VALUE_NOT_CHANGED 3 -/* Ignore this watchpoint, no matter if the value changed or not. */ -#define WP_IGNORE 4 +/* Possible return values for watchpoint_check. */ +enum wp_check_result + { + /* The watchpoint has been deleted. */ + WP_DELETED = 1, + /* The value has changed. */ + WP_VALUE_CHANGED = 2, + /* The value has not changed. */ + WP_VALUE_NOT_CHANGED = 3, + /* Ignore this watchpoint, no matter if the value changed or not. */ + WP_IGNORE = 4, + }; #define BP_TEMPFLAG 1 #define BP_HARDWAREFLAG 2 /* Evaluate watchpoint condition expression and check if its value - changed. - - P should be a pointer to struct bpstat, but is defined as a void * - in order for this function to be usable with catch_errors. */ + changed. */ -static int -watchpoint_check (void *p) +static wp_check_result +watchpoint_check (bpstat bs) { - bpstat bs = (bpstat) p; struct watchpoint *b; struct frame_info *fr; int within_current_scope; @@ -5185,16 +5172,29 @@ bpstat_check_watchpoint (bpstat bs) if (must_check_value) { - char *message - = xstrprintf ("Error evaluating expression for watchpoint %d\n", - b->number); - struct cleanup *cleanups = make_cleanup (xfree, message); - int e = catch_errors ([&] () - { - return watchpoint_check (bs); - }, - message, RETURN_MASK_ALL); - do_cleanups (cleanups); + wp_check_result e; + + TRY + { + e = watchpoint_check (bs); + } + CATCH (ex, RETURN_MASK_ALL) + { + exception_fprintf (gdb_stderr, ex, + "Error evaluating expression " + "for watchpoint %d\n", + b->number); + + SWITCH_THRU_ALL_UIS () + { + printf_filtered (_("Watchpoint %d deleted.\n"), + b->number); + } + watchpoint_del_at_next_stop (b); + e = WP_DELETED; + } + END_CATCH + switch (e) { case WP_DELETED: @@ -5290,18 +5290,6 @@ bpstat_check_watchpoint (bpstat bs) break; default: /* Can't happen. */ - case 0: - /* Error from catch_errors. */ - { - SWITCH_THRU_ALL_UIS () - { - printf_filtered (_("Watchpoint %d deleted.\n"), - b->number); - } - watchpoint_del_at_next_stop (b); - /* We've already printed what needs to be printed. */ - bs->print_it = print_it_done; - } break; } } @@ -5327,7 +5315,8 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid) { const struct bp_location *bl; struct breakpoint *b; - int value_is_zero = 0; + /* Assume stop. */ + bool condition_result = true; struct expression *cond; gdb_assert (bs->stop); @@ -5423,26 +5412,30 @@ bpstat_check_breakpoint_conditions (bpstat bs, ptid_t ptid) within_current_scope = 0; } if (within_current_scope) - value_is_zero - = catch_errors ([&] () - { - return breakpoint_cond_eval (cond); - }, - "Error in testing breakpoint condition:\n", - RETURN_MASK_ALL); + { + TRY + { + condition_result = breakpoint_cond_eval (cond); + } + CATCH (ex, RETURN_MASK_ALL) + { + exception_fprintf (gdb_stderr, ex, + "Error in testing breakpoint condition:\n"); + } + END_CATCH + } else { warning (_("Watchpoint condition cannot be tested " "in the current scope")); /* If we failed to set the right context for this watchpoint, unconditionally report it. */ - value_is_zero = 0; } /* FIXME-someday, should give breakpoint #. */ value_free_to_mark (mark); } - if (cond && value_is_zero) + if (cond && !condition_result) { bs->stop = 0; } @@ -14149,21 +14142,16 @@ prepare_re_set_context (struct breakpoint *b) return make_cleanup (null_cleanup, NULL); } -/* Reset a breakpoint given it's struct breakpoint * BINT. - The value we return ends up being the return value from catch_errors. - Unused in this case. */ +/* Reset a breakpoint. */ -static int -breakpoint_re_set_one (void *bint) +static void +breakpoint_re_set_one (breakpoint *b) { - /* Get past catch_errs. */ - struct breakpoint *b = (struct breakpoint *) bint; struct cleanup *cleanups; cleanups = prepare_re_set_context (b); b->ops->re_set (b); do_cleanups (cleanups); - return 0; } /* Re-set breakpoint locations for the current program space. @@ -14189,16 +14177,17 @@ breakpoint_re_set (void) ALL_BREAKPOINTS_SAFE (b, b_tmp) { - /* Format possible error msg. */ - char *message = xstrprintf ("Error in re-setting breakpoint %d: ", - b->number); - struct cleanup *cleanups = make_cleanup (xfree, message); - catch_errors ([&] () - { - return breakpoint_re_set_one (b); - }, - message, RETURN_MASK_ALL); - do_cleanups (cleanups); + TRY + { + breakpoint_re_set_one (b); + } + CATCH (ex, RETURN_MASK_ALL) + { + exception_fprintf (gdb_stderr, ex, + "Error in re-setting breakpoint %d: ", + b->number); + } + END_CATCH } set_language (save_language); input_radix = save_input_radix; diff --git a/gdb/exceptions.c b/gdb/exceptions.c index bbd97cc..97a56e6 100644 --- a/gdb/exceptions.c +++ b/gdb/exceptions.c @@ -215,45 +215,6 @@ catch_exceptions_with_msg (struct ui_out *func_uiout, return val; } -/* This function is superseded by catch_exceptions(). */ - -int -catch_errors (gdb::function_view func, - const char *errstring, return_mask mask) -{ - struct gdb_exception exception = exception_none; - volatile int val = 0; - struct ui_out *saved_uiout; - - /* Save the global ``struct ui_out'' builder. */ - saved_uiout = current_uiout; - - TRY - { - val = func (); - } - CATCH (ex, RETURN_MASK_ALL) - { - exception = ex; - } - END_CATCH - - /* Restore the global builder. */ - current_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); - } - - exception_fprintf (gdb_stderr, exception, "%s", errstring); - if (exception.reason != 0) - return 0; - return val; -} - /* See exceptions.h. */ int diff --git a/gdb/exceptions.h b/gdb/exceptions.h index c65974d..73aa870 100644 --- a/gdb/exceptions.h +++ b/gdb/exceptions.h @@ -48,21 +48,16 @@ extern void exception_fprintf (struct ui_file *file, struct gdb_exception e, copy of the gdb error message. This is used when a silent error is issued and the caller wants to manually issue the error message. - MASK specifies what to catch; it is normally set to - RETURN_MASK_ALL, if for no other reason than that the code which - calls catch_errors might not be set up to deal with a quit which - isn't caught. But if the code can deal with it, it generally - should be RETURN_MASK_ERROR, unless for some reason it is more - useful to abort only the portion of the operation inside the - catch_errors. Note that quit should return to the command line + MASK specifies what to catch; it is normally set to RETURN_MASK_ALL + if the code which calls catch_exceptions is not set up to deal with + a quit which isn't caught. But if the code can deal with it, it + generally should be RETURN_MASK_ERROR, unless for some reason it is + more useful to abort only the portion of the operation inside the + catch_exceptions. Note that quit should return to the command line fairly quickly, even if some further processing is being done. FIXME; cagney/2001-08-13: The need to override the global UIOUT - builder variable should just go away. - - This function supersedes catch_errors(). - - This function uses SETJMP() and LONGJUMP(). */ + builder variable should just go away. */ struct ui_out; typedef int (catch_exceptions_ftype) (struct ui_out *ui_out); @@ -75,19 +70,6 @@ extern int catch_exceptions_with_msg char **gdberrmsg, return_mask mask); -/* If CATCH_ERRORS_FTYPE throws an error, catch_errors() returns zero - otherwize the result from CATCH_ERRORS_FTYPE is returned. It is - probably useful for CATCH_ERRORS_FTYPE to always return a non-zero - value. It's unfortunate that, catch_errors() does not return an - indication of the exact exception that it caught - quit_flag might - help. - - This function is superseded by catch_exceptions(). */ - -typedef int (catch_errors_ftype) (); -extern int catch_errors (gdb::function_view, - const char *, return_mask); - /* Compare two exception objects for print equality. */ extern int exception_print_same (struct gdb_exception e1, struct gdb_exception e2); diff --git a/gdb/infrun.c b/gdb/infrun.c index 149a358..f8d5be1 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -8308,12 +8308,16 @@ normal_stop (void) struct cleanup *old_chain = make_cleanup (release_stop_context_cleanup, saved_context); - catch_errors ([&] () - { - execute_cmd_pre_hook (stop_command); - return 0; - }, - "Error while running hook_stop:\n", RETURN_MASK_ALL); + TRY + { + execute_cmd_pre_hook (stop_command); + } + CATCH (ex, RETURN_MASK_ALL) + { + exception_fprintf (gdb_stderr, ex, + "Error while running hook_stop:\n"); + } + END_CATCH /* If the stop hook resumes the target, then there's no point in trying to notify about the previous stop; its context is @@ -9021,19 +9025,22 @@ restore_infcall_control_state (struct infcall_control_state *inf_status) if (target_has_stack) { - /* The point of catch_errors is that if the stack is clobbered, + /* The point of the try/catch is that if the stack is clobbered, walking the stack might encounter a garbage pointer and error() trying to dereference it. */ - if (catch_errors - ([&] () - { - return restore_selected_frame (&inf_status->selected_frame_id); - }, - "Unable to restore previously selected frame:\n", - RETURN_MASK_ERROR) == 0) - /* Error in restoring the selected frame. Select the innermost - frame. */ - select_frame (get_current_frame ()); + TRY + { + restore_selected_frame (&inf_status->selected_frame_id); + } + CATCH (ex, RETURN_MASK_ERROR) + { + exception_fprintf (gdb_stderr, ex, + "Unable to restore previously selected frame:\n"); + /* Error in restoring the selected frame. Select the + innermost frame. */ + select_frame (get_current_frame ()); + } + END_CATCH } xfree (inf_status); diff --git a/gdb/main.c b/gdb/main.c index 8408bfc..3dd08ce 100644 --- a/gdb/main.c +++ b/gdb/main.c @@ -1147,7 +1147,15 @@ captured_main (void *data) change - SET_TOP_LEVEL() - has been eliminated. */ while (1) { - catch_errors (captured_command_loop, "", RETURN_MASK_ALL); + TRY + { + captured_command_loop (); + } + CATCH (ex, RETURN_MASK_ALL) + { + exception_print (gdb_stderr, ex); + } + END_CATCH } /* No exit -- exit is through quit_command. */ } diff --git a/gdb/objc-lang.c b/gdb/objc-lang.c index 2b84904..10ee691 100644 --- a/gdb/objc-lang.c +++ b/gdb/objc-lang.c @@ -1315,19 +1315,19 @@ find_objc_msgcall_submethod (int (*f) (CORE_ADDR, CORE_ADDR *), CORE_ADDR pc, CORE_ADDR *new_pc) { - if (catch_errors ([&] () - { - if (f (pc, new_pc) == 0) - return 1; - else - return 0; - }, - "Unable to determine target of " - "Objective-C method call (ignoring):\n", - RETURN_MASK_ALL) == 0) - return 1; - else - return 0; + TRY + { + if (f (pc, new_pc) == 0) + return 1; + } + CATCH (ex, RETURN_MASK_ALL) + { + exception_fprintf (gdb_stderr, ex, + "Unable to determine target of " + "Objective-C method call (ignoring):\n"); + } + + return 0; } int diff --git a/gdb/record-full.c b/gdb/record-full.c index 2d9085a..cf4aa5f 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -565,7 +565,7 @@ record_full_arch_list_cleanups (void *ignore) record the running message of inferior and set them to record_full_arch_list, and add it to record_full_list. */ -static int +static void record_full_message (struct regcache *regcache, enum gdb_signal signal) { int ret; @@ -633,19 +633,24 @@ record_full_message (struct regcache *regcache, enum gdb_signal signal) record_full_list_release_first (); else record_full_insn_num++; - - return 1; } -static int +static bool record_full_message_wrapper_safe (struct regcache *regcache, enum gdb_signal signal) { - return catch_errors ([&] () - { - return record_full_message (regcache, signal); - }, - "", RETURN_MASK_ALL); + TRY + { + record_full_message (regcache, signal); + } + CATCH (ex, RETURN_MASK_ALL) + { + exception_print (gdb_stderr, ex); + return false; + } + END_CATCH + + return true; } /* Set to 1 if record_full_store_registers and record_full_xfer_partial diff --git a/gdb/solib.c b/gdb/solib.c index 0349c8f..e605e6b 100644 --- a/gdb/solib.c +++ b/gdb/solib.c @@ -760,12 +760,19 @@ update_solib_list (int from_tty) have not opened a symbol file, we may be able to get its symbols now! */ if (inf->attach_flag && symfile_objfile == NULL) - catch_errors ([&] () - { - return ops->open_symbol_file_object (from_tty); - }, - "Error reading attached process's symbol file.\n", - RETURN_MASK_ALL); + { + TRY + { + ops->open_symbol_file_object (from_tty); + } + CATCH (ex, RETURN_MASK_ALL) + { + exception_fprintf (gdb_stderr, ex, + "Error reading attached " + "process's symbol file.\n"); + } + END_CATCH + } } /* GDB and the inferior's dynamic linker each maintain their own diff --git a/gdb/symmisc.c b/gdb/symmisc.c index 6236bad..aae42bb 100644 --- a/gdb/symmisc.c +++ b/gdb/symmisc.c @@ -54,16 +54,9 @@ FILE *std_err; static int block_depth (struct block *); -struct print_symbol_args - { - struct gdbarch *gdbarch; - struct symbol *symbol; - int depth; - struct ui_file *outfile; - }; - -static int print_symbol (struct gdbarch *gdbarch, struct symbol *symbol, - int depth, struct ui_file *outfile); +static void print_symbol (struct gdbarch *gdbarch, struct symbol *symbol, + int depth, struct ui_file *outfile); + void @@ -358,13 +351,16 @@ dump_symtab_1 (struct symtab *symtab, struct ui_file *outfile) block, not any blocks from included symtabs. */ ALL_DICT_SYMBOLS (BLOCK_DICT (b), iter, sym) { - catch_errors ([&] () - { - return print_symbol (gdbarch, sym, depth + 1, - outfile); - }, - "Error printing symbol:\n", - RETURN_MASK_ERROR); + TRY + { + print_symbol (gdbarch, sym, depth + 1, outfile); + } + CATCH (ex, RETURN_MASK_ERROR) + { + exception_fprintf (gdb_stderr, ex, + "Error printing symbol:\n"); + } + END_CATCH } } fprintf_filtered (outfile, "\n"); @@ -515,10 +511,9 @@ maintenance_print_symbols (const char *args, int from_tty) } } -/* Print symbol SYMBOL on OUTFILE. DEPTH says how far to indent. - Returns 0 for error, 1 for success. */ +/* Print symbol SYMBOL on OUTFILE. DEPTH says how far to indent. */ -static int +static void print_symbol (struct gdbarch *gdbarch, struct symbol *symbol, int depth, struct ui_file *outfile) { @@ -541,7 +536,6 @@ print_symbol (struct gdbarch *gdbarch, struct symbol *symbol, section->the_bfd_section)); else fprintf_filtered (outfile, "\n"); - return 1; } if (SYMBOL_DOMAIN (symbol) == STRUCT_DOMAIN) { @@ -689,7 +683,6 @@ print_symbol (struct gdbarch *gdbarch, struct symbol *symbol, } } fprintf_filtered (outfile, "\n"); - return 1; } static void