[aarch64] PR102376 - Emit better diagnostic for arch extensions in target attr

  Hi,
The attached patch emits a more verbose diagnostic for target attribute that
is an architecture extension needing a leading '+'.

For the following test,
void calculate(void) __attribute__ ((__target__ ("sve")));

With patch, the compiler now emits:
102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’
    1 | void calculate(void) __attribute__ ((__target__ ("sve")));
      | ^~~~

instead of:
102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid
    1 | void calculate(void) __attribute__ ((__target__ ("sve")));
      | ^~~~

(This isn't specific to sve though).
OK to commit after bootstrap+test ?

Thanks,
Prathamesh
  

Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> Hi,
> The attached patch emits a more verbose diagnostic for target attribute that
> is an architecture extension needing a leading '+'.
>
> For the following test,
> void calculate(void) __attribute__ ((__target__ ("sve")));
>
> With patch, the compiler now emits:
> 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’
>     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
>       | ^~~~
>
> instead of:
> 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid
>     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
>       | ^~~~

Nice :-)

> (This isn't specific to sve though).
> OK to commit after bootstrap+test ?
>
> Thanks,
> Prathamesh
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index a9a1800af53..975f7faf968 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args)
>        num_attrs++;
>        if (!aarch64_process_one_target_attr (token))
>  	{
> -	  error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> +	  /* Check if token is possibly an arch extension without
> +	     leading '+'.  */
> +	  char *str = (char *) xmalloc (strlen (token) + 2);
> +	  str[0] = '+';
> +	  strcpy(str + 1, token);

I think std::string would be better here, e.g.:

  auto with_plus = std::string ("+") + token;

> +	  if (aarch64_handle_attr_isa_flags (str))
> +	    error("arch extension %<%s%> should be prepended with %<+%>", token);

Nit: should be a space before the “(”.

In principle, a fixit hint would have been nice here, but I don't think
we have enough information to provide one.  (Just saying for the record.)

Thanks,
Richard

> +	  else
> +	    error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> +	  free (str);
>  	  return false;
>  	}
>
  

On Tue, 19 Oct 2021 at 19:58, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> > Hi,
> > The attached patch emits a more verbose diagnostic for target attribute that
> > is an architecture extension needing a leading '+'.
> >
> > For the following test,
> > void calculate(void) __attribute__ ((__target__ ("sve")));
> >
> > With patch, the compiler now emits:
> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’
> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
> >       | ^~~~
> >
> > instead of:
> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid
> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
> >       | ^~~~
>
> Nice :-)
>
> > (This isn't specific to sve though).
> > OK to commit after bootstrap+test ?
> >
> > Thanks,
> > Prathamesh
> >
> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > index a9a1800af53..975f7faf968 100644
> > --- a/gcc/config/aarch64/aarch64.c
> > +++ b/gcc/config/aarch64/aarch64.c
> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args)
> >        num_attrs++;
> >        if (!aarch64_process_one_target_attr (token))
> >       {
> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> > +       /* Check if token is possibly an arch extension without
> > +          leading '+'.  */
> > +       char *str = (char *) xmalloc (strlen (token) + 2);
> > +       str[0] = '+';
> > +       strcpy(str + 1, token);
>
> I think std::string would be better here, e.g.:
>
>   auto with_plus = std::string ("+") + token;
>
> > +       if (aarch64_handle_attr_isa_flags (str))
> > +         error("arch extension %<%s%> should be prepended with %<+%>", token);
>
> Nit: should be a space before the “(”.
>
> In principle, a fixit hint would have been nice here, but I don't think
> we have enough information to provide one.  (Just saying for the record.)
Thanks for the suggestions.
Does the attached patch look OK ?

Thanks,
Prathamesh
>
> Thanks,
> Richard
>
> > +       else
> > +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> > +       free (str);
> >         return false;
> >       }
> >
[aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute.

gcc/ChangeLog:
	PR target/102376
	* config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's
	type to const char *.
	(aarch64_process_target_attr): Check if token is possibly an arch extension
	without leading '+' and emit diagnostic accordingly.

gcc/testsuite/ChangeLog:
	PR target/102376
	* gcc.target/aarch64/pr102376.c: New test.
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index a9a1800af53..b72079bc466 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str)
    modified.  */
 
 static bool
-aarch64_handle_attr_isa_flags (char *str)
+aarch64_handle_attr_isa_flags (const char *str)
 {
   enum aarch64_parse_opt_result parse_res;
   uint64_t isa_flags = aarch64_isa_flags;
@@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args)
       num_attrs++;
       if (!aarch64_process_one_target_attr (token))
 	{
-	  error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
+	  /* Check if token is possibly an arch extension without
+	     leading '+'.  */
+	  auto with_plus = std::string("+") + token;
+	  if (aarch64_handle_attr_isa_flags (with_plus.c_str ()))
+	    error ("arch extension %<%s%> should be prepended with %<+%>", token);
+	  else
+	    error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
 	  return false;
 	}
 
diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c
new file mode 100644
index 00000000000..efd15f6ca9b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c
@@ -0,0 +1,3 @@
+/* { dg-do compile } */
+
+void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */
  

Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> On Tue, 19 Oct 2021 at 19:58, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>> > Hi,
>> > The attached patch emits a more verbose diagnostic for target attribute that
>> > is an architecture extension needing a leading '+'.
>> >
>> > For the following test,
>> > void calculate(void) __attribute__ ((__target__ ("sve")));
>> >
>> > With patch, the compiler now emits:
>> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’
>> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
>> >       | ^~~~
>> >
>> > instead of:
>> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid
>> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
>> >       | ^~~~
>>
>> Nice :-)
>>
>> > (This isn't specific to sve though).
>> > OK to commit after bootstrap+test ?
>> >
>> > Thanks,
>> > Prathamesh
>> >
>> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> > index a9a1800af53..975f7faf968 100644
>> > --- a/gcc/config/aarch64/aarch64.c
>> > +++ b/gcc/config/aarch64/aarch64.c
>> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args)
>> >        num_attrs++;
>> >        if (!aarch64_process_one_target_attr (token))
>> >       {
>> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>> > +       /* Check if token is possibly an arch extension without
>> > +          leading '+'.  */
>> > +       char *str = (char *) xmalloc (strlen (token) + 2);
>> > +       str[0] = '+';
>> > +       strcpy(str + 1, token);
>>
>> I think std::string would be better here, e.g.:
>>
>>   auto with_plus = std::string ("+") + token;
>>
>> > +       if (aarch64_handle_attr_isa_flags (str))
>> > +         error("arch extension %<%s%> should be prepended with %<+%>", token);
>>
>> Nit: should be a space before the “(”.
>>
>> In principle, a fixit hint would have been nice here, but I don't think
>> we have enough information to provide one.  (Just saying for the record.)
> Thanks for the suggestions.
> Does the attached patch look OK ?

Looks good apart from a couple of formatting nits.
>
> Thanks,
> Prathamesh
>>
>> Thanks,
>> Richard
>>
>> > +       else
>> > +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>> > +       free (str);
>> >         return false;
>> >       }
>> >
>
> [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute.
>
> gcc/ChangeLog:
> 	PR target/102376
> 	* config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's
> 	type to const char *.
> 	(aarch64_process_target_attr): Check if token is possibly an arch extension
> 	without leading '+' and emit diagnostic accordingly.
>
> gcc/testsuite/ChangeLog:
> 	PR target/102376
> 	* gcc.target/aarch64/pr102376.c: New test.
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index a9a1800af53..b72079bc466 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str)
>     modified.  */
>  
>  static bool
> -aarch64_handle_attr_isa_flags (char *str)
> +aarch64_handle_attr_isa_flags (const char *str)
>  {
>    enum aarch64_parse_opt_result parse_res;
>    uint64_t isa_flags = aarch64_isa_flags;
> @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args)
>        num_attrs++;
>        if (!aarch64_process_one_target_attr (token))
>  	{
> -	  error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> +	  /* Check if token is possibly an arch extension without
> +	     leading '+'.  */
> +	  auto with_plus = std::string("+") + token;

Should be a space before “(”.

> +	  if (aarch64_handle_attr_isa_flags (with_plus.c_str ()))
> +	    error ("arch extension %<%s%> should be prepended with %<+%>", token);

Long line, should be:

	    error ("arch extension %<%s%> should be prepended with %<+%>",
		   token);

OK with those changes, thanks.

Richard


> +	  else
> +	    error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>  	  return false;
>  	}
>  
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c
> new file mode 100644
> index 00000000000..efd15f6ca9b
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c
> @@ -0,0 +1,3 @@
> +/* { dg-do compile } */
> +
> +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */
  

On Wed, 20 Oct 2021 at 15:05, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> >>
> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> >> > Hi,
> >> > The attached patch emits a more verbose diagnostic for target attribute that
> >> > is an architecture extension needing a leading '+'.
> >> >
> >> > For the following test,
> >> > void calculate(void) __attribute__ ((__target__ ("sve")));
> >> >
> >> > With patch, the compiler now emits:
> >> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’
> >> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
> >> >       | ^~~~
> >> >
> >> > instead of:
> >> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid
> >> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
> >> >       | ^~~~
> >>
> >> Nice :-)
> >>
> >> > (This isn't specific to sve though).
> >> > OK to commit after bootstrap+test ?
> >> >
> >> > Thanks,
> >> > Prathamesh
> >> >
> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> >> > index a9a1800af53..975f7faf968 100644
> >> > --- a/gcc/config/aarch64/aarch64.c
> >> > +++ b/gcc/config/aarch64/aarch64.c
> >> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args)
> >> >        num_attrs++;
> >> >        if (!aarch64_process_one_target_attr (token))
> >> >       {
> >> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >> > +       /* Check if token is possibly an arch extension without
> >> > +          leading '+'.  */
> >> > +       char *str = (char *) xmalloc (strlen (token) + 2);
> >> > +       str[0] = '+';
> >> > +       strcpy(str + 1, token);
> >>
> >> I think std::string would be better here, e.g.:
> >>
> >>   auto with_plus = std::string ("+") + token;
> >>
> >> > +       if (aarch64_handle_attr_isa_flags (str))
> >> > +         error("arch extension %<%s%> should be prepended with %<+%>", token);
> >>
> >> Nit: should be a space before the “(”.
> >>
> >> In principle, a fixit hint would have been nice here, but I don't think
> >> we have enough information to provide one.  (Just saying for the record.)
> > Thanks for the suggestions.
> > Does the attached patch look OK ?
>
> Looks good apart from a couple of formatting nits.
> >
> > Thanks,
> > Prathamesh
> >>
> >> Thanks,
> >> Richard
> >>
> >> > +       else
> >> > +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >> > +       free (str);
> >> >         return false;
> >> >       }
> >> >
> >
> > [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute.
> >
> > gcc/ChangeLog:
> >       PR target/102376
> >       * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's
> >       type to const char *.
> >       (aarch64_process_target_attr): Check if token is possibly an arch extension
> >       without leading '+' and emit diagnostic accordingly.
> >
> > gcc/testsuite/ChangeLog:
> >       PR target/102376
> >       * gcc.target/aarch64/pr102376.c: New test.
> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > index a9a1800af53..b72079bc466 100644
> > --- a/gcc/config/aarch64/aarch64.c
> > +++ b/gcc/config/aarch64/aarch64.c
> > @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str)
> >     modified.  */
> >
> >  static bool
> > -aarch64_handle_attr_isa_flags (char *str)
> > +aarch64_handle_attr_isa_flags (const char *str)
> >  {
> >    enum aarch64_parse_opt_result parse_res;
> >    uint64_t isa_flags = aarch64_isa_flags;
> > @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args)
> >        num_attrs++;
> >        if (!aarch64_process_one_target_attr (token))
> >       {
> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> > +       /* Check if token is possibly an arch extension without
> > +          leading '+'.  */
> > +       auto with_plus = std::string("+") + token;
>
> Should be a space before “(”.
>
> > +       if (aarch64_handle_attr_isa_flags (with_plus.c_str ()))
> > +         error ("arch extension %<%s%> should be prepended with %<+%>", token);
>
> Long line, should be:
>
>             error ("arch extension %<%s%> should be prepended with %<+%>",
>                    token);
>
> OK with those changes, thanks.
Thanks, the patch regressed some target attr tests because it emitted
diagnostics twice from
aarch64_handle_attr_isa_flags.
So for eg, spellcheck_1.c:
__attribute__((target ("arch=armv8-a-typo"))) void foo () {}

results in:
spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in
‘target("arch=")’ pragma or attribute
    5 | {
      | ^
spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a
armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r
armv9-a
spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo
of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute
spellcheck_1.c:5:1: error: pragma or attribute
‘target("arch=armv8-a-typo")’ is not valid

The patch adds an additional argument to the
aarch64_handle_attr_isa_flags, to optionally not emit an error, which
works to fix the issue.
Does it look OK ?

Thanks,
Prathamesh
>
> Richard
>
>
> > +       else
> > +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >         return false;
> >       }
> >
> > diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c
> > new file mode 100644
> > index 00000000000..efd15f6ca9b
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c
> > @@ -0,0 +1,3 @@
> > +/* { dg-do compile } */
> > +
> > +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index a9a1800af53..cf345e05fbc 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str)
    modified.  */
 
 static bool
-aarch64_handle_attr_isa_flags (char *str)
+aarch64_handle_attr_isa_flags (const char *str, bool emit_diagnostic = true)
 {
   enum aarch64_parse_opt_result parse_res;
   uint64_t isa_flags = aarch64_isa_flags;
@@ -17570,6 +17570,9 @@ aarch64_handle_attr_isa_flags (char *str)
       return true;
     }
 
+  if (!emit_diagnostic)
+    return false;
+
   switch (parse_res)
     {
       case AARCH64_PARSE_MISSING_ARG:
@@ -17821,7 +17824,14 @@ aarch64_process_target_attr (tree args)
       num_attrs++;
       if (!aarch64_process_one_target_attr (token))
 	{
-	  error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
+	  /* Check if token is possibly an arch extension without
+	     leading '+'.  */
+	  auto with_plus = std::string ("+") + token;
+	  if (aarch64_handle_attr_isa_flags (with_plus.c_str (), false))
+	    error ("arch extension %<%s%> should be prepended with %<+%>",
+		   token);
+	  else
+	    error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
 	  return false;
 	}
 
diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c
new file mode 100644
index 00000000000..efd15f6ca9b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c
@@ -0,0 +1,3 @@
+/* { dg-do compile } */
+
+void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */
  

On Fri, 22 Oct 2021 at 14:41, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
>
> On Wed, 20 Oct 2021 at 15:05, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
> >
> > Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> > > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford
> > > <richard.sandiford@arm.com> wrote:
> > >>
> > >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> > >> > Hi,
> > >> > The attached patch emits a more verbose diagnostic for target attribute that
> > >> > is an architecture extension needing a leading '+'.
> > >> >
> > >> > For the following test,
> > >> > void calculate(void) __attribute__ ((__target__ ("sve")));
> > >> >
> > >> > With patch, the compiler now emits:
> > >> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’
> > >> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
> > >> >       | ^~~~
> > >> >
> > >> > instead of:
> > >> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid
> > >> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
> > >> >       | ^~~~
> > >>
> > >> Nice :-)
> > >>
> > >> > (This isn't specific to sve though).
> > >> > OK to commit after bootstrap+test ?
> > >> >
> > >> > Thanks,
> > >> > Prathamesh
> > >> >
> > >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > >> > index a9a1800af53..975f7faf968 100644
> > >> > --- a/gcc/config/aarch64/aarch64.c
> > >> > +++ b/gcc/config/aarch64/aarch64.c
> > >> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args)
> > >> >        num_attrs++;
> > >> >        if (!aarch64_process_one_target_attr (token))
> > >> >       {
> > >> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> > >> > +       /* Check if token is possibly an arch extension without
> > >> > +          leading '+'.  */
> > >> > +       char *str = (char *) xmalloc (strlen (token) + 2);
> > >> > +       str[0] = '+';
> > >> > +       strcpy(str + 1, token);
> > >>
> > >> I think std::string would be better here, e.g.:
> > >>
> > >>   auto with_plus = std::string ("+") + token;
> > >>
> > >> > +       if (aarch64_handle_attr_isa_flags (str))
> > >> > +         error("arch extension %<%s%> should be prepended with %<+%>", token);
> > >>
> > >> Nit: should be a space before the “(”.
> > >>
> > >> In principle, a fixit hint would have been nice here, but I don't think
> > >> we have enough information to provide one.  (Just saying for the record.)
> > > Thanks for the suggestions.
> > > Does the attached patch look OK ?
> >
> > Looks good apart from a couple of formatting nits.
> > >
> > > Thanks,
> > > Prathamesh
> > >>
> > >> Thanks,
> > >> Richard
> > >>
> > >> > +       else
> > >> > +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> > >> > +       free (str);
> > >> >         return false;
> > >> >       }
> > >> >
> > >
> > > [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute.
> > >
> > > gcc/ChangeLog:
> > >       PR target/102376
> > >       * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's
> > >       type to const char *.
> > >       (aarch64_process_target_attr): Check if token is possibly an arch extension
> > >       without leading '+' and emit diagnostic accordingly.
> > >
> > > gcc/testsuite/ChangeLog:
> > >       PR target/102376
> > >       * gcc.target/aarch64/pr102376.c: New test.
> > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > > index a9a1800af53..b72079bc466 100644
> > > --- a/gcc/config/aarch64/aarch64.c
> > > +++ b/gcc/config/aarch64/aarch64.c
> > > @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str)
> > >     modified.  */
> > >
> > >  static bool
> > > -aarch64_handle_attr_isa_flags (char *str)
> > > +aarch64_handle_attr_isa_flags (const char *str)
> > >  {
> > >    enum aarch64_parse_opt_result parse_res;
> > >    uint64_t isa_flags = aarch64_isa_flags;
> > > @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args)
> > >        num_attrs++;
> > >        if (!aarch64_process_one_target_attr (token))
> > >       {
> > > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> > > +       /* Check if token is possibly an arch extension without
> > > +          leading '+'.  */
> > > +       auto with_plus = std::string("+") + token;
> >
> > Should be a space before “(”.
> >
> > > +       if (aarch64_handle_attr_isa_flags (with_plus.c_str ()))
> > > +         error ("arch extension %<%s%> should be prepended with %<+%>", token);
> >
> > Long line, should be:
> >
> >             error ("arch extension %<%s%> should be prepended with %<+%>",
> >                    token);
> >
> > OK with those changes, thanks.
> Thanks, the patch regressed some target attr tests because it emitted
> diagnostics twice from
> aarch64_handle_attr_isa_flags.
> So for eg, spellcheck_1.c:
> __attribute__((target ("arch=armv8-a-typo"))) void foo () {}
>
> results in:
> spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in
> ‘target("arch=")’ pragma or attribute
>     5 | {
>       | ^
> spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a
> armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r
> armv9-a
> spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo
> of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute
> spellcheck_1.c:5:1: error: pragma or attribute
> ‘target("arch=armv8-a-typo")’ is not valid
>
> The patch adds an additional argument to the
> aarch64_handle_attr_isa_flags, to optionally not emit an error, which
> works to fix the issue.
> Does it look OK ?
ping https://gcc.gnu.org/pipermail/gcc-patches/2021-October/582345.html

Thanks,
Prathamesh
>
> Thanks,
> Prathamesh
> >
> > Richard
> >
> >
> > > +       else
> > > +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> > >         return false;
> > >       }
> > >
> > > diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c
> > > new file mode 100644
> > > index 00000000000..efd15f6ca9b
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c
> > > @@ -0,0 +1,3 @@
> > > +/* { dg-do compile } */
> > > +
> > > +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */
  

On 10/28/21 2:59 AM, Prathamesh Kulkarni via Gcc-patches wrote:
> On Fri, 22 Oct 2021 at 14:41, Prathamesh Kulkarni
> <prathamesh.kulkarni@linaro.org> wrote:
>>
>> On Wed, 20 Oct 2021 at 15:05, Richard Sandiford
>> <richard.sandiford@arm.com> wrote:
>>>
>>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>>>> On Tue, 19 Oct 2021 at 19:58, Richard Sandiford
>>>> <richard.sandiford@arm.com> wrote:
>>>>>
>>>>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>>>>>> Hi,
>>>>>> The attached patch emits a more verbose diagnostic for target attribute that
>>>>>> is an architecture extension needing a leading '+'.
>>>>>>
>>>>>> For the following test,
>>>>>> void calculate(void) __attribute__ ((__target__ ("sve")));
>>>>>>
>>>>>> With patch, the compiler now emits:
>>>>>> 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’
>>>>>>      1 | void calculate(void) __attribute__ ((__target__ ("sve")));
>>>>>>        | ^~~~
>>>>>>
>>>>>> instead of:
>>>>>> 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid
>>>>>>      1 | void calculate(void) __attribute__ ((__target__ ("sve")));
>>>>>>        | ^~~~
>>>>>
>>>>> Nice :-)
>>>>>
>>>>>> (This isn't specific to sve though).
>>>>>> OK to commit after bootstrap+test ?
>>>>>>
>>>>>> Thanks,
>>>>>> Prathamesh
>>>>>>
>>>>>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>>>>>> index a9a1800af53..975f7faf968 100644
>>>>>> --- a/gcc/config/aarch64/aarch64.c
>>>>>> +++ b/gcc/config/aarch64/aarch64.c
>>>>>> @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args)
>>>>>>         num_attrs++;
>>>>>>         if (!aarch64_process_one_target_attr (token))
>>>>>>        {
>>>>>> -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>>>>>> +       /* Check if token is possibly an arch extension without
>>>>>> +          leading '+'.  */
>>>>>> +       char *str = (char *) xmalloc (strlen (token) + 2);
>>>>>> +       str[0] = '+';
>>>>>> +       strcpy(str + 1, token);
>>>>>
>>>>> I think std::string would be better here, e.g.:
>>>>>
>>>>>    auto with_plus = std::string ("+") + token;
>>>>>
>>>>>> +       if (aarch64_handle_attr_isa_flags (str))
>>>>>> +         error("arch extension %<%s%> should be prepended with %<+%>", token);
>>>>>
>>>>> Nit: should be a space before the “(”.
>>>>>
>>>>> In principle, a fixit hint would have been nice here, but I don't think
>>>>> we have enough information to provide one.  (Just saying for the record.)
>>>> Thanks for the suggestions.
>>>> Does the attached patch look OK ?
>>>
>>> Looks good apart from a couple of formatting nits.
>>>>
>>>> Thanks,
>>>> Prathamesh
>>>>>
>>>>> Thanks,
>>>>> Richard
>>>>>
>>>>>> +       else
>>>>>> +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>>>>>> +       free (str);
>>>>>>          return false;
>>>>>>        }
>>>>>>
>>>>
>>>> [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute.
>>>>
>>>> gcc/ChangeLog:
>>>>        PR target/102376
>>>>        * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's
>>>>        type to const char *.
>>>>        (aarch64_process_target_attr): Check if token is possibly an arch extension
>>>>        without leading '+' and emit diagnostic accordingly.
>>>>
>>>> gcc/testsuite/ChangeLog:
>>>>        PR target/102376
>>>>        * gcc.target/aarch64/pr102376.c: New test.
>>>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>>>> index a9a1800af53..b72079bc466 100644
>>>> --- a/gcc/config/aarch64/aarch64.c
>>>> +++ b/gcc/config/aarch64/aarch64.c
>>>> @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str)
>>>>      modified.  */
>>>>
>>>>   static bool
>>>> -aarch64_handle_attr_isa_flags (char *str)
>>>> +aarch64_handle_attr_isa_flags (const char *str)
>>>>   {
>>>>     enum aarch64_parse_opt_result parse_res;
>>>>     uint64_t isa_flags = aarch64_isa_flags;
>>>> @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args)
>>>>         num_attrs++;
>>>>         if (!aarch64_process_one_target_attr (token))
>>>>        {
>>>> -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>>>> +       /* Check if token is possibly an arch extension without
>>>> +          leading '+'.  */
>>>> +       auto with_plus = std::string("+") + token;
>>>
>>> Should be a space before “(”.
>>>
>>>> +       if (aarch64_handle_attr_isa_flags (with_plus.c_str ()))
>>>> +         error ("arch extension %<%s%> should be prepended with %<+%>", token);
>>>
>>> Long line, should be:
>>>
>>>              error ("arch extension %<%s%> should be prepended with %<+%>",
>>>                     token);
>>>
>>> OK with those changes, thanks.
>> Thanks, the patch regressed some target attr tests because it emitted
>> diagnostics twice from
>> aarch64_handle_attr_isa_flags.
>> So for eg, spellcheck_1.c:
>> __attribute__((target ("arch=armv8-a-typo"))) void foo () {}
>>
>> results in:
>> spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in
>> ‘target("arch=")’ pragma or attribute
>>      5 | {
>>        | ^
>> spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a
>> armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r
>> armv9-a
>> spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo
>> of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute
>> spellcheck_1.c:5:1: error: pragma or attribute
>> ‘target("arch=armv8-a-typo")’ is not valid
>>
>> The patch adds an additional argument to the
>> aarch64_handle_attr_isa_flags, to optionally not emit an error, which
>> works to fix the issue.
>> Does it look OK ?
> ping https://gcc.gnu.org/pipermail/gcc-patches/2021-October/582345.html

Just a couple of minor points:

+	  if (aarch64_handle_attr_isa_flags (with_plus.c_str (), false))
+	    error ("arch extension %<%s%> should be prepended with %<+%>",
+		   token);

The phrase is to prepend something or prepend it to something,
but usually not to "prepend with."  In this context I think
either "prefixed by" or "preceded by" would be correct.

Also, although there are several other pre-existing uses,
the abbreviation arch is not an English word that lends itself
to translation.  Judging by the German and French .po files,
their authors take the trouble of spelling it out, but others
such as Spanish or Russian don't, leaving it as is, presumably
because they're not sure whether it's supposed to be translated.
In the Russian translation it looks especially odd rendered in
the latin alphabet in the middle of a sentence in Cyrillic.
I think we should follow the German and French translators'
lead and spell it out in English as well.

   "architecture extension %<%s%> should be prefixed by %<+%>"

Martin

> 
> Thanks,
> Prathamesh
>>
>> Thanks,
>> Prathamesh
>>>
>>> Richard
>>>
>>>
>>>> +       else
>>>> +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>>>>          return false;
>>>>        }
>>>>
>>>> diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c
>>>> new file mode 100644
>>>> index 00000000000..efd15f6ca9b
>>>> --- /dev/null
>>>> +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c
>>>> @@ -0,0 +1,3 @@
>>>> +/* { dg-do compile } */
>>>> +
>>>> +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */
  

On Thu, 28 Oct 2021 at 21:33, Martin Sebor <msebor@gmail.com> wrote:
>
> On 10/28/21 2:59 AM, Prathamesh Kulkarni via Gcc-patches wrote:
> > On Fri, 22 Oct 2021 at 14:41, Prathamesh Kulkarni
> > <prathamesh.kulkarni@linaro.org> wrote:
> >>
> >> On Wed, 20 Oct 2021 at 15:05, Richard Sandiford
> >> <richard.sandiford@arm.com> wrote:
> >>>
> >>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> >>>> On Tue, 19 Oct 2021 at 19:58, Richard Sandiford
> >>>> <richard.sandiford@arm.com> wrote:
> >>>>>
> >>>>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> >>>>>> Hi,
> >>>>>> The attached patch emits a more verbose diagnostic for target attribute that
> >>>>>> is an architecture extension needing a leading '+'.
> >>>>>>
> >>>>>> For the following test,
> >>>>>> void calculate(void) __attribute__ ((__target__ ("sve")));
> >>>>>>
> >>>>>> With patch, the compiler now emits:
> >>>>>> 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’
> >>>>>>      1 | void calculate(void) __attribute__ ((__target__ ("sve")));
> >>>>>>        | ^~~~
> >>>>>>
> >>>>>> instead of:
> >>>>>> 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid
> >>>>>>      1 | void calculate(void) __attribute__ ((__target__ ("sve")));
> >>>>>>        | ^~~~
> >>>>>
> >>>>> Nice :-)
> >>>>>
> >>>>>> (This isn't specific to sve though).
> >>>>>> OK to commit after bootstrap+test ?
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Prathamesh
> >>>>>>
> >>>>>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> >>>>>> index a9a1800af53..975f7faf968 100644
> >>>>>> --- a/gcc/config/aarch64/aarch64.c
> >>>>>> +++ b/gcc/config/aarch64/aarch64.c
> >>>>>> @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args)
> >>>>>>         num_attrs++;
> >>>>>>         if (!aarch64_process_one_target_attr (token))
> >>>>>>        {
> >>>>>> -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >>>>>> +       /* Check if token is possibly an arch extension without
> >>>>>> +          leading '+'.  */
> >>>>>> +       char *str = (char *) xmalloc (strlen (token) + 2);
> >>>>>> +       str[0] = '+';
> >>>>>> +       strcpy(str + 1, token);
> >>>>>
> >>>>> I think std::string would be better here, e.g.:
> >>>>>
> >>>>>    auto with_plus = std::string ("+") + token;
> >>>>>
> >>>>>> +       if (aarch64_handle_attr_isa_flags (str))
> >>>>>> +         error("arch extension %<%s%> should be prepended with %<+%>", token);
> >>>>>
> >>>>> Nit: should be a space before the “(”.
> >>>>>
> >>>>> In principle, a fixit hint would have been nice here, but I don't think
> >>>>> we have enough information to provide one.  (Just saying for the record.)
> >>>> Thanks for the suggestions.
> >>>> Does the attached patch look OK ?
> >>>
> >>> Looks good apart from a couple of formatting nits.
> >>>>
> >>>> Thanks,
> >>>> Prathamesh
> >>>>>
> >>>>> Thanks,
> >>>>> Richard
> >>>>>
> >>>>>> +       else
> >>>>>> +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >>>>>> +       free (str);
> >>>>>>          return false;
> >>>>>>        }
> >>>>>>
> >>>>
> >>>> [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute.
> >>>>
> >>>> gcc/ChangeLog:
> >>>>        PR target/102376
> >>>>        * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's
> >>>>        type to const char *.
> >>>>        (aarch64_process_target_attr): Check if token is possibly an arch extension
> >>>>        without leading '+' and emit diagnostic accordingly.
> >>>>
> >>>> gcc/testsuite/ChangeLog:
> >>>>        PR target/102376
> >>>>        * gcc.target/aarch64/pr102376.c: New test.
> >>>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> >>>> index a9a1800af53..b72079bc466 100644
> >>>> --- a/gcc/config/aarch64/aarch64.c
> >>>> +++ b/gcc/config/aarch64/aarch64.c
> >>>> @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str)
> >>>>      modified.  */
> >>>>
> >>>>   static bool
> >>>> -aarch64_handle_attr_isa_flags (char *str)
> >>>> +aarch64_handle_attr_isa_flags (const char *str)
> >>>>   {
> >>>>     enum aarch64_parse_opt_result parse_res;
> >>>>     uint64_t isa_flags = aarch64_isa_flags;
> >>>> @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args)
> >>>>         num_attrs++;
> >>>>         if (!aarch64_process_one_target_attr (token))
> >>>>        {
> >>>> -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >>>> +       /* Check if token is possibly an arch extension without
> >>>> +          leading '+'.  */
> >>>> +       auto with_plus = std::string("+") + token;
> >>>
> >>> Should be a space before “(”.
> >>>
> >>>> +       if (aarch64_handle_attr_isa_flags (with_plus.c_str ()))
> >>>> +         error ("arch extension %<%s%> should be prepended with %<+%>", token);
> >>>
> >>> Long line, should be:
> >>>
> >>>              error ("arch extension %<%s%> should be prepended with %<+%>",
> >>>                     token);
> >>>
> >>> OK with those changes, thanks.
> >> Thanks, the patch regressed some target attr tests because it emitted
> >> diagnostics twice from
> >> aarch64_handle_attr_isa_flags.
> >> So for eg, spellcheck_1.c:
> >> __attribute__((target ("arch=armv8-a-typo"))) void foo () {}
> >>
> >> results in:
> >> spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in
> >> ‘target("arch=")’ pragma or attribute
> >>      5 | {
> >>        | ^
> >> spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a
> >> armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r
> >> armv9-a
> >> spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo
> >> of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute
> >> spellcheck_1.c:5:1: error: pragma or attribute
> >> ‘target("arch=armv8-a-typo")’ is not valid
> >>
> >> The patch adds an additional argument to the
> >> aarch64_handle_attr_isa_flags, to optionally not emit an error, which
> >> works to fix the issue.
> >> Does it look OK ?
> > ping https://gcc.gnu.org/pipermail/gcc-patches/2021-October/582345.html
>
> Just a couple of minor points:
>
> +         if (aarch64_handle_attr_isa_flags (with_plus.c_str (), false))
> +           error ("arch extension %<%s%> should be prepended with %<+%>",
> +                  token);
>
> The phrase is to prepend something or prepend it to something,
> but usually not to "prepend with."  In this context I think
> either "prefixed by" or "preceded by" would be correct.
>
> Also, although there are several other pre-existing uses,
> the abbreviation arch is not an English word that lends itself
> to translation.  Judging by the German and French .po files,
> their authors take the trouble of spelling it out, but others
> such as Spanish or Russian don't, leaving it as is, presumably
> because they're not sure whether it's supposed to be translated.
> In the Russian translation it looks especially odd rendered in
> the latin alphabet in the middle of a sentence in Cyrillic.
> I think we should follow the German and French translators'
> lead and spell it out in English as well.
>
>    "architecture extension %<%s%> should be prefixed by %<+%>"
Hi Martin,
Thanks for the suggestions and sorry for late response.
The attached patch updates the diagnostic to use "prefixed by" instead.

Richard, is this patch OK to commit after bootstrap+test ?

Thanks,
Prathamesh
>
> Martin
>
> >
> > Thanks,
> > Prathamesh
> >>
> >> Thanks,
> >> Prathamesh
> >>>
> >>> Richard
> >>>
> >>>
> >>>> +       else
> >>>> +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >>>>          return false;
> >>>>        }
> >>>>
> >>>> diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c
> >>>> new file mode 100644
> >>>> index 00000000000..efd15f6ca9b
> >>>> --- /dev/null
> >>>> +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c
> >>>> @@ -0,0 +1,3 @@
> >>>> +/* { dg-do compile } */
> >>>> +
> >>>> +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prepended with '\\+'" } */
>
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index fd9249c62b3..b449241f6bd 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -17571,7 +17571,7 @@ aarch64_handle_attr_tune (const char *str)
    modified.  */
 
 static bool
-aarch64_handle_attr_isa_flags (char *str)
+aarch64_handle_attr_isa_flags (const char *str, bool emit_diagnostic = true)
 {
   enum aarch64_parse_opt_result parse_res;
   uint64_t isa_flags = aarch64_isa_flags;
@@ -17593,6 +17593,9 @@ aarch64_handle_attr_isa_flags (char *str)
       return true;
     }
 
+  if (!emit_diagnostic)
+    return false;
+
   switch (parse_res)
     {
       case AARCH64_PARSE_MISSING_ARG:
@@ -17844,7 +17847,14 @@ aarch64_process_target_attr (tree args)
       num_attrs++;
       if (!aarch64_process_one_target_attr (token))
 	{
-	  error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
+	  /* Check if token is possibly an arch extension without
+	     leading '+'.  */
+	  auto with_plus = std::string ("+") + token;
+	  if (aarch64_handle_attr_isa_flags (with_plus.c_str (), false))
+	    error ("arch extension %<%s%> should be prefixed by %<+%>",
+		   token);
+	  else
+	    error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
 	  return false;
 	}
 
diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c
new file mode 100644
index 00000000000..fc830ad4742
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c
@@ -0,0 +1,3 @@
+/* { dg-do compile } */
+
+void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prefixed by '\\+'" } */
  

Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> On Wed, 20 Oct 2021 at 15:05, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>> > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford
>> > <richard.sandiford@arm.com> wrote:
>> >>
>> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>> >> > Hi,
>> >> > The attached patch emits a more verbose diagnostic for target attribute that
>> >> > is an architecture extension needing a leading '+'.
>> >> >
>> >> > For the following test,
>> >> > void calculate(void) __attribute__ ((__target__ ("sve")));
>> >> >
>> >> > With patch, the compiler now emits:
>> >> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’
>> >> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
>> >> >       | ^~~~
>> >> >
>> >> > instead of:
>> >> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid
>> >> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
>> >> >       | ^~~~
>> >>
>> >> Nice :-)
>> >>
>> >> > (This isn't specific to sve though).
>> >> > OK to commit after bootstrap+test ?
>> >> >
>> >> > Thanks,
>> >> > Prathamesh
>> >> >
>> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> >> > index a9a1800af53..975f7faf968 100644
>> >> > --- a/gcc/config/aarch64/aarch64.c
>> >> > +++ b/gcc/config/aarch64/aarch64.c
>> >> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args)
>> >> >        num_attrs++;
>> >> >        if (!aarch64_process_one_target_attr (token))
>> >> >       {
>> >> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>> >> > +       /* Check if token is possibly an arch extension without
>> >> > +          leading '+'.  */
>> >> > +       char *str = (char *) xmalloc (strlen (token) + 2);
>> >> > +       str[0] = '+';
>> >> > +       strcpy(str + 1, token);
>> >>
>> >> I think std::string would be better here, e.g.:
>> >>
>> >>   auto with_plus = std::string ("+") + token;
>> >>
>> >> > +       if (aarch64_handle_attr_isa_flags (str))
>> >> > +         error("arch extension %<%s%> should be prepended with %<+%>", token);
>> >>
>> >> Nit: should be a space before the “(”.
>> >>
>> >> In principle, a fixit hint would have been nice here, but I don't think
>> >> we have enough information to provide one.  (Just saying for the record.)
>> > Thanks for the suggestions.
>> > Does the attached patch look OK ?
>>
>> Looks good apart from a couple of formatting nits.
>> >
>> > Thanks,
>> > Prathamesh
>> >>
>> >> Thanks,
>> >> Richard
>> >>
>> >> > +       else
>> >> > +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>> >> > +       free (str);
>> >> >         return false;
>> >> >       }
>> >> >
>> >
>> > [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute.
>> >
>> > gcc/ChangeLog:
>> >       PR target/102376
>> >       * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's
>> >       type to const char *.
>> >       (aarch64_process_target_attr): Check if token is possibly an arch extension
>> >       without leading '+' and emit diagnostic accordingly.
>> >
>> > gcc/testsuite/ChangeLog:
>> >       PR target/102376
>> >       * gcc.target/aarch64/pr102376.c: New test.
>> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> > index a9a1800af53..b72079bc466 100644
>> > --- a/gcc/config/aarch64/aarch64.c
>> > +++ b/gcc/config/aarch64/aarch64.c
>> > @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str)
>> >     modified.  */
>> >
>> >  static bool
>> > -aarch64_handle_attr_isa_flags (char *str)
>> > +aarch64_handle_attr_isa_flags (const char *str)
>> >  {
>> >    enum aarch64_parse_opt_result parse_res;
>> >    uint64_t isa_flags = aarch64_isa_flags;
>> > @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args)
>> >        num_attrs++;
>> >        if (!aarch64_process_one_target_attr (token))
>> >       {
>> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>> > +       /* Check if token is possibly an arch extension without
>> > +          leading '+'.  */
>> > +       auto with_plus = std::string("+") + token;
>>
>> Should be a space before “(”.
>>
>> > +       if (aarch64_handle_attr_isa_flags (with_plus.c_str ()))
>> > +         error ("arch extension %<%s%> should be prepended with %<+%>", token);
>>
>> Long line, should be:
>>
>>             error ("arch extension %<%s%> should be prepended with %<+%>",
>>                    token);
>>
>> OK with those changes, thanks.
> Thanks, the patch regressed some target attr tests because it emitted
> diagnostics twice from
> aarch64_handle_attr_isa_flags.
> So for eg, spellcheck_1.c:
> __attribute__((target ("arch=armv8-a-typo"))) void foo () {}
>
> results in:
> spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in
> ‘target("arch=")’ pragma or attribute
>     5 | {
>       | ^
> spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a
> armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r
> armv9-a
> spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo
> of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute
> spellcheck_1.c:5:1: error: pragma or attribute
> ‘target("arch=armv8-a-typo")’ is not valid
>
> The patch adds an additional argument to the
> aarch64_handle_attr_isa_flags, to optionally not emit an error, which
> works to fix the issue.
> Does it look OK ?

I think we should instead call aarch64_parse_arch directly, passing
temporary ISA flags instead of &aarch64_isa_flags.  That should ensure
that the call has no side effects.

I agree the new wording (in the later patch) is better, thanks.

Richard
  

On Thu, 4 Nov 2021 at 14:19, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> > On Wed, 20 Oct 2021 at 15:05, Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> >>
> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> >> > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford
> >> > <richard.sandiford@arm.com> wrote:
> >> >>
> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> >> >> > Hi,
> >> >> > The attached patch emits a more verbose diagnostic for target attribute that
> >> >> > is an architecture extension needing a leading '+'.
> >> >> >
> >> >> > For the following test,
> >> >> > void calculate(void) __attribute__ ((__target__ ("sve")));
> >> >> >
> >> >> > With patch, the compiler now emits:
> >> >> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’
> >> >> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
> >> >> >       | ^~~~
> >> >> >
> >> >> > instead of:
> >> >> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid
> >> >> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
> >> >> >       | ^~~~
> >> >>
> >> >> Nice :-)
> >> >>
> >> >> > (This isn't specific to sve though).
> >> >> > OK to commit after bootstrap+test ?
> >> >> >
> >> >> > Thanks,
> >> >> > Prathamesh
> >> >> >
> >> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> >> >> > index a9a1800af53..975f7faf968 100644
> >> >> > --- a/gcc/config/aarch64/aarch64.c
> >> >> > +++ b/gcc/config/aarch64/aarch64.c
> >> >> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args)
> >> >> >        num_attrs++;
> >> >> >        if (!aarch64_process_one_target_attr (token))
> >> >> >       {
> >> >> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >> >> > +       /* Check if token is possibly an arch extension without
> >> >> > +          leading '+'.  */
> >> >> > +       char *str = (char *) xmalloc (strlen (token) + 2);
> >> >> > +       str[0] = '+';
> >> >> > +       strcpy(str + 1, token);
> >> >>
> >> >> I think std::string would be better here, e.g.:
> >> >>
> >> >>   auto with_plus = std::string ("+") + token;
> >> >>
> >> >> > +       if (aarch64_handle_attr_isa_flags (str))
> >> >> > +         error("arch extension %<%s%> should be prepended with %<+%>", token);
> >> >>
> >> >> Nit: should be a space before the “(”.
> >> >>
> >> >> In principle, a fixit hint would have been nice here, but I don't think
> >> >> we have enough information to provide one.  (Just saying for the record.)
> >> > Thanks for the suggestions.
> >> > Does the attached patch look OK ?
> >>
> >> Looks good apart from a couple of formatting nits.
> >> >
> >> > Thanks,
> >> > Prathamesh
> >> >>
> >> >> Thanks,
> >> >> Richard
> >> >>
> >> >> > +       else
> >> >> > +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >> >> > +       free (str);
> >> >> >         return false;
> >> >> >       }
> >> >> >
> >> >
> >> > [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute.
> >> >
> >> > gcc/ChangeLog:
> >> >       PR target/102376
> >> >       * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's
> >> >       type to const char *.
> >> >       (aarch64_process_target_attr): Check if token is possibly an arch extension
> >> >       without leading '+' and emit diagnostic accordingly.
> >> >
> >> > gcc/testsuite/ChangeLog:
> >> >       PR target/102376
> >> >       * gcc.target/aarch64/pr102376.c: New test.
> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> >> > index a9a1800af53..b72079bc466 100644
> >> > --- a/gcc/config/aarch64/aarch64.c
> >> > +++ b/gcc/config/aarch64/aarch64.c
> >> > @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str)
> >> >     modified.  */
> >> >
> >> >  static bool
> >> > -aarch64_handle_attr_isa_flags (char *str)
> >> > +aarch64_handle_attr_isa_flags (const char *str)
> >> >  {
> >> >    enum aarch64_parse_opt_result parse_res;
> >> >    uint64_t isa_flags = aarch64_isa_flags;
> >> > @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args)
> >> >        num_attrs++;
> >> >        if (!aarch64_process_one_target_attr (token))
> >> >       {
> >> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >> > +       /* Check if token is possibly an arch extension without
> >> > +          leading '+'.  */
> >> > +       auto with_plus = std::string("+") + token;
> >>
> >> Should be a space before “(”.
> >>
> >> > +       if (aarch64_handle_attr_isa_flags (with_plus.c_str ()))
> >> > +         error ("arch extension %<%s%> should be prepended with %<+%>", token);
> >>
> >> Long line, should be:
> >>
> >>             error ("arch extension %<%s%> should be prepended with %<+%>",
> >>                    token);
> >>
> >> OK with those changes, thanks.
> > Thanks, the patch regressed some target attr tests because it emitted
> > diagnostics twice from
> > aarch64_handle_attr_isa_flags.
> > So for eg, spellcheck_1.c:
> > __attribute__((target ("arch=armv8-a-typo"))) void foo () {}
> >
> > results in:
> > spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in
> > ‘target("arch=")’ pragma or attribute
> >     5 | {
> >       | ^
> > spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a
> > armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r
> > armv9-a
> > spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo
> > of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute
> > spellcheck_1.c:5:1: error: pragma or attribute
> > ‘target("arch=armv8-a-typo")’ is not valid
> >
> > The patch adds an additional argument to the
> > aarch64_handle_attr_isa_flags, to optionally not emit an error, which
> > works to fix the issue.
> > Does it look OK ?
>
> I think we should instead call aarch64_parse_arch directly, passing
> temporary ISA flags instead of &aarch64_isa_flags.  That should ensure
> that the call has no side effects.
>
> I agree the new wording (in the later patch) is better, thanks.
Thanks for the suggestions, does the attached patch look OK ?

Thanks,
Prathamesh
>
> Richard
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index fd9249c62b3..218a7e06f68 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -17844,7 +17844,18 @@ aarch64_process_target_attr (tree args)
       num_attrs++;
       if (!aarch64_process_one_target_attr (token))
 	{
-	  error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
+	  /* Check if token is possibly an arch extension without
+	     leading '+'.  */
+	  uint64_t isa_temp = 0;
+	  auto with_plus = std::string ("+") + token;
+	  enum aarch64_parse_opt_result ext_res
+	    = aarch64_parse_extension (with_plus.c_str (), &isa_temp, nullptr);
+
+	  if (ext_res == AARCH64_PARSE_OK)
+	    error ("arch extension %<%s%> should be prefixed by %<+%>",
+		   token);
+	  else
+	    error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
 	  return false;
 	}
 
diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c
new file mode 100644
index 00000000000..fc830ad4742
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c
@@ -0,0 +1,3 @@
+/* { dg-do compile } */
+
+void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prefixed by '\\+'" } */
  

Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> On Thu, 4 Nov 2021 at 14:19, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>> > On Wed, 20 Oct 2021 at 15:05, Richard Sandiford
>> > <richard.sandiford@arm.com> wrote:
>> >>
>> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>> >> > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford
>> >> > <richard.sandiford@arm.com> wrote:
>> >> >>
>> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>> >> >> > Hi,
>> >> >> > The attached patch emits a more verbose diagnostic for target attribute that
>> >> >> > is an architecture extension needing a leading '+'.
>> >> >> >
>> >> >> > For the following test,
>> >> >> > void calculate(void) __attribute__ ((__target__ ("sve")));
>> >> >> >
>> >> >> > With patch, the compiler now emits:
>> >> >> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’
>> >> >> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
>> >> >> >       | ^~~~
>> >> >> >
>> >> >> > instead of:
>> >> >> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid
>> >> >> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
>> >> >> >       | ^~~~
>> >> >>
>> >> >> Nice :-)
>> >> >>
>> >> >> > (This isn't specific to sve though).
>> >> >> > OK to commit after bootstrap+test ?
>> >> >> >
>> >> >> > Thanks,
>> >> >> > Prathamesh
>> >> >> >
>> >> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> >> >> > index a9a1800af53..975f7faf968 100644
>> >> >> > --- a/gcc/config/aarch64/aarch64.c
>> >> >> > +++ b/gcc/config/aarch64/aarch64.c
>> >> >> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args)
>> >> >> >        num_attrs++;
>> >> >> >        if (!aarch64_process_one_target_attr (token))
>> >> >> >       {
>> >> >> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>> >> >> > +       /* Check if token is possibly an arch extension without
>> >> >> > +          leading '+'.  */
>> >> >> > +       char *str = (char *) xmalloc (strlen (token) + 2);
>> >> >> > +       str[0] = '+';
>> >> >> > +       strcpy(str + 1, token);
>> >> >>
>> >> >> I think std::string would be better here, e.g.:
>> >> >>
>> >> >>   auto with_plus = std::string ("+") + token;
>> >> >>
>> >> >> > +       if (aarch64_handle_attr_isa_flags (str))
>> >> >> > +         error("arch extension %<%s%> should be prepended with %<+%>", token);
>> >> >>
>> >> >> Nit: should be a space before the “(”.
>> >> >>
>> >> >> In principle, a fixit hint would have been nice here, but I don't think
>> >> >> we have enough information to provide one.  (Just saying for the record.)
>> >> > Thanks for the suggestions.
>> >> > Does the attached patch look OK ?
>> >>
>> >> Looks good apart from a couple of formatting nits.
>> >> >
>> >> > Thanks,
>> >> > Prathamesh
>> >> >>
>> >> >> Thanks,
>> >> >> Richard
>> >> >>
>> >> >> > +       else
>> >> >> > +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>> >> >> > +       free (str);
>> >> >> >         return false;
>> >> >> >       }
>> >> >> >
>> >> >
>> >> > [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute.
>> >> >
>> >> > gcc/ChangeLog:
>> >> >       PR target/102376
>> >> >       * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's
>> >> >       type to const char *.
>> >> >       (aarch64_process_target_attr): Check if token is possibly an arch extension
>> >> >       without leading '+' and emit diagnostic accordingly.
>> >> >
>> >> > gcc/testsuite/ChangeLog:
>> >> >       PR target/102376
>> >> >       * gcc.target/aarch64/pr102376.c: New test.
>> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> >> > index a9a1800af53..b72079bc466 100644
>> >> > --- a/gcc/config/aarch64/aarch64.c
>> >> > +++ b/gcc/config/aarch64/aarch64.c
>> >> > @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str)
>> >> >     modified.  */
>> >> >
>> >> >  static bool
>> >> > -aarch64_handle_attr_isa_flags (char *str)
>> >> > +aarch64_handle_attr_isa_flags (const char *str)
>> >> >  {
>> >> >    enum aarch64_parse_opt_result parse_res;
>> >> >    uint64_t isa_flags = aarch64_isa_flags;
>> >> > @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args)
>> >> >        num_attrs++;
>> >> >        if (!aarch64_process_one_target_attr (token))
>> >> >       {
>> >> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>> >> > +       /* Check if token is possibly an arch extension without
>> >> > +          leading '+'.  */
>> >> > +       auto with_plus = std::string("+") + token;
>> >>
>> >> Should be a space before “(”.
>> >>
>> >> > +       if (aarch64_handle_attr_isa_flags (with_plus.c_str ()))
>> >> > +         error ("arch extension %<%s%> should be prepended with %<+%>", token);
>> >>
>> >> Long line, should be:
>> >>
>> >>             error ("arch extension %<%s%> should be prepended with %<+%>",
>> >>                    token);
>> >>
>> >> OK with those changes, thanks.
>> > Thanks, the patch regressed some target attr tests because it emitted
>> > diagnostics twice from
>> > aarch64_handle_attr_isa_flags.
>> > So for eg, spellcheck_1.c:
>> > __attribute__((target ("arch=armv8-a-typo"))) void foo () {}
>> >
>> > results in:
>> > spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in
>> > ‘target("arch=")’ pragma or attribute
>> >     5 | {
>> >       | ^
>> > spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a
>> > armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r
>> > armv9-a
>> > spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo
>> > of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute
>> > spellcheck_1.c:5:1: error: pragma or attribute
>> > ‘target("arch=armv8-a-typo")’ is not valid
>> >
>> > The patch adds an additional argument to the
>> > aarch64_handle_attr_isa_flags, to optionally not emit an error, which
>> > works to fix the issue.
>> > Does it look OK ?
>>
>> I think we should instead call aarch64_parse_arch directly, passing
>> temporary ISA flags instead of &aarch64_isa_flags.  That should ensure
>> that the call has no side effects.
>>
>> I agree the new wording (in the later patch) is better, thanks.
> Thanks for the suggestions, does the attached patch look OK ?

Please remember to say how you tested patches.

OK assuming it passed bootstrap & regression-test on aarch64-linux-gnu.

Thanks,
Richard

>
> Thanks,
> Prathamesh
>>
>> Richard
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index fd9249c62b3..218a7e06f68 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -17844,7 +17844,18 @@ aarch64_process_target_attr (tree args)
>        num_attrs++;
>        if (!aarch64_process_one_target_attr (token))
>  	{
> -	  error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> +	  /* Check if token is possibly an arch extension without
> +	     leading '+'.  */
> +	  uint64_t isa_temp = 0;
> +	  auto with_plus = std::string ("+") + token;
> +	  enum aarch64_parse_opt_result ext_res
> +	    = aarch64_parse_extension (with_plus.c_str (), &isa_temp, nullptr);
> +
> +	  if (ext_res == AARCH64_PARSE_OK)
> +	    error ("arch extension %<%s%> should be prefixed by %<+%>",
> +		   token);
> +	  else
> +	    error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
>  	  return false;
>  	}
>  
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c
> new file mode 100644
> index 00000000000..fc830ad4742
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c
> @@ -0,0 +1,3 @@
> +/* { dg-do compile } */
> +
> +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prefixed by '\\+'" } */
  

On Tue, 9 Nov 2021 at 20:27, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> > On Thu, 4 Nov 2021 at 14:19, Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> >>
> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> >> > On Wed, 20 Oct 2021 at 15:05, Richard Sandiford
> >> > <richard.sandiford@arm.com> wrote:
> >> >>
> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> >> >> > On Tue, 19 Oct 2021 at 19:58, Richard Sandiford
> >> >> > <richard.sandiford@arm.com> wrote:
> >> >> >>
> >> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> >> >> >> > Hi,
> >> >> >> > The attached patch emits a more verbose diagnostic for target attribute that
> >> >> >> > is an architecture extension needing a leading '+'.
> >> >> >> >
> >> >> >> > For the following test,
> >> >> >> > void calculate(void) __attribute__ ((__target__ ("sve")));
> >> >> >> >
> >> >> >> > With patch, the compiler now emits:
> >> >> >> > 102376.c:1:1: error: arch extension ‘sve’ should be prepended with ‘+’
> >> >> >> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
> >> >> >> >       | ^~~~
> >> >> >> >
> >> >> >> > instead of:
> >> >> >> > 102376.c:1:1: error: pragma or attribute ‘target("sve")’ is not valid
> >> >> >> >     1 | void calculate(void) __attribute__ ((__target__ ("sve")));
> >> >> >> >       | ^~~~
> >> >> >>
> >> >> >> Nice :-)
> >> >> >>
> >> >> >> > (This isn't specific to sve though).
> >> >> >> > OK to commit after bootstrap+test ?
> >> >> >> >
> >> >> >> > Thanks,
> >> >> >> > Prathamesh
> >> >> >> >
> >> >> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> >> >> >> > index a9a1800af53..975f7faf968 100644
> >> >> >> > --- a/gcc/config/aarch64/aarch64.c
> >> >> >> > +++ b/gcc/config/aarch64/aarch64.c
> >> >> >> > @@ -17821,7 +17821,16 @@ aarch64_process_target_attr (tree args)
> >> >> >> >        num_attrs++;
> >> >> >> >        if (!aarch64_process_one_target_attr (token))
> >> >> >> >       {
> >> >> >> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >> >> >> > +       /* Check if token is possibly an arch extension without
> >> >> >> > +          leading '+'.  */
> >> >> >> > +       char *str = (char *) xmalloc (strlen (token) + 2);
> >> >> >> > +       str[0] = '+';
> >> >> >> > +       strcpy(str + 1, token);
> >> >> >>
> >> >> >> I think std::string would be better here, e.g.:
> >> >> >>
> >> >> >>   auto with_plus = std::string ("+") + token;
> >> >> >>
> >> >> >> > +       if (aarch64_handle_attr_isa_flags (str))
> >> >> >> > +         error("arch extension %<%s%> should be prepended with %<+%>", token);
> >> >> >>
> >> >> >> Nit: should be a space before the “(”.
> >> >> >>
> >> >> >> In principle, a fixit hint would have been nice here, but I don't think
> >> >> >> we have enough information to provide one.  (Just saying for the record.)
> >> >> > Thanks for the suggestions.
> >> >> > Does the attached patch look OK ?
> >> >>
> >> >> Looks good apart from a couple of formatting nits.
> >> >> >
> >> >> > Thanks,
> >> >> > Prathamesh
> >> >> >>
> >> >> >> Thanks,
> >> >> >> Richard
> >> >> >>
> >> >> >> > +       else
> >> >> >> > +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >> >> >> > +       free (str);
> >> >> >> >         return false;
> >> >> >> >       }
> >> >> >> >
> >> >> >
> >> >> > [aarch64] PR102376 - Emit better diagnostics for arch extension in target attribute.
> >> >> >
> >> >> > gcc/ChangeLog:
> >> >> >       PR target/102376
> >> >> >       * config/aarch64/aarch64.c (aarch64_handle_attr_isa_flags): Change str's
> >> >> >       type to const char *.
> >> >> >       (aarch64_process_target_attr): Check if token is possibly an arch extension
> >> >> >       without leading '+' and emit diagnostic accordingly.
> >> >> >
> >> >> > gcc/testsuite/ChangeLog:
> >> >> >       PR target/102376
> >> >> >       * gcc.target/aarch64/pr102376.c: New test.
> >> >> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> >> >> > index a9a1800af53..b72079bc466 100644
> >> >> > --- a/gcc/config/aarch64/aarch64.c
> >> >> > +++ b/gcc/config/aarch64/aarch64.c
> >> >> > @@ -17548,7 +17548,7 @@ aarch64_handle_attr_tune (const char *str)
> >> >> >     modified.  */
> >> >> >
> >> >> >  static bool
> >> >> > -aarch64_handle_attr_isa_flags (char *str)
> >> >> > +aarch64_handle_attr_isa_flags (const char *str)
> >> >> >  {
> >> >> >    enum aarch64_parse_opt_result parse_res;
> >> >> >    uint64_t isa_flags = aarch64_isa_flags;
> >> >> > @@ -17821,7 +17821,13 @@ aarch64_process_target_attr (tree args)
> >> >> >        num_attrs++;
> >> >> >        if (!aarch64_process_one_target_attr (token))
> >> >> >       {
> >> >> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >> >> > +       /* Check if token is possibly an arch extension without
> >> >> > +          leading '+'.  */
> >> >> > +       auto with_plus = std::string("+") + token;
> >> >>
> >> >> Should be a space before “(”.
> >> >>
> >> >> > +       if (aarch64_handle_attr_isa_flags (with_plus.c_str ()))
> >> >> > +         error ("arch extension %<%s%> should be prepended with %<+%>", token);
> >> >>
> >> >> Long line, should be:
> >> >>
> >> >>             error ("arch extension %<%s%> should be prepended with %<+%>",
> >> >>                    token);
> >> >>
> >> >> OK with those changes, thanks.
> >> > Thanks, the patch regressed some target attr tests because it emitted
> >> > diagnostics twice from
> >> > aarch64_handle_attr_isa_flags.
> >> > So for eg, spellcheck_1.c:
> >> > __attribute__((target ("arch=armv8-a-typo"))) void foo () {}
> >> >
> >> > results in:
> >> > spellcheck_1.c:5:1: error: invalid name ("armv8-a-typo") in
> >> > ‘target("arch=")’ pragma or attribute
> >> >     5 | {
> >> >       | ^
> >> > spellcheck_1.c:5:1: note: valid arguments are: armv8-a armv8.1-a
> >> > armv8.2-a armv8.3-a armv8.4-a armv8.5-a armv8.6-a armv8.7-a armv8-r
> >> > armv9-a
> >> > spellcheck_1.c:5:1: error: invalid feature modifier arch=armv8-a-typo
> >> > of value ("+arch=armv8-a-typo") in ‘target()’ pragma or attribute
> >> > spellcheck_1.c:5:1: error: pragma or attribute
> >> > ‘target("arch=armv8-a-typo")’ is not valid
> >> >
> >> > The patch adds an additional argument to the
> >> > aarch64_handle_attr_isa_flags, to optionally not emit an error, which
> >> > works to fix the issue.
> >> > Does it look OK ?
> >>
> >> I think we should instead call aarch64_parse_arch directly, passing
> >> temporary ISA flags instead of &aarch64_isa_flags.  That should ensure
> >> that the call has no side effects.
> >>
> >> I agree the new wording (in the later patch) is better, thanks.
> > Thanks for the suggestions, does the attached patch look OK ?
>
> Please remember to say how you tested patches.
Right, sorry will do henceforth.
>
> OK assuming it passed bootstrap & regression-test on aarch64-linux-gnu.
Thanks, committed as 145be5efaf5674a7d25c723dc5684392a6276834 after
bootstrap+test on aarch64-linux-gnu.

Thanks,
Prathamesh
>
> Thanks,
> Richard
>
> >
> > Thanks,
> > Prathamesh
> >>
> >> Richard
> >
> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > index fd9249c62b3..218a7e06f68 100644
> > --- a/gcc/config/aarch64/aarch64.c
> > +++ b/gcc/config/aarch64/aarch64.c
> > @@ -17844,7 +17844,18 @@ aarch64_process_target_attr (tree args)
> >        num_attrs++;
> >        if (!aarch64_process_one_target_attr (token))
> >       {
> > -       error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> > +       /* Check if token is possibly an arch extension without
> > +          leading '+'.  */
> > +       uint64_t isa_temp = 0;
> > +       auto with_plus = std::string ("+") + token;
> > +       enum aarch64_parse_opt_result ext_res
> > +         = aarch64_parse_extension (with_plus.c_str (), &isa_temp, nullptr);
> > +
> > +       if (ext_res == AARCH64_PARSE_OK)
> > +         error ("arch extension %<%s%> should be prefixed by %<+%>",
> > +                token);
> > +       else
> > +         error ("pragma or attribute %<target(\"%s\")%> is not valid", token);
> >         return false;
> >       }
> >
> > diff --git a/gcc/testsuite/gcc.target/aarch64/pr102376.c b/gcc/testsuite/gcc.target/aarch64/pr102376.c
> > new file mode 100644
> > index 00000000000..fc830ad4742
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/pr102376.c
> > @@ -0,0 +1,3 @@
> > +/* { dg-do compile } */
> > +
> > +void calculate(void) __attribute__ ((__target__ ("sve"))); /* { dg-error "arch extension 'sve' should be prefixed by '\\+'" } */