Fix for LTO compromised autoconf test in libiberty

GCC's ability to inline/clone self-recursive calls compromises the
find_stack_direction test in libiberty's configure.

The test essentially takes the address of a local in an outer and inner
context then compares their addresses to determine the direction of
stack growth.

Inlining causes the locals to be allocated in the same context which
totally defeats the purpose of the test.

This patch adds attributes to prevent inlining/cloning and rebuilds the
affected configure file.

Note there are configure files in other directories, but I'm pretty
sure they're using the cached value from libiberty and to fix them
requires an upstream fix to autoconf since those defintions don't come
from libiberty's aclocal.m4.

I'm particularly keen to get this in now as GCC is the master for
libiberty and Nick will be cutting a binutils release soon (and has
agreed to pull in this change already).  Getting the fix into binutils
now means the various copies in Fedora won't have to be patched
individually (mingw-binutils, avr-binutils, cross-binutils, arm-
whatever-binutils, nacl-binutils and possibly others).


You can see the effect by looking at how STACK_DIRECTION is defined in
config.h.  On x86 it should be "-1" and it is for the stage1 build if
you use an old enough compiler ;-)  But for stage2/stage3 it's "1"

Bootstrapped and regression tested on x86_64.  Verified STACK_DIRECTION
is correct via hand inspection.

OK for the trunk?

On Tue, Jan 14, 2020 at 03:02:01PM -0700, Jeff Law wrote:
> Bootstrapped and regression tested on x86_64.  Verified STACK_DIRECTION
> is correct via hand inspection.
> 
> OK for the trunk?

Wouldn't that fail due to warnings if compiled e.g. by gcc that doesn't
support noclone attribute?
Can't we e.g. instead do
      int (*volatile fn) ();
      fn = find_stack_direction;
      return fn ();
instead of
      return find_stack_direction ();
when performing the recursive call?
Though, at least current trunk emits tons of warnings on it already, so
maybe it must ignore warnings already.

> diff --git a/libiberty/aclocal.m4 b/libiberty/aclocal.m4
> index bf8a907100f..381ed3b27e3 100644
> --- a/libiberty/aclocal.m4
> +++ b/libiberty/aclocal.m4
> @@ -147,7 +147,7 @@ if test $ac_cv_os_cray = yes; then
>  fi
>  
>  AC_CACHE_CHECK(stack direction for C alloca, ac_cv_c_stack_direction,
> -[AC_TRY_RUN([find_stack_direction ()
> +[AC_TRY_RUN([__attribute__ ((noclone,noinline)) find_stack_direction ()
>  {
>    static char *addr = 0;
>    auto char dummy;
> diff --git a/libiberty/configure b/libiberty/configure
> index 7a34dabec32..e8391889cd7 100755
> --- a/libiberty/configure
> +++ b/libiberty/configure
> @@ -6532,7 +6532,7 @@ else
>  else
>    cat confdefs.h - <<_ACEOF >conftest.$ac_ext
>  /* end confdefs.h.  */
> -find_stack_direction ()
> +__attribute__ ((noclone,noinline)) find_stack_direction ()
>  {
>    static char *addr = 0;
>    auto char dummy;


	Jakub

On Jan 14 2020, Jeff Law wrote:

> diff --git a/libiberty/aclocal.m4 b/libiberty/aclocal.m4
> index bf8a907100f..381ed3b27e3 100644
> --- a/libiberty/aclocal.m4
> +++ b/libiberty/aclocal.m4
> @@ -147,7 +147,7 @@ if test $ac_cv_os_cray = yes; then
>  fi
>  
>  AC_CACHE_CHECK(stack direction for C alloca, ac_cv_c_stack_direction,
> -[AC_TRY_RUN([find_stack_direction ()
> +[AC_TRY_RUN([__attribute__ ((noclone,noinline)) find_stack_direction ()

That makes the test GCC specific, which doesn't actually need the test
in the first place.  That looks pointless.

Andreas.

On Tue, 2020-01-14 at 23:21 +0100, Andreas Schwab wrote:
> On Jan 14 2020, Jeff Law wrote:
> 
> > diff --git a/libiberty/aclocal.m4 b/libiberty/aclocal.m4
> > index bf8a907100f..381ed3b27e3 100644
> > --- a/libiberty/aclocal.m4
> > +++ b/libiberty/aclocal.m4
> > @@ -147,7 +147,7 @@ if test $ac_cv_os_cray = yes; then
> >  fi
> >  
> >  AC_CACHE_CHECK(stack direction for C alloca, ac_cv_c_stack_direction,
> > -[AC_TRY_RUN([find_stack_direction ()
> > +[AC_TRY_RUN([__attribute__ ((noclone,noinline)) find_stack_direction ()
> 
> That makes the test GCC specific, which doesn't actually need the test
> in the first place.  That looks pointless.
It's used by alloca.c to implement a C based alloca.  It needs to know
what direction stacks grow so that it knows if it's at a higher point
in the call chain and thus should collect objects allocated at deeper
points in the call chain.

Jakub has a good point though.  THe attributes probably need to be
conditional on GCC and the specific version of GCC being used.

jeff

On Tue, 2020-01-14 at 23:09 +0100, Jakub Jelinek wrote:
> On Tue, Jan 14, 2020 at 03:02:01PM -0700, Jeff Law wrote:
> > Bootstrapped and regression tested on x86_64.  Verified STACK_DIRECTION
> > is correct via hand inspection.
> > 
> > OK for the trunk?
> 
> Wouldn't that fail due to warnings if compiled e.g. by gcc that doesn't
> support noclone attribute?
> Can't we e.g. instead do
>       int (*volatile fn) ();
>       fn = find_stack_direction;
>       return fn ();
> instead of
>       return find_stack_direction ();
> when performing the recursive call?
> Though, at least current trunk emits tons of warnings on it already, so
> maybe it must ignore warnings already.
I think the safe thing to do is make it conditional on GCC and the
versions that support noclone, noinline.

jeff
>

My preference is still what I said in 
<https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02003.html>: either 
eliminate C alloca from libiberty, or don't build it with any compiler 
defining __GNUC__.  I'd be surprised if there are host compilers that 
build libiberty, have the relevant optimizations but don't support alloca.

On Tue, 2020-01-14 at 23:33 +0000, Joseph Myers wrote:
> My preference is still what I said in 
> <https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02003.html>;: either 
> eliminate C alloca from libiberty, or don't build it with any compiler 
> defining __GNUC__.  I'd be surprised if there are host compilers that 
> build libiberty, have the relevant optimizations but don't support alloca.
I wasn't even aware that c-alloca came up in that thread!  I'd been
leaving all the sanitizer related stuff to others.

I'd certainly support dropping c-alloca support from libiberty.   I
totally agree that it's only purpose was to support ancient compilers. 
I don't think I've seen c-alloca being used since the late 90s and I
think we started dropping all the alloca (0) that existed solely to
support c-alloca eons ago.

jeff

On Tue, 2020-01-14 at 23:33 +0000, Joseph Myers wrote:
> My preference is still what I said in 
> <https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02003.html>;: either 
> eliminate C alloca from libiberty, or don't build it with any compiler 
> defining __GNUC__.  I'd be surprised if there are host compilers that 
> build libiberty, have the relevant optimizations but don't support alloca.
So here's a patch that I think kills the alloca bits.

intl/ was the most interesting in that it has several #if ALLOCA
thingies.  But I reviewed all them and I'm pretty sure we'll do the
right thing (they define HAVE_ALLOCA anytime __GNUC__ is defined).

I've bootstrapped and regression tested without issue, of course.  But
that doesn't really exercise the worrisome cases. I wanted to try and
bootstrap on aix7.2, but the machine is the farm takes several seconds
for every file/directory operation.  Clearly not feasible.  So I then
wanted to try on aix7.1, but it doesn't look like we've got a usable
xlc++.  Then I tried to build on Solaris 11 in the farm.  But no Oracle
C++ compiler seems to be installed on that box.

So I'd really appreciated it if someone could do a bootstrap with
something other than GCC, preferably on AIX, but Solaris would be fine
too.

Anyway, here's the patch which removes all the alloca crud that I think
should disappear.

On Jan 14 2020, Jeff Law wrote:

> On Tue, 2020-01-14 at 23:21 +0100, Andreas Schwab wrote:
>> On Jan 14 2020, Jeff Law wrote:
>> 
>> > diff --git a/libiberty/aclocal.m4 b/libiberty/aclocal.m4
>> > index bf8a907100f..381ed3b27e3 100644
>> > --- a/libiberty/aclocal.m4
>> > +++ b/libiberty/aclocal.m4
>> > @@ -147,7 +147,7 @@ if test $ac_cv_os_cray = yes; then
>> >  fi
>> >  
>> >  AC_CACHE_CHECK(stack direction for C alloca, ac_cv_c_stack_direction,
>> > -[AC_TRY_RUN([find_stack_direction ()
>> > +[AC_TRY_RUN([__attribute__ ((noclone,noinline)) find_stack_direction ()
>> 
>> That makes the test GCC specific, which doesn't actually need the test
>> in the first place.  That looks pointless.
> It's used by alloca.c to implement a C based alloca.

Nothing uses C alloca.

Andreas.