Provide extension hint for aarch64 target (PR driver/83193).

Message ID 37fcbcb3-eab3-9929-b84f-110c1646a717@suse.cz
State New
Headers show
Series
  • Provide extension hint for aarch64 target (PR driver/83193).
Related show

Commit Message

Martin Liška July 18, 2018, 3:49 p.m.
Hi.

This patch improves aarch64 feature modifier hints.

May I please ask ARM folks to test the patch?
Thanks,
Martin

gcc/ChangeLog:

2018-07-18  Martin Liska  <mliska@suse.cz>

        PR driver/83193
	* common/config/aarch64/aarch64-common.c (aarch64_parse_extension):
        Set invalid_extension when there's any.
	(aarch64_get_all_extension_candidates): New function.
	(aarch64_rewrite_selected_cpu): Pass NULL as new argument.
	* config/aarch64/aarch64-protos.h (aarch64_get_all_extension_candidates):
        Declare new function.
	* config/aarch64/aarch64.c (aarch64_parse_arch): Record
        invalid_feature.
	(aarch64_parse_cpu): Likewise.
	(aarch64_print_hint_for_feature_modifier): New.
	(aarch64_validate_mcpu): Record invalid feature modifier
        and print hint for it.
	(aarch64_validate_march): Likewise.
	(aarch64_handle_attr_arch): Likewise.
	(aarch64_handle_attr_cpu): Likewise.
	(aarch64_handle_attr_isa_flags): Likewise.

gcc/testsuite/ChangeLog:

2018-07-18  Martin Liska  <mliska@suse.cz>

        PR driver/83193
	* gcc.target/aarch64/spellcheck_7.c: New test.
	* gcc.target/aarch64/spellcheck_8.c: New test.
---
 gcc/common/config/aarch64/aarch64-common.c    | 20 +++++-
 gcc/config/aarch64/aarch64-protos.h           |  4 +-
 gcc/config/aarch64/aarch64.c                  | 67 +++++++++++++++----
 .../gcc.target/aarch64/spellcheck_7.c         | 11 +++
 .../gcc.target/aarch64/spellcheck_8.c         | 12 ++++
 5 files changed, 97 insertions(+), 17 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_7.c
 create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_8.c

Comments

Martin Liška Aug. 1, 2018, 1:56 p.m. | #1
PING^1

On 07/18/2018 05:49 PM, Martin Liška wrote:
> Hi.

> 

> This patch improves aarch64 feature modifier hints.

> 

> May I please ask ARM folks to test the patch?

> Thanks,

> Martin

> 

> gcc/ChangeLog:

> 

> 2018-07-18  Martin Liska  <mliska@suse.cz>

> 

>         PR driver/83193

> 	* common/config/aarch64/aarch64-common.c (aarch64_parse_extension):

>         Set invalid_extension when there's any.

> 	(aarch64_get_all_extension_candidates): New function.

> 	(aarch64_rewrite_selected_cpu): Pass NULL as new argument.

> 	* config/aarch64/aarch64-protos.h (aarch64_get_all_extension_candidates):

>         Declare new function.

> 	* config/aarch64/aarch64.c (aarch64_parse_arch): Record

>         invalid_feature.

> 	(aarch64_parse_cpu): Likewise.

> 	(aarch64_print_hint_for_feature_modifier): New.

> 	(aarch64_validate_mcpu): Record invalid feature modifier

>         and print hint for it.

> 	(aarch64_validate_march): Likewise.

> 	(aarch64_handle_attr_arch): Likewise.

> 	(aarch64_handle_attr_cpu): Likewise.

> 	(aarch64_handle_attr_isa_flags): Likewise.

> 

> gcc/testsuite/ChangeLog:

> 

> 2018-07-18  Martin Liska  <mliska@suse.cz>

> 

>         PR driver/83193

> 	* gcc.target/aarch64/spellcheck_7.c: New test.

> 	* gcc.target/aarch64/spellcheck_8.c: New test.

> ---

>  gcc/common/config/aarch64/aarch64-common.c    | 20 +++++-

>  gcc/config/aarch64/aarch64-protos.h           |  4 +-

>  gcc/config/aarch64/aarch64.c                  | 67 +++++++++++++++----

>  .../gcc.target/aarch64/spellcheck_7.c         | 11 +++

>  .../gcc.target/aarch64/spellcheck_8.c         | 12 ++++

>  5 files changed, 97 insertions(+), 17 deletions(-)

>  create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_7.c

>  create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_8.c

> 

>
Martin Liška Aug. 23, 2018, 11 a.m. | #2
PING^2

On 08/01/2018 03:56 PM, Martin Liška wrote:
> PING^1

> 

> On 07/18/2018 05:49 PM, Martin Liška wrote:

>> Hi.

>>

>> This patch improves aarch64 feature modifier hints.

>>

>> May I please ask ARM folks to test the patch?

>> Thanks,

>> Martin

>>

>> gcc/ChangeLog:

>>

>> 2018-07-18  Martin Liska  <mliska@suse.cz>

>>

>>         PR driver/83193

>> 	* common/config/aarch64/aarch64-common.c (aarch64_parse_extension):

>>         Set invalid_extension when there's any.

>> 	(aarch64_get_all_extension_candidates): New function.

>> 	(aarch64_rewrite_selected_cpu): Pass NULL as new argument.

>> 	* config/aarch64/aarch64-protos.h (aarch64_get_all_extension_candidates):

>>         Declare new function.

>> 	* config/aarch64/aarch64.c (aarch64_parse_arch): Record

>>         invalid_feature.

>> 	(aarch64_parse_cpu): Likewise.

>> 	(aarch64_print_hint_for_feature_modifier): New.

>> 	(aarch64_validate_mcpu): Record invalid feature modifier

>>         and print hint for it.

>> 	(aarch64_validate_march): Likewise.

>> 	(aarch64_handle_attr_arch): Likewise.

>> 	(aarch64_handle_attr_cpu): Likewise.

>> 	(aarch64_handle_attr_isa_flags): Likewise.

>>

>> gcc/testsuite/ChangeLog:

>>

>> 2018-07-18  Martin Liska  <mliska@suse.cz>

>>

>>         PR driver/83193

>> 	* gcc.target/aarch64/spellcheck_7.c: New test.

>> 	* gcc.target/aarch64/spellcheck_8.c: New test.

>> ---

>>  gcc/common/config/aarch64/aarch64-common.c    | 20 +++++-

>>  gcc/config/aarch64/aarch64-protos.h           |  4 +-

>>  gcc/config/aarch64/aarch64.c                  | 67 +++++++++++++++----

>>  .../gcc.target/aarch64/spellcheck_7.c         | 11 +++

>>  .../gcc.target/aarch64/spellcheck_8.c         | 12 ++++

>>  5 files changed, 97 insertions(+), 17 deletions(-)

>>  create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_7.c

>>  create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_8.c

>>

>>

>
Martin Liška Sept. 19, 2018, 6:02 p.m. | #3
PING^3

On 8/23/18 1:00 PM, Martin Liška wrote:
> PING^2

> 

> On 08/01/2018 03:56 PM, Martin Liška wrote:

>> PING^1

>>

>> On 07/18/2018 05:49 PM, Martin Liška wrote:

>>> Hi.

>>>

>>> This patch improves aarch64 feature modifier hints.

>>>

>>> May I please ask ARM folks to test the patch?

>>> Thanks,

>>> Martin

>>>

>>> gcc/ChangeLog:

>>>

>>> 2018-07-18  Martin Liska  <mliska@suse.cz>

>>>

>>>          PR driver/83193

>>> 	* common/config/aarch64/aarch64-common.c (aarch64_parse_extension):

>>>          Set invalid_extension when there's any.

>>> 	(aarch64_get_all_extension_candidates): New function.

>>> 	(aarch64_rewrite_selected_cpu): Pass NULL as new argument.

>>> 	* config/aarch64/aarch64-protos.h (aarch64_get_all_extension_candidates):

>>>          Declare new function.

>>> 	* config/aarch64/aarch64.c (aarch64_parse_arch): Record

>>>          invalid_feature.

>>> 	(aarch64_parse_cpu): Likewise.

>>> 	(aarch64_print_hint_for_feature_modifier): New.

>>> 	(aarch64_validate_mcpu): Record invalid feature modifier

>>>          and print hint for it.

>>> 	(aarch64_validate_march): Likewise.

>>> 	(aarch64_handle_attr_arch): Likewise.

>>> 	(aarch64_handle_attr_cpu): Likewise.

>>> 	(aarch64_handle_attr_isa_flags): Likewise.

>>>

>>> gcc/testsuite/ChangeLog:

>>>

>>> 2018-07-18  Martin Liska  <mliska@suse.cz>

>>>

>>>          PR driver/83193

>>> 	* gcc.target/aarch64/spellcheck_7.c: New test.

>>> 	* gcc.target/aarch64/spellcheck_8.c: New test.

>>> ---

>>>   gcc/common/config/aarch64/aarch64-common.c    | 20 +++++-

>>>   gcc/config/aarch64/aarch64-protos.h           |  4 +-

>>>   gcc/config/aarch64/aarch64.c                  | 67 +++++++++++++++----

>>>   .../gcc.target/aarch64/spellcheck_7.c         | 11 +++

>>>   .../gcc.target/aarch64/spellcheck_8.c         | 12 ++++

>>>   5 files changed, 97 insertions(+), 17 deletions(-)

>>>   create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_7.c

>>>   create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_8.c

>>>

>>>

>>

>
Martin Liška Oct. 4, 2018, 8:03 a.m. | #4
PING^4

On 9/19/18 8:02 PM, Martin Liška wrote:
> PING^3

> 

> On 8/23/18 1:00 PM, Martin Liška wrote:

>> PING^2

>>

>> On 08/01/2018 03:56 PM, Martin Liška wrote:

>>> PING^1

>>>

>>> On 07/18/2018 05:49 PM, Martin Liška wrote:

>>>> Hi.

>>>>

>>>> This patch improves aarch64 feature modifier hints.

>>>>

>>>> May I please ask ARM folks to test the patch?

>>>> Thanks,

>>>> Martin

>>>>

>>>> gcc/ChangeLog:

>>>>

>>>> 2018-07-18  Martin Liska  <mliska@suse.cz>

>>>>

>>>>          PR driver/83193

>>>>     * common/config/aarch64/aarch64-common.c (aarch64_parse_extension):

>>>>          Set invalid_extension when there's any.

>>>>     (aarch64_get_all_extension_candidates): New function.

>>>>     (aarch64_rewrite_selected_cpu): Pass NULL as new argument.

>>>>     * config/aarch64/aarch64-protos.h (aarch64_get_all_extension_candidates):

>>>>          Declare new function.

>>>>     * config/aarch64/aarch64.c (aarch64_parse_arch): Record

>>>>          invalid_feature.

>>>>     (aarch64_parse_cpu): Likewise.

>>>>     (aarch64_print_hint_for_feature_modifier): New.

>>>>     (aarch64_validate_mcpu): Record invalid feature modifier

>>>>          and print hint for it.

>>>>     (aarch64_validate_march): Likewise.

>>>>     (aarch64_handle_attr_arch): Likewise.

>>>>     (aarch64_handle_attr_cpu): Likewise.

>>>>     (aarch64_handle_attr_isa_flags): Likewise.

>>>>

>>>> gcc/testsuite/ChangeLog:

>>>>

>>>> 2018-07-18  Martin Liska  <mliska@suse.cz>

>>>>

>>>>          PR driver/83193

>>>>     * gcc.target/aarch64/spellcheck_7.c: New test.

>>>>     * gcc.target/aarch64/spellcheck_8.c: New test.

>>>> ---

>>>>   gcc/common/config/aarch64/aarch64-common.c    | 20 +++++-

>>>>   gcc/config/aarch64/aarch64-protos.h           |  4 +-

>>>>   gcc/config/aarch64/aarch64.c                  | 67 +++++++++++++++----

>>>>   .../gcc.target/aarch64/spellcheck_7.c         | 11 +++

>>>>   .../gcc.target/aarch64/spellcheck_8.c         | 12 ++++

>>>>   5 files changed, 97 insertions(+), 17 deletions(-)

>>>>   create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_7.c

>>>>   create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_8.c

>>>>

>>>>

>>>

>>

>
Kyrill Tkachov Oct. 4, 2018, 9:29 a.m. | #5
Hi Martin,

On 18/07/18 16:49, Martin Liška wrote:
> Hi.

>

> This patch improves aarch64 feature modifier hints.

>

> May I please ask ARM folks to test the patch?

> Thanks,

> Martin

>


I've bootstrapped and tested it on aarch64-none-linux-gnu and I didn't see any problems.
I like the functionality! It is definitely an improvement to the user experience.

I'm not an aarch64 maintainer but I have some comments on the patch below.

Thanks,
Kyrill

> gcc/ChangeLog:

>

> 2018-07-18  Martin Liska  <mliska@suse.cz>

>

>         PR driver/83193

>         * common/config/aarch64/aarch64-common.c (aarch64_parse_extension):

>         Set invalid_extension when there's any.

>         (aarch64_get_all_extension_candidates): New function.

>         (aarch64_rewrite_selected_cpu): Pass NULL as new argument.

>         * config/aarch64/aarch64-protos.h (aarch64_get_all_extension_candidates):

>         Declare new function.

>         * config/aarch64/aarch64.c (aarch64_parse_arch): Record

>         invalid_feature.

>         (aarch64_parse_cpu): Likewise.

>         (aarch64_print_hint_for_feature_modifier): New.

>         (aarch64_validate_mcpu): Record invalid feature modifier

>         and print hint for it.

>         (aarch64_validate_march): Likewise.

>         (aarch64_handle_attr_arch): Likewise.

>         (aarch64_handle_attr_cpu): Likewise.

>         (aarch64_handle_attr_isa_flags): Likewise.

>

> gcc/testsuite/ChangeLog:

>

> 2018-07-18  Martin Liska  <mliska@suse.cz>

>

>         PR driver/83193

>         * gcc.target/aarch64/spellcheck_7.c: New test.

>         * gcc.target/aarch64/spellcheck_8.c: New test.

> ---

>  gcc/common/config/aarch64/aarch64-common.c    | 20 +++++-

>  gcc/config/aarch64/aarch64-protos.h           |  4 +-

>  gcc/config/aarch64/aarch64.c                  | 67 +++++++++++++++----

>  .../gcc.target/aarch64/spellcheck_7.c         | 11 +++

>  .../gcc.target/aarch64/spellcheck_8.c         | 12 ++++

>  5 files changed, 97 insertions(+), 17 deletions(-)

>  create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_7.c

>  create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_8.c

>

>


diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index 292fb818705..c2994514004 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -175,7 +175,8 @@ static const struct arch_to_arch_name all_architectures[] =
     aarch64_parse_opt_result describing the result.  */
  
  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,
+			 char **invalid_extension)
  {

Please document the new argument in the function comment. Same for all the other parsing functions that you extend in the patch.
In particular, I'd like to see a comment on who allocates the memory for the string and who is responsible for freeing it.

+/* Append all extension candidates and put them to CANDIDATES vector.  */
+

Given the implementation below, how about:
"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);
+}
+

<snip>

+
+/* Print a hint with a suggestion for a feature modifier name
+   that most closely resembles what the user passed in STR.  */
+
+void
+aarch64_print_hint_for_feature_modifier (const char *str)
+{


Elsewhere in the backend and this patch as well we call them extensions rather than feature modifiers.
Let's be consistent: aarch64_print_hint_for_extension would is my suggestion

+  auto_vec<const char *> candidates;
+  aarch64_get_all_extension_candidates (&candidates);
+  char *s;
+  const char *hint = candidates_list_and_hint (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);
+}
+

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..1350b865162
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/spellcheck_7.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-march=*" } { "" } } */

Another way that the extensions are used is with -mcpu. For example -mcpu=cortex-a57+crypto.
So you'll need to skip if -mcpu is given as well.
And we'll want a test for -mcpu error-checking as well.

+/* { dg-options "-march=armv8-a+typo" } */
+
+void
+foo ()
+{
+}
+
Martin Liška Oct. 4, 2018, 2:33 p.m. | #6
On 10/4/18 11:29 AM, Kyrill Tkachov wrote:
> Hi Martin,

> 

> On 18/07/18 16:49, Martin Liška wrote:

>> Hi.

>>

>> This patch improves aarch64 feature modifier hints.

>>

>> May I please ask ARM folks to test the patch?

>> Thanks,

>> Martin

>>

> 

> I've bootstrapped and tested it on aarch64-none-linux-gnu and I didn't see any problems.

> I like the functionality! It is definitely an improvement to the user experience.

> 

> I'm not an aarch64 maintainer but I have some comments on the patch below.


Hi.

Thanks for it.

> 

> Thanks,

> Kyrill

> 

>> gcc/ChangeLog:

>>

>> 2018-07-18  Martin Liska  <mliska@suse.cz>

>>

>>         PR driver/83193

>>         * common/config/aarch64/aarch64-common.c (aarch64_parse_extension):

>>         Set invalid_extension when there's any.

>>         (aarch64_get_all_extension_candidates): New function.

>>         (aarch64_rewrite_selected_cpu): Pass NULL as new argument.

>>         * config/aarch64/aarch64-protos.h (aarch64_get_all_extension_candidates):

>>         Declare new function.

>>         * config/aarch64/aarch64.c (aarch64_parse_arch): Record

>>         invalid_feature.

>>         (aarch64_parse_cpu): Likewise.

>>         (aarch64_print_hint_for_feature_modifier): New.

>>         (aarch64_validate_mcpu): Record invalid feature modifier

>>         and print hint for it.

>>         (aarch64_validate_march): Likewise.

>>         (aarch64_handle_attr_arch): Likewise.

>>         (aarch64_handle_attr_cpu): Likewise.

>>         (aarch64_handle_attr_isa_flags): Likewise.

>>

>> gcc/testsuite/ChangeLog:

>>

>> 2018-07-18  Martin Liska  <mliska@suse.cz>

>>

>>         PR driver/83193

>>         * gcc.target/aarch64/spellcheck_7.c: New test.

>>         * gcc.target/aarch64/spellcheck_8.c: New test.

>> ---

>>  gcc/common/config/aarch64/aarch64-common.c    | 20 +++++-

>>  gcc/config/aarch64/aarch64-protos.h           |  4 +-

>>  gcc/config/aarch64/aarch64.c                  | 67 +++++++++++++++----

>>  .../gcc.target/aarch64/spellcheck_7.c         | 11 +++

>>  .../gcc.target/aarch64/spellcheck_8.c         | 12 ++++

>>  5 files changed, 97 insertions(+), 17 deletions(-)

>>  create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_7.c

>>  create mode 100644 gcc/testsuite/gcc.target/aarch64/spellcheck_8.c

>>

>>

> 

> diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c

> index 292fb818705..c2994514004 100644

> --- a/gcc/common/config/aarch64/aarch64-common.c

> +++ b/gcc/common/config/aarch64/aarch64-common.c

> @@ -175,7 +175,8 @@ static const struct arch_to_arch_name all_architectures[] =

>     aarch64_parse_opt_result describing the result.  */

>  

>  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,

> +             char **invalid_extension)

>  {

> 

> Please document the new argument in the function comment. Same for all the other parsing functions that you extend in the patch.

> In particular, I'd like to see a comment on who allocates the memory for the string and who is responsible for freeing it.

> 

> +/* Append all extension candidates and put them to CANDIDATES vector.  */

> +

> 

> Given the implementation below, how about:

> "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);

> +}

> +

> 

> <snip>

> 

> +

> +/* Print a hint with a suggestion for a feature modifier name

> +   that most closely resembles what the user passed in STR.  */

> +

> +void

> +aarch64_print_hint_for_feature_modifier (const char *str)

> +{

> 

> 

> Elsewhere in the backend and this patch as well we call them extensions rather than feature modifiers.


But currently following error message is printed to user:
cc1: error: invalid feature modifier in '-mcpu=thunderx+cripto'

?

I'm working on another iteration of the patch.

Martin

> Let's be consistent: aarch64_print_hint_for_extension would is my suggestion

> 

> +  auto_vec<const char *> candidates;

> +  aarch64_get_all_extension_candidates (&candidates);

> +  char *s;

> +  const char *hint = candidates_list_and_hint (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);

> +}

> +

> 

> 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..1350b865162

> --- /dev/null

> +++ b/gcc/testsuite/gcc.target/aarch64/spellcheck_7.c

> @@ -0,0 +1,11 @@

> +/* { dg-do compile } */

> +/* { dg-skip-if "" { *-*-* } { "-march=*" } { "" } } */

> 

> Another way that the extensions are used is with -mcpu. For example -mcpu=cortex-a57+crypto.

> So you'll need to skip if -mcpu is given as well.

> And we'll want a test for -mcpu error-checking as well.

> 

> +/* { dg-options "-march=armv8-a+typo" } */

> +

> +void

> +foo ()

> +{

> +}

> +

>
Martin Liška Oct. 8, 2018, 10:34 a.m. | #7
Hi.

I'm attaching updated version of the patch.

Martin
From d36974540cda9fb0e159103fdcf92d26ef2f1b94 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>

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  <mliska@suse.cz>

	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  <mliska@suse.cz>

	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

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index ffddc4d16d8..2bebe70c021 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,
+			 char **invalid_extension)
 {
   /* The extension string is parsed left to right.  */
   const struct aarch64_option_extension *opt = NULL;
@@ -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 = xstrdup (str);
+	      (*invalid_extension)[len] = '\0';
+	    }
 	  return AARCH64_PARSE_INVALID_FEATURE;
 	}
 
@@ -283,6 +291,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 +388,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..2f1b0b10250 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 *,
+						       char **);
+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..39895c7ede3 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, char **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, char **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 char *str)
+{
+  auto_vec<const char *> candidates;
+  aarch64_get_all_extension_candidates (&candidates);
+  char *s;
+  const char *hint = candidates_list_and_hint (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)
 {
+  char *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,10 @@ 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, str);
+	aarch64_print_hint_for_extensions (invalid_extension);
+	free (invalid_extension);
 	break;
       default:
 	gcc_unreachable ();
@@ -11129,8 +11157,9 @@ static bool
 aarch64_validate_march (const char *str, const struct processor **res,
 			 unsigned long *isa_flags)
 {
+  char *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 +11174,10 @@ 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, str);
+	aarch64_print_hint_for_extensions (invalid_extension);
+	free (invalid_extension);
 	break;
       default:
 	gcc_unreachable ();
@@ -11545,8 +11577,9 @@ static bool
 aarch64_handle_attr_arch (const char *str)
 {
   const struct processor *tmp_arch = NULL;
+  char *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 +11599,10 @@ 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, str);
+	aarch64_print_hint_for_extensions (invalid_extension);
+	free (invalid_extension);
 	break;
       default:
 	gcc_unreachable ();
@@ -11581,8 +11617,9 @@ static bool
 aarch64_handle_attr_cpu (const char *str)
 {
   const struct processor *tmp_cpu = NULL;
+  char *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 +11642,10 @@ 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, str);
+	aarch64_print_hint_for_extensions (invalid_extension);
+	free (invalid_extension);
 	break;
       default:
 	gcc_unreachable ();
@@ -11663,7 +11703,8 @@ aarch64_handle_attr_isa_flags (char *str)
       str += 8;
     }
 
-  parse_res = aarch64_parse_extension (str, &isa_flags);
+  char *invalid_extension;
+  parse_res = aarch64_parse_extension (str, &isa_flags, &invalid_extension);
 
   if (parse_res == AARCH64_PARSE_OK)
     {
@@ -11678,7 +11719,9 @@ 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, str);
+	free (invalid_extension);
 	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
Kyrill Tkachov Oct. 16, 2018, 4:24 p.m. | #8
On 08/10/18 11:34, Martin Liška wrote:
> Hi.

>

> I'm attaching updated version of the patch.

>

> Martin


This looks ok to me, but you'll need approval from a maintainer.

Thanks for the iterations,
Kyrill
James Greenhalgh Oct. 16, 2018, 4:57 p.m. | #9
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.

If you can clean that up I'd be much happier. The overall patch is OK.

Thanks,
James

> From d36974540cda9fb0e159103fdcf92d26ef2f1b94 Mon Sep 17 00:00:00 2001

> From: marxin <mliska@suse.cz>

> 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  <mliska@suse.cz>

> 

> 	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  <mliska@suse.cz>

> 

> 	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

>
Martin Liška Oct. 22, 2018, 1:05 p.m. | #10
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?

Martin

> 

> Thanks,

> James

> 

>> From d36974540cda9fb0e159103fdcf92d26ef2f1b94 Mon Sep 17 00:00:00 2001

>> From: marxin <mliska@suse.cz>

>> 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  <mliska@suse.cz>

>>

>> 	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  <mliska@suse.cz>

>>

>> 	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 07c1303836b953dc14aecbae475e88a9c88b4c12 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>

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  <mliska@suse.cz>

	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  <mliska@suse.cz>

	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                  | 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, 116 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..2bcb3fb1ee1 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,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';
+	    }
 	  return AARCH64_PARSE_INVALID_FEATURE;
 	}
 
@@ -283,6 +291,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 +388,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..e3295419154 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 modified %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
Martin Sebor Oct. 23, 2018, 4:31 p.m. | #11
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?


If STR below is not nul-terminated the std::string ctor is not
safe.  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.)

@@ -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:

@@ -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 <mliska@suse.cz>

>>> 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  <mliska@suse.cz>

>>>

>>> 	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  <mliska@suse.cz>

>>>

>>> 	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

>>>

>
Martin Liška Oct. 24, 2018, 9:52 a.m. | #12
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?

> 

> @@ -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 <mliska@suse.cz>

>>>> 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  <mliska@suse.cz>

>>>>

>>>>     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  <mliska@suse.cz>

>>>>

>>>>     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 2fcde1deaf6f3dec20ad4dfa68d13ed428497e87 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>

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  <mliska@suse.cz>

	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  <mliska@suse.cz>

	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                  | 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, 116 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..4eede92892f 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,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->resize (len);
+	    }
 	  return AARCH64_PARSE_INVALID_FEATURE;
 	}
 
@@ -283,6 +291,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 +388,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
Martin Sebor Oct. 24, 2018, 5:48 p.m. | #13
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).

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).

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 <mliska@suse.cz>

>>>>> 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  <mliska@suse.cz>

>>>>>

>>>>>     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  <mliska@suse.cz>

>>>>>

>>>>>     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

>>>>>

>>>

>>

>
Martin Liška Oct. 25, 2018, 10:53 a.m. | #14
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 <mliska@suse.cz>

>>>>>> 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  <mliska@suse.cz>

>>>>>>

>>>>>>     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  <mliska@suse.cz>

>>>>>>

>>>>>>     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 <mliska@suse.cz>

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  <mliska@suse.cz>

	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  <mliska@suse.cz>

	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
James Greenhalgh Oct. 30, 2018, 7:19 p.m. | #15
On Thu, Oct 25, 2018 at 05:53:22AM -0500, Martin Liška wrote:
> 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!


OK.

Thanks,
James

Patch

diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index 292fb818705..c2994514004 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -175,7 +175,8 @@  static const struct arch_to_arch_name all_architectures[] =
    aarch64_parse_opt_result describing the result.  */
 
 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,
+			 char **invalid_extension)
 {
   /* The extension string is parsed left to right.  */
   const struct aarch64_option_extension *opt = NULL;
@@ -226,6 +227,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 = xstrdup (str);
+	      (*invalid_extension)[len] = '\0';
+	    }
 	  return AARCH64_PARSE_INVALID_FEATURE;
 	}
 
@@ -235,6 +241,16 @@  aarch64_parse_extension (const char *str, unsigned long *isa_flags)
   return AARCH64_PARSE_OK;
 }
 
+/* Append all extension candidates and put them to 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
@@ -322,7 +338,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 bc11a781c4b..4db274fb85d 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -550,7 +550,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 *,
+						       char **);
+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 1369704da3e..6fa03e4b091 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -10229,7 +10229,7 @@  static void initialize_aarch64_code_model (struct gcc_options *);
 
 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, char **invalid_feature)
 {
   char *ext;
   const struct processor *arch;
@@ -10260,7 +10260,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_feature);
 
 	      if (ext_res != AARCH64_PARSE_OK)
 		return ext_res;
@@ -10284,7 +10284,7 @@  aarch64_parse_arch (const char *to_parse, const struct processor **res,
 
 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, char **invalid_feature)
 {
   char *ext;
   const struct processor *cpu;
@@ -10316,7 +10316,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_feature);
 
 	      if (ext_res != AARCH64_PARSE_OK)
 		return ext_res;
@@ -10764,6 +10764,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 a feature modifier name
+   that most closely resembles what the user passed in STR.  */
+
+void
+aarch64_print_hint_for_feature_modifier (const char *str)
+{
+  auto_vec<const char *> candidates;
+  aarch64_get_all_extension_candidates (&candidates);
+  char *s;
+  const char *hint = candidates_list_and_hint (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
@@ -10773,8 +10793,9 @@  static bool
 aarch64_validate_mcpu (const char *str, const struct processor **res,
 		       unsigned long *isa_flags)
 {
+  char *invalid_feature;
   enum aarch64_parse_opt_result parse_res
-    = aarch64_parse_cpu (str, res, isa_flags);
+    = aarch64_parse_cpu (str, res, isa_flags, &invalid_feature);
 
   if (parse_res == AARCH64_PARSE_OK)
     return true;
@@ -10789,7 +10810,10 @@  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_feature, str);
+	aarch64_print_hint_for_feature_modifier (invalid_feature);
+	free (invalid_feature);
 	break;
       default:
 	gcc_unreachable ();
@@ -10807,8 +10831,9 @@  static bool
 aarch64_validate_march (const char *str, const struct processor **res,
 			 unsigned long *isa_flags)
 {
+  char *invalid_feature;
   enum aarch64_parse_opt_result parse_res
-    = aarch64_parse_arch (str, res, isa_flags);
+    = aarch64_parse_arch (str, res, isa_flags, &invalid_feature);
 
   if (parse_res == AARCH64_PARSE_OK)
     return true;
@@ -10823,7 +10848,10 @@  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_feature, str);
+	aarch64_print_hint_for_feature_modifier (invalid_feature);
+	free (invalid_feature);
 	break;
       default:
 	gcc_unreachable ();
@@ -11223,8 +11251,9 @@  static bool
 aarch64_handle_attr_arch (const char *str)
 {
   const struct processor *tmp_arch = NULL;
+  char *invalid_feature;
   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_feature);
 
   if (parse_res == AARCH64_PARSE_OK)
     {
@@ -11244,7 +11273,10 @@  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 modified %s of value (\"%s\") in "
+	       "%<target()%> pragma or attribute", invalid_feature, str);
+	aarch64_print_hint_for_feature_modifier (invalid_feature);
+	free (invalid_feature);
 	break;
       default:
 	gcc_unreachable ();
@@ -11259,8 +11291,9 @@  static bool
 aarch64_handle_attr_cpu (const char *str)
 {
   const struct processor *tmp_cpu = NULL;
+  char *invalid_feature;
   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_feature);
 
   if (parse_res == AARCH64_PARSE_OK)
     {
@@ -11283,7 +11316,10 @@  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 modified %s of value (\"%s\") in "
+	       "%<target()%> pragma or attribute", invalid_feature, str);
+	aarch64_print_hint_for_feature_modifier (invalid_feature);
+	free (invalid_feature);
 	break;
       default:
 	gcc_unreachable ();
@@ -11341,7 +11377,8 @@  aarch64_handle_attr_isa_flags (char *str)
       str += 8;
     }
 
-  parse_res = aarch64_parse_extension (str, &isa_flags);
+  char *invalid_feature;
+  parse_res = aarch64_parse_extension (str, &isa_flags, &invalid_feature);
 
   if (parse_res == AARCH64_PARSE_OK)
     {
@@ -11356,7 +11393,9 @@  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_feature, str);
+	free (invalid_feature);
 	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..1350b865162
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/spellcheck_7.c
@@ -0,0 +1,11 @@ 
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-march=*" } { "" } } */
+/* { 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..321678e5ef6
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/spellcheck_8.c
@@ -0,0 +1,12 @@ 
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-march=*" } { "" } } */
+/* { 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 } */
+