From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6672 invoked by alias); 25 Feb 2009 07:04:15 -0000 Received: (qmail 6656 invoked by uid 22791); 25 Feb 2009 07:04:13 -0000 X-SWARE-Spam-Status: No, hits=0.2 required=5.0 tests=AWL,BAYES_50,HK_OBFDOM,SARE_MSGID_LONG40,SPF_PASS X-Spam-Check-By: sourceware.org Received: from ti-out-0910.google.com (HELO ti-out-0910.google.com) (209.85.142.186) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 25 Feb 2009 07:04:07 +0000 Received: by ti-out-0910.google.com with SMTP id y8so2638091tia.12 for ; Tue, 24 Feb 2009 23:04:04 -0800 (PST) MIME-Version: 1.0 Received: by 10.110.11.7 with SMTP id 7mr8962091tik.56.1235545435868; Tue, 24 Feb 2009 23:03:55 -0800 (PST) In-Reply-To: References: <20090205215202.CE0251C7A1E@localhost> Date: Wed, 25 Feb 2009 10:26:00 -0000 Message-ID: Subject: Re: [RFC] Create src/gdb/common, move signals.c there. From: teawater To: Doug Evans Cc: gdb-patches@sourceware.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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/msg00479.txt.bz2 I think this idea is good. And I agree with let gdb and gdbserver just share the code. It's friendly to cross compile environment. Hui On Wed, Feb 25, 2009 at 04:22, Doug Evans wrote: > 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. =A0And 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 Makefile= s. >> 2) Pull the duplicate decls of the functions in signals.c out of >> =A0 gdb/target.h and gdbserver/server.h and put them in common/gdb_signa= ls.h. >> 3) Move signals/signals.c to common/signals.c, and cleans up signals.c >> =A0 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. =A0I think this is a reasonable goal, >> (modulo having gdb and gdbserver go to a gnulib version, if that makes s= ense) >> 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 commo= n, >> 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, >> =A0allowing us to remove the duplication in server.h >> - pull the gdb-specific functions out of signals.c, >> =A0allowing us to remove the #ifndef GDBSERVER in signals.c >> - maybe(!) move regformats to common, though it's big enough >> =A0and standalone enough that I don't have an opinion on it >> - create a common place to put things like ATTR_FORMAT, ATTR_NORETURN, e= tc. >> =A0(and various other duplication b/w defs.h/server.h), >> =A0allowing 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 =A0Doug Evans =A0 >> >> =A0 =A0 =A0 =A0* Makefile.in (GDB_CFLAGS): Add -I$(srcdir)/common. >> =A0 =A0 =A0 =A0(init.c): signals/ -> common/. >> =A0 =A0 =A0 =A0(signals.o): Update. >> =A0 =A0 =A0 =A0* target.h (target_signal_to_string,target_signal_to_stri= ng) >> =A0 =A0 =A0 =A0(target_signal_from_name,target_signal_to_host_p) >> =A0 =A0 =A0 =A0(target_signal_from_host,target_signal_to_host): Move to = ... >> =A0 =A0 =A0 =A0* common/gdb_signals.h: ... here. =A0New file. >> >> =A0 =A0 =A0 =A0* gdbserver/Makefile.in (INCLUDE_CFLAGS): Add -I$(srcdir)= /../common. >> =A0 =A0 =A0 =A0(server_h): Add gdb_signals.h. >> =A0 =A0 =A0 =A0(signals.o): Update. >> =A0 =A0 =A0 =A0* server.h (target_signal_from_host,target_signal_to_host= _p) >> =A0 =A0 =A0 =A0(target_signal_to_host,target_signal_to_name): Moved to g= db_signals.h. >> >> =A0 =A0 =A0 =A0* common/signals.c: Moved here from signals/signals.c. >> =A0 =A0 =A0 =A0#include gdb_signals.h, remove #include of target.h in gd= b case. >> =A0 =A0 =A0 =A0(target_signal_from_command,default_target_signal_to_host) >> =A0 =A0 =A0 =A0(default_target_signal_from_host): Move inside #ifndef GD= BSERVER. >> >> Index: Makefile.in >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> 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 =A0 =A0 =A01.1065 >> +++ Makefile.in 5 Feb 2009 20:24:15 -0000 >> @@ -363,7 +363,8 @@ CONFIG_UNINSTALL =3D @CONFIG_UNINSTALL@ >> =A0# your system doesn't have fcntl.h in /usr/include (which is where it >> =A0# should be according to Posix). >> =A0DEFS =3D @DEFS@ >> -GDB_CFLAGS =3D -I. -I$(srcdir) -I$(srcdir)/config -DLOCALEDIR=3D"\"$(lo= caledir)\"" $(DEFS) >> +GDB_CFLAGS =3D -I. -I$(srcdir) -I$(srcdir)/common -I$(srcdir)/config \ >> + =A0 =A0 =A0 -DLOCALEDIR=3D"\"$(localedir)\"" $(DEFS) >> >> =A0# MH_CFLAGS, if defined, has host-dependent CFLAGS from the config di= rectory. >> =A0GLOBAL_CFLAGS =3D $(MH_CFLAGS) >> @@ -1029,7 +1030,7 @@ init.c: $(INIT_FILES) >> =A0 =A0 =A0 =A0 =A0 =A0-e '/^[a-z0-9A-Z_]*_[SU].[co]$$/d' \ >> =A0 =A0 =A0 =A0 =A0 =A0-e '/[a-z0-9A-Z_]*-exp.tab.[co]$$/d' \ >> =A0 =A0 =A0 =A0 =A0 =A0-e 's/\.[co]$$/.c/' \ >> - =A0 =A0 =A0 =A0 =A0 -e 's,signals\.c,signals/signals\.c,' \ >> + =A0 =A0 =A0 =A0 =A0 -e 's,signals\.c,common/signals\.c,' \ >> =A0 =A0 =A0 =A0 =A0 =A0-e 's|^\([^ =A0/][^ =A0 =A0 ]*\)|$(srcdir)/\1|g' = | \ >> =A0 =A0 =A0 =A0while read f; do \ >> =A0 =A0 =A0 =A0 =A0 =A0sed -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 >> =A0 =A0 =A0 =A0$(POSTCOMPILE) >> >> =A0# >> -# gdb/signals/ dependencies >> +# gdb/common/ dependencies >> =A0# >> =A0# Need to explicitly specify the compile rule as make will do nothing >> =A0# or try to compile the object file into the sub-directory. >> >> -signals.o: $(srcdir)/signals/signals.c >> - =A0 =A0 =A0 $(COMPILE) $(srcdir)/signals/signals.c >> +signals.o: $(srcdir)/common/signals.c >> + =A0 =A0 =A0 $(COMPILE) $(srcdir)/common/signals.c >> =A0 =A0 =A0 =A0$(POSTCOMPILE) >> >> =A0# >> Index: target.h >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> RCS file: /cvs/src/src/gdb/target.h,v >> retrieving revision 1.141 >> diff -u -p -u -p -r1.141 target.h >> --- target.h =A0 =A01 Feb 2009 23:31:03 -0000 =A0 =A0 =A0 1.141 >> +++ target.h =A0 =A05 Feb 2009 20:24:15 -0000 >> @@ -55,6 +55,7 @@ struct regcache; >> =A0#include "dcache.h" >> =A0#include "memattr.h" >> =A0#include "vec.h" >> +#include "gdb_signals.h" >> >> =A0enum strata >> =A0 { >> @@ -176,15 +177,6 @@ enum inferior_event_type >> =A0 =A0 =A0 =A0'step n' like commands. =A0*/ >> =A0 =A0 INF_EXEC_CONTINUE >> =A0 }; >> - >> -/* Return the string for a signal. =A0*/ >> -extern const char *target_signal_to_string (enum target_signal); >> - >> -/* Return the name (SIGHUP, etc.) for a signal. =A0*/ >> -extern const char * target_signal_to_name (enum target_signal); >> - >> -/* Given a name (SIGHUP, etc.), return its signal. =A0*/ >> -enum target_signal target_signal_from_name (const char *); >> >> =A0/* Target objects which can be transfered using target_read, >> =A0 =A0target_write, et cetera. =A0*/ >> @@ -1307,28 +1299,7 @@ extern int remote_timeout; >> =A0/* This is for native targets which use a unix/POSIX-style waitstatus= . =A0*/ >> =A0extern void store_waitstatus (struct target_waitstatus *, int); >> >> -/* Predicate to target_signal_to_host(). Return non-zero if the enum >> - =A0 targ_signal SIGNO has an equivalent ``host'' representation. =A0*/ >> -/* FIXME: cagney/1999-11-22: The name below was chosen in preference >> - =A0 to the shorter target_signal_p() because it is far less ambigious. >> - =A0 In this context ``target_signal'' refers to GDB's internal >> - =A0 representation of the target's set of signals while ``host signal'' >> - =A0 refers to the target operating system's signal. =A0Confused? =A0*/ >> - >> -extern int target_signal_to_host_p (enum target_signal signo); >> - >> -/* Convert between host signal numbers and enum target_signal's. >> - =A0 target_signal_to_host() returns 0 and prints a warning() on GDB's >> - =A0 console if SIGNO has no equivalent host representation. =A0*/ >> -/* FIXME: cagney/1999-11-22: Here ``host'' is used incorrectly, it is >> - =A0 refering to the target operating system's signal numbering. >> - =A0 Similarly, ``enum target_signal'' is named incorrectly, ``enum >> - =A0 gdb_signal'' would probably be better as it is refering to GDB's >> - =A0 internal representation of a target operating system's signal. =A0= */ >> - >> -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. =A0*/ >> =A0extern enum target_signal default_target_signal_from_host (struct gdb= arch *, >> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int); >> =A0extern int default_target_signal_to_host (struct gdbarch *, >> @@ -1336,6 +1307,7 @@ extern int default_target_signal_to_host >> >> =A0/* Convert from a number used in a GDB command to an enum target_sign= al. =A0*/ >> =A0extern enum target_signal target_signal_from_command (int); >> +/* End of files in common/signals.c. =A0*/ >> >> =A0/* Set the show memory breakpoints mode to show, and installs a clean= up >> =A0 =A0to restore it back to the current value. =A0*/ >> Index: gdbserver/Makefile.in >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> 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 =A0 =A0 =A0 3 Jan 2009 05:57:56 -0000 =A0 =A0 = =A0 1.68 >> +++ gdbserver/Makefile.in =A0 =A0 =A0 5 Feb 2009 20:24:15 -0000 >> @@ -74,7 +74,8 @@ INCLUDE_DEP =3D $$(INCLUDE_DIR) >> =A0# -I. for config files. >> =A0# -I${srcdir} for our headers. >> =A0# -I$(srcdir)/../regformats for regdef.h. >> -INCLUDE_CFLAGS =3D -I. -I${srcdir} -I$(srcdir)/../regformats -I$(INCLUD= E_DIR) >> +INCLUDE_CFLAGS =3D -I. -I${srcdir} -I$(srcdir)/../common \ >> + =A0 =A0 =A0 -I$(srcdir)/../regformats -I$(INCLUDE_DIR) >> >> =A0# M{H,T}_CFLAGS, if defined, has host- and target-dependent CFLAGS >> =A0# from the config/ directory. >> @@ -263,7 +264,7 @@ regdat_sh =3D $(srcdir)/../regformats/regd >> =A0regdef_h =3D $(srcdir)/../regformats/regdef.h >> =A0regcache_h =3D $(srcdir)/regcache.h >> =A0server_h =3D $(srcdir)/server.h $(regcache_h) config.h $(srcdir)/targ= et.h \ >> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 $(srcdir)/mem-break.h >> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 $(srcdir)/mem-break.h $(srcdir)/../common/= gdb_signals.h >> >> =A0hostio.o: hostio.c $(server_h) >> =A0hostio-errno.o: hostio-errno.c $(server_h) >> @@ -278,7 +279,7 @@ thread-db.o: thread-db.c $(server_h) $(g >> =A0utils.o: utils.c $(server_h) >> =A0gdbreplay.o: gdbreplay.c config.h >> >> -signals.o: ../signals/signals.c $(server_h) >> +signals.o: ../common/signals.c $(server_h) >> =A0 =A0 =A0 =A0$(CC) -c $(CPPFLAGS) $(INTERNAL_CFLAGS) $< -DGDBSERVER >> >> =A0memmem.o: ../gnulib/memmem.c >> Index: gdbserver/server.h >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> 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 =A019 Jan 2009 00:16:46 -0000 =A0 =A0 =A01.51 >> +++ gdbserver/server.h =A05 Feb 2009 20:24:15 -0000 >> @@ -110,7 +110,7 @@ struct dll_info >> >> =A0#include "regcache.h" >> =A0#include "gdb/signals.h" >> - >> +#include "gdb_signals.h" >> =A0#include "target.h" >> =A0#include "mem-break.h" >> >> @@ -265,12 +265,6 @@ void buffer_xml_printf (struct buffer *b >> =A0#define buffer_grow_str0(BUFFER,STRING) =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0\ >> =A0 buffer_grow (BUFFER, STRING, strlen (STRING) + 1) >> >> -/* Functions from ``signals.c''. =A0*/ >> -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); >> - >> =A0/* Functions from utils.c */ >> >> =A0void *xmalloc (size_t) ATTR_MALLOC; >> >> >> >> diff -u -p common/server.c >> --- signals/signals.c =A0 2009-01-15 14:07:20.000000000 -0800 >> +++ common/signals.c =A0 =A02009-02-05 13:15:43.000000000 -0800 >> @@ -23,7 +23,6 @@ >> =A0#include "server.h" >> =A0#else >> =A0#include "defs.h" >> -#include "target.h" >> =A0#include "gdb_string.h" >> =A0#endif >> >> @@ -31,6 +30,8 @@ >> =A0#include >> =A0#endif >> >> +#include "gdb_signals.h" >> + >> =A0struct gdbarch; >> >> =A0/* Always use __SIGRTMIN if it's available. =A0SIGRTMIN is the lowest >> @@ -807,6 +808,8 @@ target_signal_to_host (enum target_signa >> =A0 =A0 return targ_signo; >> =A0} >> >> +#ifndef GDBSERVER >> + >> =A0/* In some circumstances we allow a command to specify a numeric >> =A0 =A0signal. =A0The idea is to keep these circumstances limited so that >> =A0 =A0users (and scripts) develop portable habits. =A0For comparison, >> @@ -824,7 +827,6 @@ target_signal_from_command (int num) >> =A0Use \"info signals\" for a list of symbolic signals."); >> =A0} >> >> -#ifndef GDBSERVER >> =A0extern initialize_file_ftype _initialize_signals; /* -Wmissing-protot= ype */ >> >> =A0void >> @@ -833,7 +835,6 @@ _initialize_signals (void) >> =A0 if (strcmp (signals[TARGET_SIGNAL_LAST].string, "TARGET_SIGNAL_MAGIC= ") !=3D 0) >> =A0 =A0 internal_error (__FILE__, __LINE__, "failed internal consistency= check"); >> =A0} >> -#endif >> >> =A0int >> =A0default_target_signal_to_host (struct gdbarch *gdbarch, enum target_s= ignal ts) >> @@ -846,3 +847,5 @@ default_target_signal_from_host (struct >> =A0{ >> =A0 return target_signal_from_host (signo); >> =A0} >> + >> +#endif /* ! GDBSERVER */ >> >> >> >> diff -u -p common/gdb_signals.h >> --- /dev/null =A0 2007-10-18 09:27:25.000000000 -0700 >> +++ common/gdb_signals.h =A0 =A0 =A0 =A02009-02-05 11:58:47.000000000 -0= 800 >> @@ -0,0 +1,56 @@ >> +/* Target signal translation functions for GDB. >> + =A0 Copyright (C) 1990, 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998= , 1999, >> + =A0 2000, 2001, 2002, 2003, 2006, 2007, 2008, 2009 >> + =A0 Free Software Foundation, Inc. >> + =A0 Contributed by Cygnus Support. >> + >> + =A0 This file is part of GDB. >> + >> + =A0 This program is free software; you can redistribute it and/or modi= fy >> + =A0 it under the terms of the GNU General Public License as published = by >> + =A0 the Free Software Foundation; either version 3 of the License, or >> + =A0 (at your option) any later version. >> + >> + =A0 This program is distributed in the hope that it will be useful, >> + =A0 but WITHOUT ANY WARRANTY; without even the implied warranty of >> + =A0 MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. =A0See the >> + =A0 GNU General Public License for more details. >> + >> + =A0 You should have received a copy of the GNU General Public License >> + =A0 along with this program. =A0If not, see . =A0*/ >> + >> +#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 >> + =A0 targ_signal SIGNO has an equivalent ``host'' representation. =A0*/ >> +/* FIXME: cagney/1999-11-22: The name below was chosen in preference >> + =A0 to the shorter target_signal_p() because it is far less ambigious. >> + =A0 In this context ``target_signal'' refers to GDB's internal >> + =A0 representation of the target's set of signals while ``host signal'' >> + =A0 refers to the target operating system's signal. =A0Confused? =A0*/ >> +extern int target_signal_to_host_p (enum target_signal signo); >> + >> +/* Convert between host signal numbers and enum target_signal's. >> + =A0 target_signal_to_host() returns 0 and prints a warning() on GDB's >> + =A0 console if SIGNO has no equivalent host representation. =A0*/ >> +/* FIXME: cagney/1999-11-22: Here ``host'' is used incorrectly, it is >> + =A0 refering to the target operating system's signal numbering. >> + =A0 Similarly, ``enum target_signal'' is named incorrectly, ``enum >> + =A0 gdb_signal'' would probably be better as it is refering to GDB's >> + =A0 internal representation of a target operating system's signal. =A0= */ >> +extern enum target_signal target_signal_from_host (int); >> +extern int target_signal_to_host (enum target_signal); >> + >> +/* Return the string for a signal. =A0*/ >> +extern const char *target_signal_to_string (enum target_signal); >> + >> +/* Return the name (SIGHUP, etc.) for a signal. =A0*/ >> +extern const char *target_signal_to_name (enum target_signal); >> + >> +/* Given a name (SIGHUP, etc.), return its signal. =A0*/ >> +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? >