Re: [RFC] Create src/gdb/common, move signals.c there.

[teawater <teawater at gmail dot com>]

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 <dje@google.com> wrote:
> On Thu, Feb 5, 2009 at 1:52 PM, Doug Evans <dje@google.com> 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 ?<dje@google.com>
>>
>> ? ? ? ?* 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 <signal.h>
>> ?#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 <http://www.gnu.org/licenses/>. ?*/
>> +
>> +#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?
>