Re: [PATCH] Move gdbserver to top level

[Pedro Alves <palves at redhat dot com>]

On 1/26/20 3:41 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> I think this should have a NEWS entry, given that it affects
> Pedro> people building standalone gdbserver today.  Do you have
> Pedro> documentation changes queued?
> 
> Nope, I've added those now.
> 
>>> +// Host modules specific to gdbserver.
>>> +dependencies = { module=configure-gdbserver; on=all-intl; };
> 
> Pedro> This surprised me, as I don't think gdbserver actually depends on
> Pedro> intl currently?  Might still be a good idea to actually enable it,
> Pedro> but I was surprised from an "as pure as possible" perspective.
> 
> I don't recall (I wrote this patch originally in May of last year), but
> maybe I just copied gdb's dependencies.  Anyway, I've dropped them now.

Thanks.

> 
> Tom
> 
> commit 4c5c9a7a0b64c98f60c2ab79ad705d14ea568d0f
> Author: Tom Tromey <tom@tromey.com>
> Date:   Sun Dec 15 07:37:06 2019 -0700
> 
>     Move gdbserver to top level
>     
>     This patch moves gdbserver to the top level.
>     
>     This patch is as close to a pure move as possible -- gdbserver still
>     builds its own variant of gnulib and gdbsupport.  Changing this will
>     be done in a separate patch.
>     
>     [v2] Note that, per Simon's review comment, this patch changes the
>     tree so that gdbserver is not built for or1k or score.  This makes
>     sense, because there is apparently not actually a gdbserver port here.
>     
>     [v3] This version of the patch also splits out some configury into a
>     new file, gdbserver/configure.host, so that the top-level configure
>     script can simply rely on it in order to decide whether gdbserver
>     should be built.
>     
>     [v4] This version adds documentation and removes some unnecessary
>     top-level dependencies.

Thanks.

> +# Only allow gdbserver on some systems.
> +if test -d ${srcdir}/gdbserver; then
> +    if test x$enable_gdbserver = x; then
> +        AC_MSG_CHECKING([for gdbserver support])
> +	. ${srcdir}/gdbserver/configure.host
> +	if test x$build_gdbserver = xyes; then
> +	    AC_MSG_RESULT([yes])
> +	else
> +            noconfigdirs="$noconfigdirs gdbserver"
> +	    AC_MSG_RESULT([no])
> +        fi
> +    fi
> +fi

Some tabs vs spaces mixup above.  (But see below.)

> --- a/gdb/gdbserver/README
> +++ b/gdbserver/README
> @@ -100,27 +100,24 @@ The supported targets as of November 2006 are:
>  	spu*-*-*
>  	x86_64-*-linux*
>  
> -Configuring GDBserver you should specify the same machine for host and
> -target (which are the machine that GDBserver is going to run on.  This
> -is not the same as the machine that GDB is going to run on; building
> -GDBserver automatically as part of building a whole tree of tools does
> -not currently work if cross-compilation is involved (we don't get the
> -right CC in the Makefile, to start with)).
> -
> -Building GDBserver for your target is very straightforward.  If you build
> -GDB natively on a target which GDBserver supports, it will be built
> +Building GDBserver for your host is very straightforward.  If you build
> +GDB natively on a host which GDBserver supports, it will be built
>  automatically when you build GDB.  You can also build just GDBserver:
>  
>  	% mkdir obj
>  	% cd obj
> -	% path-to-gdbserver-sources/configure
> +	% path-to-toplevel-sources/configure --disable-gdb
>  	% make
>  
> +(If you have a combined binutils+gdb tree, you may want to also
> +disable other directories when configuring, e.g., binutils, gas, gold,
> +gprof, and ld.)
> +

On a git checkout, configuring with:

 configure \
 --disable-binutils \
 --disable-ld \
 --disable-gas \
 --disable-gprof \
 --disable-gold \
 --disable-gdb

and compiling with '$ make', still builds a number of top level directories
other than gdbserver.  We end up with this in the top level Makefile.in:

 SUBDIRS =  intl libiberty opcodes bfd readline zlib libdecnumber libctf sim gdbserver etc

So to build just gdbserver, you'd need to add a --disable-foo for each of those above
in addition to the ones already indicated, something like:

 configure \
 --disable-binutils \
 --disable-ld \
 --disable-gas \
 --disable-gprof \
 --disable-gold \
 --disable-gdb \
 --disable-intl \
 --disable-libiberty \
 --disable-opcodes \
 --disable-bfd \
 --disable-readline \
 --disable-zlib \
 --disable-libdecnumber \
 --disable-libctf \
 --disable-sim \
 --disable-etc


I guess it's the intended design for top level to build readline, bfd,
etc.  by default even if no application is being built that depends
on them.  I don't know.

People may trip on this if they try to build gdbserver for a port for which
one of those top level dirs doesn't build, like bfd.  powerpc-lynxos is 
I think one such case, but there are probably others in the non-glibc space,
and also off tree.

However, a configure command, followed by "make all-gdbserver" builds only
gdbserver and its required dependencies (which are none at the moment).

So I'm thinking that it might be better to document "make all-gdbserver" instead
of the --disable approach.  Or at least, mention it as alternative.  WDYT?


>  If you prefer to cross-compile to your target, then you can also build
>  GDBserver that way.  In a Bourne shell, for example:
>  
>  	% export CC=your-cross-compiler
> -	% path-to-gdbserver-sources/configure your-target-name
> +	% path-to-topevel-sources/configure your-target-name --disable-gdb
>  	% make
>  
>  Using GDBreplay:

> index 00000000000..86d3a80148d
> --- /dev/null
> +++ b/gdbserver/configure.host

I noticed something about this file that made me look a bit more
and I have a couple questions, including a patch.

I noticed that several directories already contain a file named
configure.host, including gdb:

 $ find . -name configure.host
 ./gdb/configure.host
 ./ld/configure.host
 ./gdbserver/configure.host
 ./bfd/configure.host

And in gdb's ld's and bfd's case, this file is sourced
by the respective directory's configure script, not by
the top level script.  If we wanted to migrate gdb's top
configure bits to source a new file from gdb/ to determine
whether gdb is supported, like gdbserver, we wouldn't
be able to call the new "do we support gdb on this host"
configure snippet file gdb/configure.host, since that
file already exists with a different purpose.  Likewise
for ld and bfd.  So, with that in mind, should we name
gdbserver's new configure.host file to something else?

However, looking at how libatomic, liboffloadmic, etc.
handle this at the top level, I see that the top level
sources a file that is also sourced by their respective
configures.  I.e., the file sourced served a dual purpose.
>From the top level, the file is sourced in subshell, which
avoids variable polluting the top level.  This has the advantage
that you only have to write support for a port once in one file,
instead of in two places.

The equivalent for gdbserver would be the patch below,
which seems to work well.  Was there a reason you didn't follow
libatomic's (etc.) model?

>From 0a8e011249c5d887886b1e06d5b3b774bd6f9d10 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 27 Jan 2020 12:53:10 +0000
Subject: [PATCH] UNSUPPORTED

---
 configure                |  18 ++++----
 configure.ac             |  16 ++++---
 gdbserver/configure.host | 111 -----------------------------------------------
 gdbserver/configure.srv  |  12 +++--
 4 files changed, 27 insertions(+), 130 deletions(-)
 delete mode 100644 gdbserver/configure.host

diff --git a/configure b/configure
index c31220e8e75..8a3e7026f0b 100755
--- a/configure
+++ b/configure
@@ -3541,17 +3541,19 @@ esac
 # Only allow gdbserver on some systems.
 if test -d ${srcdir}/gdbserver; then
     if test x$enable_gdbserver = x; then
-        { $as_echo "$as_me:${as_lineno-$LINENO}: checking for gdbserver support" >&5
+	{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for gdbserver support" >&5
 $as_echo_n "checking for gdbserver support... " >&6; }
-	. ${srcdir}/gdbserver/configure.host
-	if test x$build_gdbserver = xyes; then
-	    { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5
-$as_echo "yes" >&6; }
-	else
-            noconfigdirs="$noconfigdirs gdbserver"
+	if (srcdir=${srcdir}/gdbserver; \
+		. ${srcdir}/configure.srv; \
+		test -n "$UNSUPPORTED")
+	then
 	    { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5
 $as_echo "no" >&6; }
-        fi
+	    noconfigdirs="$noconfigdirs gdbserver"
+	else
+	    { $as_echo "$as_me:${as_lineno-$LINENO}: result: yes" >&5
+$as_echo "yes" >&6; }
+	fi
     fi
 fi
 
diff --git a/configure.ac b/configure.ac
index 40669228e3c..35a9c1867d2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -785,14 +785,16 @@ esac
 # Only allow gdbserver on some systems.
 if test -d ${srcdir}/gdbserver; then
     if test x$enable_gdbserver = x; then
-        AC_MSG_CHECKING([for gdbserver support])
-	. ${srcdir}/gdbserver/configure.host
-	if test x$build_gdbserver = xyes; then
-	    AC_MSG_RESULT([yes])
-	else
-            noconfigdirs="$noconfigdirs gdbserver"
+	AC_MSG_CHECKING([for gdbserver support])
+	if (srcdir=${srcdir}/gdbserver; \
+		. ${srcdir}/configure.srv; \
+		test -n "$UNSUPPORTED")
+	then
 	    AC_MSG_RESULT([no])
-        fi
+	    noconfigdirs="$noconfigdirs gdbserver"
+	else
+	    AC_MSG_RESULT([yes])
+	fi
     fi
 fi
 
diff --git a/gdbserver/configure.host b/gdbserver/configure.host
deleted file mode 100644
index 86d3a80148d..00000000000
--- a/gdbserver/configure.host
+++ /dev/null
@@ -1,111 +0,0 @@
-# Configure helper for gdbserver
-# Copyright (C) 2000-2020 Free Software Foundation, Inc.
-#
-# 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/>.
-
-# Source this to determine whether gdbserver can be built for a given
-# host.  This will set the variable "build_gdbserver".  The variable
-# "host" must be set before sourcing.
-
-build_gdbserver=
-case "$host" in
-aarch64*-*-linux*)
-	# Target: AArch64 linux
-	build_gdbserver=yes
-	;;
-
-arm*-wince-pe | arm*-*-mingw32ce*)
-	# Target: ARM based machine running Windows CE (win32)
-	build_gdbserver=yes
-	;;
-arm*-*-linux*)
-	# Target: ARM based machine running GNU/Linux
-	build_gdbserver=yes
-	;;
-bfin-*-*linux*)
-	# Target: Blackfin Linux
-	gdb_target_obs="bfin-tdep.o bfin-linux-tdep.o linux-tdep.o"
-	build_gdbserver=yes
-	;;
-i[34567]86-*-nto*)
-	# Target: Intel 386 running qnx6.
-	build_gdbserver=yes
-	;;
-i[34567]86-*-linux*)
-	# Target: Intel 386 running GNU/Linux
-	build_gdbserver=yes
-	;;
-i[34567]86-*-cygwin*)
-	# Target: Intel 386 running win32
-	build_gdbserver=yes
-	;;
-i[34567]86-*-mingw32*)
-	# Target: Intel 386 running win32
-	build_gdbserver=yes
-	;;
-ia64-*-linux*)
-	# Target: Intel IA-64 running GNU/Linux
-	build_gdbserver=yes
-	;;
-m32r*-*-linux*)
-	# Target: Renesas M32R running GNU/Linux
-	build_gdbserver=yes
-	;;
-m68*-*-linux*)
-	# Target: Motorola m68k with a.out and ELF
-	build_gdbserver=yes
-	;;
-mips*-*-linux*)
-	# Target: Linux/MIPS
-	build_gdbserver=yes
-	;;
-powerpc*-*-linux*)
-	# Target: PowerPC running Linux
-	build_gdbserver=yes
-	;;
-s390*-*-linux*)
-	# Target: S390 running Linux
-	build_gdbserver=yes
-	;;
-sh*-*-linux*)
-	# Target: GNU/Linux Super-H
-	build_gdbserver=yes
-	;;
-sparc-*-linux*)
-	# Target: GNU/Linux SPARC
-	build_gdbserver=yes
-	;;
-sparc64-*-linux*)
-	# Target: GNU/Linux UltraSPARC
-	build_gdbserver=yes
-	;;
-tilegx-*-linux*)
-	# Target: TILE-Gx
-	build_gdbserver=yes
-	;;
-x86_64-*-linux*)
-	# Target: GNU/Linux x86-64
-	build_gdbserver=yes
-	;;
-x86_64-*-mingw* | x86_64-*-cygwin*)
-        # Target: MingW/amd64
-	build_gdbserver=yes
-        ;;
-xtensa*-*-*linux*)
-	# Target: GNU/Linux Xtensa
-	build_gdbserver=yes
-	;;
-esac
diff --git a/gdbserver/configure.srv b/gdbserver/configure.srv
index 8437c1ace06..52264be8ecd 100644
--- a/gdbserver/configure.srv
+++ b/gdbserver/configure.srv
@@ -1,6 +1,8 @@
 # Mappings from configuration triplets to gdbserver build options.
 # This is invoked from the autoconf-generated configure script, to
 # produce the appropriate Makefile substitutions.
+# It is also sourced by the top level configure script, to determine
+# whether gdbserver is supported on a given host.
 
 # This file sets the following shell variables:
 #   srv_regobj		The register protocol appropriate for this target.
@@ -12,6 +14,7 @@
 #			gdbserver in this configuration.
 #   ipa_obj		Any other target-specific modules appropriate
 #			for this target's in-process agent.
+#   UNSUPPORTED         Set to 1 if the host is unsupported.
 #
 # In addition, on GNU/Linux the following shell variables will be set:
 #   srv_linux_regsets	Set to "yes" if ptrace(PTRACE_GETREGS) and friends
@@ -30,9 +33,9 @@ ipa_ppc_linux_regobj="powerpc-32l-ipa.o powerpc-altivec32l-ipa.o powerpc-vsx32l-
 # these files over and over again.
 srv_linux_obj="linux-low.o nat/linux-osdata.o nat/linux-procfs.o nat/linux-ptrace.o nat/linux-waitpid.o nat/linux-personality.o nat/linux-namespaces.o fork-child.o nat/fork-inferior.o"
 
-# Input is taken from the "${target}" variable.
+# Input is taken from the "${host}" variable.
 
-case "${target}" in
+case "${host}" in
   aarch64*-*-linux*)	srv_tgtobj="linux-aarch64-low.o"
 			srv_tgtobj="$srv_tgtobj nat/aarch64-linux-hw-point.o"
 			srv_tgtobj="$srv_tgtobj linux-aarch32-low.o"
@@ -395,7 +398,8 @@ case "${target}" in
 			srv_linux_regsets=yes
 			srv_linux_thread_db=yes
 			;;
-  *)			echo "Error: target not supported by gdbserver."
-			exit 1
+  *)
+			# Who are you?
+			UNSUPPORTED=1
 			;;
 esac

base-commit: 4c5c9a7a0b64c98f60c2ab79ad705d14ea568d0f
-- 
2.14.5