Cleanup MIPS preconfigure script
diff mbox

Message ID 1410279506.2740.158.camel@ubuntu-sellcey
State New
Headers show

Commit Message

Steve Ellcey Sept. 9, 2014, 4:18 p.m. UTC
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.

Comments

Joseph Myers Sept. 9, 2014, 4:39 p.m. UTC | #1
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.
Maciej W. Rozycki Sept. 9, 2014, 5:08 p.m. UTC | #2
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
Steve Ellcey Sept. 9, 2014, 5:13 p.m. UTC | #3
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
Adhemerval Zanella Sept. 9, 2014, 5:22 p.m. UTC | #4
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.
Joseph Myers Sept. 9, 2014, 5:24 p.m. UTC | #5
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.
Maciej W. Rozycki Sept. 9, 2014, 6:21 p.m. UTC | #6
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
Steve Ellcey Sept. 9, 2014, 6:30 p.m. UTC | #7
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

Patch
diff mbox

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