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

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

[Ulrich Weigand]

Corinna Vinschen wrote:
> On May  4 05:52, Yaakov (Cygwin/X) wrote:
> > 2011-05-04  Yaakov Selkowitz  <yselkowitz@...>
> > 
> > 	* 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

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Corinna Vinschen]

On May  5 11:09, Ulrich Weigand wrote:
> Corinna Vinschen wrote:
> > On May  4 05:52, Yaakov (Cygwin/X) wrote:
> > > 2011-05-04  Yaakov Selkowitz  <yselkowitz@...>
> > > 
> > > 	* 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:

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

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


Corinna

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

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

[Corinna Vinschen]

On May  5 11:09, Ulrich Weigand wrote:
>   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

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

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

[DJ Delorie]

> 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.

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

[Ulrich Weigand]

DJ Delorie wrote:
> > 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

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[Corinna Vinschen]

On May  5 11:29, DJ Delorie wrote:
> 
> > 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

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

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

[Corinna Vinschen]

On May  5 18:44, Ulrich Weigand wrote:
> DJ Delorie wrote:
> > > 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

-- 
Corinna Vinschen
Cygwin Project Co-Leader
Red Hat

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

[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)

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

[DJ Delorie]

> 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.

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

[Ulrich Weigand]

DJ Delorie wrote:
> 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)

Corinna Vinschen had the same 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])

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

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

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

[DJ Delorie]

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