Re: [PATCH 01/15 v2] Introduce common/errors.h

Doug Evans
2014-07-16T18:33:44Z

On Wed, Jul 16, 2014 at 7:17 AM, Gary Benson <gbenson@redhat.com> wrote:
> 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-16 Tom Tromey <tromey@redhat.com>
> Gary Benson <gbenson@redhat.com>
>
> * common/errors.h: New file.
> * utils.h: Include errors.h.
> (perror_with_name, error, fatal, warning): 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>
>
> * utils.h: Include errors.h.
> (perror_with_name, error, fatal, warning): Don't declare.

Hi. Comments inline.

> ---
> gdb/ChangeLog | 9 +++++++
> gdb/common/common-utils.h | 5 +---
> gdb/common/errors.h | 55 +++++++++++++++++++++++++++++++++++++++++++++
> gdb/gdbserver/ChangeLog | 5 ++++
> gdb/gdbserver/utils.h | 5 +---
> gdb/utils.h | 10 +-------
> 6 files changed, 72 insertions(+), 17 deletions(-)
> create mode 100644 gdb/common/errors.h
>
> 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.h b/gdb/common/errors.h
> new file mode 100644
> index 0000000..5f998e3
> --- /dev/null
> +++ b/gdb/common/errors.h
> @@ -0,0 +1,55 @@
> +/* 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/. */
> +
> +/* Like "perror" but throws an exception with the appropriate
> + text. */
> +
> +extern void perror_with_name (const char *) ATTRIBUTE_NORETURN;
> +
> +/* Throw an exception. */
> +
> +extern void error (const char *fmt, ...)
> + ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);
> +
> +/* Cause a fatal error. */
> +
> +extern void fatal (const char *fmt, ...)
> + ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (1, 2);

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 :-)]

> +
> +/* Issue a warning. */
> +
> +extern void warning (const char *, ...) ATTRIBUTE_PRINTF (1, 2);
> +
> +/* Internal error. */
> +
> +extern void internal_error (const char *file, int line, const char *, ...)
> + ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 4);
> +
> +/* Memory allocation failure. */
> +
> +extern void malloc_failure (long size) ATTRIBUTE_NORETURN;
> +
> +#endif /* COMMON_ERRORS_H */
> 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.h b/gdb/utils.h
> index a91f551..d9fe7e3 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);
>
> @@ -286,17 +286,11 @@ 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);
> -

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

> extern void internal_verror (const char *file, int line, const char *,
> va_list ap)
> ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (3, 0);
> @@ -308,8 +302,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,
> --
> 1.7.1
>