From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10602 invoked by alias); 24 Jul 2014 17:45:35 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 10571 invoked by uid 89); 24 Jul 2014 17:45:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qa0-f74.google.com Received: from mail-qa0-f74.google.com (HELO mail-qa0-f74.google.com) (209.85.216.74) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 24 Jul 2014 17:45:29 +0000 Received: by mail-qa0-f74.google.com with SMTP id j15so540748qaq.1 for ; Thu, 24 Jul 2014 10:45:27 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:mime-version:content-type :content-transfer-encoding:message-id:date:to:cc:subject:in-reply-to :references; bh=hLMmXFy5rdVXFjgn0c+Tnrz96pn1LpwMjegX1o+o8gE=; b=W2Uu6FG0J8Sb5fFccR93DieLutyB4sFdFjPjragM2kbq/suwTAShSD77rXz/iwXH0o 1ybg56sa/2j3CK0iq+EdVczazHWZX/TvPdr/BjDJKwBRP3YBNqWjzxXEPU3sgq2uASN/ MFkwdkjM/gby/45HWw05XS8aiSaC3wBUZb4bcrOYxdqL8++zP70WKVgfuqZZFAxTVrCG v3PfFoCT0N+2kCDJJsJAtG5YDWjDBoU20+JYWMUmOVmodqjhiGkpRBrEDwdWpq5Gxgzm iuzgbb+J9BDkByQm2V5lv+/igulFisaPoawb3WwWzMRlzlS60uhJBRGoLWdvAA0xmFtA M0Dg== X-Gm-Message-State: ALoCoQnMG0qmXglAqpJNeNW40IW4qgsZcRclrvBD0mRoNKJd25u7TOMSdXcVeAsZJzGRHal6yJDa X-Received: by 10.224.171.130 with SMTP id h2mr4908279qaz.7.1406223927035; Thu, 24 Jul 2014 10:45:27 -0700 (PDT) Received: from corp2gmr1-2.hot.corp.google.com (corp2gmr1-2.hot.corp.google.com [172.24.189.93]) by gmr-mx.google.com with ESMTPS id v44si925678yhv.0.2014.07.24.10.45.27 for (version=TLSv1.1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Thu, 24 Jul 2014 10:45:27 -0700 (PDT) Received: from ruffy.mtv.corp.google.com (ruffy.mtv.corp.google.com [172.17.128.44]) by corp2gmr1-2.hot.corp.google.com (Postfix) with ESMTP id 269185A4365; Thu, 24 Jul 2014 10:45:25 -0700 (PDT) From: Doug Evans MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <21457.17973.551020.113918@ruffy.mtv.corp.google.com> Date: Thu, 24 Jul 2014 17:52:00 -0000 To: Gary Benson Cc: gdb-patches@sourceware.org, Pedro Alves , Tom Tromey Subject: Re: [PATCH 1/3 v4] Introduce common/errors.h In-Reply-To: <1406206287-6817-2-git-send-email-gbenson@redhat.com> References: <1406206287-6817-1-git-send-email-gbenson@redhat.com> <1406206287-6817-2-git-send-email-gbenson@redhat.com> X-IsSubscribed: yes X-SW-Source: 2014-07/txt/msg00648.txt.bz2 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 > Gary Benson > > * 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 > Gary Benson > > * 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 > #include > +#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 . */ > + > +#include "config.h" > +#include "ansidecl.h" > +#include > +#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 . */ > + > +#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