This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/3 v4] Introduce common/errors.h
- From: Doug Evans <dje at google dot com>
- To: Gary Benson <gbenson at redhat dot com>
- Cc: gdb-patches at sourceware dot org, Pedro Alves <palves at redhat dot com>, Tom Tromey <tromey at redhat dot com>
- Date: Thu, 24 Jul 2014 10:45:25 -0700
- Subject: Re: [PATCH 1/3 v4] Introduce common/errors.h
- Authentication-results: sourceware.org; auth=none
- References: <1406206287-6817-1-git-send-email-gbenson at redhat dot com> <1406206287-6817-2-git-send-email-gbenson at redhat dot com>
Gary Benson writes:
> This introduces common/errors.h. This holds some error- and
> warning-related declarations that can be used by the code in common.
> Clients of the "common" code must provide definitions for these
> functions.
>
> gdb/
> 2014-07-24 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, warning, vwarning):
> Don't declare.
> * common/common-utils.h: Include errors.h.
> (malloc_failure, internal_error): Don't declare.
>
> gdb/gdbserver/
> 2014-07-24 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, warning): Don't declare.
> * utils.c (warning): Renamed and rewritten as...
> (vwarning): New function.
> (error): Renamed and rewritten as...
> (verror): New function.
> (internal_error): Renamed and rewritten as...
> (internal_verror): New function.
> [...]
Hi. I like the idea of having the va_list functions provided
by gdb/gdbserver so that the stdarg versions can be defined in
one place, but it's odd to then not have just one copy of
perror_with_name and malloc_failure given that their function comment
says:
perror_with_name:
/* A special case of "error" which constructs the error message by ...
and
malloc_failure:
/* A special case of internal error used to handle memory allocation ...
When I read "special case of ..." the first thing that comes to mind
is that the function is a utility wrapper around the corresponding
function, and given that it's not I'm left wondering "Why not?".
In the case of perror_with_name,
I see gdb's perror_with_name has some extra duties
(e.g., throw_perror_with_name clears errno/bfd-errno) but
throw_perror_with_name is only ever called by perror_with_name (AFAICT).
OTOH, gdb uses safe_strerror whereas gdbserver just uses strerror.
OTOOH, gdbserver could(/should?) have a safe_strerror too.
Taking on adding safe_strerror to gdbserver would require taking on
handling mingw (see gdb/mingw-hdep.c). I can understand if you'd like
to leave that for another day.
In the case of malloc_failure,
it's not really an *internal* error,
even if in the case of gdb it calls internal_error, though arguably
it should do something different - it's more of a "fatal". :-) Though
I'm not suggesting trying to go down that path. :-) It might be possible
come up with a name other than "fatal" that could apply to both gdb and
gdbserver so that malloc_failure could call it, but no need to try to
do that now.
So, *if* you want to keep *this* part of the patch basically as-is
(which is fine by me) I would rephrase this comment to something like:
/* Call this function to handle memory allocation failures.
This function does not return. */
I don't have too strong opinion on the wording.
If someone wants a different wording, go for it.
>
> 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
Nit: $(srcdir)
> + $(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..684bdcd
> --- /dev/null
> +++ b/gdb/common/errors.c
> @@ -0,0 +1,59 @@
> +/* 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
> +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..91b58c6
> --- /dev/null
> +++ b/gdb/common/errors.h
> @@ -0,0 +1,72 @@
> +/* 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
> +
> +/* A problem was detected, but the requested operation can still
> + proceed. A warning message is constructed using 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);
> +
> +/* A non-predictable, non-fatal error was detected. The requested
> + operation cannot proceed. An error message is constructed using
> + a printf- or vprintf-style argument list. These functions do not
> + return. */
> +
> +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);
> +
> +/* 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. These
> + functions do not return. An error message is constructed using
> + a printf- or vprintf-style argument list. FILE and LINE
> + indicate the file and line number where the programming error
> + was detected. */
> +
> +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 "error" which constructs the error message by
> + combining STRING with the system error message for errno. This
> + function does not return. */
> +
> +extern void perror_with_name (const char *string) ATTRIBUTE_NORETURN;
> +
> +/* A special case of internal error used to handle memory allocation
> + failures. This function does not return. */
> +
> +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..9add0b9 100644
> --- a/gdb/gdbserver/utils.c
> +++ b/gdb/gdbserver/utils.c
> @@ -77,18 +77,15 @@ 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 top level. */
>
> void
> -error (const char *string,...)
> +verror (const char *string, 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);
> fprintf (stderr, "\n");
> @@ -116,31 +113,25 @@ fatal (const char *string,...)
> exit (1);
> }
>
> -/* VARARGS */
> +/* Print a warning message. */
> +
> void
> -warning (const char *string,...)
> +vwarning (const char *string, va_list args)
> {
> - va_list args;
> - va_start (args, string);
> fprintf (stderr, PREFIX);
> vfprintf (stderr, string, 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..b608540 100644
> --- a/gdb/gdbserver/utils.h
> +++ b/gdb/gdbserver/utils.h
> @@ -20,11 +20,9 @@
> #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 8af8827..1f1ba0e 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. */
> @@ -560,16 +544,6 @@ verror (const char *string, va_list args)
> }
>
> void
> -error (const char *string, ...)
> -{
> - va_list args;
> -
> - va_start (args, string);
> - throw_verror (GENERIC_ERROR, string, args);
> - va_end (args);
> -}
> -
> -void
> error_stream (struct ui_file *stream)
> {
> char *message = ui_file_xstrdup (stream, NULL);
> @@ -816,16 +790,6 @@ internal_verror (const char *file, int line, const char *fmt, va_list ap)
> throw_quit (_("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 cc79562..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,18 +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 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);
> @@ -302,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);
> --
> 1.7.1
>
--
/dje