RFC: introduce common.m4

RFC: introduce common.m4

[Tom Tromey]

It has bothered me for a while that files in common/ use macros
defined via autoconf checks, but rely on each configure.ac doing the
proper checks independently.

This patch introduces common/common.m4 which consolidates the checks
assumed by code in common.

The rule I propose is that if something is needed or used by common,
it should be checked for by common.m4; but that code outside this
directory also be free to use these results.  This means that removing
checks from common.m4 must first be preceded by looking at uses in gdb
and gdbserver.  I think this is pretty easy to do -- easier than what
we are doing now -- and I have documented the requirement.

This process revealed a few things not checked for in gdbserver
(nothing too crucial I think) and also that the decl check for getopt
was dead.

I wrote GDB_AC_COMMON by looking for HAVE_ symbols in common/*.[ch].
I did not look for library dependencies, so it is possible that
something is missing.  I think it is worthwhile to have this in place
now, even if it is not perfect, because it gives us a place to make
future improvements.

Built and regtested on x86-64 Fedora 18 (though this is hardly the
most strenuous case) and using the Fedora 18 mingw cross compilers.  I
also examined the config.in diffs to ensure that symbols did not go
missing.

Tom


gdb:
	* acinclude.m4: Include common.m4.
	* common/common.m4: New file.
	* configure, config.in: Rebuild.
	* configure: Move some checks to common.m4.  Use GDB_AC_COMMON.

gdbserver:
	* acinclude.m4: Include common.m4, codeset.m4.
	* configure, config.in: Rebuild.
	* configure: Move some checks to common.m4.  Use GDB_AC_COMMON.
---
 gdb/acinclude.m4           |   2 +
 gdb/common/common.m4       |  37 +++
 gdb/config.in              |   4 -
 gdb/configure              | 591 ++++++++++++++++++++++++++++++++++++++++-----
 gdb/configure.ac           |  23 +-
 gdb/gdbserver/acinclude.m4 |   2 +
 gdb/gdbserver/config.in    |  18 ++
 gdb/gdbserver/configure    | 574 +++++++++++++++++++++++++++++++++++++++++--
 gdb/gdbserver/configure.ac |  16 +-
 9 files changed, 1166 insertions(+), 101 deletions(-)
 create mode 100644 gdb/common/common.m4

diff --git a/gdb/acinclude.m4 b/gdb/acinclude.m4
index 25caddd..21d1013 100644
--- a/gdb/acinclude.m4
+++ b/gdb/acinclude.m4
@@ -49,6 +49,8 @@ sinclude([../config/codeset.m4])
 
 sinclude([../config/zlib.m4])
 
+m4_include([common/common.m4])
+
 ## ----------------------------------------- ##
 ## ANSIfy the C compiler whenever possible.  ##
 ## From Franc,ois Pinard                     ##
diff --git a/gdb/common/common.m4 b/gdb/common/common.m4
new file mode 100644
index 0000000..facaaa8
--- /dev/null
+++ b/gdb/common/common.m4
@@ -0,0 +1,37 @@
+dnl Autoconf configure snippets for common.
+dnl Copyright (C) 1995-2013 Free Software Foundation, Inc.
+dnl
+dnl This file is part of GDB.
+dnl 
+dnl This program is free software; you can redistribute it and/or modify
+dnl it under the terms of the GNU General Public License as published by
+dnl the Free Software Foundation; either version 3 of the License, or
+dnl (at your option) any later version.
+dnl
+dnl This program is distributed in the hope that it will be useful,
+dnl but WITHOUT ANY WARRANTY; without even the implied warranty of
+dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+dnl GNU General Public License for more details.
+dnl
+dnl You should have received a copy of the GNU General Public License
+dnl along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+dnl Invoke configury needed by the files in 'common'.
+dnl Note that if something is not needed here, you must
+dnl first check whether it is also needed by gdb or gdbserver.
+dnl They are free to reuse results from tests performed here.
+AC_DEFUN([GDB_AC_COMMON], [
+  AC_HEADER_STDC
+  AC_HEADER_DIRENT
+  AC_FUNC_ALLOCA
+
+  AM_LANGINFO_CODESET
+
+  AC_CHECK_HEADERS(linux/perf_event.h locale.h memory.h signal.h dnl
+                   string.h strings.h sys/resource.h sys/un.h sys/wait.h dnl
+		   thread_db.h wait.h)
+
+  AC_CHECK_FUNCS([fdwalk getrlimit pipe pipe2 socketpair])
+
+  AC_CHECK_DECLS([strerror, strstr])
+])
diff --git a/gdb/configure.ac b/gdb/configure.ac
index bb7fbdd..3873e4b 100644
--- a/gdb/configure.ac
+++ b/gdb/configure.ac
@@ -1083,14 +1083,12 @@ AC_HEADER_DIRENT
 AC_HEADER_STDC
 # elf_hp.h is for HP/UX 64-bit shared library support.
 AC_CHECK_HEADERS([nlist.h machine/reg.h poll.h sys/poll.h proc_service.h \
-                  thread_db.h signal.h stddef.h \
-		  stdlib.h string.h memory.h strings.h sys/fault.h \
+                  stddef.h stdlib.h sys/fault.h \
 		  sys/file.h sys/filio.h sys/ioctl.h sys/param.h \
-		  sys/resource.h sys/procfs.h sys/ptrace.h ptrace.h \
+		   sys/procfs.h sys/ptrace.h ptrace.h \
 		  sys/reg.h sys/debugreg.h sys/select.h sys/syscall.h \
-		  sys/types.h sys/wait.h wait.h termios.h termio.h \
-		  sgtty.h unistd.h elf_hp.h locale.h \
-		  dlfcn.h sys/un.h linux/perf_event.h])
+		  sys/types.h termios.h termio.h \
+		  sgtty.h unistd.h elf_hp.h dlfcn.h])
 AC_CHECK_HEADERS(link.h, [], [],
 [#if HAVE_SYS_TYPES_H
 # include <sys/types.h>
@@ -1132,8 +1130,7 @@ AC_CHECK_HEADERS(term.h, [], [],
 # Checks for declarations.  #
 # ------------------------- #
 
-AC_CHECK_DECLS([free, malloc, realloc, strerror, strstr, getopt,
-                snprintf, vsnprintf])
+AC_CHECK_DECLS([free, malloc, realloc, snprintf, vsnprintf])
 AM_LC_MESSAGES
 
 # ----------------------- #
@@ -1168,13 +1165,13 @@ AC_FUNC_ALLOCA
 AC_FUNC_MMAP
 AC_FUNC_VFORK
 AC_CHECK_FUNCS([canonicalize_file_name realpath getrusage getuid getgid \
-		pipe poll pread pread64 pwrite readlink resize_term \
+		poll pread pread64 pwrite readlink resize_term \
 		sbrk setpgid setpgrp setsid \
-		sigaction sigprocmask sigsetmask socketpair syscall \
+		sigaction sigprocmask sigsetmask syscall \
 		ttrace wborder wresize setlocale iconvlist libiconvlist btowc \
-		setrlimit getrlimit posix_madvise waitpid lstat \
-		fdwalk pipe2])
-AM_LANGINFO_CODESET
+		setrlimit posix_madvise waitpid lstat])
+
+GDB_AC_COMMON
 
 # Check the return and argument types of ptrace.  No canned test for
 # this, so roll our own.
diff --git a/gdb/gdbserver/acinclude.m4 b/gdb/gdbserver/acinclude.m4
index 0e0bdc8..6d7b0e2 100644
--- a/gdb/gdbserver/acinclude.m4
+++ b/gdb/gdbserver/acinclude.m4
@@ -9,8 +9,10 @@ sinclude(../../config/override.m4)
 dnl For ACX_PKGVERSION and ACX_BUGURL.
 sinclude(../../config/acx.m4)
 
+m4_include(../../config/codeset.m4)
 m4_include(../../config/depstand.m4)
 m4_include(../../config/lead-dot.m4)
+m4_include(../common/common.m4)
 
 dnl Check for existence of a type $1 in libthread_db.h
 dnl Based on BFD_HAVE_SYS_PROCFS_TYPE in bfd/bfd.m4.
diff --git a/gdb/gdbserver/configure.ac b/gdb/gdbserver/configure.ac
index b9928d7..0b479f3 100644
--- a/gdb/gdbserver/configure.ac
+++ b/gdb/gdbserver/configure.ac
@@ -63,16 +63,16 @@ AC_PROG_MAKE_SET
 # build it in the same directory, when building in the source dir.
 ACX_CONFIGURE_DIR(["../gnulib"], ["build-gnulib-gdbserver"])
 
-AC_CHECK_HEADERS(sgtty.h termio.h termios.h sys/reg.h string.h dnl
-		 proc_service.h sys/procfs.h thread_db.h linux/elf.h dnl
-		 stdlib.h unistd.h dnl
-		 errno.h fcntl.h signal.h sys/file.h malloc.h dnl
+AC_CHECK_HEADERS(sgtty.h termio.h termios.h sys/reg.h dnl
+		 proc_service.h sys/procfs.h linux/elf.h dnl
+		 stdlib.h unistd.h errno.h fcntl.h sys/file.h malloc.h dnl
 		 sys/ioctl.h netinet/in.h sys/socket.h netdb.h dnl
-		 netinet/tcp.h arpa/inet.h sys/wait.h wait.h sys/un.h dnl
-		 linux/perf_event.h)
-AC_CHECK_FUNCS(pread pwrite pread64 readlink fdwalk pipe2)
+		 netinet/tcp.h arpa/inet.h)
+AC_CHECK_FUNCS(pread pwrite pread64 readlink)
 AC_REPLACE_FUNCS(vasprintf vsnprintf)
 
+GDB_AC_COMMON
+
 # Check for UST
 ustlibs=""
 ustinc=""
@@ -188,7 +188,7 @@ AC_TRY_LINK([
   [AC_MSG_RESULT(no)])
 fi
 
-AC_CHECK_DECLS([strerror, strstr, perror, vasprintf, vsnprintf])
+AC_CHECK_DECLS([perror, vasprintf, vsnprintf])
 
 AC_CHECK_TYPES(socklen_t, [], [],
 [#include <sys/types.h>
-- 
1.8.1.4

Re: RFC: introduce common.m4

[Pedro Alves]

On 04/24/2013 06:30 PM, Tom Tromey wrote:
> It has bothered me for a while that files in common/ use macros
> defined via autoconf checks, but rely on each configure.ac doing the
> proper checks independently.
> 
> This patch introduces common/common.m4 which consolidates the checks
> assumed by code in common.
> 
> The rule I propose is that if something is needed or used by common,
> it should be checked for by common.m4; but that code outside this
> directory also be free to use these results.  This means that removing
> checks from common.m4 must first be preceded by looking at uses in gdb
> and gdbserver.  I think this is pretty easy to do -- easier than what
> we are doing now -- and I have documented the requirement.

What's the advantage of doing it this way?  Caching?  Doesn't autoconf
use the cached value if there are multiple AC_CHECK_FOOs for the same
thing?  Not super sure I like this over keeping each directory aware
of its dependencies, but I suppose I can go along.

> This process revealed a few things not checked for in gdbserver
> (nothing too crucial I think) and also that the decl check for getopt
> was dead.

Ah, an "also".  ;-)

> I wrote GDB_AC_COMMON by looking for HAVE_ symbols in common/*.[ch].
> I did not look for library dependencies, so it is possible that
> something is missing.  I think it is worthwhile to have this in place
> now, even if it is not perfect, because it gives us a place to make
> future improvements.

I definitely agree with the idea of a common common.m4 in common/. :-)

> gdb:
> 	* acinclude.m4: Include common.m4.
> 	* common/common.m4: New file.
> 	* configure, config.in: Rebuild.
> 	* configure: Move some checks to common.m4.  Use GDB_AC_COMMON.

s/configure/configure.ac/


> 
> gdbserver:
> 	* acinclude.m4: Include common.m4, codeset.m4.

codeset.m4?

> 	* configure, config.in: Rebuild.
> 	* configure: Move some checks to common.m4.  Use GDB_AC_COMMON.

s/configure/configure.ac/


> +  AC_CHECK_HEADERS(linux/perf_event.h locale.h memory.h signal.h dnl
> +                   string.h strings.h sys/resource.h sys/un.h sys/wait.h dnl
> +		   thread_db.h wait.h)

Something odd with indentation here.

Mechanically looks good to me.  I don't plan on
checking/cross-checking what was moved in detail.

-- 
Pedro Alves

Re: RFC: introduce common.m4

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> What's the advantage of doing it this way?  Caching?  Doesn't autoconf
Pedro> use the cached value if there are multiple AC_CHECK_FOOs for the same
Pedro> thing?  Not super sure I like this over keeping each directory aware
Pedro> of its dependencies, but I suppose I can go along.

The advantage is in maintenance.  Right now one must remember to update
both gdb and gdbserver configure scripts in parallel.

>> This process revealed a few things not checked for in gdbserver
>> (nothing too crucial I think) and also that the decl check for getopt
>> was dead.

Pedro> Ah, an "also".  ;-)

Yeah.  I'll split it out.

>> gdbserver:
>> * acinclude.m4: Include common.m4, codeset.m4.

Pedro> codeset.m4?

It was needed for the AM_LANGINFO_CODESET call, which was needed for
HAVE_LANGINFO_CODESET.

Tom

Re: RFC: introduce common.m4

[Pedro Alves]

On 04/24/2013 07:58 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> What's the advantage of doing it this way?  Caching?  Doesn't autoconf
> Pedro> use the cached value if there are multiple AC_CHECK_FOOs for the same
> Pedro> thing?  Not super sure I like this over keeping each directory aware
> Pedro> of its dependencies, but I suppose I can go along.
> 
> The advantage is in maintenance.  Right now one must remember to update
> both gdb and gdbserver configure scripts in parallel.

I think you misunderstood the question.  Sorry if it wasn't clear.
The "this way" was referring to:

> The rule I propose is that if something is needed or used by common,
> it should be checked for by common.m4; but that code outside this
> directory also be free to use these results.  This means that removing
> checks from common.m4 must first be preceded by looking at uses in gdb
> and gdbserver.  I think this is pretty easy to do -- easier than what
> we are doing now -- and I have documented the requirement.

over keeping common aware of the checks it needs to do (in common.m4),
and gdb/ and gdbserver/ also doing the checks they need for code under
gdb/ and gdbserver/ respectively.

Of course the current status of needing to update gdb and gdbserver in
parallel for common/ things is no good.

-- 
Pedro Alves

Re: RFC: introduce common.m4

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> I think you misunderstood the question.

Indeed.  Sorry about that.

>> The rule I propose is that if something is needed or used by common,
>> it should be checked for by common.m4; but that code outside this
>> directory also be free to use these results.  This means that removing
>> checks from common.m4 must first be preceded by looking at uses in gdb
>> and gdbserver.  I think this is pretty easy to do -- easier than what
>> we are doing now -- and I have documented the requirement.

Pedro> over keeping common aware of the checks it needs to do (in common.m4),
Pedro> and gdb/ and gdbserver/ also doing the checks they need for code under
Pedro> gdb/ and gdbserver/ respectively.

Pedro> Of course the current status of needing to update gdb and gdbserver in
Pedro> parallel for common/ things is no good.

Yeah, there is no deep reason for it other than wanting to avoid
duplicate checks.  The cache would work fine for the performance aspect.
I can certainly just drop the configure.ac changes if you think that
yields a better result.

Tom

Re: RFC: introduce common.m4

[Pedro Alves]

On 04/24/2013 09:12 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> Pedro> I think you misunderstood the question.
> 
> Indeed.  Sorry about that.
> 
>>> The rule I propose is that if something is needed or used by common,
>>> it should be checked for by common.m4; but that code outside this
>>> directory also be free to use these results.  This means that removing
>>> checks from common.m4 must first be preceded by looking at uses in gdb
>>> and gdbserver.  I think this is pretty easy to do -- easier than what
>>> we are doing now -- and I have documented the requirement.
> 
> Pedro> over keeping common aware of the checks it needs to do (in common.m4),
> Pedro> and gdb/ and gdbserver/ also doing the checks they need for code under
> Pedro> gdb/ and gdbserver/ respectively.
> 
> Pedro> Of course the current status of needing to update gdb and gdbserver in
> Pedro> parallel for common/ things is no good.
> 
> Yeah, there is no deep reason for it other than wanting to avoid
> duplicate checks.  The cache would work fine for the performance aspect.
> I can certainly just drop the configure.ac changes if you think that
> yields a better result.

So, we have:

$ grep "HAVE_LINUX_PERF_EVENT_H\|HAVE_LOCALE_H\|HAVE_MEMORY_H\|HAVE_SIGNAL_H\|HAVE_STRING_H\|HAVE_STRINGS_H\|HAVE_SYS_RESOURCE_H\|HAVE_SYS_UN_H\|HAVE_SYS_WAIT_H\|HAVE_THREAD_DB_H\|HAVE_WAIT_H" gdb/*.[ch] gdb/gdbserver/*.[ch]
gdb/m32c-tdep.c:#if defined (HAVE_STRING_H)
gdb/utils.c:#ifdef HAVE_SYS_RESOURCE_H
gdb/utils.c:#endif /* HAVE_SYS_RESOURCE_H */
gdb/gdbserver/gdbreplay.c:#if HAVE_SIGNAL_H
gdb/gdbserver/gdbreplay.c:#ifdef HAVE_STRING_H
gdb/gdbserver/remote-utils.c:#if HAVE_SIGNAL_H
gdb/gdbserver/server.c:#if HAVE_SIGNAL_H
gdb/gdbserver/server.h:#ifdef HAVE_STRING_H

This actually reveals that m32c-tdep.c, gdbreplay.c and server.h
should be including gdb_string.h (or nothing) instead of signal.h directly.

That'd leave a check for sys/resource.h in gdb, and
a check for signal.h in gdbserver.

$ grep "HAVE_DECL_STRERROR\|HAVE_DECL_STRSTR" gdb/*.[ch] gdb/gdbserver/*.[ch]
gdb/gdbserver/server.h:#if !HAVE_DECL_STRERROR

shows that server.h is doing what gdb_string.h already does
with that HAVE_DECL_STRERROR check, so that bit could be
removed when server.h is made to include gdb_string.h.

And:

$ grep "HAVE_FDWALK\|HAVE_GETRLIMIT\|HAVE_PIPE\|HAVE_PIPE2\|HAVE_SOCKETPAIR" gdb/*.[ch] gdb/gdbserver/*.[ch]
gdb/ser-pipe.c:#if !HAVE_SOCKETPAIR
gdb/ser-pipe.c:#if !HAVE_SOCKETPAIR
gdb/utils.c:#ifdef HAVE_GETRLIMIT
gdb/utils.c:#endif /* HAVE_GETRLIMIT */

Shows we would leave checks for socketpair/getrlimit in gdb/.

IMO, it's a little better if each subdirectory treats the
others more as black boxes.  gdb/ relying on common/'s
HAVE_FOO checks feels like gdb/ relying on common/'s
implementation details to me.  But I don't want to impose.

Thanks,
-- 
Pedro Alves

Re: RFC: introduce common.m4

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

[ old-ish thread... ]

Pedro> IMO, it's a little better if each subdirectory treats the
Pedro> others more as black boxes.  gdb/ relying on common/'s
Pedro> HAVE_FOO checks feels like gdb/ relying on common/'s
Pedro> implementation details to me.  But I don't want to impose.

Yeah, I agree.  When I refresh this patch I will do it this way.

Lately I have been thinking that common and gdbserver should be
top-level directories (after renaming "common" something more suitable).
This would let us use libiberty in gdbserver while still preserving, I
think, the ability to build gdbserver separately.  Also it would let us
treat "common" as a true library, not as the odd beast it is today.

Perhaps gnulib would also have to be pushed up.

Tom

Re: RFC: introduce common.m4

[Pedro Alves]

On 07/22/2013 06:48 PM, Tom Tromey wrote:
>>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:
> 
> [ old-ish thread... ]
> 
> Pedro> IMO, it's a little better if each subdirectory treats the
> Pedro> others more as black boxes.  gdb/ relying on common/'s
> Pedro> HAVE_FOO checks feels like gdb/ relying on common/'s
> Pedro> implementation details to me.  But I don't want to impose.
> 
> Yeah, I agree.  When I refresh this patch I will do it this way.
> 
> Lately I have been thinking that common and gdbserver should be
> top-level directories (after renaming "common" something more suitable).
> This would let us use libiberty in gdbserver while still preserving, I
> think, the ability to build gdbserver separately.  Also it would let us
> treat "common" as a true library, not as the odd beast it is today.

Yeah, that crossed my mind before too.  But, it's not really necessary
for libiberty in gdbserver, given we could use the same trick
we use for gnulib (ACX_CONFIGURE_DIR).  With that in mind, such a
move seems more trouble than it's worth it to me, at least for now
as long as we're using cvs+modules.

> Perhaps gnulib would also have to be pushed up.

-- 
Pedro Alves