From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15878 invoked by alias); 24 Feb 2009 20:23:08 -0000 Received: (qmail 15562 invoked by uid 22791); 24 Feb 2009 20:23:03 -0000 X-SWARE-Spam-Status: No, hits=0.9 required=5.0 tests=AWL,BAYES_50,HK_OBFDOM,HK_OBFDOMREQ,SARE_MSGID_LONG40,SPF_PASS X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (216.239.33.17) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 24 Feb 2009 20:22:55 +0000 Received: from spaceape12.eur.corp.google.com (spaceape12.eur.corp.google.com [172.28.16.146]) by smtp-out.google.com with ESMTP id n1OKMqem019466 for ; Tue, 24 Feb 2009 20:22:52 GMT Received: from rv-out-0506.google.com (rvbk40.prod.google.com [10.140.87.40]) by spaceape12.eur.corp.google.com with ESMTP id n1OKMExR009016 for ; Tue, 24 Feb 2009 12:22:50 -0800 Received: by rv-out-0506.google.com with SMTP id k40so2479971rvb.21 for ; Tue, 24 Feb 2009 12:22:50 -0800 (PST) MIME-Version: 1.0 Received: by 10.141.20.6 with SMTP id x6mr2759384rvi.159.1235506970243; Tue, 24 Feb 2009 12:22:50 -0800 (PST) In-Reply-To: <20090205215202.CE0251C7A1E@localhost> References: <20090205215202.CE0251C7A1E@localhost> Date: Tue, 24 Feb 2009 20:30:00 -0000 Message-ID: Subject: Re: [RFC] Create src/gdb/common, move signals.c there. From: Doug Evans To: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-System-Of-Record: true X-IsSubscribed: yes 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 X-SW-Source: 2009-02/txt/msg00475.txt.bz2 On Thu, Feb 5, 2009 at 1:52 PM, Doug Evans wrote: > Hi. > > Some code is shared between gdb and gdbserver, and over time more > code will be shared. And I think the lack of explicit sharing > is complicating progress. > > So I'd like to create a path for further sharing, and am starting > with a modest step to see if there's interest in pursuing this. > > This patch does these things: > > 1) Create src/gdb/common, and adds -I's to $(src)/common to the Makefiles. > 2) Pull the duplicate decls of the functions in signals.c out of > gdb/target.h and gdbserver/server.h and put them in common/gdb_signals.h. > 3) Move signals/signals.c to common/signals.c, and cleans up signals.c > a bit. > > I recognize that manually configuring gdbserver and building it apart > from gdb is explicitly supported, and this patch doesn't change this. > [Nor am I suggesting breaking this, > e.g. I'm not proposing creating libcommon.a.] > > One thing I would like to have done in this patch is remove the #include's of > defs.h and server.h from signals.c, but that involves making gdb_string.h > work in both gdb and gdbserver. I think this is a reasonable goal, > (modulo having gdb and gdbserver go to a gnulib version, if that makes sense) > but it requires a bit of effort (e.g. gdb/configure.ac and > gdbserver/configure.ac script hacking and how much of them to make common, > either as duplication or as a separate .m4 file or some such), > and I didn't want to put too much effort into this until there's general > agreement in principle of the path. > > Further steps that can be taken: > [whether any particular suggestion is worth it is debatable, > the point here is to enumerate the steps that _could_ be taken, > and no claim is made that this list is complete] > > - move version.h to common, > allowing us to remove the duplication in server.h > - pull the gdb-specific functions out of signals.c, > allowing us to remove the #ifndef GDBSERVER in signals.c > - maybe(!) move regformats to common, though it's big enough > and standalone enough that I don't have an opinion on it > - create a common place to put things like ATTR_FORMAT, ATTR_NORETURN, etc. > (and various other duplication b/w defs.h/server.h), > allowing us to remove the duplication in defs.h/server.h > - similarily for the duplication b/w gdb_string.h and server.h > > WDYT? > > > 2009-02-05 Doug Evans > > * Makefile.in (GDB_CFLAGS): Add -I$(srcdir)/common. > (init.c): signals/ -> common/. > (signals.o): Update. > * target.h (target_signal_to_string,target_signal_to_string) > (target_signal_from_name,target_signal_to_host_p) > (target_signal_from_host,target_signal_to_host): Move to ... > * common/gdb_signals.h: ... here. New file. > > * gdbserver/Makefile.in (INCLUDE_CFLAGS): Add -I$(srcdir)/../common. > (server_h): Add gdb_signals.h. > (signals.o): Update. > * server.h (target_signal_from_host,target_signal_to_host_p) > (target_signal_to_host,target_signal_to_name): Moved to gdb_signals.h. > > * common/signals.c: Moved here from signals/signals.c. > #include gdb_signals.h, remove #include of target.h in gdb case. > (target_signal_from_command,default_target_signal_to_host) > (default_target_signal_from_host): Move inside #ifndef GDBSERVER. > > Index: Makefile.in > =================================================================== > RCS file: /cvs/src/src/gdb/Makefile.in,v > retrieving revision 1.1065 > diff -u -p -u -p -r1.1065 Makefile.in > --- Makefile.in 30 Jan 2009 22:14:52 -0000 1.1065 > +++ Makefile.in 5 Feb 2009 20:24:15 -0000 > @@ -363,7 +363,8 @@ CONFIG_UNINSTALL = @CONFIG_UNINSTALL@ > # your system doesn't have fcntl.h in /usr/include (which is where it > # should be according to Posix). > DEFS = @DEFS@ > -GDB_CFLAGS = -I. -I$(srcdir) -I$(srcdir)/config -DLOCALEDIR="\"$(localedir)\"" $(DEFS) > +GDB_CFLAGS = -I. -I$(srcdir) -I$(srcdir)/common -I$(srcdir)/config \ > + -DLOCALEDIR="\"$(localedir)\"" $(DEFS) > > # MH_CFLAGS, if defined, has host-dependent CFLAGS from the config directory. > GLOBAL_CFLAGS = $(MH_CFLAGS) > @@ -1029,7 +1030,7 @@ init.c: $(INIT_FILES) > -e '/^[a-z0-9A-Z_]*_[SU].[co]$$/d' \ > -e '/[a-z0-9A-Z_]*-exp.tab.[co]$$/d' \ > -e 's/\.[co]$$/.c/' \ > - -e 's,signals\.c,signals/signals\.c,' \ > + -e 's,signals\.c,common/signals\.c,' \ > -e 's|^\([^ /][^ ]*\)|$(srcdir)/\1|g' | \ > while read f; do \ > sed -n -e 's/^_initialize_\([a-z_0-9A-Z]*\).*/\1/p' $$f 2>/dev/null; \ > @@ -1736,13 +1737,13 @@ mi-common.o: $(srcdir)/mi/mi-common.c > $(POSTCOMPILE) > > # > -# gdb/signals/ dependencies > +# gdb/common/ dependencies > # > # Need to explicitly specify the compile rule as make will do nothing > # or try to compile the object file into the sub-directory. > > -signals.o: $(srcdir)/signals/signals.c > - $(COMPILE) $(srcdir)/signals/signals.c > +signals.o: $(srcdir)/common/signals.c > + $(COMPILE) $(srcdir)/common/signals.c > $(POSTCOMPILE) > > # > Index: target.h > =================================================================== > RCS file: /cvs/src/src/gdb/target.h,v > retrieving revision 1.141 > diff -u -p -u -p -r1.141 target.h > --- target.h 1 Feb 2009 23:31:03 -0000 1.141 > +++ target.h 5 Feb 2009 20:24:15 -0000 > @@ -55,6 +55,7 @@ struct regcache; > #include "dcache.h" > #include "memattr.h" > #include "vec.h" > +#include "gdb_signals.h" > > enum strata > { > @@ -176,15 +177,6 @@ enum inferior_event_type > 'step n' like commands. */ > INF_EXEC_CONTINUE > }; > - > -/* Return the string for a signal. */ > -extern const char *target_signal_to_string (enum target_signal); > - > -/* Return the name (SIGHUP, etc.) for a signal. */ > -extern const char * target_signal_to_name (enum target_signal); > - > -/* Given a name (SIGHUP, etc.), return its signal. */ > -enum target_signal target_signal_from_name (const char *); > > /* Target objects which can be transfered using target_read, > target_write, et cetera. */ > @@ -1307,28 +1299,7 @@ extern int remote_timeout; > /* This is for native targets which use a unix/POSIX-style waitstatus. */ > extern void store_waitstatus (struct target_waitstatus *, int); > > -/* Predicate to target_signal_to_host(). Return non-zero if the enum > - targ_signal SIGNO has an equivalent ``host'' representation. */ > -/* FIXME: cagney/1999-11-22: The name below was chosen in preference > - to the shorter target_signal_p() because it is far less ambigious. > - In this context ``target_signal'' refers to GDB's internal > - representation of the target's set of signals while ``host signal'' > - refers to the target operating system's signal. Confused? */ > - > -extern int target_signal_to_host_p (enum target_signal signo); > - > -/* Convert between host signal numbers and enum target_signal's. > - target_signal_to_host() returns 0 and prints a warning() on GDB's > - console if SIGNO has no equivalent host representation. */ > -/* FIXME: cagney/1999-11-22: Here ``host'' is used incorrectly, it is > - refering to the target operating system's signal numbering. > - Similarly, ``enum target_signal'' is named incorrectly, ``enum > - gdb_signal'' would probably be better as it is refering to GDB's > - internal representation of a target operating system's signal. */ > - > -extern enum target_signal target_signal_from_host (int); > -extern int target_signal_to_host (enum target_signal); > - > +/* These are in common/signals.c, but they're only used by gdb. */ > extern enum target_signal default_target_signal_from_host (struct gdbarch *, > int); > extern int default_target_signal_to_host (struct gdbarch *, > @@ -1336,6 +1307,7 @@ extern int default_target_signal_to_host > > /* Convert from a number used in a GDB command to an enum target_signal. */ > extern enum target_signal target_signal_from_command (int); > +/* End of files in common/signals.c. */ > > /* Set the show memory breakpoints mode to show, and installs a cleanup > to restore it back to the current value. */ > Index: gdbserver/Makefile.in > =================================================================== > RCS file: /cvs/src/src/gdb/gdbserver/Makefile.in,v > retrieving revision 1.68 > diff -u -p -u -p -r1.68 Makefile.in > --- gdbserver/Makefile.in 3 Jan 2009 05:57:56 -0000 1.68 > +++ gdbserver/Makefile.in 5 Feb 2009 20:24:15 -0000 > @@ -74,7 +74,8 @@ INCLUDE_DEP = $$(INCLUDE_DIR) > # -I. for config files. > # -I${srcdir} for our headers. > # -I$(srcdir)/../regformats for regdef.h. > -INCLUDE_CFLAGS = -I. -I${srcdir} -I$(srcdir)/../regformats -I$(INCLUDE_DIR) > +INCLUDE_CFLAGS = -I. -I${srcdir} -I$(srcdir)/../common \ > + -I$(srcdir)/../regformats -I$(INCLUDE_DIR) > > # M{H,T}_CFLAGS, if defined, has host- and target-dependent CFLAGS > # from the config/ directory. > @@ -263,7 +264,7 @@ regdat_sh = $(srcdir)/../regformats/regd > regdef_h = $(srcdir)/../regformats/regdef.h > regcache_h = $(srcdir)/regcache.h > server_h = $(srcdir)/server.h $(regcache_h) config.h $(srcdir)/target.h \ > - $(srcdir)/mem-break.h > + $(srcdir)/mem-break.h $(srcdir)/../common/gdb_signals.h > > hostio.o: hostio.c $(server_h) > hostio-errno.o: hostio-errno.c $(server_h) > @@ -278,7 +279,7 @@ thread-db.o: thread-db.c $(server_h) $(g > utils.o: utils.c $(server_h) > gdbreplay.o: gdbreplay.c config.h > > -signals.o: ../signals/signals.c $(server_h) > +signals.o: ../common/signals.c $(server_h) > $(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $< -DGDBSERVER > > memmem.o: ../gnulib/memmem.c > Index: gdbserver/server.h > =================================================================== > RCS file: /cvs/src/src/gdb/gdbserver/server.h,v > retrieving revision 1.51 > diff -u -p -u -p -r1.51 server.h > --- gdbserver/server.h 19 Jan 2009 00:16:46 -0000 1.51 > +++ gdbserver/server.h 5 Feb 2009 20:24:15 -0000 > @@ -110,7 +110,7 @@ struct dll_info > > #include "regcache.h" > #include "gdb/signals.h" > - > +#include "gdb_signals.h" > #include "target.h" > #include "mem-break.h" > > @@ -265,12 +265,6 @@ void buffer_xml_printf (struct buffer *b > #define buffer_grow_str0(BUFFER,STRING) \ > buffer_grow (BUFFER, STRING, strlen (STRING) + 1) > > -/* Functions from ``signals.c''. */ > -enum target_signal target_signal_from_host (int hostsig); > -int target_signal_to_host_p (enum target_signal oursig); > -int target_signal_to_host (enum target_signal oursig); > -const char *target_signal_to_name (enum target_signal); > - > /* Functions from utils.c */ > > void *xmalloc (size_t) ATTR_MALLOC; > > > > diff -u -p common/server.c > --- signals/signals.c 2009-01-15 14:07:20.000000000 -0800 > +++ common/signals.c 2009-02-05 13:15:43.000000000 -0800 > @@ -23,7 +23,6 @@ > #include "server.h" > #else > #include "defs.h" > -#include "target.h" > #include "gdb_string.h" > #endif > > @@ -31,6 +30,8 @@ > #include > #endif > > +#include "gdb_signals.h" > + > struct gdbarch; > > /* Always use __SIGRTMIN if it's available. SIGRTMIN is the lowest > @@ -807,6 +808,8 @@ target_signal_to_host (enum target_signa > return targ_signo; > } > > +#ifndef GDBSERVER > + > /* In some circumstances we allow a command to specify a numeric > signal. The idea is to keep these circumstances limited so that > users (and scripts) develop portable habits. For comparison, > @@ -824,7 +827,6 @@ target_signal_from_command (int num) > Use \"info signals\" for a list of symbolic signals."); > } > > -#ifndef GDBSERVER > extern initialize_file_ftype _initialize_signals; /* -Wmissing-prototype */ > > void > @@ -833,7 +835,6 @@ _initialize_signals (void) > if (strcmp (signals[TARGET_SIGNAL_LAST].string, "TARGET_SIGNAL_MAGIC") != 0) > internal_error (__FILE__, __LINE__, "failed internal consistency check"); > } > -#endif > > int > default_target_signal_to_host (struct gdbarch *gdbarch, enum target_signal ts) > @@ -846,3 +847,5 @@ default_target_signal_from_host (struct > { > return target_signal_from_host (signo); > } > + > +#endif /* ! GDBSERVER */ > > > > diff -u -p common/gdb_signals.h > --- /dev/null 2007-10-18 09:27:25.000000000 -0700 > +++ common/gdb_signals.h 2009-02-05 11:58:47.000000000 -0800 > @@ -0,0 +1,56 @@ > +/* Target signal translation functions for GDB. > + Copyright (C) 1990, 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999, > + 2000, 2001, 2002, 2003, 2006, 2007, 2008, 2009 > + Free Software Foundation, Inc. > + Contributed by Cygnus Support. > + > + 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_GDB_SIGNALS_H > +#define COMMON_GDB_SIGNALS_H > + > +#include "gdb/signals.h" > + > +/* Predicate to target_signal_to_host(). Return non-zero if the enum > + targ_signal SIGNO has an equivalent ``host'' representation. */ > +/* FIXME: cagney/1999-11-22: The name below was chosen in preference > + to the shorter target_signal_p() because it is far less ambigious. > + In this context ``target_signal'' refers to GDB's internal > + representation of the target's set of signals while ``host signal'' > + refers to the target operating system's signal. Confused? */ > +extern int target_signal_to_host_p (enum target_signal signo); > + > +/* Convert between host signal numbers and enum target_signal's. > + target_signal_to_host() returns 0 and prints a warning() on GDB's > + console if SIGNO has no equivalent host representation. */ > +/* FIXME: cagney/1999-11-22: Here ``host'' is used incorrectly, it is > + refering to the target operating system's signal numbering. > + Similarly, ``enum target_signal'' is named incorrectly, ``enum > + gdb_signal'' would probably be better as it is refering to GDB's > + internal representation of a target operating system's signal. */ > +extern enum target_signal target_signal_from_host (int); > +extern int target_signal_to_host (enum target_signal); > + > +/* Return the string for a signal. */ > +extern const char *target_signal_to_string (enum target_signal); > + > +/* Return the name (SIGHUP, etc.) for a signal. */ > +extern const char *target_signal_to_name (enum target_signal); > + > +/* Given a name (SIGHUP, etc.), return its signal. */ > +enum target_signal target_signal_from_name (const char *); > + > +#endif /* COMMON_GDB_SIGNALS_H */ > Ping. For reference sake, whether any particular tidbit is made common needs to be decided on its own merits. But I think the concept is sound, and this patch is standalone enough to warrant inclusion as is. Ok to check in?