diff mbox

[AArch64] Detect assembler vector mov parsing bug

Message ID 53C43559.5000907@twiddle.net
State New
Headers show

Commit Message

Richard Henderson July 14, 2014, 7:54 p.m. UTC
The strchr implementation that was recently committed to mainline tickles an
assembler bug fixed by

commit 8db49cc2de52033990ae5d4d6aacedc8f986e803
Author: Will Newton <will.newton@linaro.org>
Date:   Thu Oct 31 17:21:11 2013 -0700

    config/tc-aarch64.c: Avoid trying to parse a vector mov as immediate.
    ...

Obviously not a problem with a relatively new binutils installed, but Carlos
ran into this problem building mainline glibc on a system using binutils 2.23.

It's not a silent error (we die linking ld.so), but its somewhat cryptic.
Carlos opined to me that we ought to error out at configure time with an
understandable message.

Rather than check for version numbers, let's check for functionality.

Ok?


r~
* sysdeps/aarch64/configure.ac: Check for assembler mov bug.
	* sysdeps/aarch64/configure: Regenerate.

Comments

Richard Earnshaw July 15, 2014, 1:38 p.m. UTC | #1
On 14/07/14 20:54, Richard Henderson wrote:
> The strchr implementation that was recently committed to mainline tickles an
> assembler bug fixed by
> 
> commit 8db49cc2de52033990ae5d4d6aacedc8f986e803
> Author: Will Newton <will.newton@linaro.org>
> Date:   Thu Oct 31 17:21:11 2013 -0700
> 
>     config/tc-aarch64.c: Avoid trying to parse a vector mov as immediate.
>     ...
> 
> Obviously not a problem with a relatively new binutils installed, but Carlos
> ran into this problem building mainline glibc on a system using binutils 2.23.
> 
> It's not a silent error (we die linking ld.so), but its somewhat cryptic.
> Carlos opined to me that we ought to error out at configure time with an
> understandable message.
> 
> Rather than check for version numbers, let's check for functionality.
> 
> Ok?
> 
> 

We could probably work around the problem by using "mov x<n>, d<m>",
since the two operations are architecturally equivalent.  But I think
the vector style more accurately reflects the intent of the code.  Since
we are only reading, rather than writing, part of the vector, I don't
think there are likely to be major performance issues from such a change.

I haven't tested the above, hence the 'probably'.

R.

> r~
> 
> 
> z
> 
> 
> 	* sysdeps/aarch64/configure.ac: Check for assembler mov bug.
> 	* sysdeps/aarch64/configure: Regenerate.
> 
> diff --git a/sysdeps/aarch64/configure.ac b/sysdeps/aarch64/configure.ac
> index 7851dd4..6654684 100644
> --- a/sysdeps/aarch64/configure.ac
> +++ b/sysdeps/aarch64/configure.ac
> @@ -20,3 +20,19 @@ if test $libc_cv_aarch64_be = yes; then
>  else
>    LIBC_CONFIG_VAR([default-abi], [lp64])
>  fi
> +
> +# Check to see if the assembler has fixed a parsing bug wrt vector "mov"
> +# instructions leading to undefined symbols.
> +AC_MSG_CHECKING([for assembler vector mov bug])
> +old_LDFLAGS="$LDFLAGS"
> +LDFLAGS="$LDFLAGS -nostdlib -nostartfiles"
> +AC_LINK_IFELSE([[void _start(void) { asm("mov x0,v0.2d[0]"); }]],
> +  [AC_MSG_RESULT([no])],
> +  [AC_MSG_RESULT([yes])
> +   AC_MSG_ERROR(
> +[*** Version of binutils is too old to support AArch64.
> +*** Your version of binutils contains a bug in the handling
> +*** of vector names. Please upgrade to a version of binutils
> +*** that contains commit:
> +*** 8db49cc2de52033990ae5d4d6aacedc8f986e803])])
> +LDFLAGS="$old_LDFLAGS"
>
Marcus Shawcroft July 15, 2014, 2:41 p.m. UTC | #2
On 15 July 2014 14:38, Richard Earnshaw <rearnsha@arm.com> wrote:
> On 14/07/14 20:54, Richard Henderson wrote:
>> The strchr implementation that was recently committed to mainline tickles an
>> assembler bug fixed by
>>
>> commit 8db49cc2de52033990ae5d4d6aacedc8f986e803
>> Author: Will Newton <will.newton@linaro.org>
>> Date:   Thu Oct 31 17:21:11 2013 -0700
>>
>>     config/tc-aarch64.c: Avoid trying to parse a vector mov as immediate.
>>     ...
>>
>> Obviously not a problem with a relatively new binutils installed, but Carlos
>> ran into this problem building mainline glibc on a system using binutils 2.23.
>>
>> It's not a silent error (we die linking ld.so), but its somewhat cryptic.
>> Carlos opined to me that we ought to error out at configure time with an
>> understandable message.
>>
>> Rather than check for version numbers, let's check for functionality.
>>
>> Ok?
>>
>>
>
> We could probably work around the problem by using "mov x<n>, d<m>",
> since the two operations are architecturally equivalent.  But I think
> the vector style more accurately reflects the intent of the code.  Since
> we are only reading, rather than writing, part of the vector, I don't
> think there are likely to be major performance issues from such a change.
>
> I haven't tested the above, hence the 'probably'.

The more recent binutils 2.24 release does not have this issue. Rather
than tweak the code to work around the broken 2.23 binutils I'd prefer
to go with RTH's proposal and simply barf the build.

/Marcus
Will Newton July 23, 2014, 1:55 p.m. UTC | #3
On 14 July 2014 20:54, Richard Henderson <rth@twiddle.net> wrote:
> The strchr implementation that was recently committed to mainline tickles an
> assembler bug fixed by
>
> commit 8db49cc2de52033990ae5d4d6aacedc8f986e803
> Author: Will Newton <will.newton@linaro.org>
> Date:   Thu Oct 31 17:21:11 2013 -0700
>
>     config/tc-aarch64.c: Avoid trying to parse a vector mov as immediate.
>     ...
>
> Obviously not a problem with a relatively new binutils installed, but Carlos
> ran into this problem building mainline glibc on a system using binutils 2.23.
>
> It's not a silent error (we die linking ld.so), but its somewhat cryptic.
> Carlos opined to me that we ought to error out at configure time with an
> understandable message.
>
> Rather than check for version numbers, let's check for functionality.
>
> Ok?

This looks ok to me, although it might be nice to mention that 2.24 is
a version containing the fix.
diff mbox

Patch

diff --git a/sysdeps/aarch64/configure.ac b/sysdeps/aarch64/configure.ac
index 7851dd4..6654684 100644
--- a/sysdeps/aarch64/configure.ac
+++ b/sysdeps/aarch64/configure.ac
@@ -20,3 +20,19 @@  if test $libc_cv_aarch64_be = yes; then
 else
   LIBC_CONFIG_VAR([default-abi], [lp64])
 fi
+
+# Check to see if the assembler has fixed a parsing bug wrt vector "mov"
+# instructions leading to undefined symbols.
+AC_MSG_CHECKING([for assembler vector mov bug])
+old_LDFLAGS="$LDFLAGS"
+LDFLAGS="$LDFLAGS -nostdlib -nostartfiles"
+AC_LINK_IFELSE([[void _start(void) { asm("mov x0,v0.2d[0]"); }]],
+  [AC_MSG_RESULT([no])],
+  [AC_MSG_RESULT([yes])
+   AC_MSG_ERROR(
+[*** Version of binutils is too old to support AArch64.
+*** Your version of binutils contains a bug in the handling
+*** of vector names. Please upgrade to a version of binutils
+*** that contains commit:
+*** 8db49cc2de52033990ae5d4d6aacedc8f986e803])])
+LDFLAGS="$old_LDFLAGS"