[C++] PR 90915 [9/10 Regression] ICE in has_attribute, at c-family/c-attribs.c:4221

Message ID db48dd81-1be8-ea40-73c4-20e91834acad@oracle.com
State New
Headers show
Series
  • [C++] PR 90915 [9/10 Regression] ICE in has_attribute, at c-family/c-attribs.c:4221
Related show

Commit Message

Paolo Carlini Jan. 29, 2020, 9:31 a.m.
Hi,

in this regression we issue a diagnostic about an incomplete type (only 
a warning by default) and then we crash when we try to query 
has_attribute on a member of the type because in such cases the member 
remains an IDENTIFIER_NODE which of course doesn't have a TREE_TYPE 
neither a DECL_ATTRIBUTES... Simply recognizing IDENTIFIER_NODEs and 
returning false works fine, not sure if we want to do something more 
sophisticated. Tested x86_64-linux.

Thanks, Paolo.

////////////////////////
Fix "PR c++/90915 ICE in has_attribute, at c-family/c-attribs.c:4221"

A rather simple ICE where we didn't properly recover upon diagnosing
an incomplete type.

Tested x86_64-linux.

       /c-family
       PR c++/90915
       * c-attribs.c (has_attribute): Handle correctly IDENTIFIER_NODEs.

       /testsuite
       PR c++/90915
       * g++.dg/ext/builtin-has-attribute-1.C: New.

Comments

Jason Merrill Jan. 29, 2020, 6 p.m. | #1
On 1/29/20 4:31 AM, Paolo Carlini wrote:
> Hi,

> 

> in this regression we issue a diagnostic about an incomplete type (only 

> a warning by default) and then we crash when we try to query 

> has_attribute on a member of the type because in such cases the member 

> remains an IDENTIFIER_NODE which of course doesn't have a TREE_TYPE 

> neither a DECL_ATTRIBUTES... Simply recognizing IDENTIFIER_NODEs and 

> returning false works fine, not sure if we want to do something more 

> sophisticated. Tested x86_64-linux.


Why are we getting to has_attribute at all for a type-dependent argument?

Jason
Paolo Carlini Jan. 29, 2020, 9:06 p.m. | #2
Hi,

On 29/01/20 19:00, Jason Merrill wrote:
> On 1/29/20 4:31 AM, Paolo Carlini wrote:

>> Hi,

>>

>> in this regression we issue a diagnostic about an incomplete type 

>> (only a warning by default) and then we crash when we try to query 

>> has_attribute on a member of the type because in such cases the 

>> member remains an IDENTIFIER_NODE which of course doesn't have a 

>> TREE_TYPE neither a DECL_ATTRIBUTES... Simply recognizing 

>> IDENTIFIER_NODEs and returning false works fine, not sure if we want 

>> to do something more sophisticated. Tested x86_64-linux.

>

> Why are we getting to has_attribute at all for a type-dependent argument?


Because the implementation of __builtin_has_attribute, largely shared 
with the C front-end, doesn't know about templates at all? :-/

Not sure it's the best time to complete it, but shouldn't be too difficult.

Paolo.
Richard Biener via Gcc-patches May 7, 2020, 11:03 p.m. | #3
On Wed, Jan 29, 2020 at 10:06:51PM +0100, Paolo Carlini wrote:
> Hi,

> 

> On 29/01/20 19:00, Jason Merrill wrote:

> > On 1/29/20 4:31 AM, Paolo Carlini wrote:

> > > Hi,

> > > 

> > > in this regression we issue a diagnostic about an incomplete type

> > > (only a warning by default) and then we crash when we try to query

> > > has_attribute on a member of the type because in such cases the

> > > member remains an IDENTIFIER_NODE which of course doesn't have a

> > > TREE_TYPE neither a DECL_ATTRIBUTES... Simply recognizing

> > > IDENTIFIER_NODEs and returning false works fine, not sure if we want

> > > to do something more sophisticated. Tested x86_64-linux.

> > 

> > Why are we getting to has_attribute at all for a type-dependent argument?

> 

> Because the implementation of __builtin_has_attribute, largely shared with

> the C front-end, doesn't know about templates at all? :-/

> 

> Not sure it's the best time to complete it, but shouldn't be too difficult.


This ICEs even with a more reasonable test like

template<typename T>
void foo ()
{
  static_assert(!__builtin_has_attribute(T::a, aligned));
}

The problem here is that __builtin_has_attribute doesn't handle type-dependent
arguments at all.  To handle type-dependent arguments we'd have to introduce
a new template code, like STATIC_ASSERT or ADDRESSOF_EXPR (or a new generic
template code for built-ins?), but that's always a pain.

Or, meanwhile, we could just sorry.  Martin, what do you think?

--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -8682,7 +8682,12 @@ cp_parser_has_attribute_expression (cp_parser *parser)
   location_t atloc = cp_lexer_peek_token (parser->lexer)->location;
   if (tree attr = cp_parser_gnu_attribute_list (parser, /*exactly_one=*/true))
     {
-      if (oper != error_mark_node)
+      if (oper == error_mark_node)
+   /* Nothing.  */;
+      else if (type_dependent_expression_p (oper))
+   sorry_at (atloc, "%<__builtin_has_attribute%> with dependent argument "
+         "not supported yet");
+      else
    {
      /* Fold constant expressions used in attributes first.  */
      cp_check_const_attributes (attr);


Marek
Richard Biener via Gcc-patches May 8, 2020, 12:09 a.m. | #4
On 5/7/20 5:03 PM, Marek Polacek via Gcc-patches wrote:
> On Wed, Jan 29, 2020 at 10:06:51PM +0100, Paolo Carlini wrote:

>> Hi,

>>

>> On 29/01/20 19:00, Jason Merrill wrote:

>>> On 1/29/20 4:31 AM, Paolo Carlini wrote:

>>>> Hi,

>>>>

>>>> in this regression we issue a diagnostic about an incomplete type

>>>> (only a warning by default) and then we crash when we try to query

>>>> has_attribute on a member of the type because in such cases the

>>>> member remains an IDENTIFIER_NODE which of course doesn't have a

>>>> TREE_TYPE neither a DECL_ATTRIBUTES... Simply recognizing

>>>> IDENTIFIER_NODEs and returning false works fine, not sure if we want

>>>> to do something more sophisticated. Tested x86_64-linux.

>>>

>>> Why are we getting to has_attribute at all for a type-dependent argument?

>>

>> Because the implementation of __builtin_has_attribute, largely shared with

>> the C front-end, doesn't know about templates at all? :-/

>>

>> Not sure it's the best time to complete it, but shouldn't be too difficult.

> 

> This ICEs even with a more reasonable test like

> 

> template<typename T>

> void foo ()

> {

>    static_assert(!__builtin_has_attribute(T::a, aligned));

> }

> 

> The problem here is that __builtin_has_attribute doesn't handle type-dependent

> arguments at all.  To handle type-dependent arguments we'd have to introduce

> a new template code, like STATIC_ASSERT or ADDRESSOF_EXPR (or a new generic

> template code for built-ins?), but that's always a pain.

> 

> Or, meanwhile, we could just sorry.  Martin, what do you think?


I never did implement the template handling and I didn't think to put
in a stopgap like the one below.  It makes sense until I get around to
implementing it, hopefully for GCC 11.

Thanks!

Martin

> 

> --- a/gcc/cp/parser.c

> +++ b/gcc/cp/parser.c

> @@ -8682,7 +8682,12 @@ cp_parser_has_attribute_expression (cp_parser *parser)

>     location_t atloc = cp_lexer_peek_token (parser->lexer)->location;

>     if (tree attr = cp_parser_gnu_attribute_list (parser, /*exactly_one=*/true))

>       {

> -      if (oper != error_mark_node)

> +      if (oper == error_mark_node)

> +   /* Nothing.  */;

> +      else if (type_dependent_expression_p (oper))

> +   sorry_at (atloc, "%<__builtin_has_attribute%> with dependent argument "

> +         "not supported yet");

> +      else

>      {

>        /* Fold constant expressions used in attributes first.  */

>        cp_check_const_attributes (attr);

> 

> 

> Marek

>
Richard Biener via Gcc-patches May 8, 2020, 1:57 a.m. | #5
On Thu, May 07, 2020 at 06:09:30PM -0600, Martin Sebor wrote:
> On 5/7/20 5:03 PM, Marek Polacek via Gcc-patches wrote:

> > On Wed, Jan 29, 2020 at 10:06:51PM +0100, Paolo Carlini wrote:

> > > Hi,

> > > 

> > > On 29/01/20 19:00, Jason Merrill wrote:

> > > > On 1/29/20 4:31 AM, Paolo Carlini wrote:

> > > > > Hi,

> > > > > 

> > > > > in this regression we issue a diagnostic about an incomplete type

> > > > > (only a warning by default) and then we crash when we try to query

> > > > > has_attribute on a member of the type because in such cases the

> > > > > member remains an IDENTIFIER_NODE which of course doesn't have a

> > > > > TREE_TYPE neither a DECL_ATTRIBUTES... Simply recognizing

> > > > > IDENTIFIER_NODEs and returning false works fine, not sure if we want

> > > > > to do something more sophisticated. Tested x86_64-linux.

> > > > 

> > > > Why are we getting to has_attribute at all for a type-dependent argument?

> > > 

> > > Because the implementation of __builtin_has_attribute, largely shared with

> > > the C front-end, doesn't know about templates at all? :-/

> > > 

> > > Not sure it's the best time to complete it, but shouldn't be too difficult.

> > 

> > This ICEs even with a more reasonable test like

> > 

> > template<typename T>

> > void foo ()

> > {

> >    static_assert(!__builtin_has_attribute(T::a, aligned));

> > }

> > 

> > The problem here is that __builtin_has_attribute doesn't handle type-dependent

> > arguments at all.  To handle type-dependent arguments we'd have to introduce

> > a new template code, like STATIC_ASSERT or ADDRESSOF_EXPR (or a new generic

> > template code for built-ins?), but that's always a pain.

> > 

> > Or, meanwhile, we could just sorry.  Martin, what do you think?

> 

> I never did implement the template handling and I didn't think to put

> in a stopgap like the one below.  It makes sense until I get around to

> implementing it, hopefully for GCC 11.


Ah, and we have PR92104 to track that.   Here's a complete patch then:

Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

From 7ed334b7998314bab12fe4741bc311a47457ea3a Mon Sep 17 00:00:00 2001
From: Marek Polacek <polacek@redhat.com>

Date: Thu, 7 May 2020 21:10:42 -0400
Subject: [PATCH] c++: Sorry about type-dependent arg for
 __builtin_has_attribute [PR90915]

Until 92104 is fixed, let's sorry rather than crash.

	PR c++/90915
	* parser.c (cp_parser_has_attribute_expression): Sorry on a
	type-dependent argument.

	* g++.dg/ext/builtin-has-attribute.C: New test.
---
 gcc/cp/parser.c                                  | 7 ++++++-
 gcc/testsuite/g++.dg/ext/builtin-has-attribute.C | 8 ++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/ext/builtin-has-attribute.C

diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
index d67fa3b13d1..f586c89b109 100644
--- a/gcc/cp/parser.c
+++ b/gcc/cp/parser.c
@@ -8682,7 +8682,12 @@ cp_parser_has_attribute_expression (cp_parser *parser)
   location_t atloc = cp_lexer_peek_token (parser->lexer)->location;
   if (tree attr = cp_parser_gnu_attribute_list (parser, /*exactly_one=*/true))
     {
-      if (oper != error_mark_node)
+      if (oper == error_mark_node)
+	/* Nothing.  */;
+      else if (type_dependent_expression_p (oper))
+	sorry_at (atloc, "%<__builtin_has_attribute%> with dependent argument "
+		  "not supported yet");
+      else
 	{
 	  /* Fold constant expressions used in attributes first.  */
 	  cp_check_const_attributes (attr);
diff --git a/gcc/testsuite/g++.dg/ext/builtin-has-attribute.C b/gcc/testsuite/g++.dg/ext/builtin-has-attribute.C
new file mode 100644
index 00000000000..3438dd59ba3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/builtin-has-attribute.C
@@ -0,0 +1,8 @@
+// PR c++/90915
+// { dg-do compile { target c++11 } }
+
+template<typename T>
+void foo ()
+{
+  static_assert(!__builtin_has_attribute(T::a, aligned), ""); // { dg-message "sorry, unimplemented: .__builtin_has_attribute. with dependent argument not supported yet" }
+}

base-commit: 74d58ad2c208c9c445bb3e8288db08e092a66316
-- 
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
Richard Biener via Gcc-patches May 18, 2020, 9:53 p.m. | #6
On 5/7/20 7:03 PM, Marek Polacek wrote:
> On Wed, Jan 29, 2020 at 10:06:51PM +0100, Paolo Carlini wrote:

>> Hi,

>>

>> On 29/01/20 19:00, Jason Merrill wrote:

>>> On 1/29/20 4:31 AM, Paolo Carlini wrote:

>>>> Hi,

>>>>

>>>> in this regression we issue a diagnostic about an incomplete type

>>>> (only a warning by default) and then we crash when we try to query

>>>> has_attribute on a member of the type because in such cases the

>>>> member remains an IDENTIFIER_NODE which of course doesn't have a

>>>> TREE_TYPE neither a DECL_ATTRIBUTES... Simply recognizing

>>>> IDENTIFIER_NODEs and returning false works fine, not sure if we want

>>>> to do something more sophisticated. Tested x86_64-linux.

>>>

>>> Why are we getting to has_attribute at all for a type-dependent argument?

>>

>> Because the implementation of __builtin_has_attribute, largely shared with

>> the C front-end, doesn't know about templates at all? :-/

>>

>> Not sure it's the best time to complete it, but shouldn't be too difficult.

> 

> This ICEs even with a more reasonable test like

> 

> template<typename T>

> void foo ()

> {

>    static_assert(!__builtin_has_attribute(T::a, aligned));

> }

> 

> The problem here is that __builtin_has_attribute doesn't handle type-dependent

> arguments at all.  To handle type-dependent arguments we'd have to introduce

> a new template code, like STATIC_ASSERT or ADDRESSOF_EXPR (or a new generic

> template code for built-ins?), but that's always a pain.


Adding an internal_fn might be easier; see where tsubst_copy_and_build 
handles IFN_LAUNDER.

> Or, meanwhile, we could just sorry.  Martin, what do you think?

> 

> --- a/gcc/cp/parser.c

> +++ b/gcc/cp/parser.c

> @@ -8682,7 +8682,12 @@ cp_parser_has_attribute_expression (cp_parser *parser)

>     location_t atloc = cp_lexer_peek_token (parser->lexer)->location;

>     if (tree attr = cp_parser_gnu_attribute_list (parser, /*exactly_one=*/true))

>       {

> -      if (oper != error_mark_node)

> +      if (oper == error_mark_node)

> +   /* Nothing.  */;

> +      else if (type_dependent_expression_p (oper))

> +   sorry_at (atloc, "%<__builtin_has_attribute%> with dependent argument "

> +         "not supported yet");

> +      else

>      {

>        /* Fold constant expressions used in attributes first.  */

>        cp_check_const_attributes (attr);


This is certainly an improvement over an ICE.

Jason
Richard Biener via Gcc-patches May 18, 2020, 9:54 p.m. | #7
On 5/7/20 9:57 PM, Marek Polacek wrote:
> On Thu, May 07, 2020 at 06:09:30PM -0600, Martin Sebor wrote:

>> On 5/7/20 5:03 PM, Marek Polacek via Gcc-patches wrote:

>>> On Wed, Jan 29, 2020 at 10:06:51PM +0100, Paolo Carlini wrote:

>>>> Hi,

>>>>

>>>> On 29/01/20 19:00, Jason Merrill wrote:

>>>>> On 1/29/20 4:31 AM, Paolo Carlini wrote:

>>>>>> Hi,

>>>>>>

>>>>>> in this regression we issue a diagnostic about an incomplete type

>>>>>> (only a warning by default) and then we crash when we try to query

>>>>>> has_attribute on a member of the type because in such cases the

>>>>>> member remains an IDENTIFIER_NODE which of course doesn't have a

>>>>>> TREE_TYPE neither a DECL_ATTRIBUTES... Simply recognizing

>>>>>> IDENTIFIER_NODEs and returning false works fine, not sure if we want

>>>>>> to do something more sophisticated. Tested x86_64-linux.

>>>>>

>>>>> Why are we getting to has_attribute at all for a type-dependent argument?

>>>>

>>>> Because the implementation of __builtin_has_attribute, largely shared with

>>>> the C front-end, doesn't know about templates at all? :-/

>>>>

>>>> Not sure it's the best time to complete it, but shouldn't be too difficult.

>>>

>>> This ICEs even with a more reasonable test like

>>>

>>> template<typename T>

>>> void foo ()

>>> {

>>>     static_assert(!__builtin_has_attribute(T::a, aligned));

>>> }

>>>

>>> The problem here is that __builtin_has_attribute doesn't handle type-dependent

>>> arguments at all.  To handle type-dependent arguments we'd have to introduce

>>> a new template code, like STATIC_ASSERT or ADDRESSOF_EXPR (or a new generic

>>> template code for built-ins?), but that's always a pain.

>>>

>>> Or, meanwhile, we could just sorry.  Martin, what do you think?

>>

>> I never did implement the template handling and I didn't think to put

>> in a stopgap like the one below.  It makes sense until I get around to

>> implementing it, hopefully for GCC 11.

> 

> Ah, and we have PR92104 to track that.   Here's a complete patch then:

> 

> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?


OK.

>  From 7ed334b7998314bab12fe4741bc311a47457ea3a Mon Sep 17 00:00:00 2001

> From: Marek Polacek <polacek@redhat.com>

> Date: Thu, 7 May 2020 21:10:42 -0400

> Subject: [PATCH] c++: Sorry about type-dependent arg for

>   __builtin_has_attribute [PR90915]

> 

> Until 92104 is fixed, let's sorry rather than crash.

> 

> 	PR c++/90915

> 	* parser.c (cp_parser_has_attribute_expression): Sorry on a

> 	type-dependent argument.

> 

> 	* g++.dg/ext/builtin-has-attribute.C: New test.

> ---

>   gcc/cp/parser.c                                  | 7 ++++++-

>   gcc/testsuite/g++.dg/ext/builtin-has-attribute.C | 8 ++++++++

>   2 files changed, 14 insertions(+), 1 deletion(-)

>   create mode 100644 gcc/testsuite/g++.dg/ext/builtin-has-attribute.C

> 

> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c

> index d67fa3b13d1..f586c89b109 100644

> --- a/gcc/cp/parser.c

> +++ b/gcc/cp/parser.c

> @@ -8682,7 +8682,12 @@ cp_parser_has_attribute_expression (cp_parser *parser)

>     location_t atloc = cp_lexer_peek_token (parser->lexer)->location;

>     if (tree attr = cp_parser_gnu_attribute_list (parser, /*exactly_one=*/true))

>       {

> -      if (oper != error_mark_node)

> +      if (oper == error_mark_node)

> +	/* Nothing.  */;

> +      else if (type_dependent_expression_p (oper))

> +	sorry_at (atloc, "%<__builtin_has_attribute%> with dependent argument "

> +		  "not supported yet");

> +      else

>   	{

>   	  /* Fold constant expressions used in attributes first.  */

>   	  cp_check_const_attributes (attr);

> diff --git a/gcc/testsuite/g++.dg/ext/builtin-has-attribute.C b/gcc/testsuite/g++.dg/ext/builtin-has-attribute.C

> new file mode 100644

> index 00000000000..3438dd59ba3

> --- /dev/null

> +++ b/gcc/testsuite/g++.dg/ext/builtin-has-attribute.C

> @@ -0,0 +1,8 @@

> +// PR c++/90915

> +// { dg-do compile { target c++11 } }

> +

> +template<typename T>

> +void foo ()

> +{

> +  static_assert(!__builtin_has_attribute(T::a, aligned), ""); // { dg-message "sorry, unimplemented: .__builtin_has_attribute. with dependent argument not supported yet" }

> +}

> 

> base-commit: 74d58ad2c208c9c445bb3e8288db08e092a66316

>

Patch

diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c
index dc9579c5c60..97590a19c0f 100644
--- a/gcc/c-family/c-attribs.c
+++ b/gcc/c-family/c-attribs.c
@@ -4759,6 +4759,10 @@  has_attribute (location_t atloc, tree t, tree attr, tree (*convert)(tree))
 	  expr = NULL_TREE;
 	  done = true;
 	}
+      else if (TREE_CODE (expr) == IDENTIFIER_NODE)
+	/* Can happen during error-recovery when querying a member of
+	   an incomplete type (c++/90915).  */
+	return false;
       else if (DECL_P (expr))
 	{
 	  /* Set TYPE to the DECL's type to process it on the next
diff --git a/gcc/testsuite/g++.dg/ext/builtin-has-attribute-1.C b/gcc/testsuite/g++.dg/ext/builtin-has-attribute-1.C
new file mode 100644
index 00000000000..3d81a078159
--- /dev/null
+++ b/gcc/testsuite/g++.dg/ext/builtin-has-attribute-1.C
@@ -0,0 +1,7 @@ 
+// { dg-do compile { target c++11 } }
+
+struct S;
+template<int> struct T
+{
+  static_assert (!__builtin_has_attribute (((S *) 0) -> a, packed), "");  // { dg-error "invalid use of incomplete type" }
+};