Re: [PATCH] Fix PR bootstrap/42798

Re: [PATCH] Fix PR bootstrap/42798

[Ralf Wildenhues]

* Paolo Bonzini wrote on Wed, Jun 02, 2010 at 02:15:51PM CEST:
> Yes, that's fine.  Just commit config/ changes to gcc and src first,
> and everything else afterwards in a separate commit.

Committed to gcc and synced to src:
(this patch causes minor trivial non-semantic changes in regenerated
configure scripts which I will address in a followup.)

Cheers,
Ralf

2010-06-03  Joern Rennecke <joern.rennecke@embecosm.com>
	    Ralf Wildenhues  <Ralf.Wildenhues@gmx.de>

	PR bootstrap/42798
	* override.m4 (_AC_CHECK_DECL_BODY, _AC_CHECK_DECLS): Import
	definitions from git Autoconf.

Index: config/override.m4
===================================================================
--- config/override.m4	(revision 160209)
+++ config/override.m4	(working copy)
@@ -272,3 +272,44 @@
   [m4_define([_$0_text], [$1])m4_builtin([m4wrap],
     [m4_default(m4_defn([_$0_text])m4_undefine([_$0_text]))])])])
 ])
+
+m4_version_prereq([2.66],, [
+dnl We need AC_CHECK_DECL which works for overloaded C++ functions.
+
+# _AC_CHECK_DECL_BODY
+# -------------------
+# Shell function body for AC_CHECK_DECL.
+m4_define([_AC_CHECK_DECL_BODY],
+[  AS_LINENO_PUSH([$[]1])
+  [as_decl_name=`echo $][2|sed 's/ *(.*//'`]
+  [as_decl_use=`echo $][2|sed -e 's/(/((/' -e 's/)/) 0&/' -e 's/,/) 0& (/g'`]
+  AC_CACHE_CHECK([whether $as_decl_name is declared], [$[]3],
+  [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([$[]4],
+[@%:@ifndef $[]as_decl_name
+@%:@ifdef __cplusplus
+  (void) $[]as_decl_use;
+@%:@else
+  (void) $[]as_decl_name;
+@%:@endif
+@%:@endif
+])],
+		   [AS_VAR_SET([$[]3], [yes])],
+		   [AS_VAR_SET([$[]3], [no])])])
+  AS_LINENO_POP
+])# _AC_CHECK_DECL_BODY
+
+# _AC_CHECK_DECLS(SYMBOL, ACTION-IF_FOUND, ACTION-IF-NOT-FOUND,
+#                 INCLUDES)
+# -------------------------------------------------------------
+# Helper to AC_CHECK_DECLS, which generates the check for a single
+# SYMBOL with INCLUDES, performs the AC_DEFINE, then expands
+# ACTION-IF-FOUND or ACTION-IF-NOT-FOUND.
+m4_define([_AC_CHECK_DECLS],
+[AC_CHECK_DECL([$1], [ac_have_decl=1], [ac_have_decl=0], [$4])]dnl
+[AC_DEFINE_UNQUOTED(AS_TR_CPP(m4_bpatsubst(HAVE_DECL_[$1],[ *(.*])),
+  [$ac_have_decl],
+  [Define to 1 if you have the declaration of `$1',
+   and to 0 if you don't.])]dnl
+[m4_ifvaln([$2$3], [AS_IF([test $ac_have_decl = 1], [$2], [$3])])])
+
+])

Re: [PATCH] Fix PR bootstrap/42798

[Ralf Wildenhues]

* Ralf Wildenhues wrote on Thu, Jun 03, 2010 at 08:33:22AM CEST:
> * Paolo Bonzini wrote on Wed, Jun 02, 2010 at 02:15:51PM CEST:
> > Yes, that's fine.  Just commit config/ changes to gcc and src first,
> > and everything else afterwards in a separate commit.
> 
> Committed to gcc and synced to src:
> (this patch causes minor trivial non-semantic changes in regenerated
> configure scripts which I will address in a followup.)

This is the followup patch for src.  Tested by building binutils+gdb,
I'm starting a combined tree with --enable-build-with-cxx now.
Ok to commit if no problems show up?

A note to the gold change: I've looked at git glibc headers, of the
declarations tested only 'basename' was overloaded.  If more are added
in the future, the configure.ac files need to be adjusted to give
parameters for them, too.  This applies to the whole tree of course for
--enable-build-with-cxx, not just for gold.

Thanks,
Ralf

bfd/ChangeLog:
2010-06-03  Ralf Wildenhues  <Ralf.Wildenhues@gmx.de>

	* configure.in: Check for declaration of 'basename(char *)',
        * configure: Regenerate.
	* config.in: Regenerate.

binutils/ChangeLog:
2010-06-03  Ralf Wildenhues  <Ralf.Wildenhues@gmx.de>

	* configure: Regenerate.

gas/ChangeLog:
2010-06-03  Ralf Wildenhues  <Ralf.Wildenhues@gmx.de>

	* configure: Regenerate.

gold/ChangeLog:
2010-06-03  Ralf Wildenhues  <Ralf.Wildenhues@gmx.de>

	* configure.ac: Remove AC_CHECK_DECLS done with the C compiler.
	With the C++ compiler, check for declaration of 'basename(char *)'.
        * configure: Regenerate.
	* config.in: Regenerate.

libiberty/ChangeLog:
2010-06-03  Joern Rennecke <joern.rennecke@embecosm.com>
	    Ralf Wildenhues  <Ralf.Wildenhues@gmx.de>

	PR bootstrap/42798
	* configure.ac: Check for declaration of 'basename(char *)'.
	* configure: Regenerate.

opcodes/ChangeLog:
2010-06-03  Ralf Wildenhues  <Ralf.Wildenhues@gmx.de>

	* configure.in: Check for declaration of 'basename(char *)',
        * configure: Regenerate.
	* config.in: Regenerate.

Index: bfd/configure.in
===================================================================
RCS file: /cvs/src/src/bfd/configure.in,v
retrieving revision 1.285
diff -u -r1.285 configure.in
--- bfd/configure.in	25 May 2010 14:12:37 -0000	1.285
+++ bfd/configure.in	3 Jun 2010 06:48:45 -0000
@@ -194,7 +194,7 @@
 AC_CHECK_FUNCS(fcntl getpagesize setitimer sysconf fdopen getuid getgid fileno)
 AC_CHECK_FUNCS(strtoull)
 
-AC_CHECK_DECLS(basename)
+AC_CHECK_DECLS([basename(char *)])
 AC_CHECK_DECLS(ftello)
 AC_CHECK_DECLS(ftello64)
 AC_CHECK_DECLS(fseeko)
Index: gold/configure.ac
===================================================================
RCS file: /cvs/src/src/gold/configure.ac,v
retrieving revision 1.56
diff -u -r1.56 configure.ac
--- gold/configure.ac	27 Apr 2010 14:12:32 -0000	1.56
+++ gold/configure.ac	3 Jun 2010 06:49:03 -0000
@@ -376,19 +376,13 @@
 AC_SEARCH_LIBS(zlibVersion, z, [AC_CHECK_HEADERS(zlib.h)])
 AM_CONDITIONAL(HAVE_ZLIB, test "$ac_cv_search_zlibVersion" != "no")
 
-dnl We have to check these in C, not C++, because autoconf generates
-dnl tests which have no type information, and current glibc provides
-dnl multiple declarations of functions like basename when compiling
-dnl with C++.
-AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf, strverscmp])
-
 AC_LANG_PUSH(C++)
 
 AC_CHECK_HEADERS(tr1/unordered_set tr1/unordered_map)
 AC_CHECK_HEADERS(ext/hash_map ext/hash_set)
 AC_CHECK_HEADERS(byteswap.h)
 AC_CHECK_FUNCS(mallinfo posix_fallocate readv sysconf times)
-AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf, strverscmp, strndup, memmem])
+AC_CHECK_DECLS([basename(char *), ffs, asprintf, vasprintf, snprintf, vsnprintf, strverscmp, strndup, memmem])
 
 # Use of ::std::tr1::unordered_map::rehash causes undefined symbols
 # at link time with some versions of GCC.
Index: libiberty/configure.ac
===================================================================
RCS file: /cvs/src/src/libiberty/configure.ac,v
retrieving revision 1.51
diff -u -r1.51 configure.ac
--- libiberty/configure.ac	5 Jan 2010 21:10:30 -0000	1.51
+++ libiberty/configure.ac	3 Jun 2010 06:49:11 -0000
@@ -379,7 +379,7 @@
     table times tmpnam \
     vasprintf vfprintf vprintf vsprintf \
     wait3 wait4 waitpid)
-  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf])
+  AC_CHECK_DECLS([basename(char *), ffs, asprintf, vasprintf, snprintf, vsnprintf])
   AC_DEFINE(HAVE_SYS_ERRLIST, 1, [Define if you have the sys_errlist variable.])
   AC_DEFINE(HAVE_SYS_NERR,    1, [Define if you have the sys_nerr variable.])
   AC_DEFINE(HAVE_SYS_SIGLIST, 1, [Define if you have the sys_siglist variable.])
@@ -663,7 +663,7 @@
     [AC_MSG_RESULT([no])])
 
   AC_CHECK_FUNCS($checkfuncs)
-  AC_CHECK_DECLS([basename, ffs, asprintf, vasprintf, snprintf, vsnprintf])
+  AC_CHECK_DECLS([basename(char *), ffs, asprintf, vasprintf, snprintf, vsnprintf])
   AC_CHECK_DECLS([calloc, getenv, getopt, malloc, realloc, sbrk])
   AC_CHECK_DECLS([strverscmp])
   libiberty_NEED_DECLARATION(canonicalize_file_name)
Index: opcodes/configure.in
===================================================================
RCS file: /cvs/src/src/opcodes/configure.in,v
retrieving revision 1.95
diff -u -r1.95 configure.in
--- opcodes/configure.in	25 Mar 2010 21:12:36 -0000	1.95
+++ opcodes/configure.in	3 Jun 2010 06:49:17 -0000
@@ -68,7 +68,7 @@
 
 AC_CHECK_HEADERS(string.h strings.h stdlib.h limits.h)
 
-AC_CHECK_DECLS([basename, stpcpy])
+AC_CHECK_DECLS([basename(char *), stpcpy])
 
 cgen_maint=no
 cgendir='$(srcdir)/../cgen'

Re: [PATCH] Fix PR bootstrap/42798

[Ian Lance Taylor]

Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:

> This is the followup patch for src.  Tested by building binutils+gdb,
> I'm starting a combined tree with --enable-build-with-cxx now.
> Ok to commit if no problems show up?
>
> A note to the gold change: I've looked at git glibc headers, of the
> declarations tested only 'basename' was overloaded.  If more are added
> in the future, the configure.ac files need to be adjusted to give
> parameters for them, too.  This applies to the whole tree of course for
> --enable-build-with-cxx, not just for gold.

With regard to the gold change.  What will happen if the system header
files provide only a declaration
    char *basename(const char *)
?

Since gold doesn't actually basename, another option here would be to
skip the check of the basename declaration entirely.  In fact, we
should consider dropping basename from libiberty; our code should in
general use lbasename anyhow.

Ian

Re: [PATCH] Fix PR bootstrap/42798

[Paolo Bonzini]

On 06/04/2010 04:59 PM, Ian Lance Taylor wrote:
> With regard to the gold change.  What will happen if the system header
> files provide only a declaration
>      char *basename(const char *)
> ?

I think it works, because C casts are allowed to do anything they want 
to constness.

Certainly both of the following compile fine:

char *f(const char *x) { }
typedef char *(*funtype) (char *g);
funtype gg = (char * (*) (char *)) f;


char *f(char *x) { }
typedef char *(*funtype) (const char *g);
funtype gg = (char * (*) (const char *)) f;


Paolo

Re: [PATCH] Fix PR bootstrap/42798

[Ralf Wildenhues]

* Paolo Bonzini wrote on Fri, Jun 04, 2010 at 05:04:15PM CEST:
> On 06/04/2010 04:59 PM, Ian Lance Taylor wrote:
> >With regard to the gold change.  What will happen if the system header
> >files provide only a declaration
> >     char *basename(const char *)
> >?
> 
> I think it works, because C casts are allowed to do anything they
> want to constness.

Well, the respective configure test for
  AC_CHECK_DECL([basename(char*)])

in C++ mode tries to compile something like
  (void) basename ((char*) 0);

it doesn't try to ensure that the declaration is the same as the one
you're using.  So in fact not using 'const' is safer that using it.
Hmm, maybe the Autoconf manual should be clearer about this.  Anyway,
this method only works for argument types for which a cast of 0 makes
sense.

Cheers,
Ralf

Re: [PATCH] Fix PR bootstrap/42798

[Ralf Wildenhues]

* Ian Lance Taylor wrote on Fri, Jun 04, 2010 at 04:59:01PM CEST:
> Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:
> 
> > This is the followup patch for src.  Tested by building binutils+gdb,
> > I'm starting a combined tree with --enable-build-with-cxx now.
> > Ok to commit if no problems show up?
> >
> > A note to the gold change: I've looked at git glibc headers, of the
> > declarations tested only 'basename' was overloaded.  If more are added
> > in the future, the configure.ac files need to be adjusted to give
> > parameters for them, too.  This applies to the whole tree of course for
> > --enable-build-with-cxx, not just for gold.
> 
> With regard to the gold change.  What will happen if the system header
> files provide only a declaration
>     char *basename(const char *)
> ?
> 
> Since gold doesn't actually basename, another option here would be to
> skip the check of the basename declaration entirely.  In fact, we
> should consider dropping basename from libiberty; our code should in
> general use lbasename anyhow.

Well, these are two additional changes on top of the one I posted though
and can easily be addressed in the future.  Meanwhile, a combined build
with maintainer-mode enabled will cause spurious changes to generated
files, and the patch is pretty minimal in that it fixes the issues but
doesn't change semantics otherwise.  So, ok to commit to src?

Thanks,
Ralf

Re: [PATCH] Fix PR bootstrap/42798

[Ian Lance Taylor]

Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:

> * Ian Lance Taylor wrote on Fri, Jun 04, 2010 at 04:59:01PM CEST:
>> Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:
>> 
>> > This is the followup patch for src.  Tested by building binutils+gdb,
>> > I'm starting a combined tree with --enable-build-with-cxx now.
>> > Ok to commit if no problems show up?
>> >
>> > A note to the gold change: I've looked at git glibc headers, of the
>> > declarations tested only 'basename' was overloaded.  If more are added
>> > in the future, the configure.ac files need to be adjusted to give
>> > parameters for them, too.  This applies to the whole tree of course for
>> > --enable-build-with-cxx, not just for gold.
>> 
>> With regard to the gold change.  What will happen if the system header
>> files provide only a declaration
>>     char *basename(const char *)
>> ?
>> 
>> Since gold doesn't actually basename, another option here would be to
>> skip the check of the basename declaration entirely.  In fact, we
>> should consider dropping basename from libiberty; our code should in
>> general use lbasename anyhow.
>
> Well, these are two additional changes on top of the one I posted though
> and can easily be addressed in the future.  Meanwhile, a combined build
> with maintainer-mode enabled will cause spurious changes to generated
> files, and the patch is pretty minimal in that it fixes the issues but
> doesn't change semantics otherwise.  So, ok to commit to src?

My question about a basename declaration in the system header was a
genuine question.  My concern is that on some systems this patch might
decide incorrectly whether or not basename is defined, which could
then possibly lead to a compilation error when libiberty.h is
included.

The gold patch is fine if you omit basename entirely from the gold
configure.ac.  I don't see how that could cause trouble given the
current libiberty.h.

Ian

Re: [PATCH] Fix PR bootstrap/42798

[Ralf Wildenhues]

* Ian Lance Taylor wrote on Sun, Jun 06, 2010 at 11:42:13PM CEST:
> Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:
> > * Ian Lance Taylor wrote on Fri, Jun 04, 2010 at 04:59:01PM CEST:
> >> With regard to the gold change.  What will happen if the system header
> >> files provide only a declaration
> >>     char *basename(const char *)
> >> ?
> >> 
> >> Since gold doesn't actually basename, another option here would be to
> >> skip the check of the basename declaration entirely.  In fact, we
> >> should consider dropping basename from libiberty; our code should in
> >> general use lbasename anyhow.
> >
> > Well, these are two additional changes on top of the one I posted though
> > and can easily be addressed in the future.  Meanwhile, a combined build
> > with maintainer-mode enabled will cause spurious changes to generated
> > files, and the patch is pretty minimal in that it fixes the issues but
> > doesn't change semantics otherwise.  So, ok to commit to src?
> 
> My question about a basename declaration in the system header was a
> genuine question.  My concern is that on some systems this patch might
> decide incorrectly whether or not basename is defined, which could
> then possibly lead to a compilation error when libiberty.h is
> included.

This concern should be addressed with my reply
<http://sourceware.org/ml/binutils/2010-06/msg00059.html>:
In short, the Autoconf change is designed to handle exactly this case
right both in C and in C++ mode.

> The gold patch is fine if you omit basename entirely from the gold
> configure.ac.  I don't see how that could cause trouble given the
> current libiberty.h.

OK, thanks.

I still need approval for the (really trivial) bfd and opcodes changes.

Cheers,
Ralf

Re: [PATCH] Fix PR bootstrap/42798

[Ian Lance Taylor]

Ralf Wildenhues <Ralf.Wildenhues@gmx.de> writes:

> * Ian Lance Taylor wrote on Sun, Jun 06, 2010 at 11:42:13PM CEST:
>
>> My question about a basename declaration in the system header was a
>> genuine question.  My concern is that on some systems this patch might
>> decide incorrectly whether or not basename is defined, which could
>> then possibly lead to a compilation error when libiberty.h is
>> included.
>
> This concern should be addressed with my reply
> <http://sourceware.org/ml/binutils/2010-06/msg00059.html>:
> In short, the Autoconf change is designed to handle exactly this case
> right both in C and in C++ mode.

That doesn't really address my concern.  That shows that autoconf will
get a workable answer if I use only C++.  But there is only one
config.h file.  gold has both .c and .cc files (the .c files are used
via AC_REPLACE_FUNCS).  It's not obvious to me that a version of
HAVE_DECL_BASENAME for C++ will necessarily work when compiling C.

Ian

Re: [PATCH] Fix PR bootstrap/42798

[Ralf Wildenhues]

[ adding bug-autoconf ]

* Ian Lance Taylor wrote on Mon, Jun 07, 2010 at 04:03:47PM CEST:
> Ralf Wildenhues writes:
> > * Ian Lance Taylor wrote on Sun, Jun 06, 2010 at 11:42:13PM CEST:
> >
> >> My question about a basename declaration in the system header was a
> >> genuine question.  My concern is that on some systems this patch might
> >> decide incorrectly whether or not basename is defined, which could
> >> then possibly lead to a compilation error when libiberty.h is
> >> included.
> >
> > This concern should be addressed with my reply
> > <http://sourceware.org/ml/binutils/2010-06/msg00059.html>:
> > In short, the Autoconf change is designed to handle exactly this case
> > right both in C and in C++ mode.
> 
> That doesn't really address my concern.  That shows that autoconf will
> get a workable answer if I use only C++.  But there is only one
> config.h file.  gold has both .c and .cc files (the .c files are used
> via AC_REPLACE_FUNCS).  It's not obvious to me that a version of
> HAVE_DECL_BASENAME for C++ will necessarily work when compiling C.

OK, fair point.  The current Autoconf solution for AC_CHECK_DECLS
assumes that the developer either uses the answer for the compiler
language tested only, or that the answer is consistent between the
C and the C++ compiler.

When we find a case where this assumption does not hold, we can still
work with the current framework by using AC_CHECK_DECL (without 'S')
and setting per-language defines, e.g.:

  AC_LANG_PUSH([C])
  AC_CHECK_DECLS([basename]) dnl defines HAVE_DECL_BASENAME
  AC_LANG_POP([C])

  AC_LANG_PUSH([C++])
  AC_CHECK_DECL([basename(char *)],
                [AC_DEFINE([HAVE_DECL_BASENAME_CXX], [1],
                           [Define to 1 if you have a C++ declaration ]dnl
                           [of `basename(char *)'])])
  AC_LANG_POP([C++])

and then go from there.

But I don't think we need to go there unless we find a system that
warrants this.  Autoconf is assuming similar in a number of cases
already: With AC_C_RESTRICT, it first tries a spelling of restrict
that is likely to work with both C++ and C compilers, but only the
C compiler is tested (and when you mix compilers from different
vendors, such as GNU and Solaris, this is a problem).

Cheers,
Ralf