Message ID | 55352847.8080302@arm.com |
---|---|
State | New |
Headers | show |
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
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 >
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 >
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 >>
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
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; }