[v2] Fix target attribute handling (PR c++/81355).

Message ID 8464ad76-cf88-efdf-2dc5-402ea6946f7a@suse.cz
State New
Headers show

Commit Message

Martin Liška Aug. 9, 2017, 10:45 a.m.
On 08/08/2017 08:03 PM, Martin Sebor wrote:
> On 08/07/2017 10:59 PM, Martin Liška wrote:
>> On 08/02/2017 09:56 PM, Martin Sebor wrote:
>>> On 08/02/2017 01:04 PM, Jeff Law wrote:
>>>> On 07/28/2017 05:13 AM, Martin Liška wrote:
>>>>> Hello.
>>>>>
>>>>> Following patch skips empty strings in 'target' attribute.
>>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>>>>
>>>>> Ready to be installed?
>>>>> Martin
>>>>>
>>>>> gcc/ChangeLog:
>>>>>
>>>>> 2017-07-13  Martin Liska  <mliska@suse.cz>
>>>>>
>>>>>     PR c++/81355
>>>>>     * attribs.c (sorted_attr_string): Skip empty strings.
>>>>>
>>>>> gcc/testsuite/ChangeLog:
>>>>>
>>>>> 2017-07-13  Martin Liska  <mliska@suse.cz>
>>>>>
>>>>>     PR c++/81355
>>>>>     * g++.dg/other/pr81355.C: New test.
>>>> OK.  THough one could legitimately ask if this ought to be a parsing error.
>>>
>>> I would suggest to at least issue a warning.  It seems that
>>> the empty string must almost certainly be a bug here, say due
>>> to unintended macro expansion.
>>>
>>> Otherwise, if it should remain silently (and intentionally)
>>> accepted, I recommend to document it.  As it is, the manual
>>> says that the "string" argument is equivalent to compiling
>>> with -mstring which for "" would be rejected by the driver.
>>>
>>> Martin
>>
>> Thanks you both for feedback. I decided to come up with an error message for that.
>>
>> Feedback appreciated.
> 
> My only comment is on the text of the error message.  I think
> the %' directive is supposed to be used instead of a bare
> apostrophe.  But rather than using the somewhat informal "can't"
> I would suggest to follow other similar diagnostics that might
> print something like:
> 
>   empty string in attribute %<target%>
> 
> (analogous to "empty precision in %s format" or "empty scalar
> initializer" etc. in gcc.pot)
> 
> or
> 
>   attribute %<target%> argument may not be empty
> 
> (similar to "output filename may not be empty").
> 
> Martin

Hi.

Thank you both for review, both notes resolved in v3.
Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.

Ready to be installed?
Martin

Comments

Jason Merrill Aug. 9, 2017, 7:26 p.m. | #1
On Wed, Aug 9, 2017 at 6:45 AM, Martin Liška <mliska@suse.cz> wrote:
> On 08/08/2017 08:03 PM, Martin Sebor wrote:
>> On 08/07/2017 10:59 PM, Martin Liška wrote:
>>> On 08/02/2017 09:56 PM, Martin Sebor wrote:
>>>> On 08/02/2017 01:04 PM, Jeff Law wrote:
>>>>> On 07/28/2017 05:13 AM, Martin Liška wrote:
>>>>>> Hello.
>>>>>>
>>>>>> Following patch skips empty strings in 'target' attribute.
>>>>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>>>>>>
>>>>>> Ready to be installed?
>>>>>> Martin
>>>>>>
>>>>>> gcc/ChangeLog:
>>>>>>
>>>>>> 2017-07-13  Martin Liska  <mliska@suse.cz>
>>>>>>
>>>>>>     PR c++/81355
>>>>>>     * attribs.c (sorted_attr_string): Skip empty strings.
>>>>>>
>>>>>> gcc/testsuite/ChangeLog:
>>>>>>
>>>>>> 2017-07-13  Martin Liska  <mliska@suse.cz>
>>>>>>
>>>>>>     PR c++/81355
>>>>>>     * g++.dg/other/pr81355.C: New test.
>>>>> OK.  THough one could legitimately ask if this ought to be a parsing error.
>>>>
>>>> I would suggest to at least issue a warning.  It seems that
>>>> the empty string must almost certainly be a bug here, say due
>>>> to unintended macro expansion.
>>>>
>>>> Otherwise, if it should remain silently (and intentionally)
>>>> accepted, I recommend to document it.  As it is, the manual
>>>> says that the "string" argument is equivalent to compiling
>>>> with -mstring which for "" would be rejected by the driver.
>>>>
>>>> Martin
>>>
>>> Thanks you both for feedback. I decided to come up with an error message for that.
>>>
>>> Feedback appreciated.
>>
>> My only comment is on the text of the error message.  I think
>> the %' directive is supposed to be used instead of a bare
>> apostrophe.  But rather than using the somewhat informal "can't"
>> I would suggest to follow other similar diagnostics that might
>> print something like:
>>
>>   empty string in attribute %<target%>
>>
>> (analogous to "empty precision in %s format" or "empty scalar
>> initializer" etc. in gcc.pot)
>>
>> or
>>
>>   attribute %<target%> argument may not be empty
>>
>> (similar to "output filename may not be empty").
>>
>> Martin
>
> Hi.
>
> Thank you both for review, both notes resolved in v3.
> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests.
>
> Ready to be installed?

OK.

Jason

Patch

From 227a41219ebb9a10bda80767f5b5f3e460a3e9b9 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Wed, 12 Jul 2017 15:51:45 +0200
Subject: [PATCH] Fix target attribute handling (PR c++/81355).

gcc/ChangeLog:

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

	PR c++/81355
	* c-attribs.c (handle_target_attribute):
	Report warning for an empty string argument of target attribute.

gcc/testsuite/ChangeLog:

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

	PR c++/81355
	* g++.dg/other/pr81355.C: New test.
---
 gcc/c-family/c-attribs.c             | 13 +++++++++++++
 gcc/testsuite/g++.dg/other/pr81355.C | 14 ++++++++++++++
 2 files changed, 27 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/other/pr81355.C

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index 626ffa1cde7..b4252229beb 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -3116,6 +3116,19 @@  handle_target_attribute (tree *node, tree name, tree args, int flags,
 						      flags))
     *no_add_attrs = true;
 
+  /* Check that there's no empty string in values of the attribute.  */
+  for (tree t = args; t != NULL_TREE; t = TREE_CHAIN (t))
+    {
+      tree value = TREE_VALUE (t);
+      if (TREE_CODE (value) == STRING_CST
+	  && TREE_STRING_LENGTH (value) == 1
+	  && TREE_STRING_POINTER (value)[0] == '\0')
+	{
+	  warning (OPT_Wattributes, "empty string in attribute %<target%>");
+	  *no_add_attrs = true;
+	}
+    }
+
   return NULL_TREE;
 }
 
diff --git a/gcc/testsuite/g++.dg/other/pr81355.C b/gcc/testsuite/g++.dg/other/pr81355.C
new file mode 100644
index 00000000000..89d1b419581
--- /dev/null
+++ b/gcc/testsuite/g++.dg/other/pr81355.C
@@ -0,0 +1,14 @@ 
+/* { dg-do compile { target x86_64-*-* } } */
+
+__attribute__((target("default")))
+int foo() {return 1;}
+
+__attribute__((target("arch=core2", "")))
+int foo2() {return 2;} /* { dg-warning "empty string in attribute .target." } */
+
+__attribute__((target("sse4.2", "", "")))
+int foo3() {return 2;} /* { dg-warning "empty string in attribute .target." } */
+
+int main() {
+    return foo() + foo2() + foo3();
+}
-- 
2.13.3