[PATCH][AArch64] Increase static buffer size in aarch64_rewrite_selected_cpu

[PATCH][AArch64] Increase static buffer size in aarch64_rewrite_selected_cpu

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1337 bytes --]

Hi all,

When trying to compile a testcase with -mcpu=cortex-a57+crypto+nocrc I got the weird assembler error:
Assembler messages:
Error: missing architectural extension
Error: unrecognized option -mcpu=cortex-a57+crypto+no

The problem is the aarch64_rewrite_selected_cpu that is used to rewrite -mcpu for big.LITTLE options
has a limit of 20 characters in what it handles, which we can exhaust quickly if we specify
architectural extensions in a fine-grained manner.

This patch increases that character limit to 128 and adds an assert to confirm that no bad things
happen.


It also fixes another problem: If we pass a big.LITTLE combination with feature modifiers like:
-mcpu=cortex-a57.cortex-a53+nosimd

the code will truncate everything after '.', thus destroying the extensions that we want to pass.
The patch adds code to stitch the extensions back on after the LITTLE cpu is removed.

Tested aarch64-none-elf and made sure the given mcpu option works fine with the assembler.

Ok for trunk?

Thanks,
Kyrill

2015-04-20  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * common/config/aarch64/aarch64-common.c (AARCH64_CPU_NAME_LENGTH):
     Increase to 128.
     (aarch64_rewrite_selected_cpu): Do not chop off extensions starting
     at '.'.  Assert that there's enough space for everything.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: aarch64-cpu-name-length.patch --]
[-- Type: text/x-patch; name=aarch64-cpu-name-length.patch, Size: 1984 bytes --]

commit 9623c859d5f4d0da1a364184bf0ce0dbbc7907b4
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Thu Feb 19 17:05:48 2015 +0000

    [AArch64] Increase static buffer size in aarch64_rewrite_selected_cpu

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index 308f19c..b3fd9dc 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -27,6 +27,7 @@
 #include "common/common-target-def.h"
 #include "opts.h"
 #include "flags.h"
+#include "errors.h"
 
 #ifdef  TARGET_BIG_ENDIAN_DEFAULT
 #undef  TARGET_DEFAULT_TARGET_FLAGS
@@ -89,23 +90,34 @@ aarch64_handle_option (struct gcc_options *opts,
 
 struct gcc_targetm_common targetm_common = TARGETM_COMMON_INITIALIZER;
 
-#define AARCH64_CPU_NAME_LENGTH 20
+#define AARCH64_CPU_NAME_LENGTH 128
 
-/* Truncate NAME at the first '.' character seen, or return
-   NAME unmodified.  */
+/* Truncate NAME at the first '.' character seen up to the first '+'
+   or return NAME unmodified.  */
 
 const char *
 aarch64_rewrite_selected_cpu (const char *name)
 {
   static char output_buf[AARCH64_CPU_NAME_LENGTH + 1] = {0};
-  char *arg_pos;
+  const char *bL_sep;
+  const char *feats;
+  size_t pref_size;
+  size_t feat_size;
 
-  strncpy (output_buf, name, AARCH64_CPU_NAME_LENGTH);
-  arg_pos = strchr (output_buf, '.');
+  bL_sep = strchr (name, '.');
+  if (!bL_sep)
+    return name;
 
-  /* If we found a '.' truncate the entry at that point.  */
-  if (arg_pos)
-    *arg_pos = '\0';
+  feats = strchr (name, '+');
+  feat_size = feats ? strnlen (feats, AARCH64_CPU_NAME_LENGTH) : 0;
+  pref_size = bL_sep - name;
+
+  if ((feat_size + pref_size) > AARCH64_CPU_NAME_LENGTH)
+    internal_error ("-mcpu string too large");
+
+  strncpy (output_buf, name, pref_size);
+  if (feats)
+    strncpy (output_buf + pref_size, feats, feat_size);
 
   return output_buf;
 }

Re: [PATCH][AArch64] Increase static buffer size in aarch64_rewrite_selected_cpu

[James Greenhalgh]

On Mon, Apr 20, 2015 at 05:24:39PM +0100, Kyrill Tkachov wrote:
> Hi all,
> 
> When trying to compile a testcase with -mcpu=cortex-a57+crypto+nocrc I got
> the weird assembler error:
> Assembler messages:
> Error: missing architectural extension
> Error: unrecognized option -mcpu=cortex-a57+crypto+no
> 
> The problem is the aarch64_rewrite_selected_cpu that is used to rewrite -mcpu
> for big.LITTLE options has a limit of 20 characters in what it handles, which
> we can exhaust quickly if we specify architectural extensions in a
> fine-grained manner.
> 
> This patch increases that character limit to 128 and adds an assert to
> confirm that no bad things happen.

You've implemented this as a hard ICE, was that intended?

> It also fixes another problem: If we pass a big.LITTLE combination with
> feature modifiers like: -mcpu=cortex-a57.cortex-a53+nosimd
> 
> the code will truncate everything after '.', thus destroying the extensions
> that we want to pass.  The patch adds code to stitch the extensions back on
> after the LITTLE cpu is removed.

UGH, I should not be allowed near strings! This code is on my list of
things I'd love to rewrite to this year! For now, this is OK and please
also queue it for 5.2 when that opens for patches.

> Ok for trunk?

Yes, thanks. And sorry again for introducing this in the first place.

James

Re: [PATCH][AArch64] Increase static buffer size in aarch64_rewrite_selected_cpu

[Kyrill Tkachov]

On 20/04/15 21:30, James Greenhalgh wrote:
> On Mon, Apr 20, 2015 at 05:24:39PM +0100, Kyrill Tkachov wrote:
>> Hi all,
>>
>> When trying to compile a testcase with -mcpu=cortex-a57+crypto+nocrc I got
>> the weird assembler error:
>> Assembler messages:
>> Error: missing architectural extension
>> Error: unrecognized option -mcpu=cortex-a57+crypto+no
>>
>> The problem is the aarch64_rewrite_selected_cpu that is used to rewrite -mcpu
>> for big.LITTLE options has a limit of 20 characters in what it handles, which
>> we can exhaust quickly if we specify architectural extensions in a
>> fine-grained manner.
>>
>> This patch increases that character limit to 128 and adds an assert to
>> confirm that no bad things happen.
> You've implemented this as a hard ICE, was that intended?

Yeah, the idea is that before this we would silently truncate i.e. do the wrong thing.
Now, if we exceed the limit we ICE. I don't think it should be a user error because
it's not really the user's fault that the compiler doesn't handle crazy long strings
but handling arbitrary large strings would make this function more complex than I think
is needed for the majority of cases. If you plan to rewrite this in the future, we can
revisit that.


>
>> It also fixes another problem: If we pass a big.LITTLE combination with
>> feature modifiers like: -mcpu=cortex-a57.cortex-a53+nosimd
>>
>> the code will truncate everything after '.', thus destroying the extensions
>> that we want to pass.  The patch adds code to stitch the extensions back on
>> after the LITTLE cpu is removed.
> UGH, I should not be allowed near strings! This code is on my list of
> things I'd love to rewrite to this year! For now, this is OK and please
> also queue it for 5.2 when that opens for patches.

Committed to trunk with r222258.

Thanks for looking at it,
Kyrill

>
>> Ok for trunk?
> Yes, thanks. And sorry again for introducing this in the first place.
>
> James
>

Re: [PATCH][AArch64] Increase static buffer size in aarch64_rewrite_selected_cpu

[Kyrill Tkachov]

On 20/04/15 21:30, James Greenhalgh wrote:
> On Mon, Apr 20, 2015 at 05:24:39PM +0100, Kyrill Tkachov wrote:
>> Hi all,
>>
>> When trying to compile a testcase with -mcpu=cortex-a57+crypto+nocrc I got
>> the weird assembler error:
>> Assembler messages:
>> Error: missing architectural extension
>> Error: unrecognized option -mcpu=cortex-a57+crypto+no
>>
>> The problem is the aarch64_rewrite_selected_cpu that is used to rewrite -mcpu
>> for big.LITTLE options has a limit of 20 characters in what it handles, which
>> we can exhaust quickly if we specify architectural extensions in a
>> fine-grained manner.
>>
>> This patch increases that character limit to 128 and adds an assert to
>> confirm that no bad things happen.
> You've implemented this as a hard ICE, was that intended?
>
>> It also fixes another problem: If we pass a big.LITTLE combination with
>> feature modifiers like: -mcpu=cortex-a57.cortex-a53+nosimd
>>
>> the code will truncate everything after '.', thus destroying the extensions
>> that we want to pass.  The patch adds code to stitch the extensions back on
>> after the LITTLE cpu is removed.
> UGH, I should not be allowed near strings! This code is on my list of
> things I'd love to rewrite to this year! For now, this is OK and please
> also queue it for 5.2 when that opens for patches.

Hi all,
Just to confirm.
Is it ok to backport this patch to the GCC 5 branch?

Thanks,
Kyrill

>
>> Ok for trunk?
> Yes, thanks. And sorry again for introducing this in the first place.
>
> James
>

Re: [PATCH][AArch64] Increase static buffer size in aarch64_rewrite_selected_cpu

[Kyrill Tkachov]

Ping on the backport.
Is this ok for the GCC 5 branch?

Thanks,
Kyrill

On 29/04/15 14:59, Kyrill Tkachov wrote:
> On 20/04/15 21:30, James Greenhalgh wrote:
>> On Mon, Apr 20, 2015 at 05:24:39PM +0100, Kyrill Tkachov wrote:
>>> Hi all,
>>>
>>> When trying to compile a testcase with -mcpu=cortex-a57+crypto+nocrc I got
>>> the weird assembler error:
>>> Assembler messages:
>>> Error: missing architectural extension
>>> Error: unrecognized option -mcpu=cortex-a57+crypto+no
>>>
>>> The problem is the aarch64_rewrite_selected_cpu that is used to rewrite -mcpu
>>> for big.LITTLE options has a limit of 20 characters in what it handles, which
>>> we can exhaust quickly if we specify architectural extensions in a
>>> fine-grained manner.
>>>
>>> This patch increases that character limit to 128 and adds an assert to
>>> confirm that no bad things happen.
>> You've implemented this as a hard ICE, was that intended?
>>
>>> It also fixes another problem: If we pass a big.LITTLE combination with
>>> feature modifiers like: -mcpu=cortex-a57.cortex-a53+nosimd
>>>
>>> the code will truncate everything after '.', thus destroying the extensions
>>> that we want to pass.  The patch adds code to stitch the extensions back on
>>> after the LITTLE cpu is removed.
>> UGH, I should not be allowed near strings! This code is on my list of
>> things I'd love to rewrite to this year! For now, this is OK and please
>> also queue it for 5.2 when that opens for patches.
> Hi all,
> Just to confirm.
> Is it ok to backport this patch to the GCC 5 branch?
>
> Thanks,
> Kyrill
>
>>> Ok for trunk?
>> Yes, thanks. And sorry again for introducing this in the first place.
>>
>> James
>>

Re: [PATCH][AArch64] Increase static buffer size in aarch64_rewrite_selected_cpu

[James Greenhalgh]

On Tue, May 12, 2015 at 10:08:55AM +0100, Kyrill Tkachov wrote:
> Ping on the backport.
> Is this ok for the GCC 5 branch?
> 

As below, yes this is OK for the GCC 5 branch.

> On 29/04/15 14:59, Kyrill Tkachov wrote:
> > On 20/04/15 21:30, James Greenhalgh wrote:
> >> On Mon, Apr 20, 2015 at 05:24:39PM +0100, Kyrill Tkachov wrote:
> >> UGH, I should not be allowed near strings! This code is on my list of
> >> things I'd love to rewrite to this year! For now, this is OK and please
> >> also queue it for 5.2 when that opens for patches.

Thanks,
James