[PATCH] Cleanup MIPS preconfigure script

[PATCH] Cleanup MIPS preconfigure script

[Steve Ellcey]

I would like to simplify the MIPS preconfigure script.  Right now it looks
at the GCC flags and tries to determine what it should set 'machine' to based
on those flags (and os_config).  I would like to run the compiler and check
what macros are set and use that to set the machine variable instead.

This seems safer since right now a platform that generated the n64 ABI by
default (as an example) might not work correctly because the '-mabi=64' flag
would not need to appear in $CFLAGS to generate n64 code.

Other platforms like arm, m68k, powerpc, and tile are already using this
technique of calling the compiler and checking macros and the macros I am
checking on MIPS (_MIPS_SIM, _MIPS_ISA, and __mips16) are already used in
various glibc source files.

Note that I removed the reference to kern64 since it no longer exists
and I removed the addition of $machine from the end of the 'machine='
line for mips32 as there did not seem to be any reason for it.

Tested with mips-mti-linux-gnu and mips-linux-gnu and mips64-linux-gnu
builds.

OK to checkin?

Steve Ellcey
sellcey@mips.com


2014-09-04  Steve Ellcey  <sellcey@mips.com>

	* sysdeps/mips/preconfigure: Modify ABI tests.


diff --git a/sysdeps/mips/preconfigure b/sysdeps/mips/preconfigure
index b215eb2..df958d6 100644
--- a/sysdeps/mips/preconfigure
+++ b/sysdeps/mips/preconfigure
@@ -1,34 +1,25 @@
-case "$machine" in
-mips64*)	base_machine=mips64
-		case "$CC $CFLAGS $CPPFLAGS " in
-		*" -mabi=n32 "*) mips_cc_abi=n32 ;;
-		*" -mabi=64 "*|*" -mabi=n64 "*) mips_cc_abi=64 ;;
-		*" -mabi=32 "*|*" -mabi=o32 "*) mips_cc_abi=32 ;;
-		*) mips_cc_abi=default ;;
-		esac
-		case $config_os in
-		*abin32*) mips_config_abi=n32 ;;
-		*abi64*|*abin64*) mips_config_abi=64 ;;
-		*abi32*|*abio32*) mips_config_abi=32 ;;
-		*) mips_config_abi=$mips_cc_abi ;;
-		esac
-		case $mips_config_abi in
-		default) machine=mips/mips64/n32 mips_config_abi=n32 ;;
-		n32) machine=mips/mips64/n32 ;;
-		64) machine=mips/mips64/n64 ;;
-		32) machine=mips/mips32/kern64 ;;
-		esac
-		machine=$machine/$config_machine
-		if test $mips_config_abi != $mips_cc_abi; then
-		  # This won't make it to config.make, but we want to
-		  # set this in case configure tests depend on it.
-		  CPPFLAGS="$CPPFLAGS -mabi=$mips_config_abi"
-		fi
-		;;
-mips*)		base_machine=mips
-		case "$CC $CFLAGS $CPPFLAGS " in
-		*" -mips16 "*) machine=mips/mips32/mips16/$machine ;;
-		*) machine=mips/mips32/$machine ;;
-		esac
-		;;
-esac
+
+abiflag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n 's/^#define _MIPS_SIM \(.*\)/\1/p'`
+isaflag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n 's/^#define _MIPS_ISA \(.*\)/\1/p'`
+mips16flag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n 's/^#define __mips16 \(.*\)/\1/p'`
+
+if  test "$isaflag" = "_MIPS_ISA_MIPS64"; then
+	base_machine=mips64
+	if test "$abiflag" = "_ABIO32" ; then
+		machine=mips/mips32
+	elif test "$abiflag" = "_ABIN32" ; then
+		machine=mips/mips64/n32
+	elif test "$abiflag" = "_ABI64" ; then
+		machine=mips/mips64/n64
+	else
+		as_fn_error $? "Unable to determine ABI." "$LINENO" 5
+	fi
+else
+	base_machine=mips
+	if test "$mips16flag" = "1" ; then
+		machine=mips/mips32/mips16
+	else
+		machine=mips/mips32
+	fi
+fi
+machine=$machine/$config_machine

Re: [PATCH] Cleanup MIPS preconfigure script

[Maciej W. Rozycki]

On Thu, 4 Sep 2014, Steve Ellcey  wrote:

> diff --git a/sysdeps/mips/preconfigure b/sysdeps/mips/preconfigure
> index b215eb2..df958d6 100644
> --- a/sysdeps/mips/preconfigure
> +++ b/sysdeps/mips/preconfigure
> @@ -1,34 +1,25 @@
> -case "$machine" in
> -mips64*)	base_machine=mips64
> -		case "$CC $CFLAGS $CPPFLAGS " in
> -		*" -mabi=n32 "*) mips_cc_abi=n32 ;;
> -		*" -mabi=64 "*|*" -mabi=n64 "*) mips_cc_abi=64 ;;
> -		*" -mabi=32 "*|*" -mabi=o32 "*) mips_cc_abi=32 ;;
> -		*) mips_cc_abi=default ;;
> -		esac
> -		case $config_os in
> -		*abin32*) mips_config_abi=n32 ;;
> -		*abi64*|*abin64*) mips_config_abi=64 ;;
> -		*abi32*|*abio32*) mips_config_abi=32 ;;
> -		*) mips_config_abi=$mips_cc_abi ;;
> -		esac
> -		case $mips_config_abi in
> -		default) machine=mips/mips64/n32 mips_config_abi=n32 ;;
> -		n32) machine=mips/mips64/n32 ;;
> -		64) machine=mips/mips64/n64 ;;
> -		32) machine=mips/mips32/kern64 ;;
> -		esac
> -		machine=$machine/$config_machine
> -		if test $mips_config_abi != $mips_cc_abi; then
> -		  # This won't make it to config.make, but we want to
> -		  # set this in case configure tests depend on it.
> -		  CPPFLAGS="$CPPFLAGS -mabi=$mips_config_abi"
> -		fi
> -		;;
> -mips*)		base_machine=mips
> -		case "$CC $CFLAGS $CPPFLAGS " in
> -		*" -mips16 "*) machine=mips/mips32/mips16/$machine ;;
> -		*) machine=mips/mips32/$machine ;;
> -		esac
> -		;;
> -esac
> +
> +abiflag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n 's/^#define _MIPS_SIM \(.*\)/\1/p'`
> +isaflag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n 's/^#define _MIPS_ISA \(.*\)/\1/p'`
> +mips16flag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n 's/^#define __mips16 \(.*\)/\1/p'`
> +
> +if  test "$isaflag" = "_MIPS_ISA_MIPS64"; then
> +	base_machine=mips64
> +	if test "$abiflag" = "_ABIO32" ; then
> +		machine=mips/mips32
> +	elif test "$abiflag" = "_ABIN32" ; then
> +		machine=mips/mips64/n32
> +	elif test "$abiflag" = "_ABI64" ; then
> +		machine=mips/mips64/n64
> +	else
> +		as_fn_error $? "Unable to determine ABI." "$LINENO" 5
> +	fi
> +else
> +	base_machine=mips
> +	if test "$mips16flag" = "1" ; then
> +		machine=mips/mips32/mips16
> +	else
> +		machine=mips/mips32
> +	fi
> +fi
> +machine=$machine/$config_machine

 Hmm, that's quite a change in the interpretation of host triplets, but 
I'm leaning towards finding it acceptable, I think we have sufficient 
means in GCC nowadays to control the default ABI so that we don't have to 
rely on suffixes in the triplets.  I wonder if a sanity check wouldn't be 
good to have though, such as rejecting mips64* with the compiler set to 
the 32-bit ABI to trap accidental silly use.

 Your change has a flaw though, you can't rely on _MIPS_ISA being set 
exactly to _MIPS_ISA_MIPS64 on determining if you want a 64-bit or a 
32-bit configuration, there are other 64-bit ISAs, starting from 
_MIPS_ISA_MIPS3, e.g. I have an n64 MIPS III compiler.  I think you can 
just skip this check altogether, GCC will have set the ABI and the ISA 
consistently already and you can merge the two legs of this conditional 
into one.  You may just sanity-check that a 64-bit ABI is not used 
together with the MIPS16 option as we have no 64-bit MIPS16 PIC support.

 Thanks for thinking of cleaning this piece up.

  Maciej

Re: [PATCH] Cleanup MIPS preconfigure script

[Steve Ellcey]

On Thu, 2014-09-04 at 23:29 +0100, Maciej W. Rozycki wrote:

>  Hmm, that's quite a change in the interpretation of host triplets, but 
> I'm leaning towards finding it acceptable, I think we have sufficient 
> means in GCC nowadays to control the default ABI so that we don't have to 
> rely on suffixes in the triplets.  I wonder if a sanity check wouldn't be 
> good to have though, such as rejecting mips64* with the compiler set to 
> the 32-bit ABI to trap accidental silly use.

Someone might want to use a 32 bit ABI on mips64 though, how about a
warning instead of an error for that case?

>  Your change has a flaw though, you can't rely on _MIPS_ISA being set 
> exactly to _MIPS_ISA_MIPS64 on determining if you want a 64-bit or a 
> 32-bit configuration, there are other 64-bit ISAs, starting from 
> _MIPS_ISA_MIPS3, e.g. I have an n64 MIPS III compiler.  I think you can 
> just skip this check altogether, GCC will have set the ABI and the ISA 
> consistently already and you can merge the two legs of this conditional 
> into one.  You may just sanity-check that a 64-bit ABI is not used 
> together with the MIPS16 option as we have no 64-bit MIPS16 PIC support.

I like the idea of not using _MIPS_ISA.  The other difference between
the 32 and 64 branches is setting 'base_machine' and it doesn't look to
me that we use base_machine anywhere.  The only use of it anywhere in
glibc that I see is in the x86_64 preconfigure script.  I agree that
using mips16 with n32 or n64 should be an error.  I will update the
patch.

Steve Ellcey

Re: [PATCH] Cleanup MIPS preconfigure script

[Maciej W. Rozycki]

On Thu, 4 Sep 2014, Steve Ellcey wrote:

> >  Hmm, that's quite a change in the interpretation of host triplets, but 
> > I'm leaning towards finding it acceptable, I think we have sufficient 
> > means in GCC nowadays to control the default ABI so that we don't have to 
> > rely on suffixes in the triplets.  I wonder if a sanity check wouldn't be 
> > good to have though, such as rejecting mips64* with the compiler set to 
> > the 32-bit ABI to trap accidental silly use.
> 
> Someone might want to use a 32 bit ABI on mips64 though, how about a
> warning instead of an error for that case?

 Hmm, I never thought of such use and I keep considering it weird, but 
having looked through the current script again we already permit e.g. 
`mips64-linux-gnuabi32' to select o32, so I take my concern back, no need 
for any checks here.

> >  Your change has a flaw though, you can't rely on _MIPS_ISA being set 
> > exactly to _MIPS_ISA_MIPS64 on determining if you want a 64-bit or a 
> > 32-bit configuration, there are other 64-bit ISAs, starting from 
> > _MIPS_ISA_MIPS3, e.g. I have an n64 MIPS III compiler.  I think you can 
> > just skip this check altogether, GCC will have set the ABI and the ISA 
> > consistently already and you can merge the two legs of this conditional 
> > into one.  You may just sanity-check that a 64-bit ABI is not used 
> > together with the MIPS16 option as we have no 64-bit MIPS16 PIC support.
> 
> I like the idea of not using _MIPS_ISA.  The other difference between
> the 32 and 64 branches is setting 'base_machine' and it doesn't look to
> me that we use base_machine anywhere.  The only use of it anywhere in
> glibc that I see is in the x86_64 preconfigure script.  I agree that
> using mips16 with n32 or n64 should be an error.  I will update the
> patch.

 Good.  Frankly (with the observation I made above) I think there should 
be no difference between configuring for `mips-linux-gnu' and 
`mips64-linux-gnuabi32'.

 I think the current arrangement has its historical reasons, there was a 
time when the Linux kernel had separate `mips' and `mips64' ports -- 
arch/mips and arch/mips64 as well as include/asm-mips and 
include/asm-mips64 for code and headers respectively (now asm headers live 
under arch/ too).  For glibc it was the latter that mattered as kernel 
headers are needed in our build process and had to be fetched from the 
correct place, with the usual default being /usr/src/linux/include/linux 
and /usr/src/linux/include/asm-<cpu>.  Hence the distinction and maybe the 
presence of `kern64'.  The `mips64' port supported all the three ABIs so 
any of them could have been selected to be built for.

 Of course all it does not matter anymore, the MIPS port of Linux was the 
first to merge its 32-bit and its 64-bit variant and other ports followed, 
e.g. `sparc' and `sparc64'.

  Maciej

Re: [PATCH] Cleanup MIPS preconfigure script

[Steve Ellcey]

On Fri, 2014-09-05 at 00:23 +0100, Maciej W. Rozycki wrote:

>  Hmm, I never thought of such use and I keep considering it weird, but 
> having looked through the current script again we already permit e.g. 
> `mips64-linux-gnuabi32' to select o32, so I take my concern back, no need 
> for any checks here.

OK, that shrinks the preconfigure script even more.

>  Good.  Frankly (with the observation I made above) I think there should 
> be no difference between configuring for `mips-linux-gnu' and 
> `mips64-linux-gnuabi32'.

I agree, here is a new patch.  I complete removed the setting of
base_machine since it is not used and I removed the line:

machine=$machine/$config_machine

that was in my earlier patch because that did serve any purpose
that I could find, it was part of the original 32 bit mips preconfigure
path, which is why I originally had left it in.

Steve Ellcey
sellcey@mips.com


2014-09-05  Steve Ellcey  <sellcey@mips.com>

	* sysdeps/mips/preconfigure: Modify ABI tests.


diff --git a/sysdeps/mips/preconfigure b/sysdeps/mips/preconfigure
index b215eb2..343ef16 100644
--- a/sysdeps/mips/preconfigure
+++ b/sysdeps/mips/preconfigure
@@ -1,34 +1,20 @@
-case "$machine" in
-mips64*)	base_machine=mips64
-		case "$CC $CFLAGS $CPPFLAGS " in
-		*" -mabi=n32 "*) mips_cc_abi=n32 ;;
-		*" -mabi=64 "*|*" -mabi=n64 "*) mips_cc_abi=64 ;;
-		*" -mabi=32 "*|*" -mabi=o32 "*) mips_cc_abi=32 ;;
-		*) mips_cc_abi=default ;;
-		esac
-		case $config_os in
-		*abin32*) mips_config_abi=n32 ;;
-		*abi64*|*abin64*) mips_config_abi=64 ;;
-		*abi32*|*abio32*) mips_config_abi=32 ;;
-		*) mips_config_abi=$mips_cc_abi ;;
-		esac
-		case $mips_config_abi in
-		default) machine=mips/mips64/n32 mips_config_abi=n32 ;;
-		n32) machine=mips/mips64/n32 ;;
-		64) machine=mips/mips64/n64 ;;
-		32) machine=mips/mips32/kern64 ;;
-		esac
-		machine=$machine/$config_machine
-		if test $mips_config_abi != $mips_cc_abi; then
-		  # This won't make it to config.make, but we want to
-		  # set this in case configure tests depend on it.
-		  CPPFLAGS="$CPPFLAGS -mabi=$mips_config_abi"
-		fi
-		;;
-mips*)		base_machine=mips
-		case "$CC $CFLAGS $CPPFLAGS " in
-		*" -mips16 "*) machine=mips/mips32/mips16/$machine ;;
-		*) machine=mips/mips32/$machine ;;
-		esac
-		;;
-esac
+abiflag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n 's/^#define _MIPS_SIM \(.*\)/\1/p'`
+mips16flag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n 's/^#define __mips16 \(.*\)/\1/p'`
+
+if test "$abiflag" = "_ABIO32" ; then
+	if test "$mips16flag" = "1" ; then
+		machine=mips/mips32/mips16
+	else
+		machine=mips/mips32
+	fi
+elif test "$abiflag" = "_ABIN32" ; then
+	machine=mips/mips64/n32
+elif test "$abiflag" = "_ABI64" ; then
+	machine=mips/mips64/n64
+else
+	as_fn_error $? "Unable to determine ABI." "$LINENO" 5
+fi
+
+if test "$abiflag" != "_ABIO32" -a "$mips16flag" = "1"; then
+	as_fn_error $? "mips16 is only supported with the O32 ABI." "$LINENO" 5
+fi

Re: [PATCH] Cleanup MIPS preconfigure script

[Joseph S. Myers]

On Fri, 5 Sep 2014, Steve Ellcey wrote:

> I agree, here is a new patch.  I complete removed the setting of
> base_machine since it is not used and I removed the line:

base_machine *is* used, to set base-machine in config.make.  But the only 
things that's used for now are (mach/Makefile for powerpc and) libc-abis 
handling (regarding which see 
<https://sourceware.org/ml/libc-alpha/2014-01/msg00375.html>) - and 
whatever the libc-abis handling does or does not work for, there is no use 
in a special setting of base_machine for mips64; just setting to mips for 
all MIPS cases seems most appropriate and consistent with other 
architectures.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Cleanup MIPS preconfigure script

[Steve Ellcey]

On Fri, 2014-09-05 at 21:55 +0000, Joseph S. Myers wrote:
> On Fri, 5 Sep 2014, Steve Ellcey wrote:
> 
> > I agree, here is a new patch.  I complete removed the setting of
> > base_machine since it is not used and I removed the line:
> 
> base_machine *is* used, to set base-machine in config.make.  But the only 
> things that's used for now are (mach/Makefile for powerpc and) libc-abis 
> handling (regarding which see 
> <https://sourceware.org/ml/libc-alpha/2014-01/msg00375.html>) - and 
> whatever the libc-abis handling does or does not work for, there is no use 
> in a special setting of base_machine for mips64; just setting to mips for 
> all MIPS cases seems most appropriate and consistent with other 
> architectures.

OK, I will put 'base_machine=mips' back in.  I also found a problem with
my removal of

machine=$machine/$config_machine

Everything was fine building mips-mti-linux-gnu and mips-linux-gnu but I
forgot to rebuild mips64-linux-gnu.  When I did it failed because
sysdeps/unix/sysv/linux/mips/configure.ac contains:

case $machine in
mips/mips64/n64/*)
  LIBC_SLIBDIR_RTLDDIR([lib64], [lib64])
  ;;
mips/mips64/n32/*)
  LIBC_SLIBDIR_RTLDDIR([lib32], [lib32])
  ;;
esac

and if machine is set to 'mips/mips64/n64' or 'mips/mips64/n32' it
doesn't match the case statements.  I am not sure if I should put
the 'machine=$machine/$config_machine' line back or change the
case statement to match n64* and n32* instead of n64/* and n32/*.
Opinions?

Steve Ellcey

Re: [PATCH] Cleanup MIPS preconfigure script

[Steve Ellcey]

On Fri, 2014-09-05 at 21:55 +0000, Joseph S. Myers wrote:
> On Fri, 5 Sep 2014, Steve Ellcey wrote:
> 
> > I agree, here is a new patch.  I complete removed the setting of
> > base_machine since it is not used and I removed the line:
> 
> base_machine *is* used, to set base-machine in config.make.  But the only 
> things that's used for now are (mach/Makefile for powerpc and) libc-abis 
> handling (regarding which see 
> <https://sourceware.org/ml/libc-alpha/2014-01/msg00375.html>) - and 
> whatever the libc-abis handling does or does not work for, there is no use 
> in a special setting of base_machine for mips64; just setting to mips for 
> all MIPS cases seems most appropriate and consistent with other 
> architectures.

Here is a new patch.  I set base_machine to mips and I put back the
'machine=$machine/$config_machine' line along with a comment about why
it is needed.  I rebuilt  mips-mti-linux-gnu, mipsel-linux-gnu, and
mips64-linux-gnu toolchains to test the change.

OK to checkin?

Steve Ellcey
sellcey@mips.com



2014-09-09  Steve Ellcey  <sellcey@mips.com>

	* sysdeps/mips/preconfigure: Modify ABI tests.


diff --git a/sysdeps/mips/preconfigure b/sysdeps/mips/preconfigure
index b215eb2..fb572d7 100644
--- a/sysdeps/mips/preconfigure
+++ b/sysdeps/mips/preconfigure
@@ -1,34 +1,24 @@
-case "$machine" in
-mips64*)	base_machine=mips64
-		case "$CC $CFLAGS $CPPFLAGS " in
-		*" -mabi=n32 "*) mips_cc_abi=n32 ;;
-		*" -mabi=64 "*|*" -mabi=n64 "*) mips_cc_abi=64 ;;
-		*" -mabi=32 "*|*" -mabi=o32 "*) mips_cc_abi=32 ;;
-		*) mips_cc_abi=default ;;
-		esac
-		case $config_os in
-		*abin32*) mips_config_abi=n32 ;;
-		*abi64*|*abin64*) mips_config_abi=64 ;;
-		*abi32*|*abio32*) mips_config_abi=32 ;;
-		*) mips_config_abi=$mips_cc_abi ;;
-		esac
-		case $mips_config_abi in
-		default) machine=mips/mips64/n32 mips_config_abi=n32 ;;
-		n32) machine=mips/mips64/n32 ;;
-		64) machine=mips/mips64/n64 ;;
-		32) machine=mips/mips32/kern64 ;;
-		esac
-		machine=$machine/$config_machine
-		if test $mips_config_abi != $mips_cc_abi; then
-		  # This won't make it to config.make, but we want to
-		  # set this in case configure tests depend on it.
-		  CPPFLAGS="$CPPFLAGS -mabi=$mips_config_abi"
-		fi
-		;;
-mips*)		base_machine=mips
-		case "$CC $CFLAGS $CPPFLAGS " in
-		*" -mips16 "*) machine=mips/mips32/mips16/$machine ;;
-		*) machine=mips/mips32/$machine ;;
-		esac
-		;;
-esac
+abiflag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n 's/^#define _MIPS_SIM \(.*\)/\1/p'`
+mips16flag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n 's/^#define __mips16 \(.*\)/\1/p'`
+
+base_machine=mips
+if test "$abiflag" = "_ABIO32" ; then
+	if test "$mips16flag" = "1" ; then
+		machine=mips/mips32/mips16
+	else
+		machine=mips/mips32
+	fi
+elif test "$abiflag" = "_ABIN32" ; then
+	machine=mips/mips64/n32
+elif test "$abiflag" = "_ABI64" ; then
+	machine=mips/mips64/n64
+else
+	as_fn_error $? "Unable to determine ABI." "$LINENO" 5
+fi
+# $config_machine is not really needed here but the slash after $machine is
+# needed by the case statement in sysdeps/unix/sysv/linux/mips/configure.ac.
+machine=$machine/$config_machine
+
+if test "$abiflag" != "_ABIO32" -a "$mips16flag" = "1"; then
+	as_fn_error $? "mips16 is only supported with the O32 ABI." "$LINENO" 5
+fi

Re: [PATCH] Cleanup MIPS preconfigure script

[Joseph S. Myers]

On Tue, 9 Sep 2014, Steve Ellcey wrote:

> Here is a new patch.  I set base_machine to mips and I put back the
> 'machine=$machine/$config_machine' line along with a comment about why
> it is needed.  I rebuilt  mips-mti-linux-gnu, mipsel-linux-gnu, and
> mips64-linux-gnu toolchains to test the change.
> 
> OK to checkin?

OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Cleanup MIPS preconfigure script

[Maciej W. Rozycki]

On Tue, 9 Sep 2014, Steve Ellcey wrote:

> +if test "$abiflag" != "_ABIO32" -a "$mips16flag" = "1"; then
> +	as_fn_error $? "mips16 is only supported with the O32 ABI." "$LINENO" 5
> +fi

 Hmm, I think the capitalisation is weird here, why not:

+	as_fn_error $? "MIPS16 is only supported with the o32 ABI." "$LINENO" 5

?

  Maciej

Re: [PATCH] Cleanup MIPS preconfigure script

[Steve Ellcey]

On Tue, 2014-09-09 at 18:08 +0100, Maciej W. Rozycki wrote:
> On Tue, 9 Sep 2014, Steve Ellcey wrote:
> 
> > +if test "$abiflag" != "_ABIO32" -a "$mips16flag" = "1"; then
> > +	as_fn_error $? "mips16 is only supported with the O32 ABI." "$LINENO" 5
> > +fi
> 
>  Hmm, I think the capitalisation is weird here, why not:
> 
> +	as_fn_error $? "MIPS16 is only supported with the o32 ABI." "$LINENO" 5
> 
> ?
> 
>   Maciej

I already checked it in but I can go back and tweak the capitalization
if you want.

Steve Ellcey
sellcey@mips.com

Re: [PATCH] Cleanup MIPS preconfigure script

[Adhemerval Zanella]

On 09-09-2014 14:13, Steve Ellcey wrote:
> On Tue, 2014-09-09 at 18:08 +0100, Maciej W. Rozycki wrote:
>> On Tue, 9 Sep 2014, Steve Ellcey wrote:
>>
>>> +if test "$abiflag" != "_ABIO32" -a "$mips16flag" = "1"; then
>>> +	as_fn_error $? "mips16 is only supported with the O32 ABI." "$LINENO" 5
>>> +fi
>>  Hmm, I think the capitalisation is weird here, why not:
>>
>> +	as_fn_error $? "MIPS16 is only supported with the o32 ABI." "$LINENO" 5
>>
>> ?
>>
>>   Maciej
> I already checked it in but I can go back and tweak the capitalization
> if you want.
>
> Steve Ellcey
> sellcey@mips.com
>
And patch 0febba23ddabcd971be5259ee20236b9e3efa690 broke powerpc64 configure:

$ /home/azanella/glibc/glibc-git/configure --prefix=/usr --with-cpu=power8 
checking build system type... powerpc64-unknown-linux-gnu
checking host system type... powerpc64-unknown-linux-gnu
checking for gcc... gcc
checking for suffix of object files... o
checking whether we are using the GNU C compiler... yes
checking whether gcc accepts -g... yes
checking for g++... g++
checking whether we are using the GNU C++ compiler... yes
checking whether g++ accepts -g... yes
checking for readelf... readelf
checking for sysdeps preconfigure fragments... aarch64 alpha arm hppa i386 m68k microblaze mips configure: error: Unable to determine ABI.

Re: [PATCH] Cleanup MIPS preconfigure script

[Joseph S. Myers]

On Tue, 9 Sep 2014, Adhemerval Zanella wrote:

> And patch 0febba23ddabcd971be5259ee20236b9e3efa690 broke powerpc64 configure:

Steve, you need to put preconfigure back inside a case statement so 
nothing runs for non-mips* machines.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Cleanup MIPS preconfigure script

[Steve Ellcey]

On Tue, 2014-09-09 at 17:24 +0000, Joseph S. Myers wrote:
> On Tue, 9 Sep 2014, Adhemerval Zanella wrote:
> 
> > And patch 0febba23ddabcd971be5259ee20236b9e3efa690 broke powerpc64 configure:
> 
> Steve, you need to put preconfigure back inside a case statement so 
> nothing runs for non-mips* machines.
> 

Ack, I didn't realize that was necessary.  Here is a patch to fix that,
should I go ahead and check it in?  I also made the capitalization
changes that Maciej recommended.

Steve Ellcey
sellcey@mips.com


2014-09-09  Steve Ellcey  <sellcey@mips.com>

	* sysdeps/mips/preconfigure: Put code inside mips* case statement.
	Fix capitalization of error message.


diff --git a/sysdeps/mips/preconfigure b/sysdeps/mips/preconfigure
index fb572d7..c118592 100644
--- a/sysdeps/mips/preconfigure
+++ b/sysdeps/mips/preconfigure
@@ -1,24 +1,29 @@
-abiflag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n
's/^#define _MIPS_SIM \(.*\)/\1/p'`
-mips16flag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n
's/^#define __mips16 \(.*\)/\1/p'`
+case "$machine" in
+mips*)
+	abiflag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n
's/^#define _MIPS_SIM \(.*\)/\1/p'`
+	mips16flag=`$CC $CFLAGS $CPPFLAGS -E -dM -xc /dev/null | sed -n
's/^#define __mips16 \(.*\)/\1/p'`
 
-base_machine=mips
-if test "$abiflag" = "_ABIO32" ; then
-	if test "$mips16flag" = "1" ; then
-		machine=mips/mips32/mips16
+	base_machine=mips
+	if test "$abiflag" = "_ABIO32" ; then
+		if test "$mips16flag" = "1" ; then
+			machine=mips/mips32/mips16
+		else
+			machine=mips/mips32
+		fi
+	elif test "$abiflag" = "_ABIN32" ; then
+		machine=mips/mips64/n32
+	elif test "$abiflag" = "_ABI64" ; then
+		machine=mips/mips64/n64
 	else
-		machine=mips/mips32
+		as_fn_error $? "Unable to determine ABI." "$LINENO" 5
 	fi
-elif test "$abiflag" = "_ABIN32" ; then
-	machine=mips/mips64/n32
-elif test "$abiflag" = "_ABI64" ; then
-	machine=mips/mips64/n64
-else
-	as_fn_error $? "Unable to determine ABI." "$LINENO" 5
-fi
-# $config_machine is not really needed here but the slash after
$machine is
-# needed by the case statement in
sysdeps/unix/sysv/linux/mips/configure.ac.
-machine=$machine/$config_machine
+	# $config_machine is not really needed here but the slash after
+	# $machine is needed by the case statement in
+	# sysdeps/unix/sysv/linux/mips/configure.ac.
+	machine=$machine/$config_machine
 
-if test "$abiflag" != "_ABIO32" -a "$mips16flag" = "1"; then
-	as_fn_error $? "mips16 is only supported with the O32 ABI." "$LINENO"
5
-fi
+	if test "$abiflag" != "_ABIO32" -a "$mips16flag" = "1"; then
+		as_fn_error $? "MIPS16 is only supported with the o32 ABI." "$LINENO"
5
+	fi
+	;;
+esac

Re: [PATCH] Cleanup MIPS preconfigure script

[Joseph S. Myers]

On Tue, 9 Sep 2014, Steve Ellcey wrote:

> On Tue, 2014-09-09 at 17:24 +0000, Joseph S. Myers wrote:
> > On Tue, 9 Sep 2014, Adhemerval Zanella wrote:
> > 
> > > And patch 0febba23ddabcd971be5259ee20236b9e3efa690 broke powerpc64 configure:
> > 
> > Steve, you need to put preconfigure back inside a case statement so 
> > nothing runs for non-mips* machines.
> > 
> 
> Ack, I didn't realize that was necessary.  Here is a patch to fix that,
> should I go ahead and check it in?  I also made the capitalization
> changes that Maciej recommended.

Yes, please commit.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Cleanup MIPS preconfigure script

[Steve Ellcey]

On Tue, 2014-09-09 at 17:48 +0000, Joseph S. Myers wrote:

> > 
> > Ack, I didn't realize that was necessary.  Here is a patch to fix that,
> > should I go ahead and check it in?  I also made the capitalization
> > changes that Maciej recommended.
> 
> Yes, please commit.

OK, I have commited the new patch.  Hopefully that fixes everything. I
didn't realize that all the preconfigure scripts get run when building
so it never occurred to me that changing something under sysdeps/mips
would affect non-mips platforms.  Sorry for the breakage.

Steve Ellcey
sellcey@mips.com

Re: [PATCH] Cleanup MIPS preconfigure script

[Maciej W. Rozycki]

On Tue, 9 Sep 2014, Steve Ellcey wrote:

> >  Hmm, I think the capitalisation is weird here, why not:
> > 
> > +	as_fn_error $? "MIPS16 is only supported with the o32 ABI." "$LINENO" 5
> > 
> > ?
> 
> I already checked it in but I can go back and tweak the capitalization
> if you want.

 That would be my preference, thanks.

 While historically across the toolchain we don't have a very good record 
of keeping the spelling of such stuff correct, I think we really ought to 
keep it consistent with published documentation.  Especially in messages 
shown to the user or external documentation, although it won't hurt doing 
that everywhere including internal documentation, debug messages and 
comments, to make getting good habits easier if nothing else.  

 Inconsistent or bad spelling gives users the impression code itself is 
sloppy and that is something we'd rather avoid.  Having also made sure 
code actually is not sloppy that is of course, that we strive to achieve 
through our review process.

  Maciej

Re: [PATCH] Cleanup MIPS preconfigure script

[Steve Ellcey]

On Tue, 2014-09-09 at 19:21 +0100, Maciej W. Rozycki wrote:
> On Tue, 9 Sep 2014, Steve Ellcey wrote:
> 
> > >  Hmm, I think the capitalisation is weird here, why not:
> > > 
> > > +	as_fn_error $? "MIPS16 is only supported with the o32 ABI." "$LINENO" 5
> > > 
> > > ?
> > 
> > I already checked it in but I can go back and tweak the capitalization
> > if you want.
> 
>  That would be my preference, thanks.

I included this change with the bug fix I checked in.

https://sourceware.org/ml/libc-alpha/2014-09/msg00136.html



Steve Ellcey
sellcey@mips.com