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

Yao Qi
2011-02-24T03:51:32Z

[-- 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);
+}