On 10/24/18 7:48 PM, Martin Sebor wrote:
> On 10/24/2018 03:52 AM, Martin Liška wrote:
>> On 10/23/18 6:31 PM, Martin Sebor wrote:
>>> On 10/22/2018 07:05 AM, Martin Liška wrote:
>>>> On 10/16/18 6:57 PM, James Greenhalgh wrote:
>>>>> On Mon, Oct 08, 2018 at 05:34:52AM -0500, Martin Liška wrote:
>>>>>> Hi.
>>>>>>
>>>>>> I'm attaching updated version of the patch.
>>>>>
>>>>> Can't say I'm thrilled by the allocation/free (aarch64_parse_extension
>>>>> allocates, everyone else has to free) responsibilities here.
>>>>
>>>> Agreed.
>>>>
>>>>>
>>>>> If you can clean that up I'd be much happier. The overall patch is OK.
>>>>
>>>> I rewrote that to use std::string, hope it's improvement?
>>>
>>
>> Hi Martin
>>
>>> If STR below is not nul-terminated the std::string ctor is not
>>> safe.
>>
>> Appreciate the help. The string should be null-terminated, it either comes
>> from GCC command line or it's a valid of an attribute in source code.
>>
>> If it is nul-terminated but LEN is equal to its length
>>> then the nul assignment should be unnecessary. If LEN is less
>>> than its length and the goal is to truncate the string then
>>> calling resize() would be the right way to do it. Otherwise,
>>> assigning a nul to an element into the middle won't truncate
>>> (it will leave the remaining elements there). (This may not
>>> matter if the string isn't appended to after that.)
>>
>> That's new for me, I reworked the patch to use resize. Btw. it sounds
>> a candidate for a new warning ;) ? Must be quite common mistake?
>
> I should have also mentioned that there is constructor that
> takes a pointer and a count:
>
> *invalid_extension = std::string (str, len);
>
> That would be even better than calling resize (sorry about that).
That's fine, I'm sending updated patch. Tested just locally as cross compiler
in valgind.
>
> There are lots of opportunities for warnings about misuses of
> the standard library. I think we need to first solve
> the -Wno-system-headers problem (which disables most warnings
> for standard library headers).
I see!
Martin
>
> Martin
>
>>
>>>
>>> @@ -274,6 +277,11 @@
>>> aarch64_parse_extension (const char *str, unsigned long *isa_flags)
>>> if (opt->name == NULL)
>>> {
>>> /* Extension not found in list. */
>>> + if (invalid_extension)
>>> + {
>>> + *invalid_extension = std::string (str);
>>> + (*invalid_extension)[len] = '\0';
>>> + }
>>>
>>> I also noticed a minor typo while quickly skimming the rest
>>> of the patch:
>>
>> Fixed, thanks.
>>
>> Martin
>>
>>>
>>> @@ -11678,7 +11715,8 @@
>>> aarch64_handle_attr_isa_flags (char *str)
>>> break;
>>>
>>> case AARCH64_PARSE_INVALID_FEATURE:
>>> - error ("invalid value (\"%s\") in %<target()%> pragma or attribute",
>>> str);
>>> + error ("invalid feature modified %s of value (\"%s\") in "
>>> + "%<target()%> pragma or attribute", invalid_extension.c_str (),
>>> str);
>>> break;
>>>
>>> default:
>>>
>>> Based on the other messages in the patch the last word in "invalid
>>> feature modified" should be "modifier"
>>>
>>>
>>> Martin
>>>
>>>>
>>>> Martin
>>>>
>>>>>
>>>>> Thanks,
>>>>> James
>>>>>
>>>>>> From d36974540cda9fb0e159103fdcf92d26ef2f1b94 Mon Sep 17 00:00:00 2001
>>>>>> From: marxin <[email protected]>
>>>>>> Date: Thu, 4 Oct 2018 16:31:49 +0200
>>>>>> Subject: [PATCH] Provide extension hint for aarch64 target (PR
>>>>>> driver/83193).
>>>>>>
>>>>>> gcc/ChangeLog:
>>>>>>
>>>>>> 2018-10-05 Martin Liska <[email protected]>
>>>>>>
>>>>>> PR driver/83193
>>>>>> * common/config/aarch64/aarch64-common.c (aarch64_parse_extension):
>>>>>> Add new argument invalid_extension.
>>>>>> (aarch64_get_all_extension_candidates): New function.
>>>>>> (aarch64_rewrite_selected_cpu): Add NULL to function call.
>>>>>> * config/aarch64/aarch64-protos.h (aarch64_parse_extension): Add
>>>>>> new argument.
>>>>>> (aarch64_get_all_extension_candidates): New function.
>>>>>> * config/aarch64/aarch64.c (aarch64_parse_arch): Add new
>>>>>> argument invalid_extension.
>>>>>> (aarch64_parse_cpu): Likewise.
>>>>>> (aarch64_print_hint_for_extensions): New function.
>>>>>> (aarch64_validate_mcpu): Provide hint about invalid extension.
>>>>>> (aarch64_validate_march): Likewise.
>>>>>> (aarch64_handle_attr_arch): Pass new argument.
>>>>>> (aarch64_handle_attr_cpu): Provide hint about invalid extension.
>>>>>> (aarch64_handle_attr_isa_flags): Likewise.
>>>>>>
>>>>>> gcc/testsuite/ChangeLog:
>>>>>>
>>>>>> 2018-10-05 Martin Liska <[email protected]>
>>>>>>
>>>>>> PR driver/83193
>>>>>> * gcc.target/aarch64/spellcheck_7.c: New test.
>>>>>> * gcc.target/aarch64/spellcheck_8.c: New test.
>>>>>> * gcc.target/aarch64/spellcheck_9.c: New test.
>>>>>> ---
>>>>>> gcc/common/config/aarch64/aarch64-common.c | 24 +++++-
>>>>>> gcc/config/aarch64/aarch64-protos.h | 4 +-
>>>>>> gcc/config/aarch64/aarch64.c | 75 +++++++++++++++----
>>>>>> .../gcc.target/aarch64/spellcheck_7.c | 12 +++
>>>>>> .../gcc.target/aarch64/spellcheck_8.c | 13 ++++
>>>>>> .../gcc.target/aarch64/spellcheck_9.c | 13 ++++
>>>>>> 6 files changed, 121 insertions(+), 20 deletions(-)
>>>>>> create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_7.c
>>>>>> create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_8.c
>>>>>> create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_9.c
>>>>>>
>>>>
>>>
>>
>
>From d0e7be2ef3b70957516498acbb39de19ff5bcd1f Mon Sep 17 00:00:00 2001
From: marxin <[email protected]>
Date: Thu, 4 Oct 2018 16:31:49 +0200
Subject: [PATCH] Provide extension hint for aarch64 target (PR driver/83193).
gcc/ChangeLog:
2018-10-05 Martin Liska <[email protected]>
PR driver/83193
* common/config/aarch64/aarch64-common.c (aarch64_parse_extension):
Add new argument invalid_extension.
(aarch64_get_all_extension_candidates): New function.
(aarch64_rewrite_selected_cpu): Add NULL to function call.
* config/aarch64/aarch64-protos.h (aarch64_parse_extension): Add
new argument.
(aarch64_get_all_extension_candidates): New function.
* config/aarch64/aarch64.c (aarch64_parse_arch): Add new
argument invalid_extension.
(aarch64_parse_cpu): Likewise.
(aarch64_print_hint_for_extensions): New function.
(aarch64_validate_mcpu): Provide hint about invalid extension.
(aarch64_validate_march): Likewise.
(aarch64_handle_attr_arch): Pass new argument.
(aarch64_handle_attr_cpu): Provide hint about invalid extension.
(aarch64_handle_attr_isa_flags): Likewise.
gcc/testsuite/ChangeLog:
2018-10-05 Martin Liska <[email protected]>
PR driver/83193
* gcc.target/aarch64/spellcheck_7.c: New test.
* gcc.target/aarch64/spellcheck_8.c: New test.
* gcc.target/aarch64/spellcheck_9.c: New test.
---
gcc/common/config/aarch64/aarch64-common.c | 21 +++++-
gcc/config/aarch64/aarch64-protos.h | 4 +-
gcc/config/aarch64/aarch64.c | 70 ++++++++++++++-----
.../gcc.target/aarch64/spellcheck_7.c | 12 ++++
.../gcc.target/aarch64/spellcheck_8.c | 13 ++++
.../gcc.target/aarch64/spellcheck_9.c | 13 ++++
6 files changed, 113 insertions(+), 20 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_7.c
create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_8.c
create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_9.c
diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index ffddc4d16d8..dd7d4267340 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -220,10 +220,13 @@ static const struct arch_to_arch_name all_architectures[] =
/* Parse the architecture extension string STR and update ISA_FLAGS
with the architecture features turned on or off. Return a
- aarch64_parse_opt_result describing the result. */
+ aarch64_parse_opt_result describing the result.
+ When the STR string contains an invalid extension,
+ a copy of the string is created and stored to INVALID_EXTENSION. */
enum aarch64_parse_opt_result
-aarch64_parse_extension (const char *str, unsigned long *isa_flags)
+aarch64_parse_extension (const char *str, unsigned long *isa_flags,
+ std::string *invalid_extension)
{
/* The extension string is parsed left to right. */
const struct aarch64_option_extension *opt = NULL;
@@ -274,6 +277,8 @@ aarch64_parse_extension (const char *str, unsigned long *isa_flags)
if (opt->name == NULL)
{
/* Extension not found in list. */
+ if (invalid_extension)
+ *invalid_extension = std::string (str, len);
return AARCH64_PARSE_INVALID_FEATURE;
}
@@ -283,6 +288,16 @@ aarch64_parse_extension (const char *str, unsigned long *isa_flags)
return AARCH64_PARSE_OK;
}
+/* Append all architecture extension candidates to the CANDIDATES vector. */
+
+void
+aarch64_get_all_extension_candidates (auto_vec<const char *> *candidates)
+{
+ const struct aarch64_option_extension *opt;
+ for (opt = all_extensions; opt->name != NULL; opt++)
+ candidates->safe_push (opt->name);
+}
+
/* Return a string representation of ISA_FLAGS. DEFAULT_ARCH_FLAGS
gives the default set of flags which are implied by whatever -march
we'd put out. Our job is to figure out the minimal set of "+" and
@@ -370,7 +385,7 @@ aarch64_rewrite_selected_cpu (const char *name)
fatal_error (input_location, "unknown value %qs for -mcpu", name);
unsigned long extensions = p_to_a->flags;
- aarch64_parse_extension (extension_str.c_str (), &extensions);
+ aarch64_parse_extension (extension_str.c_str (), &extensions, NULL);
std::string outstr = a_to_an->arch_name
+ aarch64_get_extension_string_for_isa_flags (extensions,
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index 5f18837418e..0c7927aed7c 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -616,7 +616,9 @@ bool aarch64_handle_option (struct gcc_options *, struct gcc_options *,
const struct cl_decoded_option *, location_t);
const char *aarch64_rewrite_selected_cpu (const char *name);
enum aarch64_parse_opt_result aarch64_parse_extension (const char *,
- unsigned long *);
+ unsigned long *,
+ std::string *);
+void aarch64_get_all_extension_candidates (auto_vec<const char *> *candidates);
std::string aarch64_get_extension_string_for_isa_flags (unsigned long,
unsigned long);
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index d385b246a74..5bc1f4c1f64 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -10513,11 +10513,13 @@ static void initialize_aarch64_code_model (struct gcc_options *);
/* Parse the TO_PARSE string and put the architecture struct that it
selects into RES and the architectural features into ISA_FLAGS.
Return an aarch64_parse_opt_result describing the parse result.
- If there is an error parsing, RES and ISA_FLAGS are left unchanged. */
+ If there is an error parsing, RES and ISA_FLAGS are left unchanged.
+ When the TO_PARSE string contains an invalid extension,
+ a copy of the string is created and stored to INVALID_EXTENSION. */
static enum aarch64_parse_opt_result
aarch64_parse_arch (const char *to_parse, const struct processor **res,
- unsigned long *isa_flags)
+ unsigned long *isa_flags, std::string *invalid_extension)
{
char *ext;
const struct processor *arch;
@@ -10548,7 +10550,7 @@ aarch64_parse_arch (const char *to_parse, const struct processor **res,
{
/* TO_PARSE string contains at least one extension. */
enum aarch64_parse_opt_result ext_res
- = aarch64_parse_extension (ext, &isa_temp);
+ = aarch64_parse_extension (ext, &isa_temp, invalid_extension);
if (ext_res != AARCH64_PARSE_OK)
return ext_res;
@@ -10568,11 +10570,13 @@ aarch64_parse_arch (const char *to_parse, const struct processor **res,
/* Parse the TO_PARSE string and put the result tuning in RES and the
architecture flags in ISA_FLAGS. Return an aarch64_parse_opt_result
describing the parse result. If there is an error parsing, RES and
- ISA_FLAGS are left unchanged. */
+ ISA_FLAGS are left unchanged.
+ When the TO_PARSE string contains an invalid extension,
+ a copy of the string is created and stored to INVALID_EXTENSION. */
static enum aarch64_parse_opt_result
aarch64_parse_cpu (const char *to_parse, const struct processor **res,
- unsigned long *isa_flags)
+ unsigned long *isa_flags, std::string *invalid_extension)
{
char *ext;
const struct processor *cpu;
@@ -10604,7 +10608,7 @@ aarch64_parse_cpu (const char *to_parse, const struct processor **res,
{
/* TO_PARSE string contains at least one extension. */
enum aarch64_parse_opt_result ext_res
- = aarch64_parse_extension (ext, &isa_temp);
+ = aarch64_parse_extension (ext, &isa_temp, invalid_extension);
if (ext_res != AARCH64_PARSE_OK)
return ext_res;
@@ -11086,6 +11090,26 @@ aarch64_print_hint_for_arch (const char *str)
aarch64_print_hint_for_core_or_arch (str, true);
}
+
+/* Print a hint with a suggestion for an extension name
+ that most closely resembles what the user passed in STR. */
+
+void
+aarch64_print_hint_for_extensions (const std::string &str)
+{
+ auto_vec<const char *> candidates;
+ aarch64_get_all_extension_candidates (&candidates);
+ char *s;
+ const char *hint = candidates_list_and_hint (str.c_str (), s, candidates);
+ if (hint)
+ inform (input_location, "valid arguments are: %s;"
+ " did you mean %qs?", s, hint);
+ else
+ inform (input_location, "valid arguments are: %s;", s);
+
+ XDELETEVEC (s);
+}
+
/* Validate a command-line -mcpu option. Parse the cpu and extensions (if any)
specified in STR and throw errors if appropriate. Put the results if
they are valid in RES and ISA_FLAGS. Return whether the option is
@@ -11095,8 +11119,9 @@ static bool
aarch64_validate_mcpu (const char *str, const struct processor **res,
unsigned long *isa_flags)
{
+ std::string invalid_extension;
enum aarch64_parse_opt_result parse_res
- = aarch64_parse_cpu (str, res, isa_flags);
+ = aarch64_parse_cpu (str, res, isa_flags, &invalid_extension);
if (parse_res == AARCH64_PARSE_OK)
return true;
@@ -11111,7 +11136,9 @@ aarch64_validate_mcpu (const char *str, const struct processor **res,
aarch64_print_hint_for_core (str);
break;
case AARCH64_PARSE_INVALID_FEATURE:
- error ("invalid feature modifier in %<-mcpu=%s%>", str);
+ error ("invalid feature modifier %qs in %<-mcpu=%s%>",
+ invalid_extension.c_str (), str);
+ aarch64_print_hint_for_extensions (invalid_extension);
break;
default:
gcc_unreachable ();
@@ -11129,8 +11156,9 @@ static bool
aarch64_validate_march (const char *str, const struct processor **res,
unsigned long *isa_flags)
{
+ std::string invalid_extension;
enum aarch64_parse_opt_result parse_res
- = aarch64_parse_arch (str, res, isa_flags);
+ = aarch64_parse_arch (str, res, isa_flags, &invalid_extension);
if (parse_res == AARCH64_PARSE_OK)
return true;
@@ -11145,7 +11173,9 @@ aarch64_validate_march (const char *str, const struct processor **res,
aarch64_print_hint_for_arch (str);
break;
case AARCH64_PARSE_INVALID_FEATURE:
- error ("invalid feature modifier in %<-march=%s%>", str);
+ error ("invalid feature modifier %qs in %<-march=%s%>",
+ invalid_extension.c_str (), str);
+ aarch64_print_hint_for_extensions (invalid_extension);
break;
default:
gcc_unreachable ();
@@ -11545,8 +11575,9 @@ static bool
aarch64_handle_attr_arch (const char *str)
{
const struct processor *tmp_arch = NULL;
+ std::string invalid_extension;
enum aarch64_parse_opt_result parse_res
- = aarch64_parse_arch (str, &tmp_arch, &aarch64_isa_flags);
+ = aarch64_parse_arch (str, &tmp_arch, &aarch64_isa_flags, &invalid_extension);
if (parse_res == AARCH64_PARSE_OK)
{
@@ -11566,7 +11597,9 @@ aarch64_handle_attr_arch (const char *str)
aarch64_print_hint_for_arch (str);
break;
case AARCH64_PARSE_INVALID_FEATURE:
- error ("invalid value (\"%s\") in %<target()%> pragma or attribute", str);
+ error ("invalid feature modifier %s of value (\"%s\") in "
+ "%<target()%> pragma or attribute", invalid_extension.c_str (), str);
+ aarch64_print_hint_for_extensions (invalid_extension);
break;
default:
gcc_unreachable ();
@@ -11581,8 +11614,9 @@ static bool
aarch64_handle_attr_cpu (const char *str)
{
const struct processor *tmp_cpu = NULL;
+ std::string invalid_extension;
enum aarch64_parse_opt_result parse_res
- = aarch64_parse_cpu (str, &tmp_cpu, &aarch64_isa_flags);
+ = aarch64_parse_cpu (str, &tmp_cpu, &aarch64_isa_flags, &invalid_extension);
if (parse_res == AARCH64_PARSE_OK)
{
@@ -11605,7 +11639,9 @@ aarch64_handle_attr_cpu (const char *str)
aarch64_print_hint_for_core (str);
break;
case AARCH64_PARSE_INVALID_FEATURE:
- error ("invalid value (\"%s\") in %<target()%> pragma or attribute", str);
+ error ("invalid feature modifier %s of value (\"%s\") in "
+ "%<target()%> pragma or attribute", invalid_extension.c_str (), str);
+ aarch64_print_hint_for_extensions (invalid_extension);
break;
default:
gcc_unreachable ();
@@ -11663,7 +11699,8 @@ aarch64_handle_attr_isa_flags (char *str)
str += 8;
}
- parse_res = aarch64_parse_extension (str, &isa_flags);
+ std::string invalid_extension;
+ parse_res = aarch64_parse_extension (str, &isa_flags, &invalid_extension);
if (parse_res == AARCH64_PARSE_OK)
{
@@ -11678,7 +11715,8 @@ aarch64_handle_attr_isa_flags (char *str)
break;
case AARCH64_PARSE_INVALID_FEATURE:
- error ("invalid value (\"%s\") in %<target()%> pragma or attribute", str);
+ error ("invalid feature modifier %s of value (\"%s\") in "
+ "%<target()%> pragma or attribute", invalid_extension.c_str (), str);
break;
default:
diff --git a/gcc/testsuite/gcc.target/aarch64/spellcheck_7.c b/gcc/testsuite/gcc.target/aarch64/spellcheck_7.c
new file mode 100644
index 00000000000..1d31950cb61
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/spellcheck_7.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-march=*" } { "" } } */
+/* { dg-skip-if "" { *-*-* } { "-mcpu=*" } { "" } } */
+/* { dg-options "-march=armv8-a+typo" } */
+
+void
+foo ()
+{
+}
+
+/* { dg-error "invalid feature modifier .typo. in .-march=armv8-a\\+typo." "" { target *-*-* } 0 } */
+/* { dg-message "valid arguments are: \[^\n\r]*;'?" "" { target *-*-* } 0 } */
diff --git a/gcc/testsuite/gcc.target/aarch64/spellcheck_8.c b/gcc/testsuite/gcc.target/aarch64/spellcheck_8.c
new file mode 100644
index 00000000000..1b8c5ebfeb1
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/spellcheck_8.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-march=*" } { "" } } */
+/* { dg-skip-if "" { *-*-* } { "-mcpu=*" } { "" } } */
+/* { dg-options "-march=armv8-a+cripto" } */
+
+void
+foo ()
+{
+}
+
+/* { dg-error "invalid feature modifier .cripto. in .-march=armv8-a\\+cripto." "" { target *-*-* } 0 } */
+/* { dg-message "valid arguments are: \[^\n\r]*; did you mean .crypto.?" "" { target *-*-* } 0 } */
+
diff --git a/gcc/testsuite/gcc.target/aarch64/spellcheck_9.c b/gcc/testsuite/gcc.target/aarch64/spellcheck_9.c
new file mode 100644
index 00000000000..ad5b82589c1
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/spellcheck_9.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-march=*" } { "" } } */
+/* { dg-skip-if "" { *-*-* } { "-mcpu=*" } { "" } } */
+/* { dg-options "-mcpu=thunderx+cripto" } */
+
+void
+foo ()
+{
+}
+
+/* { dg-error "invalid feature modifier .cripto. in .-mcpu=thunderx\\+cripto." "" { target *-*-* } 0 } */
+/* { dg-message "valid arguments are: \[^\n\r]*; did you mean .crypto.?" "" { target *-*-* } 0 } */
+
--
2.19.0
