diff mbox series

[PR83492] Fix selection of aarch64 big-endian shift parameters based on __AARCH64EB__

Message ID 20171219171011.GA9667@weiser.dinsnail.net
State New
Headers show
Series [PR83492] Fix selection of aarch64 big-endian shift parameters based on __AARCH64EB__ | expand

Commit Message

Michael Weiser Dec. 19, 2017, 5:10 p.m. UTC 
Hello,

below patch fixes PR 83492.

Running the preprocessor on any source gives various errors on
aarch64_be. These suggest that the proprocessor misidentifies token
boundaries. This can be traced to an optimized search_line_fast for
aarch64 in libcpp/lex.c which has a special provision for aarch64_be
based on the __AARCH64EB. This check can never succeed since the correct
name of the macro is __AARCH64EB__. Consequently the code for big-endian
aarch64 is never activated. As expected, changing __AARCH64EB to
__AARCH64EB__ or disabling the accelerated routine by substituting
search_line_acc_char() fixes the problem.

Thanks,
Michael

2017-12-19  Michael Weiser  <michael.weiser@gmx.de>

	PR preprocessor/83492
	* lex.c (search_line_fast) [__ARM_NEON && __ARM_64BIT_STATE]:
	Fix selection of big-endian shift parameters based on __AARCH64EB__.

Comments

Kyrill Tkachov Dec. 19, 2017, 5:35 p.m. UTC | #1 
Hi Michael,

[cc'ing the aarch64 maintainers]

On 19/12/17 17:10, Michael Weiser wrote:
> Hello,
>
> below patch fixes PR 83492.
>
> Running the preprocessor on any source gives various errors on
> aarch64_be. These suggest that the proprocessor misidentifies token
> boundaries. This can be traced to an optimized search_line_fast for
> aarch64 in libcpp/lex.c which has a special provision for aarch64_be
> based on the __AARCH64EB. This check can never succeed since the correct
> name of the macro is __AARCH64EB__. Consequently the code for big-endian
> aarch64 is never activated. As expected, changing __AARCH64EB to
> __AARCH64EB__ or disabling the accelerated routine by substituting
> search_line_acc_char() fixes the problem.
>
> Thanks,
> Michael
>

I agree with your analysis of the bug and your patch will fix the
problem when compiling with GCC. However, I believe the more
appropriate macro to check here is __ARM_BIG_ENDIAN as that
is the macro defined by the ACLE specification [1].

__AARCH64EB__ is indeed defined by aarch64 GCC when compiling
for big-endian but I don't think it's standardised and may in theory
not be defined by other host compilers the user may use to compile GCC.

But, it may not be a big deal in practice. It's up to the maintainers...

Thanks,
Kyrill

[1] https://static.docs.arm.com/ihi0053/c/IHI0053C_acle_2_0.pdf

> 2017-12-19  Michael Weiser  <michael.weiser@gmx.de>
>
>         PR preprocessor/83492
>         * lex.c (search_line_fast) [__ARM_NEON && __ARM_64BIT_STATE]:
>         Fix selection of big-endian shift parameters based on 
> __AARCH64EB__.
>
> Index: lex.c
> ===================================================================
> --- lex.c       (revision 255828)
> +++ lex.c       (working copy)
> @@ -772,7 +772,7 @@
>    const uint8x16_t repl_qm = vdupq_n_u8 ('?');
>    const uint8x16_t xmask = (uint8x16_t) vdupq_n_u64 
> (0x8040201008040201ULL);
>
> -#ifdef __AARCH64EB
> +#ifdef __AARCH64EB__
>    const int16x8_t shift = {8, 8, 8, 8, 0, 0, 0, 0};
>  #else
>    const int16x8_t shift = {0, 0, 0, 0, 8, 8, 8, 8};
Michael Weiser Dec. 19, 2017, 6:01 p.m. UTC | #2 
Hi Kyrill,

On Tue, Dec 19, 2017 at 05:35:03PM +0000, Kyrill Tkachov wrote:

> > below patch fixes PR 83492.
> I agree with your analysis of the bug and your patch will fix the
> problem when compiling with GCC. However, I believe the more
> appropriate macro to check here is __ARM_BIG_ENDIAN as that
> is the macro defined by the ACLE specification [1].

> __AARCH64EB__ is indeed defined by aarch64 GCC when compiling
> for big-endian but I don't think it's standardised and may in theory
> not be defined by other host compilers the user may use to compile GCC.

Since the macro is used in various places around the code, I'd suggest
fixing this bug and getting back in line with the rest of the codebase
by applying my patch as-is. The move towards a more appropriate macro to
base the checks on can then be done in a separate step using
search'n'replace.

$ grep -r __AARCH64EB__ . | cut -d: -f1 | sort -u | grep -v \.svn
./gcc/config/aarch64/aarch64-c.c
./gcc/config/aarch64/arm_neon.h
./gcc/testsuite/lib/target-supports.exp
./libcpp/ChangeLog
./libcpp/lex.c
./libffi/src/aarch64/ffi.c
./libffi/src/aarch64/sysv.S
./libgcc/config/aarch64/linux-unwind.h
./libgcc/config/aarch64/sfp-machine.h
./libstdc++-v3/config/cpu/aarch64/opt/ext/opt_random.h
Jakub Jelinek Dec. 19, 2017, 6:23 p.m. UTC | #3 
On Tue, Dec 19, 2017 at 07:01:44PM +0100, Michael Weiser wrote:
> Hi Kyrill,
> 
> On Tue, Dec 19, 2017 at 05:35:03PM +0000, Kyrill Tkachov wrote:
> 
> > > below patch fixes PR 83492.
> > I agree with your analysis of the bug and your patch will fix the
> > problem when compiling with GCC. However, I believe the more
> > appropriate macro to check here is __ARM_BIG_ENDIAN as that
> > is the macro defined by the ACLE specification [1].
> 
> > __AARCH64EB__ is indeed defined by aarch64 GCC when compiling
> > for big-endian but I don't think it's standardised and may in theory
> > not be defined by other host compilers the user may use to compile GCC.
> 
> Since the macro is used in various places around the code, I'd suggest
> fixing this bug and getting back in line with the rest of the codebase
> by applying my patch as-is. The move towards a more appropriate macro to
> base the checks on can then be done in a separate step using
> search'n'replace.
> 
> $ grep -r __AARCH64EB__ . | cut -d: -f1 | sort -u | grep -v \.svn
> ./gcc/config/aarch64/aarch64-c.c
> ./gcc/config/aarch64/arm_neon.h
> ./gcc/testsuite/lib/target-supports.exp
> ./libcpp/ChangeLog
> ./libcpp/lex.c
> ./libffi/src/aarch64/ffi.c
> ./libffi/src/aarch64/sysv.S
> ./libgcc/config/aarch64/linux-unwind.h
> ./libgcc/config/aarch64/sfp-machine.h
> ./libstdc++-v3/config/cpu/aarch64/opt/ext/opt_random.h

That doesn't tell anything on what should be used in libcpp.  Unlike the
rest, libcpp is a library that needs to be built on the host, with the
host compiler (which doesn't have to be GCC).  The only case that is in
the same category is opt_random.h, because it is an installed header and
so again, can be used by GCC or other compilers.
GCC isn't going to stop defining __AARCH64EB__ nor __ARM_BIG_ENDIAN,
it is purely about other compilers.

	Jakub
Jeff Law Dec. 20, 2017, 12:23 a.m. UTC | #4 
On 12/19/2017 11:23 AM, Jakub Jelinek wrote:
> On Tue, Dec 19, 2017 at 07:01:44PM +0100, Michael Weiser wrote:
>> Hi Kyrill,
>>
>> On Tue, Dec 19, 2017 at 05:35:03PM +0000, Kyrill Tkachov wrote:
>>
>>>> below patch fixes PR 83492.
>>> I agree with your analysis of the bug and your patch will fix the
>>> problem when compiling with GCC. However, I believe the more
>>> appropriate macro to check here is __ARM_BIG_ENDIAN as that
>>> is the macro defined by the ACLE specification [1].
>>
>>> __AARCH64EB__ is indeed defined by aarch64 GCC when compiling
>>> for big-endian but I don't think it's standardised and may in theory
>>> not be defined by other host compilers the user may use to compile GCC.
>>
>> Since the macro is used in various places around the code, I'd suggest
>> fixing this bug and getting back in line with the rest of the codebase
>> by applying my patch as-is. The move towards a more appropriate macro to
>> base the checks on can then be done in a separate step using
>> search'n'replace.
>>
>> $ grep -r __AARCH64EB__ . | cut -d: -f1 | sort -u | grep -v \.svn
>> ./gcc/config/aarch64/aarch64-c.c
>> ./gcc/config/aarch64/arm_neon.h
>> ./gcc/testsuite/lib/target-supports.exp
>> ./libcpp/ChangeLog
>> ./libcpp/lex.c
>> ./libffi/src/aarch64/ffi.c
>> ./libffi/src/aarch64/sysv.S
>> ./libgcc/config/aarch64/linux-unwind.h
>> ./libgcc/config/aarch64/sfp-machine.h
>> ./libstdc++-v3/config/cpu/aarch64/opt/ext/opt_random.h
> 
> That doesn't tell anything on what should be used in libcpp.  Unlike the
> rest, libcpp is a library that needs to be built on the host, with the
> host compiler (which doesn't have to be GCC).  The only case that is in
> the same category is opt_random.h, because it is an installed header and
> so again, can be used by GCC or other compilers.
> GCC isn't going to stop defining __AARCH64EB__ nor __ARM_BIG_ENDIAN,
> it is purely about other compilers.
I think using ACLE specified macro is the way to go here.  At least for
libcpp.  opt_random seems like a good one to fix, particularly for
situations where folks may be using LLVM with GCC's libstdc++.

A patch to use __ARM_BIG_ENDIAN in those two spots is approved.

jeff
Kyrill Tkachov Dec. 20, 2017, 9:25 a.m. UTC | #5 
On 20/12/17 00:23, Jeff Law wrote:
> On 12/19/2017 11:23 AM, Jakub Jelinek wrote:
>> On Tue, Dec 19, 2017 at 07:01:44PM +0100, Michael Weiser wrote:
>>> Hi Kyrill,
>>>
>>> On Tue, Dec 19, 2017 at 05:35:03PM +0000, Kyrill Tkachov wrote:
>>>
>>>>> below patch fixes PR 83492.
>>>> I agree with your analysis of the bug and your patch will fix the
>>>> problem when compiling with GCC. However, I believe the more
>>>> appropriate macro to check here is __ARM_BIG_ENDIAN as that
>>>> is the macro defined by the ACLE specification [1].
>>>> __AARCH64EB__ is indeed defined by aarch64 GCC when compiling
>>>> for big-endian but I don't think it's standardised and may in theory
>>>> not be defined by other host compilers the user may use to compile GCC.
>>> Since the macro is used in various places around the code, I'd suggest
>>> fixing this bug and getting back in line with the rest of the codebase
>>> by applying my patch as-is. The move towards a more appropriate macro to
>>> base the checks on can then be done in a separate step using
>>> search'n'replace.
>>>
>>> $ grep -r __AARCH64EB__ . | cut -d: -f1 | sort -u | grep -v \.svn
>>> ./gcc/config/aarch64/aarch64-c.c
>>> ./gcc/config/aarch64/arm_neon.h
>>> ./gcc/testsuite/lib/target-supports.exp
>>> ./libcpp/ChangeLog
>>> ./libcpp/lex.c
>>> ./libffi/src/aarch64/ffi.c
>>> ./libffi/src/aarch64/sysv.S
>>> ./libgcc/config/aarch64/linux-unwind.h
>>> ./libgcc/config/aarch64/sfp-machine.h
>>> ./libstdc++-v3/config/cpu/aarch64/opt/ext/opt_random.h
>> That doesn't tell anything on what should be used in libcpp.  Unlike the
>> rest, libcpp is a library that needs to be built on the host, with the
>> host compiler (which doesn't have to be GCC).  The only case that is in
>> the same category is opt_random.h, because it is an installed header and
>> so again, can be used by GCC or other compilers.
>> GCC isn't going to stop defining __AARCH64EB__ nor __ARM_BIG_ENDIAN,
>> it is purely about other compilers.
> I think using ACLE specified macro is the way to go here.  At least for
> libcpp.  opt_random seems like a good one to fix, particularly for
> situations where folks may be using LLVM with GCC's libstdc++.
>
> A patch to use __ARM_BIG_ENDIAN in those two spots is approved.

I'll take care of the opt_random occurrence.
Michael, would you like to respin your patch to libcpp to use 
__ARM_BIG_ENDIAN?

Thanks,
Kyrill

> jeff
Michael Weiser Dec. 20, 2017, 2:58 p.m. UTC | #6 
Hi Kyrill,

On Wed, Dec 20, 2017 at 09:25:07AM +0000, Kyrill Tkachov wrote:

> > A patch to use __ARM_BIG_ENDIAN in those two spots is approved.
> I'll take care of the opt_random occurrence.
> Michael, would you like to respin your patch to libcpp to use
> __ARM_BIG_ENDIAN?

Happy to. Created yesterday against trunk, no changes with today's trunk.
Tested successfully by applying to 7.2.0, bootstrapping on
aarch64_be-unknown-linux-gnu and compiling various OS packages with the
resulting compiler.

2017-12-20  Michael Weiser  <michael.weiser@gmx.de>

	PR preprocessor/83492
	* lex.c (search_line_fast) [__ARM_NEON && __ARM_64BIT_STATE]:
	Fix selection of big-endian shift parameters by using
	__ARM_BIG_ENDIAN.

Index: libcpp/lex.c
===================================================================
--- libcpp/lex.c	(revision 255828)
+++ libcpp/lex.c	(working copy)
@@ -772,7 +772,7 @@
   const uint8x16_t repl_qm = vdupq_n_u8 ('?');
   const uint8x16_t xmask = (uint8x16_t) vdupq_n_u64 (0x8040201008040201ULL);
 
-#ifdef __AARCH64EB
+#ifdef __ARM_BIG_ENDIAN
   const int16x8_t shift = {8, 8, 8, 8, 0, 0, 0, 0};
 #else
   const int16x8_t shift = {0, 0, 0, 0, 8, 8, 8, 8};
Kyrill Tkachov Dec. 20, 2017, 3:09 p.m. UTC | #7 
On 20/12/17 14:58, Michael Weiser wrote:
> Hi Kyrill,
>
> On Wed, Dec 20, 2017 at 09:25:07AM +0000, Kyrill Tkachov wrote:
>
>>> A patch to use __ARM_BIG_ENDIAN in those two spots is approved.
>> I'll take care of the opt_random occurrence.
>> Michael, would you like to respin your patch to libcpp to use
>> __ARM_BIG_ENDIAN?
> Happy to. Created yesterday against trunk, no changes with today's trunk.
> Tested successfully by applying to 7.2.0, bootstrapping on
> aarch64_be-unknown-linux-gnu and compiling various OS packages with the
> resulting compiler.

Thanks! I think that's the first report I've seen of an 
aarch64_be-unknown-linux-gnu bootstrap :)
Since Jeff approved such a change already I've applied it
for you to trunk as r255896.

I'll give it a day or two for any auto-testers to give it a
try before backporting to the GCC 7 branch.

Thanks,
Kyrill

> 2017-12-20  Michael Weiser  <michael.weiser@gmx.de>
>
> 	PR preprocessor/83492
> 	* lex.c (search_line_fast) [__ARM_NEON && __ARM_64BIT_STATE]:
> 	Fix selection of big-endian shift parameters by using
> 	__ARM_BIG_ENDIAN.
>
> Index: libcpp/lex.c
> ===================================================================
> --- libcpp/lex.c	(revision 255828)
> +++ libcpp/lex.c	(working copy)
> @@ -772,7 +772,7 @@
>     const uint8x16_t repl_qm = vdupq_n_u8 ('?');
>     const uint8x16_t xmask = (uint8x16_t) vdupq_n_u64 (0x8040201008040201ULL);
>   
> -#ifdef __AARCH64EB
> +#ifdef __ARM_BIG_ENDIAN
>     const int16x8_t shift = {8, 8, 8, 8, 0, 0, 0, 0};
>   #else
>     const int16x8_t shift = {0, 0, 0, 0, 8, 8, 8, 8};
diff mbox series

Patch

Index: lex.c
===================================================================
--- lex.c	(revision 255828)
+++ lex.c	(working copy)
@@ -772,7 +772,7 @@ 
   const uint8x16_t repl_qm = vdupq_n_u8 ('?');
   const uint8x16_t xmask = (uint8x16_t) vdupq_n_u64 (0x8040201008040201ULL);
 
-#ifdef __AARCH64EB
+#ifdef __AARCH64EB__
   const int16x8_t shift = {8, 8, 8, 8, 0, 0, 0, 0};
 #else
   const int16x8_t shift = {0, 0, 0, 0, 8, 8, 8, 8};