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

Message ID e133bfeb-e307-426e-6918-115a40da1ad3@suse.cz
State New
Headers show

Commit Message

Martin Liška Aug. 8, 2017, 4:59 a.m.
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.

Thanks,
Martin

Comments

Jason Merrill Aug. 8, 2017, 4:35 p.m. | #1
On Tue, Aug 8, 2017 at 12:59 AM, Martin Liška <mliska@suse.cz> 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.

I'd think to check it in handle_target_argument rather than require
all targets to check it.

Jason
Martin Sebor Aug. 8, 2017, 6:03 p.m. | #2
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

Patch

From 7fd264acf60829b8ed4921c6659f01a87c944d44 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
	* config/i386/i386.c (ix86_valid_target_attribute_inner_p):
	Report error 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/config/i386/i386.c               |  6 ++++++
 gcc/testsuite/g++.dg/other/pr81355.C | 14 ++++++++++++++
 2 files changed, 20 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/other/pr81355.C

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 42e0ddaca56..62f08f9a80d 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -7140,6 +7140,12 @@  ix86_valid_target_attribute_inner_p (tree args, char *p_strings[],
   /* Handle multiple arguments separated by commas.  */
   next_optstr = ASTRDUP (TREE_STRING_POINTER (args));
 
+  if (next_optstr && *next_optstr == '\0')
+  {
+      error ("attribute %<target%> argument can't be an empty string");
+      return false;
+  }
+
   while (next_optstr && *next_optstr != '\0')
     {
       char *p = next_optstr;
diff --git a/gcc/testsuite/g++.dg/other/pr81355.C b/gcc/testsuite/g++.dg/other/pr81355.C
new file mode 100644
index 00000000000..42b25b72611
--- /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-error "attribute .target. argument can't be an empty string" } */
+
+__attribute__((target("sse4.2", "", "")))
+int foo3() {return 2;} /* { dg-error "attribute .target. argument can't be an empty string" } */
+
+int main() {
+    return foo() + foo2() + foo3();
+}
-- 
2.13.3