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: [PATCH 1/3 v4] Introduce common/errors.h


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


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