This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFA] Change exceptions.h functions to use gdb::function_view


On 09/27/2017 09:25 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> 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 <palves@redhat.com>
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<catch_errors_ftype> 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<catch_errors_ftype>,
-			 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
-- 
2.5.5



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]