| Message ID | ZW7T8HZqnnI2FXJQ@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | c++: Implement C++ DR 2262 - Attributes for asm-definition [PR110734] | expand |
On 12/5/23 02:40, Jakub Jelinek wrote: > Hi! > > Seems in 2017 attribute-specifier-seq[opt] was added to asm-declaration > and the change was voted in as a DR. > > The following patch implements it by parsing the attributes and warning > about them. > > I found one attribute parsing bug I'll send a fix for momentarily. > > And there is another thing I wonder about: with -Wno-attributes= we are > supposed to ignore the attributes altogether, but we are actually still > warning about them when we emit these generic warnings about ignoring > all attributes which appertain to this and that (perhaps with some > exceptions we first remove from the attribute chain), like: > void foo () { [[foo::bar]]; } > with -Wattributes -Wno-attributes=foo::bar > Shouldn't we call some helper function in cases like this and warn > not when std_attrs (or how the attribute chain var is called) is non-NULL, > but if it is non-NULL and contains at least one non-attribute_ignored_p > attribute? Sounds good. > cp_parser_declaration at least tries: > if (std_attrs != NULL_TREE && !attribute_ignored_p (std_attrs)) > warning_at (make_location (attrs_loc, attrs_loc, parser->lexer), > OPT_Wattributes, "attribute ignored"); > but attribute_ignored_p here checks the first attribute rather than the > whole chain. So it will incorrectly not warn if there is an ignored > attribute followed by non-ignored. I agree. > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? OK. > 2023-12-05 Jakub Jelinek <jakub@redhat.com> > > PR c++/110734 > * parser.cc (cp_parser_block_declaration): Implement C++ DR 2262 > - Attributes for asm-definition. Call cp_parser_asm_definition > even if RID_ASM token is only seen after sequence of standard > attributes. > (cp_parser_asm_definition): Parse standard attributes before > RID_ASM token and warn for them with -Wattributes. > > * g++.dg/DRs/dr2262.C: New test. > * g++.dg/cpp0x/gen-attrs-76.C (foo, bar): Don't expect errors > on attributes on asm definitions. > * g++.dg/gomp/attrs-11.C: Remove 2 expected errors. > > --- gcc/cp/parser.cc.jj 2023-12-04 08:59:06.871357329 +0100 > +++ gcc/cp/parser.cc 2023-12-04 20:23:53.225009856 +0100 > @@ -15398,7 +15398,6 @@ cp_parser_block_declaration (cp_parser * > /* Peek at the next token to figure out which kind of declaration is > present. */ > cp_token *token1 = cp_lexer_peek_token (parser->lexer); > - size_t attr_idx; > > /* If the next keyword is `asm', we have an asm-definition. */ > if (token1->keyword == RID_ASM) > @@ -15452,22 +15451,36 @@ cp_parser_block_declaration (cp_parser * > /* If the next token is `static_assert' we have a static assertion. */ > else if (token1->keyword == RID_STATIC_ASSERT) > cp_parser_static_assert (parser, /*member_p=*/false); > - /* If the next tokens after attributes is `using namespace', then we have > - a using-directive. */ > - else if ((attr_idx = cp_parser_skip_std_attribute_spec_seq (parser, 1)) != 1 > - && cp_lexer_nth_token_is_keyword (parser->lexer, attr_idx, > - RID_USING) > - && cp_lexer_nth_token_is_keyword (parser->lexer, attr_idx + 1, > - RID_NAMESPACE)) > + else > { > - if (statement_p) > - cp_parser_commit_to_tentative_parse (parser); > - cp_parser_using_directive (parser); > + size_t attr_idx = cp_parser_skip_std_attribute_spec_seq (parser, 1); > + cp_token *after_attr = NULL; > + if (attr_idx != 1) > + after_attr = cp_lexer_peek_nth_token (parser->lexer, attr_idx); > + /* If the next tokens after attributes is `using namespace', then we have > + a using-directive. */ > + if (after_attr > + && after_attr->keyword == RID_USING > + && cp_lexer_nth_token_is_keyword (parser->lexer, attr_idx + 1, > + RID_NAMESPACE)) > + { > + if (statement_p) > + cp_parser_commit_to_tentative_parse (parser); > + cp_parser_using_directive (parser); > + } > + /* If the next token after attributes is `asm', then we have > + an asm-definition. */ > + else if (after_attr && after_attr->keyword == RID_ASM) > + { > + if (statement_p) > + cp_parser_commit_to_tentative_parse (parser); > + cp_parser_asm_definition (parser); > + } > + /* Anything else must be a simple-declaration. */ > + else > + cp_parser_simple_declaration (parser, !statement_p, > + /*maybe_range_for_decl*/NULL); > } > - /* Anything else must be a simple-declaration. */ > - else > - cp_parser_simple_declaration (parser, !statement_p, > - /*maybe_range_for_decl*/NULL); > } > > /* Parse a simple-declaration. > @@ -22424,6 +22437,7 @@ cp_parser_asm_definition (cp_parser* par > bool invalid_inputs_p = false; > bool invalid_outputs_p = false; > required_token missing = RT_NONE; > + tree std_attrs = cp_parser_std_attribute_spec_seq (parser); > location_t asm_loc = cp_lexer_peek_token (parser->lexer)->location; > > /* Look for the `asm' keyword. */ > @@ -22657,6 +22671,10 @@ cp_parser_asm_definition (cp_parser* par > else > symtab->finalize_toplevel_asm (string); > } > + > + if (std_attrs) > + warning_at (asm_loc, OPT_Wattributes, > + "attributes ignored on %<asm%> declaration"); > } > > /* Given the type TYPE of a declaration with declarator DECLARATOR, return the > --- gcc/testsuite/g++.dg/DRs/dr2262.C.jj 2023-12-04 19:58:06.433811915 +0100 > +++ gcc/testsuite/g++.dg/DRs/dr2262.C 2023-12-04 20:23:01.655737020 +0100 > @@ -0,0 +1,16 @@ > +// DR 2262 - Attributes for asm-definition > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wattributes" } > + > +[[]] asm ("nop"); > +[[foo::bar]] asm ("nop"); // { dg-warning "attributes ignored on 'asm' declaration" } > + > +void > +foo () > +{ > + int i = 42; > + [[]] asm ("nop"); > + [[foo::bar]] asm ("nop"); // { dg-warning "attributes ignored on 'asm' declaration" } > + [[]] asm ("nop" : "+r" (i)); > + [[foo::bar]] [[bar::baz]] asm ("nop" : "+r" (i)); // { dg-warning "attributes ignored on 'asm' declaration" } > +} > --- gcc/testsuite/g++.dg/cpp0x/gen-attrs-76.C.jj 2021-08-12 09:34:16.094246634 +0200 > +++ gcc/testsuite/g++.dg/cpp0x/gen-attrs-76.C 2023-12-04 20:43:45.002188817 +0100 > @@ -8,9 +8,9 @@ namespace P {} > void > foo () > { > - [[]] asm (""); // { dg-error "expected" } > + [[]] asm (""); > [[]] __extension__ asm (""); // { dg-error "expected" } > - __extension__ [[]] asm (""); // { dg-error "expected" } > + __extension__ [[]] asm (""); > [[]] namespace M = ::N; // { dg-error "expected" } > [[]] using namespace N; // { dg-bogus "expected" } > using namespace P [[]]; // { dg-error "expected" } > @@ -22,9 +22,9 @@ foo () > void > bar () > { > - [[gnu::unused]] asm (""); // { dg-error "expected" } > + [[gnu::unused]] asm (""); > [[gnu::unused]] __extension__ asm (""); // { dg-error "expected" } > - __extension__ [[gnu::unused]] asm (""); // { dg-error "expected" } > + __extension__ [[gnu::unused]] asm (""); > [[gnu::unused]] namespace M = ::N; // { dg-error "expected" } > [[gnu::unused]] using namespace N; // { dg-bogus "expected" } > using namespace P [[gnu::unused]]; // { dg-error "expected" } > --- gcc/testsuite/g++.dg/gomp/attrs-11.C.jj 2021-08-12 09:34:16.720237822 +0200 > +++ gcc/testsuite/g++.dg/gomp/attrs-11.C 2023-12-05 07:47:03.336641204 +0100 > @@ -7,9 +7,9 @@ namespace O { typedef int T; }; > void > foo () > { > - [[omp::directive (parallel)]] asm (""); // { dg-error "expected" } > + [[omp::directive (parallel)]] asm (""); > [[omp::directive (parallel)]] __extension__ asm (""); // { dg-error "expected" } > - __extension__ [[omp::directive (parallel)]] asm (""); // { dg-error "expected" } > + __extension__ [[omp::directive (parallel)]] asm (""); > [[omp::directive (parallel)]] namespace M = ::N; // { dg-error "expected" } > [[omp::directive (parallel)]] using namespace N; // { dg-error "not allowed to be specified in this context" } > [[omp::directive (parallel)]] using O::T; // { dg-error "expected" } > > Jakub >
--- gcc/cp/parser.cc.jj 2023-12-04 08:59:06.871357329 +0100 +++ gcc/cp/parser.cc 2023-12-04 20:23:53.225009856 +0100 @@ -15398,7 +15398,6 @@ cp_parser_block_declaration (cp_parser * /* Peek at the next token to figure out which kind of declaration is present. */ cp_token *token1 = cp_lexer_peek_token (parser->lexer); - size_t attr_idx; /* If the next keyword is `asm', we have an asm-definition. */ if (token1->keyword == RID_ASM) @@ -15452,22 +15451,36 @@ cp_parser_block_declaration (cp_parser * /* If the next token is `static_assert' we have a static assertion. */ else if (token1->keyword == RID_STATIC_ASSERT) cp_parser_static_assert (parser, /*member_p=*/false); - /* If the next tokens after attributes is `using namespace', then we have - a using-directive. */ - else if ((attr_idx = cp_parser_skip_std_attribute_spec_seq (parser, 1)) != 1 - && cp_lexer_nth_token_is_keyword (parser->lexer, attr_idx, - RID_USING) - && cp_lexer_nth_token_is_keyword (parser->lexer, attr_idx + 1, - RID_NAMESPACE)) + else { - if (statement_p) - cp_parser_commit_to_tentative_parse (parser); - cp_parser_using_directive (parser); + size_t attr_idx = cp_parser_skip_std_attribute_spec_seq (parser, 1); + cp_token *after_attr = NULL; + if (attr_idx != 1) + after_attr = cp_lexer_peek_nth_token (parser->lexer, attr_idx); + /* If the next tokens after attributes is `using namespace', then we have + a using-directive. */ + if (after_attr + && after_attr->keyword == RID_USING + && cp_lexer_nth_token_is_keyword (parser->lexer, attr_idx + 1, + RID_NAMESPACE)) + { + if (statement_p) + cp_parser_commit_to_tentative_parse (parser); + cp_parser_using_directive (parser); + } + /* If the next token after attributes is `asm', then we have + an asm-definition. */ + else if (after_attr && after_attr->keyword == RID_ASM) + { + if (statement_p) + cp_parser_commit_to_tentative_parse (parser); + cp_parser_asm_definition (parser); + } + /* Anything else must be a simple-declaration. */ + else + cp_parser_simple_declaration (parser, !statement_p, + /*maybe_range_for_decl*/NULL); } - /* Anything else must be a simple-declaration. */ - else - cp_parser_simple_declaration (parser, !statement_p, - /*maybe_range_for_decl*/NULL); } /* Parse a simple-declaration. @@ -22424,6 +22437,7 @@ cp_parser_asm_definition (cp_parser* par bool invalid_inputs_p = false; bool invalid_outputs_p = false; required_token missing = RT_NONE; + tree std_attrs = cp_parser_std_attribute_spec_seq (parser); location_t asm_loc = cp_lexer_peek_token (parser->lexer)->location; /* Look for the `asm' keyword. */ @@ -22657,6 +22671,10 @@ cp_parser_asm_definition (cp_parser* par else symtab->finalize_toplevel_asm (string); } + + if (std_attrs) + warning_at (asm_loc, OPT_Wattributes, + "attributes ignored on %<asm%> declaration"); } /* Given the type TYPE of a declaration with declarator DECLARATOR, return the --- gcc/testsuite/g++.dg/DRs/dr2262.C.jj 2023-12-04 19:58:06.433811915 +0100 +++ gcc/testsuite/g++.dg/DRs/dr2262.C 2023-12-04 20:23:01.655737020 +0100 @@ -0,0 +1,16 @@ +// DR 2262 - Attributes for asm-definition +// { dg-do compile { target c++11 } } +// { dg-options "-Wattributes" } + +[[]] asm ("nop"); +[[foo::bar]] asm ("nop"); // { dg-warning "attributes ignored on 'asm' declaration" } + +void +foo () +{ + int i = 42; + [[]] asm ("nop"); + [[foo::bar]] asm ("nop"); // { dg-warning "attributes ignored on 'asm' declaration" } + [[]] asm ("nop" : "+r" (i)); + [[foo::bar]] [[bar::baz]] asm ("nop" : "+r" (i)); // { dg-warning "attributes ignored on 'asm' declaration" } +} --- gcc/testsuite/g++.dg/cpp0x/gen-attrs-76.C.jj 2021-08-12 09:34:16.094246634 +0200 +++ gcc/testsuite/g++.dg/cpp0x/gen-attrs-76.C 2023-12-04 20:43:45.002188817 +0100 @@ -8,9 +8,9 @@ namespace P {} void foo () { - [[]] asm (""); // { dg-error "expected" } + [[]] asm (""); [[]] __extension__ asm (""); // { dg-error "expected" } - __extension__ [[]] asm (""); // { dg-error "expected" } + __extension__ [[]] asm (""); [[]] namespace M = ::N; // { dg-error "expected" } [[]] using namespace N; // { dg-bogus "expected" } using namespace P [[]]; // { dg-error "expected" } @@ -22,9 +22,9 @@ foo () void bar () { - [[gnu::unused]] asm (""); // { dg-error "expected" } + [[gnu::unused]] asm (""); [[gnu::unused]] __extension__ asm (""); // { dg-error "expected" } - __extension__ [[gnu::unused]] asm (""); // { dg-error "expected" } + __extension__ [[gnu::unused]] asm (""); [[gnu::unused]] namespace M = ::N; // { dg-error "expected" } [[gnu::unused]] using namespace N; // { dg-bogus "expected" } using namespace P [[gnu::unused]]; // { dg-error "expected" } --- gcc/testsuite/g++.dg/gomp/attrs-11.C.jj 2021-08-12 09:34:16.720237822 +0200 +++ gcc/testsuite/g++.dg/gomp/attrs-11.C 2023-12-05 07:47:03.336641204 +0100 @@ -7,9 +7,9 @@ namespace O { typedef int T; }; void foo () { - [[omp::directive (parallel)]] asm (""); // { dg-error "expected" } + [[omp::directive (parallel)]] asm (""); [[omp::directive (parallel)]] __extension__ asm (""); // { dg-error "expected" } - __extension__ [[omp::directive (parallel)]] asm (""); // { dg-error "expected" } + __extension__ [[omp::directive (parallel)]] asm (""); [[omp::directive (parallel)]] namespace M = ::N; // { dg-error "expected" } [[omp::directive (parallel)]] using namespace N; // { dg-error "not allowed to be specified in this context" } [[omp::directive (parallel)]] using O::T; // { dg-error "expected" }
Hi! Seems in 2017 attribute-specifier-seq[opt] was added to asm-declaration and the change was voted in as a DR. The following patch implements it by parsing the attributes and warning about them. I found one attribute parsing bug I'll send a fix for momentarily. And there is another thing I wonder about: with -Wno-attributes= we are supposed to ignore the attributes altogether, but we are actually still warning about them when we emit these generic warnings about ignoring all attributes which appertain to this and that (perhaps with some exceptions we first remove from the attribute chain), like: void foo () { [[foo::bar]]; } with -Wattributes -Wno-attributes=foo::bar Shouldn't we call some helper function in cases like this and warn not when std_attrs (or how the attribute chain var is called) is non-NULL, but if it is non-NULL and contains at least one non-attribute_ignored_p attribute? cp_parser_declaration at least tries: if (std_attrs != NULL_TREE && !attribute_ignored_p (std_attrs)) warning_at (make_location (attrs_loc, attrs_loc, parser->lexer), OPT_Wattributes, "attribute ignored"); but attribute_ignored_p here checks the first attribute rather than the whole chain. So it will incorrectly not warn if there is an ignored attribute followed by non-ignored. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2023-12-05 Jakub Jelinek <jakub@redhat.com> PR c++/110734 * parser.cc (cp_parser_block_declaration): Implement C++ DR 2262 - Attributes for asm-definition. Call cp_parser_asm_definition even if RID_ASM token is only seen after sequence of standard attributes. (cp_parser_asm_definition): Parse standard attributes before RID_ASM token and warn for them with -Wattributes. * g++.dg/DRs/dr2262.C: New test. * g++.dg/cpp0x/gen-attrs-76.C (foo, bar): Don't expect errors on attributes on asm definitions. * g++.dg/gomp/attrs-11.C: Remove 2 expected errors. Jakub