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

[Jason Merrill]

On Wed, Aug 9, 2017 at 6:45 AM, Martin Liška  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  
>>
>> PR c++/81355
>> * attribs.c (sorted_attr_string): Skip empty strings.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2017-07-13  Martin Liska  
>>
>> 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 %
>>
>> (analogous to "empty precision in %s format" or "empty scalar
>> initializer" etc. in gcc.pot)
>>
>> or
>>
>>   attribute % 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

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

[Martin Liška]

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  
>
> PR c++/81355
> * attribs.c (sorted_attr_string): Skip empty strings.
>
> gcc/testsuite/ChangeLog:
>
> 2017-07-13  Martin Liska  
>
> 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 %
> 
> (analogous to "empty precision in %s format" or "empty scalar
> initializer" etc. in gcc.pot)
> 
> or
> 
>   attribute % 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
>From 227a41219ebb9a10bda80767f5b5f3e460a3e9b9 Mon Sep 17 00:00:00 2001
From: marxin 
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  

	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  

	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 %");
+	  *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 000..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

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

[Martin Sebor]

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  

PR c++/81355
* attribs.c (sorted_attr_string): Skip empty strings.

gcc/testsuite/ChangeLog:

2017-07-13  Martin Liska  

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 %

(analogous to "empty precision in %s format" or "empty scalar
initializer" etc. in gcc.pot)

or

  attribute % argument may not be empty

(similar to "output filename may not be empty").

Martin

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

[Jason Merrill]

On Tue, Aug 8, 2017 at 12:59 AM, 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  

 PR c++/81355
 * attribs.c (sorted_attr_string): Skip empty strings.

 gcc/testsuite/ChangeLog:

 2017-07-13  Martin Liska  

 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