Re: AARCH64 configure check for gas -mabi support

[Yufeng Zhang]

Hi Kugan,

Thanks for the quick action.

On 12/09/13 11:20, Kugan wrote:
> Thanks Yufeng for the review.
>
> On 07/12/13 03:18, Yufeng Zhang wrote:
>
> > > >>  gcc trunk aarch64 bootstrapping fails with gas version 2.23.2 (with
> > > >>  error message similar to cannot compute suffix of object files) as this
> > > >>  particular version does not support -mabi=lp64. It succeeds with later
> > > >>  versions of gas that supports -mabi.
> > >
> > >  The -mabi option was introduced to gas when the support for ILP32 was
> > >  added.  Initially the options were named -milp32 and -mlp64:
> > >
> > >     http://sourceware.org/ml/binutils/2013-06/msg00178.html
> > >
> > >  and later on they were change to -mabi=ilp32 and -mabi=lp64 for
> > >  consistency with those in the aarch64 gcc:
> > >
> > >     http://sourceware.org/ml/binutils/2013-07/msg00180.html
> > >
> > >  The following gcc patch made the driver use the explicit option to drive
> > >  gas:
> > >
> > >     http://gcc.gnu.org/ml/gcc-patches/2013-07/msg00083.html
> > >
> > >  It is a neglect of the backward compatibility with binutils 2.23.
> > >
> > > >>
> > > >>  Attached patch add checking for -mabi=lp64 and prompts upgradation. Is
> > > >>  this Ok?
> > >
> > >  I think instead of mandating the support for the -mabi option, the
> > >  compiler shall be changed able to work with binutils 2.23.  The 2.23
> > >  binutils have a good support for aarch64 and the main difference from
> > >  2.24 is the ILP32 support.  I think it is necessary to maintain the
> > >  backward compatibility, and it should be achieved by suppressing the
> > >  compiler's support for ILP32 when the -mabi option is not found
> > >  available in gas during the configuration time.
> > >
> > >  I had a quick look at areas need to be updated:
> > >
> > >  * multilib support
> > >
> > >  In gcc/config.gcc, the default and the only accepted value for
> > >  --with-multilib-list and --with-abi shall be lp64 when -mabi is not
> > >  available.
> > >
> > >  * -mabi option
> > >
> > >  I suggest we keep the -mabi option, but reject -mabi=ilp32 in
> > >  gcc/config/aarch64/aarch64.c:aarch64_override_options ()
> > >
> > >  * driver spec
> > >
> > >  In gcc/config/aarch64/aarch64-elf.h, the DRIVER_SELF_SPECS and ASM_SPEC
> > >  shall be updated to not pass/specify -mabi for gas.
> > >
> > >  * documentation
> > >
> > >  I think it needs to be mentioned in gcc/doc/install.texi the constraint
> > >  of using pre-2.24 binutils with aarch64 gcc that is 4.9 or later.
> > >
> > >  It is a quick scouting, but hopefully it has provided provide some
> > >  guidance.  If you need more help, just let me know.
> > >
> > >
> > >  Yufeng
> > >
> > >  P.s. some minor comments on the attached patch.
> > >
> > > >>
> > > >>  diff --git a/gcc/configure b/gcc/configure
> > > >>  index fdf0cd0..17b6e85 100755
> > > >>  --- a/gcc/configure
> > > >>  +++ b/gcc/configure
> > >
> > >  Diff result of auto-generation is usually excluded from a patch.
> > >
> > > >>  diff --git a/gcc/configure.ac b/gcc/configure.ac
> > > >>  index 91a22d5..730ada0 100644
> > > >>  --- a/gcc/configure.ac
> > > >>  +++ b/gcc/configure.ac
> > > >>  @@ -3532,6 +3532,15 @@ case "$target" in
> > > >>             [Define if your assembler supports the -no-mul-bug-abort
> > > >>  option.])])
> > > >>         ;;
> > > >>
> > > >>  + aarch64-*-*)
> > >
> > >  aarch64*-*-*
> > >
> > > >>  +    gcc_GAS_CHECK_FEATURE([-mabi option],
> > > >>  +      gcc_cv_as_aarch64_mabi,,
> > > >>  +      [-mabi=lp64], [.text],,,)
> > > >>  +    if test x$gcc_cv_as_aarch64_mabi = xno; then
> > > >>  +    AC_MSG_ERROR([Assembler support for -mabi=lp64 is required.
> > > >>  Upgrade the Assembler.])
> > > >>  +    fi
> > > >>  +    ;;
> > > >>  +
> > > >>       sparc*-*-*)
> > > >>         gcc_GAS_CHECK_FEATURE([.register], gcc_cv_as_sparc_register_op,,,
> > > >>           [.register %g2, #scratch],,
> > > >>
> > >
> > >
> Here is an attempt to do it the way you have suggested.
>
> Thanks,
> Kugan
>
> gcc/
>
> +2013-12-09  Kugan Vivekanandarajah<address@hidden>
> +       * configure.ac: Add check for aarch64 assembler -mabi support.
> +       * configure: Regenerate.
> +       * config.in: Regenerate.
> +       * config/aarch64/aarch64-elf.h (ASM_MABI_SPEC): New define.
> +       (ASM_SPEC): Update to substitute -mabi with ASM_MABI_SPEC.
> +       * config/aarch64/aarch64.h (aarch64_override_options):  Issue error if
> +       Assembler does not support -mabi and option ilp32 is selected.

Assembler/assembler

> +       * doc/install.texi: Added note that building gcc 4.9 and after with pre
> +       2.24 binutils will not support -mabi=ilp32.
> +
>
>
>
> p.txt
>
>
> diff --git a/gcc/config/aarch64/aarch64-elf.h b/gcc/config/aarch64/aarch64-elf.h
> index 4757d22..b260b7c 100644
> --- a/gcc/config/aarch64/aarch64-elf.h
> +++ b/gcc/config/aarch64/aarch64-elf.h
> @@ -134,13 +134,19 @@
>     " %{!mbig-endian:%{!mlittle-endian:" ENDIAN_SPEC "}}" \
>     " %{!mabi=*:" ABI_SPEC "}"
>
> +#ifdef HAVE_AS_MABI_OPTION
> +#define ASM_MABI_SPEC  "%{mabi=*:-mabi=%*}"
> +#else
> +#define ASM_MABI_SPEC  "%{mabi=lp64*:}"

Is '*' necessary here?

> +#endif
> +
>   #ifndef ASM_SPEC
>   #define ASM_SPEC "\
>   %{mbig-endian:-EB} \
>   %{mlittle-endian:-EL} \
>   %{mcpu=*:-mcpu=%*} \
> -%{march=*:-march=%*} \
> -%{mabi=*:-mabi=%*}"
> +%{march=*:-march=%*}" \
> +ASM_MABI_SPEC
>   #endif
>
>   #undef TYPE_OPERAND_FMT
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index b1b4eef..c1a9cbd 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -5186,6 +5186,10 @@ aarch64_override_options (void)
>       {
>         aarch64_parse_tune ();
>       }
> +#ifndef HAVE_AS_MABI_OPTION
> +  if (TARGET_ILP32)
> +    error ("Assembler does not supprt -mabi=ilp32");
> +#endif

A blank line before #ifndef and some comment to explain the reason please.

>     initialize_aarch64_code_model ();
>
> diff --git a/gcc/configure.ac b/gcc/configure.ac
> index 91a22d5..fcfdbdb 100644
> --- a/gcc/configure.ac
> +++ b/gcc/configure.ac
> @@ -3532,6 +3532,19 @@ case "$target" in
>                 [Define if your assembler supports the -no-mul-bug-abort 
> option.])])
>       ;;
>
> + aarch64*-*-*)

Alphabetically, this should be placed before alpha*.

> +    gcc_GAS_CHECK_FEATURE([-mabi option],
> +      gcc_cv_as_aarch64_mabi,,
> +      [-mabi=lp64], [.text],,,)
> +    if test $gcc_cv_as_aarch64_mabi = yes ; then
> +       AC_DEFINE(HAVE_AS_MABI_OPTION, 1,
> +                 [Define if your assembler supports the -mabi option.])
> +    fi
> +    if test x$gcc_cv_as_aarch64_mabi = xno&&  test x$with_abi = xilp32; then
> +       AC_MSG_ERROR([Assembler doesnot support -mabi=ilp32. Upgrade the 
> Assembler.])
> +    fi
> +    ;;

It is not sufficient to only check with_abi itself.  By default, 
aarch64*-*-elf builds both ilp32 and lp64 libraries (e.g. libgcc).  This 
needs to be turned off if test x$gcc_cv_as_aarch64_mabi = xno.  We also 
need to detect the situation where users explicitly configure the 
toolchain with --with-multilib-list=lp64,ilp32

Here is an incremental diff based on your change to gcc/configure.ac to 
give an example on a more thorough check:

diff --git a/gcc/configure.ac b/gcc/configure.ac
index c8cf274..c590ad7 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -3488,12 +3488,27 @@ case "$target" in
     gcc_GAS_CHECK_FEATURE([-mabi option],
       gcc_cv_as_aarch64_mabi,,
       [-mabi=lp64], [.text],,,)
-    if test $gcc_cv_as_aarch64_mabi = yes ; then
+    if test x$gcc_cv_as_aarch64_mabi = xyes ; then
        AC_DEFINE(HAVE_AS_MABI_OPTION, 1,
                  [Define if your assembler supports the -mabi option.])
-    fi
-    if test x$gcc_cv_as_aarch64_mabi = xno && test x$with_abi = xilp32; 
then
-	AC_MSG_ERROR([Assembler doesnot support -mabi=ilp32. Upgrade the 
Assembler.])
+    else
+       if test x$with_abi = xilp32; then
+	  AC_MSG_ERROR([Assembler does not support -mabi=ilp32.  Upgrade the 
Assembler.])
+       fi
+       if test x"$with_multilib_list" = xdefault; then
+         TM_MULTILIB_CONFIG=lp64
+       else
+         aarch64_multilibs=`echo $with_multilib_list | sed -e 's/,/ /g'`
+         for aarch64_multilib in ${aarch64_multilibs}; do
+           case ${aarch64_multilib} in
+             ilp32 )
+		AC_MSG_ERROR([Assembler does not support -mabi=ilp32.  Upgrade the 
Assembler.])
+               ;;
+             *)
+               ;;
+           esac
+         done
+       fi
     fi
     ;;


> +
>     sparc*-*-*)
>       gcc_GAS_CHECK_FEATURE([.register], gcc_cv_as_sparc_register_op,,,
>         [.register %g2, #scratch],,
> diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
> index a8f9f8a..e4de2d2 100644
> --- a/gcc/doc/install.texi
> +++ b/gcc/doc/install.texi
> @@ -3735,6 +3735,15 @@ removed and the system libunwind library will always be 
> used.
>
>   @html
>   <hr />
> address@hidden html
> address@hidden
> address@hidden aarch64-*-*

aarch64*-*-*


Please also indicate what tests you have done with the patch.


Thanks,
Yufeng

> +Pre 2.24 binutils does not have support for selecting -mabi and does not
> +support ILP32.  If GCC 4.9 or later is built with pre 2.24, GCC will not
> +support option -mabi=ilp32.
> +
> address@hidden
> +<hr />
>   <!-- rs6000-ibm-aix*, powerpc-ibm-aix* -->
>   @end html
>   @anchor{x-ibm-aix}