Patchwork PATCH: Correctly configure all big-endian ARM archs, not just arm*-*-linux-*.

login
register
mail settings
Submitter Seth LaForge
Date Feb. 15, 2013, 7:15 p.m.
Message ID <CAC_HCCPwjkdygbmLzeC4mJ+mMXnyz3tUSV_2tbFkzZqW+NqNHA@mail.gmail.com>
Download mbox | patch
Permalink /patch/220821/
State New
Headers show

Comments

Seth LaForge - Feb. 15, 2013, 7:15 p.m.
On Fri, Feb 15, 2013 at 10:41 AM, Richard Earnshaw <rearnsha@arm.com> wrote:
> On 15/02/13 18:33, Seth LaForge wrote:
>> If so, I could certainly change the test to use a regexp, or inline it
>> all of the ARM handlers in the case statement below.
>
> Yes, anything with linux in it will now be a quadruplet.  There may be other
> cases as well.  Once a rule has been broken for one case, you can hardly
> enforce it against others.  The rot has set in...

Ooo, messy.

Given that config.gcc seems to have a lot of assumptions that $target
is a triplet baked in, seems like it'd make sense to have a bit of
code at the top that parsed target into four variables, and then have
the various case statements use those variables, something like:

# arm-blob-linux-gnueabi -> proc=arm thingy=blob os=linux abi=gnueabi
proc=`expr ${target} : '^\([^-]*\)-'`
thingy=`expr ${target} : '^[^-]*-\([^-]*\)-[^-]*-[^-]*$'`
os=`expr ${target} : '^.*-\([^-]*\)-[^-]*$'`
abi=`expr ${target} : '^.*-[^-]*-\([^-]*\)$'`

case $proc/$thingy/$os/$abi in
  arm*/blob/linux/*) ... ;;
  arm*/*/*/eabi) ... ;;
esac

That'd be a major clean-up, though.  How about the version of my patch
below?  If you hate the use of expr, I could inline the test into all
of the ARM cases below, but I don't like that approach since it's what
caused this problem in the first place (somebody adding BE support to
one ARM arch without adding it to the others).

Seth

 	tm_file="$tm_file arm/bpabi.h arm/linux-eabi.h arm/aout.h arm/arm.h"
 	# Define multilib configuration for arm-linux-androideabi.
Mike Stump - Feb. 15, 2013, 11:29 p.m.
On Feb 15, 2013, at 11:15 AM, Seth LaForge <sethml@google.com> wrote:
> On Fri, Feb 15, 2013 at 10:41 AM, Richard Earnshaw <rearnsha@arm.com> wrote:
>> On 15/02/13 18:33, Seth LaForge wrote:
>>> If so, I could certainly change the test to use a regexp, or inline it
>>> all of the ARM handlers in the case statement below.
>> 
>> Yes, anything with linux in it will now be a quadruplet.  There may be other
>> cases as well.  Once a rule has been broken for one case, you can hardly
>> enforce it against others.  The rot has set in...
> 
> Ooo, messy.
> 
> Given that config.gcc seems to have a lot of assumptions that $target
> is a triplet baked in, seems like it'd make sense to have a bit of
> code at the top that parsed target into four variables, and then have
> the various case statements use those variables, something like:

No.  That is wrong.  We can't be messy if we had structure.  The messy is to forever defeat any and every strict mandate that one could ever envision.  I know, seems wrong and backwards, but there is a reason for it.  By having it loose, we get to adapt it anyway we want over long periods of time to meet any need we might then have.

> # arm-blob-linux-gnueabi -> proc=arm thingy=blob os=linux abi=gnueabi
> proc=`expr ${target} : '^\([^-]*\)-'`
> thingy=`expr ${target} : '^[^-]*-\([^-]*\)-[^-]*-[^-]*$'`
> os=`expr ${target} : '^.*-\([^-]*\)-[^-]*$'`
> abi=`expr ${target} : '^.*-[^-]*-\([^-]*\)$'`
> 
> case $proc/$thingy/$os/$abi in
>  arm*/blob/linux/*) ... ;;
>  arm*/*/*/eabi) ... ;;
> esac

You've just reinvented target triplets but changed the - to be a /.  Since they are totally isomorphic, you've not done anything but change it, and change is bad.

> How about the version of my patch

> below?

No.  Counter proposal, let's handle the cases that don't work.  So, you said in your original email that armeb-unknown-eabi doesn't work.

So, in the existing case statement for:

arm*-*-eabi*)

let's just add:

        case ${target} in
        armeb-*-eabi*)
	  tm_defines="${tm_defines} TARGET_BIG_ENDIAN_DEFAULT=1"
        esac

Is this not exactly what you want?  Doesn't this solve exactly what you stated was the problem?

> If you hate the use of expr,

I do.

> I could inline the test into all
> of the ARM cases below, but I don't like that approach since it's what
> caused this problem in the first place (somebody adding BE support to
> one ARM arch without adding it to the others).

And does it work on uclinux?  Does it work on rtems?  Does it work on every arm that every existed and will exist?  If the answer is no, then it is less ideal than putting this in the config for eabi*).  If it always works, then moving the existing on up to the existing:

case ${target} in                                                                                                                          
i[34567]86-*-*)           

would be the right approach.  x86 uses this location already to set tm_defines="${tm_defines} USE_IX86_FRAME_POINTER=1" for example.  The documentation above that group can state that this is the location for things that are cpu specific and os and vendor neutral.
Seth LaForge - Feb. 20, 2013, 11:21 p.m.
On Fri, Feb 15, 2013 at 3:29 PM, Mike Stump <mikestump@comcast.net> wrote:
> No.  Counter proposal, let's handle the cases that don't work.  So, you said in your original email that armeb-unknown-eabi doesn't work.
>
> So, in the existing case statement for:
>
> arm*-*-eabi*)
>
> let's just add:
>
>         case ${target} in
>         armeb-*-eabi*)
>           tm_defines="${tm_defines} TARGET_BIG_ENDIAN_DEFAULT=1"
>         esac
>
> Is this not exactly what you want?  Doesn't this solve exactly what you stated was the problem?

OK, attached patch
0001-Add-TARGET_BIG_ENDIAN_DEFAULT-1-for-all-armeb-eabi-a.patch does
as you describe, and works for my particular use-case.

>> I could inline the test into all
>> of the ARM cases below, but I don't like that approach since it's what
>> caused this problem in the first place (somebody adding BE support to
>> one ARM arch without adding it to the others).
>
> And does it work on uclinux?  Does it work on rtems?  Does it work on every arm that every existed and will exist?  If the answer is no, then it is less ideal than putting this in the config for eabi*).

Well, the current config is certainly broken when giving a big-endian
spec for uclinux, rtems, and every other arm that ever existed or will
exist.  It's possible there are other issues with using a big-endian
processor for uclinux, rtems, etc, but adding
TARGET_BIG_ENDIAN_DEFAULT=1 certainly gets those targets closer to
working.

> If it always works, then moving the existing on up to the existing:
>
> case ${target} in
> i[34567]86-*-*)
>
> would be the right approach.  x86 uses this location already to set tm_defines="${tm_defines} USE_IX86_FRAME_POINTER=1" for example.  The documentation above that group can state that this is the location for things that are cpu specific and os and vendor neutral.

I like that solution better.  Following your suggested list of
big-endian architectures, attached patch
0001-Add-TARGET_BIG_ENDIAN_DEFAULT-1-for-all-arm-big-endi.patch adds
TARGET_BIG_ENDIAN_DEFAULT=1 for $target matching "armeb-* | armbe-* |
armv[3-8]b-*".  I left out xscaleeb, since config.sub canonicalizes
xscaleeb-* to armeb-*.

Please, pick whichever patch pleases you most.  I prefer
0001-Add-TARGET_BIG_ENDIAN_DEFAULT-1-for-all-arm-big-endi.patch.

Changelog entry for
0001-Add-TARGET_BIG_ENDIAN_DEFAULT-1-for-all-arm-big-endi.patch:

gcc/
	* config.gcc: Add TARGET_BIG_ENDIAN_DEFAULT=1 for all arm big-endian archs.

Changelog entry for
0001-Add-TARGET_BIG_ENDIAN_DEFAULT-1-for-all-armeb-eabi-a.patch:

gcc/
	* config.gcc: Add TARGET_BIG_ENDIAN_DEFAULT=1 for all armeb-*-eabi* archs.

Seth

Patch

diff -urp gcc-4.8-20130210.orig/gcc/config.gcc gcc-4.8-20130210/gcc/config.gcc
--- gcc-4.8-20130210.orig/gcc/config.gcc	2013-02-08 08:02:47.000000000 -0800
+++ gcc-4.8-20130210/gcc/config.gcc	2013-02-15 11:01:56.049978834 -0800
@@ -809,6 +809,11 @@  case ${target} in
   ;;
 esac

+# Handle big-endian ARM architectures.
+if expr ${target} : 'arm[^-]*b-' >/dev/null ; then
+  tm_defines="${tm_defines} TARGET_BIG_ENDIAN_DEFAULT=1"
+fi
+
 case ${target} in
 aarch64*-*-elf)
 	tm_file="${tm_file} dbxelf.h elfos.h newlib-stdint.h"
@@ -867,11 +872,6 @@  arm*-*-netbsdelf*)
 	;;
 arm*-*-linux-*)			# ARM GNU/Linux with ELF
 	tm_file="dbxelf.h elfos.h gnu-user.h linux.h linux-android.h
glibc-stdint.h arm/elf.h arm/linux-gas.h arm/linux-elf.h"
-	case $target in
-	arm*b-*-linux*)
-	    tm_defines="${tm_defines} TARGET_BIG_ENDIAN_DEFAULT=1"
-	    ;;
-	esac
 	tmake_file="${tmake_file} arm/t-arm arm/t-arm-elf arm/t-bpabi
arm/t-linux-eabi"