| 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 | expand |
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
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.
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
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 >
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
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
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 >
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" } +};
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.