newlib vs. libiberty mismatch breaks build (Re: [PATCH] Export psignal on all platforms)

[Ulrich Weigand]

* libc/include/signal.h (psignal): Declare.
* libc/sys/linux/psignal.c: Move from here...
* libc/signal/psignal.c: ... to here. Document.
* libc/sys/linux/Makefile.am (GENERAL_SOURCES): Move psignal.c from here...
* libc/signal/Makefile.am (LIB_SOURCES): ... to here.
(CHEWOUT_FILES): Add psignal.def.
* libc/sys/linux/Makefile.in: Regenerate.
* libc/signal/Makefile.in: Ditto.
* libc/signal/signal.tex: Add references to psignal.

The patch looks good to me. Please apply.

This breaks building a cross-toolchain to SPU (and probably other newlib
based platforms) with newlib head and GCC head:

/home/kwerner/dailybuild/spu-tc-2011-05-05/gcc-head/src/libiberty/strsignal.c:554:1: error: conflicting types for 'psignal'
/home/kwerner/dailybuild/spu-tc-2011-05-05/spu-toolchain/spu/include/signal.h:27:6: note: previous declaration of 'psignal' was here

There are a couple of factors contributing to the problem:

- For one, the libiberty prototype of psignal is probably wrong:

#ifndef HAVE_PSIGNAL

void
psignal (int signo, char *message)
{

newlib has "const char *message" instead (just like glibc).

- On the other hand, as newlib now provides psignal itself, this copy in
libiberty should not actually get built at all ...

- ... however, it does, because of a configure problem. The libliberty
configure.ac tries to avoid link tests when cross-compiling. Therefore,
it simply hard-codes the set of functions it assumes newlib provides:

# We are being configured as a target library. AC_REPLACE_FUNCS
# may not work correctly, because the compiler may not be able to
# link executables. Note that we may still be being configured
# native.

# If we are being configured for newlib, we know which functions
# newlib provide and which ones we will be expected to provide.

if test "x${with_newlib}" = "xyes"; then
AC_LIBOBJ([asprintf])
AC_LIBOBJ([basename])
AC_LIBOBJ([insque])
AC_LIBOBJ([random])
AC_LIBOBJ([strdup])
AC_LIBOBJ([vasprintf])

for f in $funcs; do
case "$f" in
asprintf | basename | insque | random | strdup | vasprintf)
;;
*)
n=HAVE_`echo $f | tr 'abcdefghijklmnopqrstuvwxyz' 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'`
AC_DEFINE_UNQUOTED($n)
;;
esac
done

# newlib doesnt provide any of the variables in $vars, so we
# dont have to check them here.

# Of the functions in $checkfuncs, newlib only has strerror.
AC_DEFINE(HAVE_STRERROR)

setobjs=yes

fi

This list does not include psignal, which indeed newlib did not provide
-- until yesterday, when that patch was committed ...


I'm not quite sure how to fix this ... any suggestions? Did this problem
occur in the past when newlib was extended to provide some new function?

I guess the immediate build problem will disappear once the libiberty
prototype is fixed to include const, but then we'll just have two copies
of psignal (one in newlib, one in libiberty), which may not be ideal
either.

Bye,
Ulrich

[Corinna Vinschen]

Post by Ulrich Weigand

* libc/include/signal.h (psignal): Declare.
* libc/sys/linux/psignal.c: Move from here...
* libc/signal/psignal.c: ... to here. Document.
* libc/sys/linux/Makefile.am (GENERAL_SOURCES): Move psignal.c from here...
* libc/signal/Makefile.am (LIB_SOURCES): ... to here.
(CHEWOUT_FILES): Add psignal.def.
* libc/sys/linux/Makefile.in: Regenerate.
* libc/signal/Makefile.in: Ditto.
* libc/signal/signal.tex: Add references to psignal.

The patch looks good to me. Please apply.

This breaks building a cross-toolchain to SPU (and probably other newlib

That's why I sent this mail to gcc-patches:

http://gcc.gnu.org/ml/gcc-patches/2011-05/msg00374.html


Corinna

[Corinna Vinschen]

Post by Ulrich Weigand
This list does not include psignal, which indeed newlib did not provide
-- until yesterday, when that patch was committed ...
I'm not quite sure how to fix this ... any suggestions? Did this problem
occur in the past when newlib was extended to provide some new function?
I guess the immediate build problem will disappear once the libiberty
prototype is fixed to include const, but then we'll just have two copies
of psignal (one in newlib, one in libiberty), which may not be ideal
either.

Developers using libiberty don't have to use newlib so the definition
in libiberty still makes sense, right?

I don't know how to avoid this problem in configure, other than by adding
AC_LIBOBJ([psignal]).


Corinna

[DJ Delorie]

Post by Corinna Vinschen
I don't know how to avoid this problem in configure, other than by adding
AC_LIBOBJ([psignal]).

Right, the correct solution is - libiberty shouldn't provide psignal
if newlib does. Making it match newlib is the wrong solution.

[Ulrich Weigand]

Post by DJ Delorie

Post by Corinna Vinschen
I don't know how to avoid this problem in configure, other than by adding
AC_LIBOBJ([psignal]).

Right, the correct solution is - libiberty shouldn't provide psignal
if newlib does. Making it match newlib is the wrong solution.

I guess I agree, but the problem is exactly how to implement "if newlib
does" ... Usually, this would be a link test, which libiberty configure
deliberately avoids for cross-builds, so it hardcodes "what newlib does".
Do you suggest we should do the link test after all, or make the hard-
coded newlib capability list somehow dynamic (depending on what)?

Maybe I'm missing something, but I don't understand the AC_LIBOBJ suggestion.
This would add "psignal.o" to the list of object files to be linked into
libiberty. But: 1.) there is no psignal.o / psignal.c in libiberty, but the
symbol psignal is defined within strsignal.c; and 2.) strsignal.o is already
added unconditionally to the list of libiberty objects ... [ But even if we
*did* split psignal into its own object file, that would still not answer
the question of when to link it in and when not. ]

Bye,
Ulrich

[Corinna Vinschen]

Post by Ulrich Weigand

Post by DJ Delorie

Post by Corinna Vinschen
I don't know how to avoid this problem in configure, other than by adding
AC_LIBOBJ([psignal]).

Right, the correct solution is - libiberty shouldn't provide psignal
if newlib does. Making it match newlib is the wrong solution.

I guess I agree, but the problem is exactly how to implement "if newlib
does" ... Usually, this would be a link test, which libiberty configure
deliberately avoids for cross-builds, so it hardcodes "what newlib does".
Do you suggest we should do the link test after all, or make the hard-
coded newlib capability list somehow dynamic (depending on what)?
Maybe I'm missing something, but I don't understand the AC_LIBOBJ suggestion.

Sorry about that. I guess that should have been something along the
lines of

AC_DEFINE(HAVE_PSIGNAL,1,[Define if you have psignal])


Corinna

[DJ Delorie]

No, we've always hard-coded what newlib does to avoid the link
problems. I think we should continue with that for now.

I suspect we need to AC_DEFINE(HAVE_PSIGNAL)

[Ulrich Weigand]

Post by DJ Delorie
No, we've always hard-coded what newlib does to avoid the link
problems. I think we should continue with that for now.
I suspect we need to AC_DEFINE(HAVE_PSIGNAL)
Sorry about that. I guess that should have been something along the
lines of
AC_DEFINE(HAVE_PSIGNAL,1,[Define if you have psignal])

Just so I understand correctly: adding this AC_DEFINE would *always*
define HAVE_PSIGNAL when configuring libiberty with --with-newlib,
and thus libiberty would never export psignal.

This would of course be fine when building against current newlib
head, because that does provide psignal. However, when building
against some older newlib version, we still wouldn't get psignal
from libiberty, and therefore not have it available at all, right?

[ Maybe this would be just fine. GCC for example never calls
psignal anyway ... ]

If we do add AC_DEFINE(psignal), shouldn't we then also add
AC_DEFINE(strsignal), since this is also provided by newlib
(and having strsignal from libiberty but psignal from newlib,
using two potentially different lists of signal names, would
seem to be just wierd ...)?

Thanks,
Ulrich

[DJ Delorie]

IIRC there's a compile-time way to test for prototypes at least.

[Corinna Vinschen]

Post by DJ Delorie

Post by Corinna Vinschen
I don't know how to avoid this problem in configure, other than by adding
AC_LIBOBJ([psignal]).

Right, the correct solution is - libiberty shouldn't provide psignal
if newlib does. Making it match newlib is the wrong solution.

You don't mean the prototype, do you? IMHO the char * should still be
changed to const char * to adhere to POSIX.


Corinna

[DJ Delorie]

Post by Corinna Vinschen
You don't mean the prototype, do you? IMHO the char * should still be
changed to const char * to adhere to POSIX.

So far, it's served as a good tool for detecting when libiberty's
configure isn't doing its job :-)

At this point, we should avoid "fixing" it until the newlib/configure
issue is fixed, otherwise the problem will get hidden away again. If,
after that, you want to posixify libiberty some more, we'll worry
about it then.