[PATCH 0/8] Error handling cleanups

[PATCH 0/8] Error handling cleanups

[Gary Benson]

Hi all,

GDB's various error handling functions do not work until a certain
amount of setup has been performed.  For regular warnings and errors
this is an annoyance: error handling needs to be hardwired with
fprintf and/or exit until error and warning may be called.  For
internal warnings and errors this is a real problem: internal errors
can occur from almost the first line of captured_main, but they don't
actually work until much later.

This series modifies internal_vproblem to always work, and modifies
warning and error to work much earlier.  It then replaces all the
hardwired early warning/error code with standard calls to warning and
error.

While looking at this I found several other hardwired error handlers
that do not seem to be necessary.  I've replaced those in separate
patches.

While switching to warning/error I've fixed some inconsistencies that
have crept in over time: some messages wern't wrapped with _(), for
example, and some but not all warning messages were prefixed with
argv[0].  For the latter case I've made it so that all warnings
emitted prior to and during option processing--before print_gdb_version
basically--are prefixed by argv[0] and all other warnings are not.

Built and regtested on RHEL6.5 x86_64.

Ok to commit?

Thanks,
Gary

[PATCH 3/8] Make warning usable earlier

[Gary Benson]

warning will crash if called before the first call to set_width.  This
commit makes the warning usable from the moment gdb_stderr is set up.

gdb/
2014-08-05  Gary Benson  <gbenson@redhat.com>

	* utils.c (vwarning): Protect calls to target_terminal_ours
	and wrap_here.
---
 gdb/ChangeLog |    5 +++++
 gdb/utils.c   |    6 ++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/gdb/utils.c b/gdb/utils.c
index fce5baa..5a1269c 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -522,8 +522,10 @@ vwarning (const char *string, va_list args)
     (*deprecated_warning_hook) (string, args);
   else
     {
-      target_terminal_ours ();
-      wrap_here ("");		/* Force out any buffered output.  */
+      if (target_supports_terminal_ours ())
+	target_terminal_ours ();
+      if (filtered_printing_initialized ())
+	wrap_here ("");		/* Force out any buffered output.  */
       gdb_flush (gdb_stdout);
       if (warning_pre_print)
 	fputs_unfiltered (warning_pre_print, gdb_stderr);
-- 
1.7.1

[PATCH 4/8] Replace hardwired error handlers in tui_initialize_io

[Gary Benson]

tui_initialize_io contains a pair of hardwired fprintf/exit error
handlers.  I was unable to find any documentation as to why they're
hardwired (the code appeared in a monolithic block back in 2001:
https://sourceware.org/ml/gdb-patches/2001-07/msg00490.html) and I
was also unable to come up with a situation where error would not
be suitable, so I have replaced both handlers with calls to error.

gdb/
2014-08-05  Gary Benson  <gbenson@redhat.com>

	* tui/tui-io.c (tui_initialize_io): Replace two fprintf/exit
	pairs with calls to error.  Wrap the message with _().
---
 gdb/ChangeLog    |    5 +++++
 gdb/tui/tui-io.c |   12 ++++--------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index 75eb4b8..11b2366 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -617,16 +617,12 @@ tui_initialize_io (void)
      readline output in a pipe, read that pipe and output the content
      in the curses command window.  */
   if (gdb_pipe_cloexec (tui_readline_pipe) != 0)
-    {
-      fprintf_unfiltered (gdb_stderr, "Cannot create pipe for readline");
-      exit (1);
-    }
+    error (_("Cannot create pipe for readline"));
+
   tui_rl_outstream = fdopen (tui_readline_pipe[1], "w");
   if (tui_rl_outstream == 0)
-    {
-      fprintf_unfiltered (gdb_stderr, "Cannot redirect readline output");
-      exit (1);
-    }
+    error (_("Cannot redirect readline output"));
+
   setvbuf (tui_rl_outstream, (char*) NULL, _IOLBF, 0);
 
 #ifdef O_NONBLOCK
-- 
1.7.1

[PATCH 8/8] Unify startup and option-parsing warnings

[Gary Benson]

Various warnings are emitted during startup and option-parsing using
fprintf_unfiltered.  One warning is prefixed with the command name,
the others are not.  This commit replaces these hardwired warnings
with calls to warning.  It also sets warning_pre_print to prefix all
warnings with the command name until option parsing is complete.

gdb/
2014-08-06  Gary Benson  <gbenson@redhat.com>

	* main.c (captured_main): Use warning during startup.
	Prefix startup warning messages with command name.
---
 gdb/ChangeLog |    5 +++++
 gdb/main.c    |   31 ++++++++++---------------------
 2 files changed, 15 insertions(+), 21 deletions(-)

diff --git a/gdb/main.c b/gdb/main.c
index a850edf..a15b3fd 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -519,14 +519,12 @@ captured_main (void *data)
   gdb_program_name = xstrdup (argv[0]);
 #endif
 
+  /* Prefix warning messages with the command name.  */
+  warning_pre_print = xstrprintf ("%s: warning: ", gdb_program_name);
+
   if (! getcwd (gdb_dirbuf, sizeof (gdb_dirbuf)))
-    /* Don't use *_filtered or warning() (which relies on
-       current_target) until after initialize_all_files().  */
-    fprintf_unfiltered (gdb_stderr,
-			_("%s: warning: error finding "
-			  "working directory: %s\n"),
-                        argv[0], safe_strerror (errno));
-    
+    perror_warning_with_name (_("error finding working directory"));
+
   current_directory = gdb_dirbuf;
 
   /* Set the sysroot path.  */
@@ -809,13 +807,8 @@ captured_main (void *data)
 
 	      i = strtol (optarg, &p, 0);
 	      if (i == 0 && p == optarg)
-
-		/* Don't use *_filtered or warning() (which relies on
-		   current_target) until after initialize_all_files().  */
-
-		fprintf_unfiltered
-		  (gdb_stderr,
-		   _("warning: could not set baud rate to `%s'.\n"), optarg);
+		warning (_("could not set baud rate to `%s'."),
+			 optarg);
 	      else
 		baud_rate = i;
 	    }
@@ -827,13 +820,8 @@ captured_main (void *data)
 
 	      i = strtol (optarg, &p, 0);
 	      if (i == 0 && p == optarg)
-
-		/* Don't use *_filtered or warning() (which relies on
-		   current_target) until after initialize_all_files().  */
-
-		fprintf_unfiltered (gdb_stderr,
-				    _("warning: could not set "
-				      "timeout limit to `%s'.\n"), optarg);
+		warning (_("could not set timeout limit to `%s'."),
+			 optarg);
 	      else
 		remote_timeout = i;
 	    }
@@ -987,6 +975,7 @@ captured_main (void *data)
     }
 
   /* Set off error and warning messages with a blank line.  */
+  xfree (warning_pre_print);
   warning_pre_print = _("\nwarning: ");
 
   /* Read and execute the system-wide gdbinit file, if it exists.
-- 
1.7.1

[PATCH 2/8] Make error usable earlier

[Gary Benson]

error (and other exception-throwing functions) are callable from the
first line of captured_main, but the exception printing code will
crash if called before the first call to set_width.  This commit makes
the exception printing code usable from the moment gdb_stderr is set
up.

gdb/
2014-08-05  Gary Benson  <gbenson@redhat.com>

	* exceptions.c (print_flush): Protect calls to
	target_terminal_ours and wrap_here.
---
 gdb/ChangeLog    |    5 +++++
 gdb/exceptions.c |    7 +++++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/gdb/exceptions.c b/gdb/exceptions.c
index ddaf250..377d073 100644
--- a/gdb/exceptions.c
+++ b/gdb/exceptions.c
@@ -241,7 +241,9 @@ print_flush (void)
 
   if (deprecated_error_begin_hook)
     deprecated_error_begin_hook ();
-  target_terminal_ours ();
+
+  if (target_supports_terminal_ours ())
+    target_terminal_ours ();
 
   /* We want all output to appear now, before we print the error.  We
      have 3 levels of buffering we have to flush (it's possible that
@@ -249,7 +251,8 @@ print_flush (void)
      too):  */
 
   /* 1.  The _filtered buffer.  */
-  wrap_here ("");
+  if (filtered_printing_initialized ())
+    wrap_here ("");
 
   /* 2.  The stdio buffer.  */
   gdb_flush (gdb_stdout);
-- 
1.7.1

[PATCH 7/8] Replace all usage errors with calls to error

[Gary Benson]

This commit replaces the hardwired fprintf/exit error handlers
for usage errors with calls to error.

gdb/
2014-08-06  Gary Benson  <gbenson@redhat.com>

	* main.c (captured_main): Handle usage errors with error.
---
 gdb/ChangeLog |    4 ++++
 gdb/main.c    |   40 ++++++++++------------------------------
 2 files changed, 14 insertions(+), 30 deletions(-)

diff --git a/gdb/main.c b/gdb/main.c
index 2f99157..a850edf 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -689,10 +689,7 @@ captured_main (void *data)
 	    xfree (interpreter_p);
 	    interpreter_p = xstrdup (INTERP_TUI);
 #else
-	    fprintf_unfiltered (gdb_stderr,
-				_("%s: TUI mode is not supported\n"),
-				argv[0]);
-	    exit (1);
+	    error (_("%s: TUI mode is not supported"), gdb_program_name);
 #endif
 	    break;
 	  case OPT_WINDOWS:
@@ -758,13 +755,8 @@ captured_main (void *data)
 	    break;
 	  case 'D':
 	    if (optarg[0] == '\0')
-	      {
-		fprintf_unfiltered (gdb_stderr,
-				    _("%s: empty path for"
-				      " `--data-directory'\n"),
-				    argv[0]);
-		exit (1);
-	      }
+	      error (_("%s: empty path for `--data-directory'"),
+		     gdb_program_name);
 	    set_gdb_data_directory (optarg);
 	    gdb_datadir_provided = 1;
 	    break;
@@ -774,13 +766,8 @@ captured_main (void *data)
 	      extern int gdbtk_test (char *);
 
 	      if (!gdbtk_test (optarg))
-		{
-		  fprintf_unfiltered (gdb_stderr,
-				      _("%s: unable to load "
-					"tclcommand file \"%s\""),
-				      argv[0], optarg);
-		  exit (1);
-		}
+		error (_("%s: unable to load tclcommand file \"%s\""),
+		       gdb_program_name, optarg);
 	      break;
 	    }
 	  case 'y':
@@ -853,11 +840,8 @@ captured_main (void *data)
 	    break;
 
 	  case '?':
-	    fprintf_unfiltered (gdb_stderr,
-				_("Use `%s --help' for a "
-				  "complete list of options.\n"),
-				argv[0]);
-	    exit (1);
+	    error (_("Use `%s --help' for a complete list of options."),
+		   gdb_program_name);
 	  }
       }
 
@@ -880,13 +864,9 @@ captured_main (void *data)
 	 inferior.  The first one is the sym/exec file, and the rest
 	 are arguments.  */
       if (optind >= argc)
-	{
-	  fprintf_unfiltered (gdb_stderr,
-			      _("%s: `--args' specified but "
-				"no program specified\n"),
-			      argv[0]);
-	  exit (1);
-	}
+	error (_("%s: `--args' specified but no program specified"),
+	       gdb_program_name);
+
       symarg = argv[optind];
       execarg = argv[optind];
       ++optind;
-- 
1.7.1

[PATCH 5/8] Replace hardwired error handler in captured_main

[Gary Benson]

If the requested interpreter cannot be set captured_main reports the
error with a hardwired fprintf/exit pair.  A fprintf/exit pair on the
previous line was replaced with a call to error in March 2003
(https://sourceware.org/ml/gdb-patches/2003-03/msg00444.html) but I
found no documentation as to why this particular hardwired handler
was left untouched.  I was also unable to come up with a situation
where error would not be suitable, so I have replaced it with a call
to error.

gdb/
2014-08-05  Gary Benson  <gbenson@redhat.com>

	* main.c (captured_main): Replace a fprintf/exit
	pair with a call to error.  Wrap the message with _().
---
 gdb/ChangeLog |    5 +++++
 gdb/main.c    |    7 +------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gdb/main.c b/gdb/main.c
index 06b3c90..2f99157 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -986,12 +986,7 @@ captured_main (void *data)
       error (_("Interpreter `%s' unrecognized"), interpreter_p);
     /* Install it.  */
     if (!interp_set (interp, 1))
-      {
-        fprintf_unfiltered (gdb_stderr,
-			    "Interpreter `%s' failed to initialize.\n",
-                            interpreter_p);
-        exit (1);
-      }
+      error (_("Interpreter `%s' failed to initialize."), interpreter_p);
   }
 
   /* FIXME: cagney/2003-02-03: The big hack (part 2 of 2) that lets
-- 
1.7.1

[PATCH 1/8] Make internal_vproblem always work

[Gary Benson]

internal_vproblem can be called (via malloc_failure) from almost the
first line of captured_main, but it will crash if called before the
first call to set_width.  This commit makes internal_vproblem work
at any time.

There are two parts to this.  If called before gdb_stderr is set up,
internal_vproblem will fall back to printing the message on regular
stderr and aborting.  If called after gdb_stderr is set up but before
filtered printing is set up, internal_vproblem will operate as usual
except that it can not query whether to quit and/or dump core so it
defaults to doing both.

gdb/
2014-08-05  Gary Benson  <gbenson@redhat.com>

	* utils.h (filtered_printing_initialized): New declaration.
	* utils.c (abort_with_message): New function.
	(internal_vproblem): Use abort_with_message for first level
	recursive internal problems, and if gdb_stderr is not set up.
	Protect calls to target_terminal_ours, begin_line and query.
---
 gdb/ChangeLog |    8 ++++++++
 gdb/utils.c   |   48 ++++++++++++++++++++++++++++++++++++++++--------
 gdb/utils.h   |    3 +++
 3 files changed, 51 insertions(+), 8 deletions(-)

diff --git a/gdb/utils.c b/gdb/utils.c
index 3f753bb..fce5baa 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -577,6 +577,19 @@ error_stream (struct ui_file *stream)
   error (("%s"), message);
 }
 
+/* Emit a message and abort.  */
+
+static void ATTRIBUTE_NORETURN
+abort_with_message (const char *msg)
+{
+  if (gdb_stderr == NULL)
+    fputs (msg, stderr);
+  else
+    fputs_unfiltered (msg, gdb_stderr);
+
+  abort ();		/* NOTE: GDB has only three calls to abort().  */
+}
+
 /* Dump core trying to increase the core soft limit to hard limit first.  */
 
 void
@@ -699,8 +712,7 @@ internal_vproblem (struct internal_problem *problem,
 	break;
       case 1:
 	dejavu = 2;
-	fputs_unfiltered (msg, gdb_stderr);
-	abort ();	/* NOTE: GDB has only three calls to abort().  */
+	abort_with_message (msg);
       default:
 	dejavu = 3;
         /* Newer GLIBC versions put the warn_unused_result attribute
@@ -714,10 +726,6 @@ internal_vproblem (struct internal_problem *problem,
       }
   }
 
-  /* Try to get the message out and at the start of a new line.  */
-  target_terminal_ours ();
-  begin_line ();
-
   /* Create a string containing the full error/warning message.  Need
      to call query with this full string, as otherwize the reason
      (error/warning) and question become separated.  Format using a
@@ -735,8 +743,23 @@ internal_vproblem (struct internal_problem *problem,
     make_cleanup (xfree, reason);
   }
 
+  /* Fall back to abort_with_message if gdb_stderr is not set up.  */
+  if (gdb_stderr == NULL)
+    {
+      fputs (reason, stderr);
+      abort_with_message ("\n");
+    }
+
+  /* Try to get the message out and at the start of a new line.  */
+  if (target_supports_terminal_ours ())
+    target_terminal_ours ();
+  if (filtered_printing_initialized ())
+    begin_line ();
+
   /* Emit the message unless query will emit it below.  */
-  if (problem->should_quit != internal_problem_ask || !confirm)
+  if (problem->should_quit != internal_problem_ask
+      || !confirm
+      || !filtered_printing_initialized ())
     fprintf_unfiltered (gdb_stderr, "%s\n", reason);
 
   if (problem->should_quit == internal_problem_ask)
@@ -744,7 +767,7 @@ internal_vproblem (struct internal_problem *problem,
       /* Default (yes/batch case) is to quit GDB.  When in batch mode
 	 this lessens the likelihood of GDB going into an infinite
 	 loop.  */
-      if (!confirm)
+      if (!confirm || !filtered_printing_initialized ())
 	quit_p = 1;
       else
         quit_p = query (_("%s\nQuit this debugging session? "), reason);
@@ -766,6 +789,8 @@ internal_vproblem (struct internal_problem *problem,
     {
       if (!can_dump_core_warn (LIMIT_MAX, reason))
 	dump_core_p = 0;
+      else if (!filtered_printing_initialized ())
+	dump_core_p = 1;
       else
 	{
 	  /* Default (yes/batch case) is to dump core.  This leaves a GDB
@@ -1738,6 +1763,13 @@ init_page_info (void)
   set_width ();
 }
 
+/* Return nonzero if filtered printing is initialized.  */
+int
+filtered_printing_initialized (void)
+{
+  return wrap_buffer != NULL;
+}
+
 /* Helper for make_cleanup_restore_page_info.  */
 
 static void
diff --git a/gdb/utils.h b/gdb/utils.h
index cc79562..0fddecb 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -245,6 +245,9 @@ extern void fputstrn_filtered (const char *str, int n, int quotr,
 extern void fputstrn_unfiltered (const char *str, int n, int quotr,
 				 struct ui_file * stream);
 
+/* Return nonzero if filtered printing is initialized.  */
+extern int filtered_printing_initialized (void);
+
 /* Display the host ADDR on STREAM formatted as ``0x%x''.  */
 extern void gdb_print_host_address (const void *addr, struct ui_file *stream);
 
-- 
1.7.1

[PATCH 6/8] Replace hardwired error handler in go32_create_inferior

[Gary Benson]

go32_create_inferior invokes a hardwired fprintf/exit error handler
if v2loadimage fails.  I could find no reason for this other than that
the block seems to have been copy-and-pasted from v2loadimage's
manpage.  This commit replaces the hardwired handler with a call to
error.

gdb/
2014-08-05  Gary Benson  <gbenson@redhat.com>

	* go32-nat.c (go32_create_inferior): Replace a fprintf/
	exit pair with a call to error.  Wrap the message with _().
---
 gdb/ChangeLog  |    5 +++++
 gdb/go32-nat.c |   12 ++++++------
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/gdb/go32-nat.c b/gdb/go32-nat.c
index b2570e8..eb3cde9 100644
--- a/gdb/go32-nat.c
+++ b/gdb/go32-nat.c
@@ -642,6 +642,7 @@ go32_create_inferior (struct target_ops *ops, char *exec_file,
   char **env_save = environ;
   size_t cmdlen;
   struct inferior *inf;
+  int result;
 
   /* If no exec file handed to us, get it from the exec-file command -- with
      a good, common error message if none is specified.  */
@@ -693,15 +694,14 @@ go32_create_inferior (struct target_ops *ops, char *exec_file,
 
   environ = env;
 
-  if (v2loadimage (exec_file, cmdline, start_state))
-    {
-      environ = env_save;
-      printf_unfiltered ("Load failed for image %s\n", exec_file);
-      exit (1);
-    }
+  result = v2loadimage (exec_file, cmdline, start_state);
+
   environ = env_save;
   xfree (cmdline);
 
+  if (!result)
+    error (_("Load failed for image %s", exec_file);
+
   edi_init (start_state);
 #if __DJGPP_MINOR__ < 3
   save_npx ();
-- 
1.7.1

Re: [PATCH 6/8] Replace hardwired error handler in go32_create_inferior

[Eli Zaretskii]

> From: Gary Benson <gbenson@redhat.com>
> Date: Wed,  6 Aug 2014 11:12:26 +0100
> 
> go32_create_inferior invokes a hardwired fprintf/exit error handler
> if v2loadimage fails.  I could find no reason for this other than that
> the block seems to have been copy-and-pasted from v2loadimage's
> manpage.

AFAIR, it's actually the other way around: the example in the
documentation was copy-pasted from GDB, bit never mind.

> This commit replaces the hardwired handler with a call to error.

Thanks, but...

> -  if (v2loadimage (exec_file, cmdline, start_state))
> -    {
> -      environ = env_save;
> -      printf_unfiltered ("Load failed for image %s\n", exec_file);
> -      exit (1);
> -    }
> +  result = v2loadimage (exec_file, cmdline, start_state);
> +
>    environ = env_save;
>    xfree (cmdline);
>  
> +  if (!result)
> +    error (_("Load failed for image %s", exec_file);

...the last test is inverted: v2loadimage returns zero if it succeeds,
not if it fails (see also the old code).

OK with that change.

Re: [PATCH 6/8] Replace hardwired error handler in go32_create_inferior

[Gary Benson]

Eli Zaretskii wrote:
> > From: Gary Benson <gbenson@redhat.com>
> > Date: Wed,  6 Aug 2014 11:12:26 +0100
> > 
> > go32_create_inferior invokes a hardwired fprintf/exit error
> > handler if v2loadimage fails.  I could find no reason for this
> > other than that the block seems to have been copy-and-pasted
> > from v2loadimage's manpage.
> 
> AFAIR, it's actually the other way around: the example in the
> documentation was copy-pasted from GDB, bit never mind.
> 
> > This commit replaces the hardwired handler with a call to error.
> 
> Thanks, but...
> 
> > -  if (v2loadimage (exec_file, cmdline, start_state))
> > -    {
> > -      environ = env_save;
> > -      printf_unfiltered ("Load failed for image %s\n", exec_file);
> > -      exit (1);
> > -    }
> > +  result = v2loadimage (exec_file, cmdline, start_state);
> > +
> >    environ = env_save;
> >    xfree (cmdline);
> >  
> > +  if (!result)
> > +    error (_("Load failed for image %s", exec_file);
> 
> ...the last test is inverted: v2loadimage returns zero if it
> succeeds, not if it fails (see also the old code).
> 
> OK with that change.

Thanks Eli.  I've updated my tree to "if (result != 0) error".
I won't mail an updated patch unless anyone wants one.

Cheers,
Gary

-- 
http://gbenson.net/

Re: [PATCH 0/8] Error handling cleanups

[Pedro Alves]

Hi Gary,

This all looks good to me.

Thanks!

Pedro Alves

[COMMITTED PATCH 0/8] Error handling cleanups

[Gary Benson]

Pedro Alves wrote:
> This all looks good to me.

Thanks for reviewing this, I've pushed it.

Cheers,
Gary

-- 
http://gbenson.net/