diff mbox

[01/15,v3] Introduce common/errors.h

Message ID 20140717134728.GB31916@blade.nx
State Changes Requested, archived
Headers show

Commit Message

Gary Benson July 17, 2014, 1:47 p.m. UTC
Doug Evans wrote:
> fatal is a confusing name IMO.  In gdb it just throws RETURN_QUIT.
> That's not your fault of course, but I wonder if I can trouble you
> to expand on the function comment a bit.  E.g., something like the
> following?
> 
> /* Cause a fatal error.
>    What this does in gdb is throw a RETURN_QUIT exception.  */
> 
> One could expand on the comment, but I didn't want to turn it into
> a FIXME.  [which is really what it should be, but that's a topic
> for another patch :-)]

I have rewritten the documentation on all these functions.  I don't
want to mention details of GDB's and/or gdbserver's implementation
in this file, so what I have done is described what callers should
expect the functions to do.  I hope this is clear enough.

> It's odd to separate out fatal and not vfatal.
> [ditto for the others]
> Can you add vfatal, et.al. to this patch?

I've done that now.  I made the non-v-prefixed functions part of
the common codebase; users of the common codebase are required to
implement the v-prefixed functions only.

Thanks,
Gary

--
gdb/
2014-07-16  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* common/errors.h: New file.
	* common/errors.c: Likewise.
	* Makefile.in (HFILES_NO_SRCDIR) [common/errors.h]: New header.
	(COMMON_OBS) [errors.o]: New object file.
	(errors.o): New rule.
	* utils.h: Include errors.h.
	(perror_with_name, error, verror, fatal, vfatal, warning
	vwarning): Don't declare.
	* common/common-utils.h: Include errors.h.
	(malloc_failure, internal_error): Don't declare.

gdb/gdbserver/
2014-07-16  Tom Tromey  <tromey@redhat.com>
	    Gary Benson  <gbenson@redhat.com>

	* Makefile.in (SFILES) [common/errors.c]: New sourcefile.
	(OBS) [errors.o]: New object file.
	(IPA_OBS) [errors-ipa.o]: Likewise.
	(errors.o): New rule.
	(errors-ipa.o): Likewise.
	* utils.h: Include errors.h.
	(perror_with_name, error, fatal, warning): Don't declare.
	* utils.c (warning): Renamed and rewritten as...
	(vwarning): New function.
	(error): Renamed and rewritten as...
	(verror): New function.
	(fatal): Renamed and rewritten as...
	(vfatal): New function.
	(internal_error): Renamed and rewritten as...
	(internal_verror): New function.
---
 gdb/ChangeLog             |   14 ++++++++
 gdb/Makefile.in           |    8 +++-
 gdb/common/common-utils.h |    5 +--
 gdb/common/errors.c       |   71 +++++++++++++++++++++++++++++++++++++++
 gdb/common/errors.h       |   81 +++++++++++++++++++++++++++++++++++++++++++++
 gdb/gdbserver/ChangeLog   |   19 ++++++++++
 gdb/gdbserver/Makefile.in |   16 +++++++--
 gdb/gdbserver/utils.c     |   35 +++++--------------
 gdb/gdbserver/utils.h     |    5 +--
 gdb/utils.c               |   46 -------------------------
 gdb/utils.h               |   22 +------------
 11 files changed, 217 insertions(+), 105 deletions(-)
 create mode 100644 gdb/common/errors.c
 create mode 100644 gdb/common/errors.h

Comments

Pedro Alves July 17, 2014, 3:07 p.m. UTC | #1
On 07/17/2014 02:47 PM, Gary Benson wrote:
> +/* Throw an error.  The current operation will be aborted.  The
> +   message will be issued to the user.  The application will
> +   return to a state where it is accepting commands from the user.  */

These comments aren't really true, though.  We have plenty of places
catching errors with TRY_CATCH and proceeding without returning to a
state where we're accepting commands from the user.

> +
> +extern void error (const char *fmt, ...)
> +     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
> +

(We should really bite the bullet and move exceptions.{h|c} and
cleanups.{h|c} to common/ and make gdbserver use them too.)
Gary Benson July 17, 2014, 3:39 p.m. UTC | #2
Pedro Alves wrote:
> On 07/17/2014 02:47 PM, Gary Benson wrote:
> > +/* Throw an error.  The current operation will be aborted.  The
> > +   message will be issued to the user.  The application will
> > +   return to a state where it is accepting commands from the user.  */
> 
> These comments aren't really true, though.  We have plenty of places
> catching errors with TRY_CATCH and proceeding without returning to a
> state where we're accepting commands from the user.

How about "Throw an error.  The current operation will be aborted.
The application may catch and process the error, or, if not, the
message will be issued to the user and the application will return
to a state where it is accepting commands from the user."

> (We should really bite the bullet and move exceptions.{h|c} and
> cleanups.{h|c} to common/ and make gdbserver use them too.)

Can I leave this for another series please?  This one's big enough
already.

Thanks,
Gary
Pedro Alves July 17, 2014, 4:12 p.m. UTC | #3
On 07/17/2014 04:39 PM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 07/17/2014 02:47 PM, Gary Benson wrote:
>>> +/* Throw an error.  The current operation will be aborted.  The
>>> +   message will be issued to the user.  The application will
>>> +   return to a state where it is accepting commands from the user.  */
>>
>> These comments aren't really true, though.  We have plenty of places
>> catching errors with TRY_CATCH and proceeding without returning to a
>> state where we're accepting commands from the user.
> 
> How about "Throw an error.  The current operation will be aborted.
> The application may catch and process the error, or, if not, the
> message will be issued to the user and the application will return
> to a state where it is accepting commands from the user."

Still infers what the application does with the error.  IMO, it's
best to describe the interface.  Like:

 Throw a generic error.  This function not return, instead
 it executes a long jump, aborting the current operation.
 The function takes printf-style arguments, which are used to
 construct the error message.

>> (We should really bite the bullet and move exceptions.{h|c} and
>> cleanups.{h|c} to common/ and make gdbserver use them too.)
> 
> Can I leave this for another series please?  This one's big enough
> already.

Yes, of course.  That's why it was a parenthesis.
Gary Benson July 18, 2014, 9:06 a.m. UTC | #4
Pedro Alves wrote:
> On 07/17/2014 04:39 PM, Gary Benson wrote:
> > Pedro Alves wrote:
> > > On 07/17/2014 02:47 PM, Gary Benson wrote:
> > > > +/* Throw an error.  The current operation will be aborted.  The
> > > > +   message will be issued to the user.  The application will
> > > > +   return to a state where it is accepting commands from the user.  */
> > >
> > > These comments aren't really true, though.  We have plenty of
> > > places catching errors with TRY_CATCH and proceeding without
> > > returning to a state where we're accepting commands from the
> > > user.
> > 
> > How about "Throw an error.  The current operation will be aborted.
> > The application may catch and process the error, or, if not, the
> > message will be issued to the user and the application will return
> > to a state where it is accepting commands from the user."
> 
> Still infers what the application does with the error.  IMO, it's
> best to describe the interface.  Like:
> 
>  Throw a generic error.  This function not return, instead
>  it executes a long jump, aborting the current operation.
>  The function takes printf-style arguments, which are used to
>  construct the error message.

Surely "it executes a long jump" is describing what the application
does with the error?  Besides, it's not true for IPA, where error
just calls exit (1).

In the context of this whole file, you have:

 - warning (one type)
 - errors (three types)
 - special cases (perror_with_name, malloc_failure)

The only difference between the three types of error is what the
application does with them:

 - error (may be handled, may return to command level, may exit)
 - fatal (will exit)
 - internal_error (may return to command level, may exit)

I don't think you can properly document these functions without
spelling this out.

Also, the documentation in this file is not just for callers of the
functions, it has to document them for implementors too.

How about the following?

Cheers,
Gary

--
/* Display a warning message to the user.  The message is constructed
   using a printf-style format string and a printf- or vprintf-style
   argument list.  */

extern void warning (const char *fmt, ...) ATTRIBUTE_PRINTF (1, 2);

extern void vwarning (const char *fmt, va_list args)
     ATTRIBUTE_PRINTF (1, 0);

/* Throw a non-fatal error, constructing the message using a printf-
   style format string and a printf- or vprintf-style argument list.
   This function does not return.  The application may attempt to
   handle the error.  */

extern void error (const char *fmt, ...)
     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);

extern void verror (const char *fmt, va_list args)
     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);

/* Throw a non-fatal error, constructing the message by combining
   STRING with the system error message for errno.  This function does
   not return.  The application may attempt to handle the error.  */

extern void perror_with_name (const char *string) ATTRIBUTE_NORETURN;

/* Throw a fatal error, constructing the message using a printf-style
   format string and a printf- or vprintf-style argument list.  This
   function does not return.  The application will exit.  */

extern void fatal (const char *fmt, ...)
     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);

extern void vfatal (const char *fmt, va_list args)
     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);

/* Throw an internal error, constructing the message using a printf-
   style format string and a printf- or vprintf-style argument list.
   This function does not return.  Internal errors indicate
   programming errors such as assertion failures, as opposed to more
   general errors the user may encounter.  Internal errors are treated
   as fatal errors, except that the application may offer the user the
   choice to return to the command level rather than exiting.  */

extern void internal_error (const char *file, int line,
			    const char *fmt, ...)
     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 4);

extern void internal_verror (const char *file, int line,
			     const char *fmt, va_list args)
     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0);

/* A special case of internal error used to handle memory allocation
   failures.  */

extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
Doug Evans July 18, 2014, 9:20 a.m. UTC | #5
On Fri, Jul 18, 2014 at 10:06 AM, Gary Benson <gbenson@redhat.com> wrote:
> [...]
> /* Throw a fatal error, constructing the message using a printf-style
>    format string and a printf- or vprintf-style argument list.  This
>    function does not return.  The application will exit.  */
>
> extern void fatal (const char *fmt, ...)
>      ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
>
> extern void vfatal (const char *fmt, va_list args)
>      ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
> [...]

Remember gdb doesn't exit on fatal().
fatal() in gdb is essentially ^c (quit() calls fatal()!).

Can I repeat my request to please document this in the function comment.
Pedro Alves July 18, 2014, 10:41 a.m. UTC | #6
On 07/18/2014 10:06 AM, Gary Benson wrote:
> Pedro Alves wrote:
>> On 07/17/2014 04:39 PM, Gary Benson wrote:
>>> Pedro Alves wrote:
>>>> On 07/17/2014 02:47 PM, Gary Benson wrote:
>>>>> +/* Throw an error.  The current operation will be aborted.  The
>>>>> +   message will be issued to the user.  The application will
>>>>> +   return to a state where it is accepting commands from the user.  */
>>>>
>>>> These comments aren't really true, though.  We have plenty of
>>>> places catching errors with TRY_CATCH and proceeding without
>>>> returning to a state where we're accepting commands from the
>>>> user.
>>>
>>> How about "Throw an error.  The current operation will be aborted.
>>> The application may catch and process the error, or, if not, the
>>> message will be issued to the user and the application will return
>>> to a state where it is accepting commands from the user."
>>
>> Still infers what the application does with the error.  IMO, it's
>> best to describe the interface.  Like:
>>
>>  Throw a generic error.  This function not return, instead
>>  it executes a long jump, aborting the current operation.
>>  The function takes printf-style arguments, which are used to
>>  construct the error message.
> 
> Surely "it executes a long jump" is describing what the application
> does with the error?

By "does with the error" I was talking about what the application
does after the error is thrown, like talking about accepting
commands from the user, not really the internal implementation.

> Besides, it's not true for IPA, where error
> just calls exit (1).

Yeah, forgot that, though that's conceptually the same as the IPA having
an outer TRY_CATCH and calling exit(1) there.

OK, thinking this through.

Conceptually, what matters to the user of the API is when to call which error
function variant, and this one is to be called on generic errors.  That
the IPA chooses the treat generic errors as fatal errors, it's IPA's
business.  That's the detail of what the application decides to do
with the error.

> 
> In the context of this whole file, you have:
> 
>  - warning (one type)
>  - errors (three types)
>  - special cases (perror_with_name, malloc_failure)
> 
> The only difference between the three types of error is what the
> application does with them:
> 
>  - error (may be handled, may return to command level, may exit)
>  - fatal (will exit)
>  - internal_error (may return to command level, may exit)

I think that's not the best way to look at this.  We're documenting
an API, the contract the shared/core code will rely on to decide which
function to call.  If the contract is different in GDB vs GDBserver
then we shall fix up the differences first.

So, what is the contract we want ?  Where do we want to get to?

I think it should be:

- warning (something odd detected, but we'll carry on anyway.)
- error (generic, non-predicatable, non-fatal condition detected.
         The requested operation can't proceed.  E.g.: trying to
         compute expressions with optimized values; invalid input;invalid
         conversion attempted; etc.)
- internal_error (faulty logic detected)

and then, I think we should just not use 'fatal' as is in common code,
and so not move that to common code.  It's just confusing and behaves
different in GDB and GDBserver.  We have a bunch of callers on the
GDBserver side, but can't those all be internal_error / gdb_assert?
Why not?  If not, then I propose really describing 'fatal' as
being a non-recoverable fatal error.

On the GDB side it's only used in two situations -- throw
a QUIT when the user wants to try recovering from an internal error,
and, throw a QUIT when the user presses ctrl-c.  It's not called
when a fatal / internal error is detected, like on the
GDBserver side.

I think we should rename the existing 'fatal' on the GDB side for
clarity to something like throw_quit or some such.  How GDB
recovers from internal_error is GDB's business.  It just happens
today, that results in a QUIT being thrown.

For situations in GDB where we might have a fatal error that is
truly not recoverable, and that should not even consult the user
on whether to quit/dump core, etc., then we wouldn't call the
existing 'fatal' function as it is.
E.g., if GDB's 'fatal' where truly a 'fatal'-like function like
GDBserver's, this in main.c:

    /* Install it.  */
    if (!interp_set (interp, 1))
      {
        fprintf_unfiltered (gdb_stderr,
			    "Interpreter `%s' failed to initialize.\n",
                            interpreter_p);
        exit (1);
      }

would be written as:

    /* Install it.  */
    if (!interp_set (interp, 1))
      {
        fatal ("Interpreter `%s' failed to initialize.\n",
               interpreter_p);
      }

with 'fatal' printing the error and exiting, just like gdbserver.

I'd support doing that.

> I don't think you can properly document these functions without
> spelling this out.
> 
> Also, the documentation in this file is not just for callers of the
> functions, it has to document them for implementors too.

The way to do that is to document the contract callers use to
determine the class of error at hand, not what the application
does with such class of error.  If we're thinking in terms of library,
then listing what current applications do just results in comments
getting stale as implementations change or more applications are added.

> /* Throw an internal error, constructing the message using a printf-
>    style format string and a printf- or vprintf-style argument list.
>    This function does not return.  Internal errors indicate
>    programming errors such as assertion failures, as opposed to more
>    general errors the user may encounter.  Internal errors are treated
>    as fatal errors, except that the application may offer the user the
>    choice to return to the command level rather than exiting.  */

Here we're talking about "the command level", which doesn't
even exist on GDBserver or the IPA.  And I'm now thinking that
saying "Throw" here isn't exactly the best to say.  Instead,
I think it'd be better to say something like "Call when an internal
error was detected.", and don't even talk about the command level.
Like:

 /* An internal error was detected.  Internal errors indicate
    programming errors such as assertion failures, as opposed to more
    general errors beyond the application's control.
    This function does not return, and constructs an error message
    using a printf-style argument list.  FILE and LINE indicate the
    file and line number where the programming error was detected.

Thanks,
Gary Benson July 18, 2014, 10:44 a.m. UTC | #7
Doug Evans wrote:
> On Fri, Jul 18, 2014 at 10:06 AM, Gary Benson <gbenson@redhat.com> wrote:
> > [...]
> > /* Throw a fatal error, constructing the message using a printf-style
> >    format string and a printf- or vprintf-style argument list.  This
> >    function does not return.  The application will exit.  */
> >
> > extern void fatal (const char *fmt, ...)
> >      ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
> >
> > extern void vfatal (const char *fmt, va_list args)
> >      ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
> > [...]
> 
> Remember gdb doesn't exit on fatal().
> fatal() in gdb is essentially ^c (quit() calls fatal()!).
> 
> Can I repeat my request to please document this in the function
> comment.

I wanted to avoid putting implementation details here, but I see this
isn't going to happen.  Is the attached ok?

Cheers,
Gary
Doug Evans July 18, 2014, 11:23 a.m. UTC | #8
On Fri, Jul 18, 2014 at 11:44 AM, Gary Benson <gbenson@redhat.com> wrote:
> Doug Evans wrote:
>> On Fri, Jul 18, 2014 at 10:06 AM, Gary Benson <gbenson@redhat.com> wrote:
>> > [...]
>> > /* Throw a fatal error, constructing the message using a printf-style
>> >    format string and a printf- or vprintf-style argument list.  This
>> >    function does not return.  The application will exit.  */
>> >
>> > extern void fatal (const char *fmt, ...)
>> >      ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
>> >
>> > extern void vfatal (const char *fmt, va_list args)
>> >      ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
>> > [...]
>>
>> Remember gdb doesn't exit on fatal().
>> fatal() in gdb is essentially ^c (quit() calls fatal()!).
>>
>> Can I repeat my request to please document this in the function
>> comment.
>
> I wanted to avoid putting implementation details here,

I realize that.

> but I see this
> isn't going to happen.  Is the attached ok?

I'm trying to not impose on you too much churn and back-and-forth in
what is clearly a stepping stone.
Eventually I think fatal needs to disappear from the gdb side (renamed
or whatever).
Until that happens (i.e., *if* you just want to keep the patch
basically as is), then at the least I don't want to lie to the reader,
and I want to make the reader aware of the issue.  That's the high
order bit for me as far as "fatal" goes (within the context of trying
to keep the patch basically as is).

If you want to take on the task of getting this 100% correct in this
pass (or at least within some fraction thereof :-)), then we can take
a different route.  Your choice IMO.  [And I mean that sincerely - I
*am* trying to help you make progress here without being too
pedantic.]

> /* Throw a fatal error, constructing the message using a printf-style
>    format string and a printf- or vprintf-style argument list.  This
>    function does not return.  Fatal errors cause GDB to return to the
>    command level.  Fatal errors cause gdbserver to exit.  */
>
> extern void fatal (const char *fmt, ...)
>      ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
>
> extern void vfatal (const char *fmt, va_list args)
>      ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);

I would tweak that a little, and instead just point out that gdb's
usage of the name "fatal" is broken.  I don't care too much about
the wording, I just want to make sure we don't lie to the reader
and make the reader aware of the issue.

/* Throw a fatal error, constructing the message using a printf-style
   format string and a printf- or vprintf-style argument list.  This
   function does not return.  Fatal errors cause the app to exit,
   except in the case of gdb where it just throws a RETURN_QUIT
   exception.  */
diff mbox

Patch

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index ce15501..074ea78 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -935,7 +935,7 @@  gdb_bfd.h sparc-ravenscar-thread.h ppc-ravenscar-thread.h nat/linux-btrace.h \
 ctf.h nat/i386-cpuid.h nat/i386-gcc-cpuid.h target/resume.h \
 target/wait.h target/waitstatus.h nat/linux-nat.h nat/linux-waitpid.h \
 common/print-utils.h common/rsp-low.h nat/i386-dregs.h x86-linux-nat.h \
-i386-linux-nat.h
+i386-linux-nat.h common/errors.h
 
 # Header files that already have srcdir in them, or which are in objdir.
 
@@ -1034,7 +1034,7 @@  COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 	gdb_vecs.o jit.o progspace.o skip.o probe.o \
 	common-utils.o buffer.o ptid.o gdb-dlfcn.o common-agent.o \
 	format.o registry.o btrace.o record-btrace.o waitstatus.o \
-	print-utils.o rsp-low.o
+	print-utils.o rsp-low.o errors.o
 
 TSOBS = inflow.o
 
@@ -2144,6 +2144,10 @@  rsp-low.o: ${srcdir}/common/rsp-low.c
 	$(COMPILE) $(srcdir)/common/rsp-low.c
 	$(POSTCOMPILE)
 
+errors.o: ${srcdir}/common/errors.c
+	$(COMPILE) $(srcdir)/common/errors.c
+	$(POSTCOMPILE)
+
 #
 # gdb/target/ dependencies
 #
diff --git a/gdb/common/common-utils.h b/gdb/common/common-utils.h
index 063698d..53be2f8 100644
--- a/gdb/common/common-utils.h
+++ b/gdb/common/common-utils.h
@@ -24,6 +24,7 @@ 
 #include "ansidecl.h"
 #include <stddef.h>
 #include <stdarg.h>
+#include "errors.h"
 
 /* If possible, define FUNCTION_NAME, a macro containing the name of
    the function being defined.  Since this macro may not always be
@@ -43,10 +44,6 @@ 
 #endif
 #endif
 
-extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
-extern void internal_error (const char *file, int line, const char *, ...)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 4);
-
 /* xmalloc(), xrealloc() and xcalloc() have already been declared in
    "libiberty.h". */
 
diff --git a/gdb/common/errors.c b/gdb/common/errors.c
new file mode 100644
index 0000000..78a6d3e
--- /dev/null
+++ b/gdb/common/errors.c
@@ -0,0 +1,71 @@ 
+/* Error reporting facilities.
+
+   Copyright (C) 1986-2014 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "config.h"
+#include "ansidecl.h"
+#include <stdarg.h>
+#include "errors.h"
+
+/* See common/errors.h.  */
+
+void
+warning (const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  vwarning (fmt, ap);
+  va_end (ap);
+}
+
+/* See common/errors.h.  */
+
+void
+error (const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  verror (fmt, ap);
+  va_end (ap);
+}
+
+/* See common/errors.h.  */
+
+void
+fatal (const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  vfatal (fmt, ap);
+  va_end (ap);
+}
+
+/* See common/errors.h.  */
+
+void
+internal_error (const char *file, int line, const char *fmt, ...)
+{
+  va_list ap;
+
+  va_start (ap, fmt);
+  internal_verror (file, line, fmt, ap);
+  va_end (ap);
+}
diff --git a/gdb/common/errors.h b/gdb/common/errors.h
new file mode 100644
index 0000000..4e8f4a7
--- /dev/null
+++ b/gdb/common/errors.h
@@ -0,0 +1,81 @@ 
+/* Declarations for error-reporting facilities.
+
+   Copyright (C) 1986-2014 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef COMMON_ERRORS_H
+#define COMMON_ERRORS_H
+
+/* The declarations in this file are, for the time being, separately
+   implemented by gdb and gdbserver.  However they share a common
+   definition so that they can be used by code in common/.  */
+
+/* Issue a warning to the user.  */
+
+extern void warning (const char *fmt, ...) ATTRIBUTE_PRINTF (1, 2);
+
+extern void vwarning (const char *fmt, va_list args)
+     ATTRIBUTE_PRINTF (1, 0);
+
+/* Throw an error.  The current operation will be aborted.  The
+   message will be issued to the user.  The application will
+   return to a state where it is accepting commands from the user.  */
+
+extern void error (const char *fmt, ...)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
+
+extern void verror (const char *fmt, va_list args)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
+
+/* Throw an error, creating the message by combining STRING with the
+   system error message for errno.  The current operation will be
+   aborted.  The message will be issued to the user.  The application
+   will return to a state where it is accepting commands from the
+   user.  */
+
+extern void perror_with_name (const char *string) ATTRIBUTE_NORETURN;
+
+/* Throw a fatal error.  The current operation will be aborted.  The
+   message will be issued to the user.  The application will exit.  */
+
+extern void fatal (const char *fmt, ...)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
+
+extern void vfatal (const char *fmt, va_list args)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
+
+/* Throw an internal error.  This should be used to indicate
+   programming errors such as assertion failures, as opposed
+   to more general errors the user may encounter.  The current
+   operation will be aborted.  The message will be issued to
+   the user.  The application may offer the user the choice to
+   return to the command level, or the application may exit.  */
+
+extern void internal_error (const char *file, int line,
+			    const char *fmt, ...)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 4);
+
+extern void internal_verror (const char *file, int line,
+			     const char *fmt, va_list args)
+     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0);
+
+/* Special case internal error used to handle memory allocation
+   failures.  */
+
+extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
+
+#endif /* COMMON_ERRORS_H */
diff --git a/gdb/gdbserver/Makefile.in b/gdb/gdbserver/Makefile.in
index f9a2f17..1faa00c 100644
--- a/gdb/gdbserver/Makefile.in
+++ b/gdb/gdbserver/Makefile.in
@@ -169,7 +169,7 @@  SFILES=	$(srcdir)/gdbreplay.c $(srcdir)/inferiors.c $(srcdir)/dll.c \
 	$(srcdir)/common/buffer.c $(srcdir)/nat/linux-btrace.c \
 	$(srcdir)/common/filestuff.c $(srcdir)/target/waitstatus.c \
 	$(srcdir)/nat/mips-linux-watch.c $(srcdir)/common/print-utils.c \
-	$(srcdir)/common/rsp-low.c
+	$(srcdir)/common/rsp-low.c $(srcdir)/common/errors.c
 
 DEPFILES = @GDBSERVER_DEPFILES@
 
@@ -182,7 +182,8 @@  OBS = agent.o ax.o inferiors.o regcache.o remote-utils.o server.o signals.o \
       target.o waitstatus.o utils.o debug.o version.o vec.o gdb_vecs.o \
       mem-break.o hostio.o event-loop.o tracepoint.o xml-utils.o \
       common-utils.o ptid.o buffer.o format.o filestuff.o dll.o notif.o \
-      tdesc.o print-utils.o rsp-low.o $(XML_BUILTIN) $(DEPFILES) $(LIBOBJS)
+      tdesc.o print-utils.o rsp-low.o errors.o $(XML_BUILTIN) $(DEPFILES) \
+      $(LIBOBJS)
 GDBREPLAY_OBS = gdbreplay.o version.o
 GDBSERVER_LIBS = @GDBSERVER_LIBS@
 XM_CLIBS = @LIBS@
@@ -300,7 +301,10 @@  gdbreplay$(EXEEXT): $(GDBREPLAY_OBS) $(LIBGNU) $(LIBIBERTY)
 	${CC-LD} $(INTERNAL_CFLAGS) $(INTERNAL_LDFLAGS) -o gdbreplay$(EXEEXT) $(GDBREPLAY_OBS) \
 	  $(XM_CLIBS) $(LIBGNU) $(LIBIBERTY)
 
-IPA_OBJS=ax-ipa.o tracepoint-ipa.o format-ipa.o utils-ipa.o regcache-ipa.o remote-utils-ipa.o common-utils-ipa.o tdesc-ipa.o print-utils-ipa.o rsp-low-ipa.o ${IPA_DEPFILES}
+IPA_OBJS=ax-ipa.o tracepoint-ipa.o format-ipa.o utils-ipa.o \
+	regcache-ipa.o remote-utils-ipa.o common-utils-ipa.o \
+	tdesc-ipa.o print-utils-ipa.o rsp-low-ipa.o errors-ipa.o \
+	${IPA_DEPFILES}
 
 IPA_LIB=libinproctrace.so
 
@@ -489,6 +493,9 @@  print-utils-ipa.o: ../common/print-utils.c
 rsp-low-ipa.o: ../common/rsp-low.c
 	$(IPAGENT_COMPILE) $<
 	$(POSTCOMPILE)
+errors-ipa.o: ../common/errors.c
+	$(IPAGENT_COMPILE) $<
+	$(POSTCOMPILE)
 
 ax.o: ax.c
 	$(COMPILE) $(WARN_CFLAGS_NO_FORMAT) $<
@@ -530,6 +537,9 @@  filestuff.o: ../common/filestuff.c
 agent.o: ../common/agent.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
+errors.o: ../common/errors.c
+	$(COMPILE) $<
+	$(POSTCOMPILE)
 waitstatus.o: ../target/waitstatus.c
 	$(COMPILE) $<
 	$(POSTCOMPILE)
diff --git a/gdb/gdbserver/utils.c b/gdb/gdbserver/utils.c
index 2d0b331..6c666e1 100644
--- a/gdb/gdbserver/utils.c
+++ b/gdb/gdbserver/utils.c
@@ -77,20 +77,17 @@  perror_with_name (const char *string)
   error ("%s.", combined);
 }
 
-/* Print an error message and return to command level.
-   STRING is the error message, used as a fprintf string,
-   and ARG is passed as an argument to it.  */
+/* Print an error message and return to command level.  */
 
 void
-error (const char *string,...)
+verror (const char *fmt, va_list args)
 {
 #ifndef IN_PROCESS_AGENT
   extern jmp_buf toplevel;
 #endif
-  va_list args;
-  va_start (args, string);
+
   fflush (stdout);
-  vfprintf (stderr, string, args);
+  vfprintf (stderr, fmt, args);
   fprintf (stderr, "\n");
 #ifndef IN_PROCESS_AGENT
   longjmp (toplevel, 1);
@@ -99,48 +96,36 @@  error (const char *string,...)
 #endif
 }
 
-/* Print an error message and exit reporting failure.
-   This is for a error that we cannot continue from.
-   STRING and ARG are passed to fprintf.  */
+/* Print an error message and exit reporting failure.  */
 
 /* VARARGS */
 void
-fatal (const char *string,...)
+vfatal (const char *fmt, va_list args)
 {
-  va_list args;
-  va_start (args, string);
   fprintf (stderr, PREFIX);
-  vfprintf (stderr, string, args);
+  vfprintf (stderr, fmt, args);
   fprintf (stderr, "\n");
-  va_end (args);
   exit (1);
 }
 
 /* VARARGS */
 void
-warning (const char *string,...)
+vwarning (const char *fmt, va_list args)
 {
-  va_list args;
-  va_start (args, string);
   fprintf (stderr, PREFIX);
-  vfprintf (stderr, string, args);
+  vfprintf (stderr, fmt, args);
   fprintf (stderr, "\n");
-  va_end (args);
 }
 
 /* Report a problem internal to GDBserver, and exit.  */
 
 void
-internal_error (const char *file, int line, const char *fmt, ...)
+internal_verror (const char *file, int line, const char *fmt, va_list args)
 {
-  va_list args;
-  va_start (args, fmt);
-
   fprintf (stderr,  "\
 %s:%d: A problem internal to " TOOLNAME " has been detected.\n", file, line);
   vfprintf (stderr, fmt, args);
   fprintf (stderr, "\n");
-  va_end (args);
   exit (1);
 }
 
diff --git a/gdb/gdbserver/utils.h b/gdb/gdbserver/utils.h
index 906924b..819fa35 100644
--- a/gdb/gdbserver/utils.h
+++ b/gdb/gdbserver/utils.h
@@ -20,11 +20,8 @@ 
 #define UTILS_H
 
 #include "print-utils.h"
+#include "errors.h"
 
-void perror_with_name (const char *string) ATTRIBUTE_NORETURN;
-void error (const char *string,...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
-void fatal (const char *string,...) ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
-void warning (const char *string,...) ATTRIBUTE_PRINTF (1, 2);
 char *paddress (CORE_ADDR addr);
 char *pfildes (gdb_fildes_t fd);
 
diff --git a/gdb/utils.c b/gdb/utils.c
index d324227..b2f9f0f 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -533,22 +533,6 @@  vwarning (const char *string, va_list args)
     }
 }
 
-/* Print a warning message.
-   The first argument STRING is the warning message, used as a fprintf string,
-   and the remaining args are passed as arguments to it.
-   The primary difference between warnings and errors is that a warning
-   does not force the return to command level.  */
-
-void
-warning (const char *string, ...)
-{
-  va_list args;
-
-  va_start (args, string);
-  vwarning (string, args);
-  va_end (args);
-}
-
 /* Print an error message and return to command level.
    The first argument STRING is the error message, used as a fprintf string,
    and the remaining args are passed as arguments to it.  */
@@ -559,16 +543,6 @@  verror (const char *string, va_list args)
   throw_verror (GENERIC_ERROR, string, args);
 }
 
-void
-error (const char *string, ...)
-{
-  va_list args;
-
-  va_start (args, string);
-  throw_verror (GENERIC_ERROR, string, args);
-  va_end (args);
-}
-
 /* Print an error message and quit.
    The first argument STRING is the error message, used as a fprintf string,
    and the remaining args are passed as arguments to it.  */
@@ -580,16 +554,6 @@  vfatal (const char *string, va_list args)
 }
 
 void
-fatal (const char *string, ...)
-{
-  va_list args;
-
-  va_start (args, string);
-  throw_vfatal (string, args);
-  va_end (args);
-}
-
-void
 error_stream (struct ui_file *stream)
 {
   char *message = ui_file_xstrdup (stream, NULL);
@@ -836,16 +800,6 @@  internal_verror (const char *file, int line, const char *fmt, va_list ap)
   fatal (_("Command aborted."));
 }
 
-void
-internal_error (const char *file, int line, const char *string, ...)
-{
-  va_list ap;
-
-  va_start (ap, string);
-  internal_verror (file, line, string, ap);
-  va_end (ap);
-}
-
 static struct internal_problem internal_warning_problem = {
   "internal-warning", 1, internal_problem_ask, 1, internal_problem_ask
 };
diff --git a/gdb/utils.h b/gdb/utils.h
index a91f551..e48fc43 100644
--- a/gdb/utils.h
+++ b/gdb/utils.h
@@ -24,6 +24,7 @@ 
 #include "cleanups.h"
 #include "exceptions.h"
 #include "print-utils.h"
+#include "errors.h"
 
 extern void initialize_utils (void);
 
@@ -269,7 +270,6 @@  extern void fprintf_symbol_filtered (struct ui_file *, const char *,
 
 extern void throw_perror_with_name (enum errors errcode, const char *string)
   ATTRIBUTE_NORETURN;
-extern void perror_with_name (const char *) ATTRIBUTE_NORETURN;
 
 extern void perror_warning_with_name (const char *string);
 
@@ -283,24 +283,8 @@  extern void (*deprecated_error_begin_hook) (void);
 
 extern char *warning_pre_print;
 
-extern void verror (const char *fmt, va_list ap)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
-
-extern void error (const char *fmt, ...)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
-
 extern void error_stream (struct ui_file *) ATTRIBUTE_NORETURN;
 
-extern void vfatal (const char *fmt, va_list ap)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 0);
-
-extern void fatal (const char *fmt, ...)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
-
-extern void internal_verror (const char *file, int line, const char *,
-			     va_list ap)
-     ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0);
-
 extern void internal_vwarning (const char *file, int line,
 			       const char *, va_list ap)
      ATTRIBUTE_PRINTF (3, 0);
@@ -308,10 +292,6 @@  extern void internal_vwarning (const char *file, int line,
 extern void internal_warning (const char *file, int line,
 			      const char *, ...) ATTRIBUTE_PRINTF (3, 4);
 
-extern void warning (const char *, ...) ATTRIBUTE_PRINTF (1, 2);
-
-extern void vwarning (const char *, va_list args) ATTRIBUTE_PRINTF (1, 0);
-
 extern void demangler_vwarning (const char *file, int line,
 			       const char *, va_list ap)
      ATTRIBUTE_PRINTF (3, 0);