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

[Doug Evans <dje at google dot com>]

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?