[PATCH 0/3] Do not ICE on unsupported value of target attribute.

[PATCH 0/3] Do not ICE on unsupported value of target attribute.

[marxin]

Hello.

There are various targets that support target attribute. However do ICE
when one passes a wrong value. I hope displaying an error message
(similar to what we do on i386) is the proper thing.

Apart from that, multiversioning should not rely on just target ifunc
support.

Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
Tested with cross-compilers that it does not ICE any longer.

Thanks,
Martin

marxin (3):
  Error message on target attribute on power target (PR target/79906)
  Error message on target attribute on aarch64 target (PR target/79889).
  Verify that target can create a dispatcher call (PR target/79892).

 gcc/config/aarch64/aarch64.c   | 8 ++++++--
 gcc/config/rs6000/rs6000.c     | 5 ++++-
 gcc/multiple_target.c          | 7 +++++++
 gcc/testsuite/g++.dg/ext/mv8.C | 2 +-
 4 files changed, 18 insertions(+), 4 deletions(-)

-- 
2.11.1

[PATCH 2/3] Error message on target attribute on aarch64 target (PR target/79889).

[marxin]

gcc/testsuite/ChangeLog:

2017-03-13  Martin Liska  <mliska@suse.cz>

	* g++.dg/ext/mv8.C: Add aarch64* targets.

gcc/ChangeLog:

2017-03-13  Martin Liska  <mliska@suse.cz>

	* config/aarch64/aarch64.c (aarch64_process_target_attr):
	Show error message instead of an ICE.
---
 gcc/config/aarch64/aarch64.c   | 8 ++++++--
 gcc/testsuite/g++.dg/ext/mv8.C | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index a069427f576..3107d6b84bf 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -9533,8 +9533,12 @@ aarch64_process_target_attr (tree args, const char* pragma_or_attr)
 
       return true;
     }
-  /* We expect to find a string to parse.  */
-  gcc_assert (TREE_CODE (args) == STRING_CST);
+
+  if (TREE_CODE (args) != STRING_CST)
+    {
+      error ("attribute %<target%> argument not a string");
+      return false;
+    }
 
   size_t len = strlen (TREE_STRING_POINTER (args));
   char *str_to_check = (char *) alloca (len + 1);
diff --git a/gcc/testsuite/g++.dg/ext/mv8.C b/gcc/testsuite/g++.dg/ext/mv8.C
index bbf90b5a328..b49ef84f392 100644
--- a/gcc/testsuite/g++.dg/ext/mv8.C
+++ b/gcc/testsuite/g++.dg/ext/mv8.C
@@ -1,4 +1,4 @@
-// { dg-do compile { target i?86-*-* x86_64-*-* powerpc*-*-* } }
+// { dg-do compile { target i?86-*-* x86_64-*-* powerpc*-*-* aarch64*-*-* } }
 // { dg-options "" }
 
 __attribute__((target (11,12)))
-- 
2.11.1

Re: [PATCH 2/3] Error message on target attribute on aarch64 target (PR target/79889).

[Martin Liška]

PING^ + adding aarch64 maintainer.

Thanks,
Martin

Re: [PATCH 2/3] Error message on target attribute on aarch64 target (PR target/79889).

[Kyrill Tkachov]

<CC-ing aarch64 maintainers/reviewers>

Hi Martin,

On 13/03/17 08:25, marxin wrote:
> gcc/testsuite/ChangeLog:
>
> 2017-03-13  Martin Liska  <mliska@suse.cz>
>
> 	* g++.dg/ext/mv8.C: Add aarch64* targets.
>
> gcc/ChangeLog:
>
> 2017-03-13  Martin Liska  <mliska@suse.cz>
>
> 	* config/aarch64/aarch64.c (aarch64_process_target_attr):
> 	Show error message instead of an ICE.

You should mention PR target/79889 in your ChangeLog
> ---
>   gcc/config/aarch64/aarch64.c   | 8 ++++++--
>   gcc/testsuite/g++.dg/ext/mv8.C | 2 +-
>   2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index a069427f576..3107d6b84bf 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -9533,8 +9533,12 @@ aarch64_process_target_attr (tree args, const char* pragma_or_attr)
>   
>         return true;
>       }
> -  /* We expect to find a string to parse.  */
> -  gcc_assert (TREE_CODE (args) == STRING_CST);
> +
> +  if (TREE_CODE (args) != STRING_CST)
> +    {
> +      error ("attribute %<target%> argument not a string");
> +      return false;
> +    }
>   

Looks reasonable to me, but you'll need approval from the folks CC-ed.

Thanks,
Kyrill

>     size_t len = strlen (TREE_STRING_POINTER (args));
>     char *str_to_check = (char *) alloca (len + 1);
> diff --git a/gcc/testsuite/g++.dg/ext/mv8.C b/gcc/testsuite/g++.dg/ext/mv8.C
> index bbf90b5a328..b49ef84f392 100644
> --- a/gcc/testsuite/g++.dg/ext/mv8.C
> +++ b/gcc/testsuite/g++.dg/ext/mv8.C
> @@ -1,4 +1,4 @@
> -// { dg-do compile { target i?86-*-* x86_64-*-* powerpc*-*-* } }
> +// { dg-do compile { target i?86-*-* x86_64-*-* powerpc*-*-* aarch64*-*-* } }
>   // { dg-options "" }
>   
>   __attribute__((target (11,12)))

Re: [PATCH 2/3] Error message on target attribute on aarch64 target (PR target/79889).

[Martin Liška]

Thanks Kyrill for review. I'm pinging the patch.

Martin

On 03/21/2017 10:24 AM, Kyrill Tkachov wrote:
> <CC-ing aarch64 maintainers/reviewers>
> 
> Hi Martin,
> 
> On 13/03/17 08:25, marxin wrote:
>> gcc/testsuite/ChangeLog:
>>
>> 2017-03-13  Martin Liska  <mliska@suse.cz>
>>
>>     * g++.dg/ext/mv8.C: Add aarch64* targets.
>>
>> gcc/ChangeLog:
>>
>> 2017-03-13  Martin Liska  <mliska@suse.cz>
>>
>>     * config/aarch64/aarch64.c (aarch64_process_target_attr):
>>     Show error message instead of an ICE.
> 
> You should mention PR target/79889 in your ChangeLog
>> ---
>>   gcc/config/aarch64/aarch64.c   | 8 ++++++--
>>   gcc/testsuite/g++.dg/ext/mv8.C | 2 +-
>>   2 files changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
>> index a069427f576..3107d6b84bf 100644
>> --- a/gcc/config/aarch64/aarch64.c
>> +++ b/gcc/config/aarch64/aarch64.c
>> @@ -9533,8 +9533,12 @@ aarch64_process_target_attr (tree args, const char* pragma_or_attr)
>>           return true;
>>       }
>> -  /* We expect to find a string to parse.  */
>> -  gcc_assert (TREE_CODE (args) == STRING_CST);
>> +
>> +  if (TREE_CODE (args) != STRING_CST)
>> +    {
>> +      error ("attribute %<target%> argument not a string");
>> +      return false;
>> +    }
>>   
> 
> Looks reasonable to me, but you'll need approval from the folks CC-ed.
> 
> Thanks,
> Kyrill
> 
>>     size_t len = strlen (TREE_STRING_POINTER (args));
>>     char *str_to_check = (char *) alloca (len + 1);
>> diff --git a/gcc/testsuite/g++.dg/ext/mv8.C b/gcc/testsuite/g++.dg/ext/mv8.C
>> index bbf90b5a328..b49ef84f392 100644
>> --- a/gcc/testsuite/g++.dg/ext/mv8.C
>> +++ b/gcc/testsuite/g++.dg/ext/mv8.C
>> @@ -1,4 +1,4 @@
>> -// { dg-do compile { target i?86-*-* x86_64-*-* powerpc*-*-* } }
>> +// { dg-do compile { target i?86-*-* x86_64-*-* powerpc*-*-* aarch64*-*-* } }
>>   // { dg-options "" }
>>     __attribute__((target (11,12)))
> 

Re: [PATCH 2/3] Error message on target attribute on aarch64 target (PR target/79889).

[James Greenhalgh]

On Mon, Apr 03, 2017 at 09:26:16AM +0200, Martin Liška wrote:
> Thanks Kyrill for review. I'm pinging the patch.

OK.

Thanks,
James

> 
> Martin
> 
> On 03/21/2017 10:24 AM, Kyrill Tkachov wrote:
> > <CC-ing aarch64 maintainers/reviewers>
> > 
> > Hi Martin,
> > 
> > On 13/03/17 08:25, marxin wrote:
> >> gcc/testsuite/ChangeLog:
> >>
> >> 2017-03-13  Martin Liska  <mliska@suse.cz>
> >>
> >>     * g++.dg/ext/mv8.C: Add aarch64* targets.
> >>
> >> gcc/ChangeLog:
> >>
> >> 2017-03-13  Martin Liska  <mliska@suse.cz>
> >>
> >>     * config/aarch64/aarch64.c (aarch64_process_target_attr):
> >>     Show error message instead of an ICE.
> > 
 

[PATCH 3/3] Verify that target can create a dispatcher call (PR target/79892).

[marxin]

gcc/ChangeLog:

2017-03-13  Martin Liska  <mliska@suse.cz>

	* multiple_target.c (create_dispatcher_calls): Check that
	a target can create a function dispatcher.
---
 gcc/multiple_target.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/gcc/multiple_target.c b/gcc/multiple_target.c
index 7b735ae81ae..cb792262d6e 100644
--- a/gcc/multiple_target.c
+++ b/gcc/multiple_target.c
@@ -68,6 +68,13 @@ create_dispatcher_calls (struct cgraph_node *node)
 		    " supported by this target");
 	  break;
 	}
+      else if (!targetm.get_function_versions_dispatcher)
+	{
+	  error_at (gimple_location (call),
+		    "target does not support function version dispatcher");
+	  break;
+	}
+
       e_next = e->next_caller;
       idecl = targetm.get_function_versions_dispatcher (decl);
       if (!idecl)
-- 
2.11.1

Re: [PATCH 3/3] Verify that target can create a dispatcher call (PR target/79892).

[Richard Biener]

On Mon, Mar 13, 2017 at 9:26 AM, marxin <mliska@suse.cz> wrote:
> gcc/ChangeLog:

Ok.

Richard.

> 2017-03-13  Martin Liska  <mliska@suse.cz>
>
>         * multiple_target.c (create_dispatcher_calls): Check that
>         a target can create a function dispatcher.
> ---
>  gcc/multiple_target.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/gcc/multiple_target.c b/gcc/multiple_target.c
> index 7b735ae81ae..cb792262d6e 100644
> --- a/gcc/multiple_target.c
> +++ b/gcc/multiple_target.c
> @@ -68,6 +68,13 @@ create_dispatcher_calls (struct cgraph_node *node)
>                     " supported by this target");
>           break;
>         }
> +      else if (!targetm.get_function_versions_dispatcher)
> +       {
> +         error_at (gimple_location (call),
> +                   "target does not support function version dispatcher");
> +         break;
> +       }
> +
>        e_next = e->next_caller;
>        idecl = targetm.get_function_versions_dispatcher (decl);
>        if (!idecl)
> --
> 2.11.1
>

[PATCH 1/3] Error message on target attribute on power target (PR target/79906)

[marxin]

gcc/ChangeLog:

2017-03-13  Martin Liska  <mliska@suse.cz>

	PR target/79906
	* config/rs6000/rs6000.c (rs6000_inner_target_options): Show
	error message instead of an ICE.

gcc/testsuite/ChangeLog:

2017-03-13  Martin Liska  <mliska@suse.cz>

	PR target/79906
	* g++.dg/ext/mv8.C: Add power* targets.
---
 gcc/config/rs6000/rs6000.c     | 5 ++++-
 gcc/testsuite/g++.dg/ext/mv8.C | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index a1b58e21450..8bf5ee098dd 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -39204,7 +39204,10 @@ rs6000_inner_target_options (tree args, bool attr_p)
     }
 
   else
-    gcc_unreachable ();
+    {
+      error ("attribute %<target%> argument not a string");
+      return false;
+    }
 
   return ret;
 }
diff --git a/gcc/testsuite/g++.dg/ext/mv8.C b/gcc/testsuite/g++.dg/ext/mv8.C
index 2e98dd7ad85..bbf90b5a328 100644
--- a/gcc/testsuite/g++.dg/ext/mv8.C
+++ b/gcc/testsuite/g++.dg/ext/mv8.C
@@ -1,4 +1,4 @@
-// { dg-do compile { target i?86-*-* x86_64-*-* } }
+// { dg-do compile { target i?86-*-* x86_64-*-* powerpc*-*-* } }
 // { dg-options "" }
 
 __attribute__((target (11,12)))
-- 
2.11.1

Re: [PATCH 1/3] Error message on target attribute on power target (PR target/79906)

[Martin Liška]

PING^ + adding power maintainer.

Thanks,
Martin

Re: [PATCH 1/3] Error message on target attribute on power target (PR target/79906)

[Segher Boessenkool]

On Mon, Mar 13, 2017 at 09:23:51AM +0100, marxin wrote:
> gcc/ChangeLog:
> 
> 2017-03-13  Martin Liska  <mliska@suse.cz>
> 
> 	PR target/79906
> 	* config/rs6000/rs6000.c (rs6000_inner_target_options): Show
> 	error message instead of an ICE.
> 
> gcc/testsuite/ChangeLog:
> 
> 2017-03-13  Martin Liska  <mliska@suse.cz>
> 
> 	PR target/79906
> 	* g++.dg/ext/mv8.C: Add power* targets.

Okay, thanks!


Segher

[PATCH 4/N] Do not ICE on an invalid input for MV.

[Martin Liška]

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

Hello.

This fixes ICE when one does not provide valid target names:
__attribute__((target_clones("default,foo,bar")))

In that situation I suggest to print:

$ ./xgcc -B. /tmp/mvc-ice.c
/tmp/mvc-ice.c:6:1: error: attribute(target("foo")) is unknown
 foo ()
 ^~~
/tmp/mvc-ice.c:6:1: error: attribute(target("bar")) is unknown
/tmp/mvc-ice.c: In function ‘foo.resolver’:
cc1: fatal error: At least one more version other than the default is expected

Should I add a test-case for a fatal error?

Thanks,
Martin

[-- Attachment #2: 0001-Do-not-ICE-on-an-invalid-input-for-MV.patch --]
[-- Type: text/x-patch, Size: 1075 bytes --]

From b00d0845869e82e7607fad692f278083520d6b47 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 14 Mar 2017 11:16:51 +0100
Subject: [PATCH] Do not ICE on an invalid input for MV.

gcc/ChangeLog:

2017-03-14  Martin Liska  <mliska@suse.cz>

	* config/i386/i386.c (dispatch_function_versions): Display fatal
	error instead of ICE.
---
 gcc/config/i386/i386.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 5fcd51f1385..ded9f14d02c 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -32670,7 +32670,10 @@ dispatch_function_versions (tree dispatch_decl,
 
   /* At least one more version other than the default.  */
   num_versions = fndecls->length ();
-  gcc_assert (num_versions >= 2);
+  if (num_versions < 2)
+    fatal_error (UNKNOWN_LOCATION,
+		 "At least one more version other than the default is "
+		 "expected");
 
   function_version_info = (struct _function_version_info *)
     XNEWVEC (struct _function_version_info, (num_versions - 1));
-- 
2.11.1

Re: [PATCH 4/N] Do not ICE on an invalid input for MV.

[Richard Biener]

On Tue, Mar 14, 2017 at 11:24 AM, Martin Liška <mliska@suse.cz> wrote:
> Hello.
>
> This fixes ICE when one does not provide valid target names:
> __attribute__((target_clones("default,foo,bar")))
>
> In that situation I suggest to print:
>
> $ ./xgcc -B. /tmp/mvc-ice.c
> /tmp/mvc-ice.c:6:1: error: attribute(target("foo")) is unknown
>  foo ()
>  ^~~
> /tmp/mvc-ice.c:6:1: error: attribute(target("bar")) is unknown
> /tmp/mvc-ice.c: In function ‘foo.resolver’:
> cc1: fatal error: At least one more version other than the default is expected
>
> Should I add a test-case for a fatal error?

Better avoid fatal_error, can't you just return early after regular error()?

Do we error similarly for __attribute__((target_clones("default")))
from generic code?

I wonder if we can just go ahead even with num_versions == 1?


>
> Thanks,
> Martin

Re: [PATCH 4/N] Do not ICE on an invalid input for MV.

[Martin Liška]

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

On 03/14/2017 12:09 PM, Richard Biener wrote:
> On Tue, Mar 14, 2017 at 11:24 AM, Martin Liška <mliska@suse.cz> wrote:
>> Hello.
>>
>> This fixes ICE when one does not provide valid target names:
>> __attribute__((target_clones("default,foo,bar")))
>>
>> In that situation I suggest to print:
>>
>> $ ./xgcc -B. /tmp/mvc-ice.c
>> /tmp/mvc-ice.c:6:1: error: attribute(target("foo")) is unknown
>>  foo ()
>>  ^~~
>> /tmp/mvc-ice.c:6:1: error: attribute(target("bar")) is unknown
>> /tmp/mvc-ice.c: In function ‘foo.resolver’:
>> cc1: fatal error: At least one more version other than the default is expected
>>
>> Should I add a test-case for a fatal error?
> 
> Better avoid fatal_error, can't you just return early after regular error()?

Yep. Early bail out would be better.

> 
> Do we error similarly for __attribute__((target_clones("default")))
> from generic code?
> 
> I wonder if we can just go ahead even with num_versions == 1?

It's covered in MV pass:

$ ./xgcc -B. /tmp/mvc-single.c 
/tmp/mvc-single.c:6:1: warning: single target_clones attribute is ignored
 foo ()

Martin

> 
> 
>>
>> Thanks,
>> Martin


[-- Attachment #2: 0001-Do-not-ICE-on-an-invalid-input-for-MV.patch --]
[-- Type: text/x-patch, Size: 955 bytes --]

From 5e7aa55549fabeb4c7fc4b8339dba210c49614d0 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Tue, 14 Mar 2017 11:16:51 +0100
Subject: [PATCH] Do not ICE on an invalid input for MV.

gcc/ChangeLog:

2017-03-14  Martin Liska  <mliska@suse.cz>

	* multiple_target.c (expand_target_clones): Bail out for
	an invalid attribute.
---
 gcc/multiple_target.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/gcc/multiple_target.c b/gcc/multiple_target.c
index 7b735ae81ae..1e02116a2db 100644
--- a/gcc/multiple_target.c
+++ b/gcc/multiple_target.c
@@ -296,10 +296,7 @@ expand_target_clones (struct cgraph_node *node, bool definition)
       if (!targetm.target_option.valid_attribute_p (new_node->decl, NULL,
 						    TREE_VALUE (attributes),
 						    0))
-	{
-	  input_location = saved_loc;
-	  continue;
-	}
+	return false;
 
       input_location = saved_loc;
       decl2_v = new_node->function_version ();
-- 
2.11.1

Re: [PATCH 4/N] Do not ICE on an invalid input for MV.

[Richard Biener]

On Tue, Mar 14, 2017 at 1:24 PM, Martin Liška <mliska@suse.cz> wrote:
> On 03/14/2017 12:09 PM, Richard Biener wrote:
>> On Tue, Mar 14, 2017 at 11:24 AM, Martin Liška <mliska@suse.cz> wrote:
>>> Hello.
>>>
>>> This fixes ICE when one does not provide valid target names:
>>> __attribute__((target_clones("default,foo,bar")))
>>>
>>> In that situation I suggest to print:
>>>
>>> $ ./xgcc -B. /tmp/mvc-ice.c
>>> /tmp/mvc-ice.c:6:1: error: attribute(target("foo")) is unknown
>>>  foo ()
>>>  ^~~
>>> /tmp/mvc-ice.c:6:1: error: attribute(target("bar")) is unknown
>>> /tmp/mvc-ice.c: In function ‘foo.resolver’:
>>> cc1: fatal error: At least one more version other than the default is expected
>>>
>>> Should I add a test-case for a fatal error?
>>
>> Better avoid fatal_error, can't you just return early after regular error()?
>
> Yep. Early bail out would be better.
>
>>
>> Do we error similarly for __attribute__((target_clones("default")))
>> from generic code?
>>
>> I wonder if we can just go ahead even with num_versions == 1?
>
> It's covered in MV pass:
>
> $ ./xgcc -B. /tmp/mvc-single.c
> /tmp/mvc-single.c:6:1: warning: single target_clones attribute is ignored
>  foo ()

I see.

Attached patch is ok.

Thanks,
Richard.

> Martin
>
>>
>>
>>>
>>> Thanks,
>>> Martin
>