Re: [rfa/rfc] Build libcommon.a for gdb and gdbserver

[Yao Qi <yao@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 2066 bytes --]

On 02/24/2011 12:33 AM, Tom Tromey wrote:
> Since then I have been wondering why we need build infrastructure in
> common/ at all.  It seems to me that it can cause problems, but
> conversely doesn't provide much benefit.
> 

I assume the problems here are C macros confusion/conflict, mentioned in
http://sourceware.org/ml/gdb-patches/2011-02/msg00466.html

AFAICS, the conflict can not happen.  When building libcommon, config.h
is used from parent directory (gdb or gdbserver).  GDB_INCLUDE in
common/configure.ac makes sure correct directories are included.

The only problem I can see is this:

    #ifdef GDBSERVER
    #include "server.h"
    #else
    #include "defs.h"
    #include "gdb_string.h"
    #endif

#ifndef GDBSERVER
enum target_signal
target_signal_from_command (int num)
...
#endif

However, IMO, it is not a configure/make problem.  The real problem here
is we include some gdb-specific code in common.  We can fix this problem
by moving gdb-specific part to gdb/ dir.  Patch attached is for this
purpose.

> My reasoning is based on the fact that we are going to be building two
> libraries for the foreseeable future.  It seems to me that it would be
> simpler to just integrate the common/ build rules into
> gdbserver/Makefile.in and gdb/Makefile.in.
> 
> What do you think of that?  I may write a patch to do it.

The goal of this piece of work is to remove duplicated source code, and
merge them together.  However, the complexity of configure/make is
underestimated.  I am not objecting to Tom's approach, because it is simple.

Personally, I still prefer a separated configure/makefile in common/,
because,
1.  if my patch works, configure/make is not a problem,
2.  if we look forward, there should be quite a few *.c and *.h files in
common in the future.  Write rules in both gdb/Makefile.in and
gdbserver/Makefile.in doesn't scale.

I am sorry if this change makes some troubles, but it is a right
direction we have to go.  In short, please review my approach again, and
I am not objecting to Tom's approach.

-- 
Yao (齐尧)

[-- Attachment #2: gdb-specific-signals-0224.patch --]
[-- Type: text/x-patch, Size: 5892 bytes --]

gdb/

	* common/gdb_signals.h (struct gdb_signal_desc): New.
	* common/signals.c: Move gdb-specific part to ...
	* signals.c: ... here.  New.
	* Makefile.in (COMMON_OBS): Add signals.o
 
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index c6049fa..7ce68ff 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -866,6 +866,7 @@ COMMON_OBS = $(DEPFILES) $(CONFIG_OBS) $(YYOBJ) \
 	memattr.o mem-break.o target.o parse.o language.o buildsym.o \
 	findcmd.o \
 	std-regs.o \
+	signals.o \
 	exec.o reverse.o \
 	bcache.o objfiles.o observer.o minsyms.o maint.o demangle.o \
 	dbxread.o coffread.o coff-pe-read.o \
diff --git a/gdb/common/gdb_signals.h b/gdb/common/gdb_signals.h
index c04d795..09dd120 100644
--- a/gdb/common/gdb_signals.h
+++ b/gdb/common/gdb_signals.h
@@ -24,6 +24,12 @@
 
 #include "gdb/signals.h"
 
+struct gdb_signal_desc
+{
+  const char *name;
+  const char *string;
+};
+
 /* 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
diff --git a/gdb/common/signals.c b/gdb/common/signals.c
index 3c7ffe4..f87842b 100644
--- a/gdb/common/signals.c
+++ b/gdb/common/signals.c
@@ -19,13 +19,9 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
-#ifdef GDBSERVER
-#include "server.h"
-#else
-#include "defs.h"
-#include "gdb_string.h"
-#endif
+#include <stdio.h>
 
+#include "../config.h"
 #ifdef HAVE_SIGNAL_H
 #include <signal.h>
 #endif
@@ -51,10 +47,7 @@ struct gdbarch;
 /* This table must match in order and size the signals in enum
    target_signal.  */
 
-static const struct {
-  const char *name;
-  const char *string;
-  } signals [] =
+const struct gdb_signal_desc signals [] =
 {
 #define SET(symbol, constant, name, string) { name, string },
 #include "gdb/signals.def"
@@ -651,45 +644,3 @@ target_signal_to_host (enum target_signal oursig)
   else
     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,
-   POSIX.2 `kill' requires that 1,2,3,6,9,14, and 15 work (and using a
-   numeric signal at all is obsolescent.  We are slightly more
-   lenient and allow 1-15 which should match host signal numbers on
-   most systems.  Use of symbolic signal names is strongly encouraged.  */
-
-enum target_signal
-target_signal_from_command (int num)
-{
-  if (num >= 1 && num <= 15)
-    return (enum target_signal) num;
-  error ("Only signals 1-15 are valid as numeric signals.\n\
-Use \"info signals\" for a list of symbolic signals.");
-}
-
-extern initialize_file_ftype _initialize_signals; /* -Wmissing-prototype */
-
-void
-_initialize_signals (void)
-{
-  if (strcmp (signals[TARGET_SIGNAL_LAST].string, "TARGET_SIGNAL_MAGIC") != 0)
-    internal_error (__FILE__, __LINE__, "failed internal consistency check");
-}
-
-int
-default_target_signal_to_host (struct gdbarch *gdbarch, enum target_signal ts)
-{
-  return target_signal_to_host (ts);
-}
-
-enum target_signal
-default_target_signal_from_host (struct gdbarch *gdbarch, int signo)
-{
-  return target_signal_from_host (signo);
-}
-
-#endif /* ! GDBSERVER */
diff --git a/gdb/signals.c b/gdb/signals.c
new file mode 100644
index 0000000..512fcfb
--- /dev/null
+++ b/gdb/signals.c
@@ -0,0 +1,64 @@
+/* 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, 2010, 2011
+   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/>.  */
+
+#include "defs.h"
+#include "gdb_string.h"
+#include "gdb_signals.h"
+
+extern struct gdb_signal_desc *signals;
+
+/* 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,
+   POSIX.2 `kill' requires that 1,2,3,6,9,14, and 15 work (and using a
+   numeric signal at all is obsolescent.  We are slightly more
+   lenient and allow 1-15 which should match host signal numbers on
+   most systems.  Use of symbolic signal names is strongly encouraged.  */
+
+enum target_signal
+target_signal_from_command (int num)
+{
+  if (num >= 1 && num <= 15)
+    return (enum target_signal) num;
+  error ("Only signals 1-15 are valid as numeric signals.\n\
+Use \"info signals\" for a list of symbolic signals.");
+}
+
+extern initialize_file_ftype _initialize_signals; /* -Wmissing-prototype */
+
+void
+_initialize_signals (void)
+{
+  if (strcmp (signals[TARGET_SIGNAL_LAST].string, "TARGET_SIGNAL_MAGIC") != 0)
+    internal_error (__FILE__, __LINE__, "failed internal consistency check");
+}
+
+int
+default_target_signal_to_host (struct gdbarch *gdbarch, enum target_signal ts)
+{
+  return target_signal_to_host (ts);
+}
+
+enum target_signal
+default_target_signal_from_host (struct gdbarch *gdbarch, int signo)
+{
+  return target_signal_from_host (signo);
+}