* [PATCH 0/4] c/c++, asm: Various updates @ 2018-12-10 22:47 Segher Boessenkool 2018-12-10 22:47 ` [PATCH 1/4] c/c++, asm: Write the asm-qualifier loop without "done" boolean Segher Boessenkool ` (4 more replies) 0 siblings, 5 replies; 19+ messages in thread From: Segher Boessenkool @ 2018-12-10 22:47 UTC (permalink / raw) To: gcc-patches; +Cc: jakub, Joseph Myers, jason, Segher Boessenkool This ties up some loose ends, and adds some more testcases. Tested on powerpc64-linux {-m32,-m64}. Is this okay for trunk? Segher Segher Boessenkool (4): c/c++, asm: Write the asm-qualifier loop without "done" boolean c/c++, asm: Use nicer error for duplicate asm qualifiers c/c++, asm: Use nicer warning for const and restrict c++, asm: Do not handle any asm-qualifiers in top-level asm gcc/c/c-parser.c | 106 ++++++++++++++++++++++--------------- gcc/c/c-tree.h | 2 +- gcc/c/c-typeck.c | 4 +- gcc/cp/parser.c | 107 ++++++++++++++++++++++---------------- gcc/testsuite/g++.dg/asm-qual-1.C | 13 +++++ gcc/testsuite/g++.dg/asm-qual-2.C | 46 ++++++++++++++++ gcc/testsuite/g++.dg/asm-qual-3.C | 12 +++++ gcc/testsuite/gcc.dg/asm-qual-1.c | 6 +-- gcc/testsuite/gcc.dg/asm-qual-3.c | 9 ++++ 9 files changed, 209 insertions(+), 96 deletions(-) create mode 100644 gcc/testsuite/g++.dg/asm-qual-1.C create mode 100644 gcc/testsuite/g++.dg/asm-qual-2.C create mode 100644 gcc/testsuite/g++.dg/asm-qual-3.C create mode 100644 gcc/testsuite/gcc.dg/asm-qual-3.c -- 1.8.3.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/4] c/c++, asm: Write the asm-qualifier loop without "done" boolean 2018-12-10 22:47 [PATCH 0/4] c/c++, asm: Various updates Segher Boessenkool @ 2018-12-10 22:47 ` Segher Boessenkool 2018-12-18 20:41 ` Jason Merrill 2018-12-10 22:48 ` [PATCH 4/4] c++, asm: Do not handle any asm-qualifiers in top-level asm Segher Boessenkool ` (3 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Segher Boessenkool @ 2018-12-10 22:47 UTC (permalink / raw) To: gcc-patches; +Cc: jakub, Joseph Myers, jason, Segher Boessenkool As suggested by Jason. Segher 2018-12-10 Segher Boessenkool <segher@kernel.crashing.org> c/ * c-parser.c (c_parser_asm_statement): Rewrite the loop to work without "done" boolean variable. cp/ * parser.c (cp_parser_asm_definition): Rewrite the loop to work without "done" boolean variable. --- gcc/c/c-parser.c | 66 +++++++++++++++++++++++++--------------------------- gcc/cp/parser.c | 71 +++++++++++++++++++++++++------------------------------- 2 files changed, 63 insertions(+), 74 deletions(-) diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c index b875c4f..121a91c 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -6372,40 +6372,37 @@ c_parser_asm_statement (c_parser *parser) is_volatile = false; is_inline = false; is_goto = false; - for (bool done = false; !done; ) - switch (c_parser_peek_token (parser)->keyword) - { - case RID_VOLATILE: - if (!is_volatile) - { - is_volatile = true; - quals = c_parser_peek_token (parser)->value; - c_parser_consume_token (parser); - } - else - done = true; - break; - case RID_INLINE: - if (!is_inline) - { - is_inline = true; - c_parser_consume_token (parser); - } - else - done = true; - break; - case RID_GOTO: - if (!is_goto) - { - is_goto = true; - c_parser_consume_token (parser); - } - else - done = true; - break; - default: - done = true; - } + for (;;) + { + switch (c_parser_peek_token (parser)->keyword) + { + case RID_VOLATILE: + if (is_volatile) + break; + is_volatile = true; + quals = c_parser_peek_token (parser)->value; + c_parser_consume_token (parser); + continue; + + case RID_INLINE: + if (is_inline) + break; + is_inline = true; + c_parser_consume_token (parser); + continue; + + case RID_GOTO: + if (is_goto) + break; + is_goto = true; + c_parser_consume_token (parser); + continue; + + default: + break; + } + break; + } /* ??? Follow the C++ parser rather than using the lex_untranslated_string kludge. */ diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index adfe09e..1cc34ba 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -19648,47 +19648,38 @@ cp_parser_asm_definition (cp_parser* parser) cp_function_chain->invalid_constexpr = true; } - /* See if the next token is `volatile'. */ + /* Handle the asm-qualifier-list. */ if (cp_parser_allow_gnu_extensions_p (parser)) - for (bool done = false; !done ; ) - switch (cp_lexer_peek_token (parser->lexer)->keyword) - { - case RID_VOLATILE: - if (!volatile_p) - { - /* Remember that we saw the `volatile' keyword. */ - volatile_p = true; - /* Consume the token. */ - cp_lexer_consume_token (parser->lexer); - } - else - done = true; - break; - case RID_INLINE: - if (!inline_p && parser->in_function_body) - { - /* Remember that we saw the `inline' keyword. */ - inline_p = true; - /* Consume the token. */ - cp_lexer_consume_token (parser->lexer); - } - else - done = true; - break; - case RID_GOTO: - if (!goto_p && parser->in_function_body) - { - /* Remember that we saw the `goto' keyword. */ - goto_p = true; - /* Consume the token. */ - cp_lexer_consume_token (parser->lexer); - } - else - done = true; - break; - default: - done = true; - } + for (;;) + { + switch (cp_lexer_peek_token (parser->lexer)->keyword) + { + case RID_VOLATILE: + if (volatile_p) + break; + volatile_p = true; + cp_lexer_consume_token (parser->lexer); + continue; + + case RID_INLINE: + if (inline_p || !parser->in_function_body) + break; + inline_p = true; + cp_lexer_consume_token (parser->lexer); + continue; + + case RID_GOTO: + if (goto_p || !parser->in_function_body) + break; + goto_p = true; + cp_lexer_consume_token (parser->lexer); + continue; + + default: + break; + } + break; + } /* Look for the opening `('. */ if (!cp_parser_require (parser, CPP_OPEN_PAREN, RT_OPEN_PAREN)) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] c/c++, asm: Write the asm-qualifier loop without "done" boolean 2018-12-10 22:47 ` [PATCH 1/4] c/c++, asm: Write the asm-qualifier loop without "done" boolean Segher Boessenkool @ 2018-12-18 20:41 ` Jason Merrill 0 siblings, 0 replies; 19+ messages in thread From: Jason Merrill @ 2018-12-18 20:41 UTC (permalink / raw) To: Segher Boessenkool, gcc-patches; +Cc: jakub, Joseph Myers On 12/10/18 5:47 PM, Segher Boessenkool wrote: > As suggested by Jason. > > > Segher > > > 2018-12-10 Segher Boessenkool <segher@kernel.crashing.org> > > c/ > * c-parser.c (c_parser_asm_statement): Rewrite the loop to work without > "done" boolean variable. > > cp/ > * parser.c (cp_parser_asm_definition): Rewrite the loop to work without > "done" boolean variable. OK, thanks. Jason ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/4] c++, asm: Do not handle any asm-qualifiers in top-level asm 2018-12-10 22:47 [PATCH 0/4] c/c++, asm: Various updates Segher Boessenkool 2018-12-10 22:47 ` [PATCH 1/4] c/c++, asm: Write the asm-qualifier loop without "done" boolean Segher Boessenkool @ 2018-12-10 22:48 ` Segher Boessenkool 2018-12-18 20:42 ` Jason Merrill 2018-12-10 22:48 ` [PATCH 3/4] c/c++, asm: Use nicer error for const and restrict Segher Boessenkool ` (2 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Segher Boessenkool @ 2018-12-10 22:48 UTC (permalink / raw) To: gcc-patches; +Cc: jakub, Joseph Myers, jason, Segher Boessenkool Previously, "volatile" was allowed. Changing this simplifies the code, makes things more regular, and makes the C and C++ frontends handle this the same way. 2018-12-10 Segher Boessenkool <segher@kernel.crashing.org> cp/ * parser.c (cp_parser_asm_definition): Do not allow any asm qualifiers on top-level asm. testsuite/ * g++.dg/asm-qual-3.C: New testcase. * gcc.dg/asm-qual-3.c: New testcase. --- gcc/cp/parser.c | 7 ++----- gcc/testsuite/g++.dg/asm-qual-3.C | 12 ++++++++++++ gcc/testsuite/gcc.dg/asm-qual-3.c | 9 +++++++++ 3 files changed, 23 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/g++.dg/asm-qual-3.C create mode 100644 gcc/testsuite/gcc.dg/asm-qual-3.c diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 4072afe..62c2a4f 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -19649,7 +19649,8 @@ cp_parser_asm_definition (cp_parser* parser) location_t volatile_loc = UNKNOWN_LOCATION; location_t inline_loc = UNKNOWN_LOCATION; location_t goto_loc = UNKNOWN_LOCATION; - if (cp_parser_allow_gnu_extensions_p (parser)) + + if (cp_parser_allow_gnu_extensions_p (parser) && parser->in_function_body) for (;;) { cp_token *token = cp_lexer_peek_token (parser->lexer); @@ -19668,8 +19669,6 @@ cp_parser_asm_definition (cp_parser* parser) continue; case RID_INLINE: - if (!parser->in_function_body) - break; if (inline_loc) { error_at (loc, "duplicate asm qualifier %qT", token->u.value); @@ -19681,8 +19680,6 @@ cp_parser_asm_definition (cp_parser* parser) continue; case RID_GOTO: - if (!parser->in_function_body) - break; if (goto_loc) { error_at (loc, "duplicate asm qualifier %qT", token->u.value); diff --git a/gcc/testsuite/g++.dg/asm-qual-3.C b/gcc/testsuite/g++.dg/asm-qual-3.C new file mode 100644 index 0000000..95c9b57 --- /dev/null +++ b/gcc/testsuite/g++.dg/asm-qual-3.C @@ -0,0 +1,12 @@ +// Test that asm-qualifiers are not allowed on toplevel asm. +// { dg-do compile } +// { dg-options "-std=gnu++98" } + +asm const (""); // { dg-error {expected '\(' before 'const'} } +asm volatile (""); // { dg-error {expected '\(' before 'volatile'} } +asm restrict (""); // { dg-error {expected '\(' before 'restrict'} } +asm inline (""); // { dg-error {expected '\(' before 'inline'} } +asm goto (""); // { dg-error {expected '\(' before 'goto'} } + +// There are many other things wrong with this code, so: +// { dg-excess-errors "" } diff --git a/gcc/testsuite/gcc.dg/asm-qual-3.c b/gcc/testsuite/gcc.dg/asm-qual-3.c new file mode 100644 index 0000000..f85d8bf --- /dev/null +++ b/gcc/testsuite/gcc.dg/asm-qual-3.c @@ -0,0 +1,9 @@ +/* Test that asm-qualifiers are not allowed on toplevel asm. */ +/* { dg-do compile } */ +/* { dg-options "-std=gnu99" } */ + +asm const (""); /* { dg-error {expected '\(' before 'const'} } */ +asm volatile (""); /* { dg-error {expected '\(' before 'volatile'} } */ +asm restrict (""); /* { dg-error {expected '\(' before 'restrict'} } */ +asm inline (""); /* { dg-error {expected '\(' before 'inline'} } */ +asm goto (""); /* { dg-error {expected '\(' before 'goto'} } */ -- 1.8.3.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] c++, asm: Do not handle any asm-qualifiers in top-level asm 2018-12-10 22:48 ` [PATCH 4/4] c++, asm: Do not handle any asm-qualifiers in top-level asm Segher Boessenkool @ 2018-12-18 20:42 ` Jason Merrill 0 siblings, 0 replies; 19+ messages in thread From: Jason Merrill @ 2018-12-18 20:42 UTC (permalink / raw) To: Segher Boessenkool, gcc-patches; +Cc: jakub, Joseph Myers On 12/10/18 5:47 PM, Segher Boessenkool wrote: > Previously, "volatile" was allowed. Changing this simplifies the code, > makes things more regular, and makes the C and C++ frontends handle > this the same way. > > > 2018-12-10 Segher Boessenkool <segher@kernel.crashing.org> > > cp/ > * parser.c (cp_parser_asm_definition): Do not allow any asm qualifiers > on top-level asm. OK. Jason ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/4] c/c++, asm: Use nicer error for const and restrict 2018-12-10 22:47 [PATCH 0/4] c/c++, asm: Various updates Segher Boessenkool 2018-12-10 22:47 ` [PATCH 1/4] c/c++, asm: Write the asm-qualifier loop without "done" boolean Segher Boessenkool 2018-12-10 22:48 ` [PATCH 4/4] c++, asm: Do not handle any asm-qualifiers in top-level asm Segher Boessenkool @ 2018-12-10 22:48 ` Segher Boessenkool 2018-12-18 20:43 ` Jason Merrill 2018-12-10 22:48 ` [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers Segher Boessenkool 2018-12-17 20:24 ` Ping: [PATCH 0/4] c/c++, asm: Various updates Segher Boessenkool 4 siblings, 1 reply; 19+ messages in thread From: Segher Boessenkool @ 2018-12-10 22:48 UTC (permalink / raw) To: gcc-patches; +Cc: jakub, Joseph Myers, jason, Segher Boessenkool Not all qualifiers are asm qualifiers. We can talk about that in a nicer way than just giving a generic parser error. This also adds two testcases for C++, that previously were for C only. 2018-12-10 Segher Boessenkool <segher@kernel.crashing.org> c/ * c-parser.c (c_parser_asm_statement) <RID_CONST, RID_RESTRICT>: Give a more specific error message (instead of just falling through). cp/ * parser.c (cp_parser_asm_definition) <RID_CONST, RID_RESTRICT>: Give a more specific error message (instead of just falling through). testsuite/ * g++.dg/asm-qual-1.C: New testcase. * g++.dg/asm-qual-2.C: New testcase. * gcc.dg/asm-qual-1.c: Update. --- gcc/c/c-parser.c | 6 +++++ gcc/cp/parser.c | 6 +++++ gcc/testsuite/g++.dg/asm-qual-1.C | 13 +++++++++++ gcc/testsuite/g++.dg/asm-qual-2.C | 46 +++++++++++++++++++++++++++++++++++++++ gcc/testsuite/gcc.dg/asm-qual-1.c | 6 ++--- 5 files changed, 73 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/g++.dg/asm-qual-1.C create mode 100644 gcc/testsuite/g++.dg/asm-qual-2.C diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c index 652e53c..0def497 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -6411,6 +6411,12 @@ c_parser_asm_statement (c_parser *parser) c_parser_consume_token (parser); continue; + case RID_CONST: + case RID_RESTRICT: + error_at (loc, "%qE is not an asm qualifier", token->value); + c_parser_consume_token (parser); + continue; + default: break; } diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 06a6bb0..4072afe 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -19693,6 +19693,12 @@ cp_parser_asm_definition (cp_parser* parser) cp_lexer_consume_token (parser->lexer); continue; + case RID_CONST: + case RID_RESTRICT: + error_at (loc, "%qT is not an asm qualifier", token->u.value); + cp_lexer_consume_token (parser->lexer); + continue; + default: break; } diff --git a/gcc/testsuite/g++.dg/asm-qual-1.C b/gcc/testsuite/g++.dg/asm-qual-1.C new file mode 100644 index 0000000..3fba592 --- /dev/null +++ b/gcc/testsuite/g++.dg/asm-qual-1.C @@ -0,0 +1,13 @@ +// Test that qualifiers other than volatile are disallowed on asm. +// { dg-do compile } +// { dg-options "-std=gnu++98" } + +void +f () +{ + asm volatile (""); + + asm const (""); // { dg-error {'const' is not an asm qualifier} } + + asm __restrict (""); // { dg-error {'__restrict' is not an asm qualifier} } +} diff --git a/gcc/testsuite/g++.dg/asm-qual-2.C b/gcc/testsuite/g++.dg/asm-qual-2.C new file mode 100644 index 0000000..52968bd --- /dev/null +++ b/gcc/testsuite/g++.dg/asm-qual-2.C @@ -0,0 +1,46 @@ +// Test that qualifiers on asm are allowed in any order. +// { dg-do compile } +// { dg-options "-std=c++98" } + +void +f () +{ + asm volatile goto ("" :::: lab); + asm volatile inline ("" :::); + asm inline volatile ("" :::); + asm inline goto ("" :::: lab); + asm goto volatile ("" :::: lab); + asm goto inline ("" :::: lab); + + asm volatile inline goto ("" :::: lab); + asm volatile goto inline ("" :::: lab); + asm inline volatile goto ("" :::: lab); + asm inline goto volatile ("" :::: lab); + asm goto volatile inline ("" :::: lab); + asm goto inline volatile ("" :::: lab); + + /* Duplicates are not allowed. */ + asm goto volatile volatile ("" :::: lab); /* { dg-error "" } */ + asm volatile goto volatile ("" :::: lab); /* { dg-error "" } */ + asm volatile volatile goto ("" :::: lab); /* { dg-error "" } */ + asm goto goto volatile ("" :::: lab); /* { dg-error "" } */ + asm goto volatile goto ("" :::: lab); /* { dg-error "" } */ + asm volatile goto goto ("" :::: lab); /* { dg-error "" } */ + + asm inline volatile volatile ("" :::); /* { dg-error "" } */ + asm volatile inline volatile ("" :::); /* { dg-error "" } */ + asm volatile volatile inline ("" :::); /* { dg-error "" } */ + asm inline inline volatile ("" :::); /* { dg-error "" } */ + asm inline volatile inline ("" :::); /* { dg-error "" } */ + asm volatile inline inline ("" :::); /* { dg-error "" } */ + + asm goto inline inline ("" :::: lab); /* { dg-error "" } */ + asm inline goto inline ("" :::: lab); /* { dg-error "" } */ + asm inline inline goto ("" :::: lab); /* { dg-error "" } */ + asm goto goto inline ("" :::: lab); /* { dg-error "" } */ + asm goto inline goto ("" :::: lab); /* { dg-error "" } */ + asm inline goto goto ("" :::: lab); /* { dg-error "" } */ + +lab: + ; +} diff --git a/gcc/testsuite/gcc.dg/asm-qual-1.c b/gcc/testsuite/gcc.dg/asm-qual-1.c index cb37283..eff6b45 100644 --- a/gcc/testsuite/gcc.dg/asm-qual-1.c +++ b/gcc/testsuite/gcc.dg/asm-qual-1.c @@ -8,9 +8,7 @@ f (void) { asm volatile (""); - asm const (""); /* { dg-error {expected '\(' before 'const'} } */ - /* { dg-error {expected identifier} {} {target *-*-*} .-1 } */ + asm const (""); /* { dg-error {'const' is not an asm qualifier} } */ - asm restrict (""); /* { dg-error {expected '\(' before 'restrict'} } */ - /* { dg-error {expected identifier} {} {target *-*-*} .-1 } */ + asm restrict (""); /* { dg-error {'restrict' is not an asm qualifier} } */ } -- 1.8.3.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] c/c++, asm: Use nicer error for const and restrict 2018-12-10 22:48 ` [PATCH 3/4] c/c++, asm: Use nicer error for const and restrict Segher Boessenkool @ 2018-12-18 20:43 ` Jason Merrill 0 siblings, 0 replies; 19+ messages in thread From: Jason Merrill @ 2018-12-18 20:43 UTC (permalink / raw) To: Segher Boessenkool, gcc-patches; +Cc: jakub, Joseph Myers On 12/10/18 5:47 PM, Segher Boessenkool wrote: > Not all qualifiers are asm qualifiers. We can talk about that in a > nicer way than just giving a generic parser error. > > This also adds two testcases for C++, that previously were for C only. > > > 2018-12-10 Segher Boessenkool <segher@kernel.crashing.org> > > c/ > * c-parser.c (c_parser_asm_statement) <RID_CONST, RID_RESTRICT>: Give > a more specific error message (instead of just falling through). > > cp/ > * parser.c (cp_parser_asm_definition) <RID_CONST, RID_RESTRICT>: Give > a more specific error message (instead of just falling through). OK. Jason ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers 2018-12-10 22:47 [PATCH 0/4] c/c++, asm: Various updates Segher Boessenkool ` (2 preceding siblings ...) 2018-12-10 22:48 ` [PATCH 3/4] c/c++, asm: Use nicer error for const and restrict Segher Boessenkool @ 2018-12-10 22:48 ` Segher Boessenkool 2018-12-11 15:35 ` David Malcolm 2018-12-11 17:31 ` Martin Sebor 2018-12-17 20:24 ` Ping: [PATCH 0/4] c/c++, asm: Various updates Segher Boessenkool 4 siblings, 2 replies; 19+ messages in thread From: Segher Boessenkool @ 2018-12-10 22:48 UTC (permalink / raw) To: gcc-patches; +Cc: jakub, Joseph Myers, jason, Segher Boessenkool Also as suggested by Jason. Segher 2018-12-10 Segher Boessenkool <segher@kernel.crashing.org> c/ * c-parser.c (c_parser_asm_statement): Keep track of the location each asm qualifier is first seen; use that to give nicer "duplicate asm qualifier" messages. Delete 'quals" variable, instead pass the "is_volatile_ flag to build_asm_stmt directly. * c-tree.h (build_asm_stmt): Make the first arg bool instead of tree. * c-typeck.c (build_asm_stmt): Ditto; adjust. cp/ * parser.c (cp_parser_asm_definition): Rewrite the loop to work without "done" boolean variable. * parser.c (cp_parser_asm_definition): Keep track of the location each asm qualifier is first seen; use that to give nicer "duplicate asm qualifier" messages. --- gcc/c/c-parser.c | 58 ++++++++++++++++++++++++++++++++++++-------------------- gcc/c/c-tree.h | 2 +- gcc/c/c-typeck.c | 4 ++-- gcc/cp/parser.c | 45 +++++++++++++++++++++++++++++++------------ 4 files changed, 73 insertions(+), 36 deletions(-) diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c index 121a91c..652e53c 100644 --- a/gcc/c/c-parser.c +++ b/gcc/c/c-parser.c @@ -6360,41 +6360,54 @@ c_parser_for_statement (c_parser *parser, bool ivdep, unsigned short unroll, static tree c_parser_asm_statement (c_parser *parser) { - tree quals, str, outputs, inputs, clobbers, labels, ret; - bool simple, is_volatile, is_inline, is_goto; + tree str, outputs, inputs, clobbers, labels, ret; + bool simple; location_t asm_loc = c_parser_peek_token (parser)->location; int section, nsections; gcc_assert (c_parser_next_token_is_keyword (parser, RID_ASM)); c_parser_consume_token (parser); - quals = NULL_TREE; - is_volatile = false; - is_inline = false; - is_goto = false; + /* Handle the asm-qualifier-list. */ + location_t volatile_loc = UNKNOWN_LOCATION; + location_t inline_loc = UNKNOWN_LOCATION; + location_t goto_loc = UNKNOWN_LOCATION; for (;;) { - switch (c_parser_peek_token (parser)->keyword) + c_token *token = c_parser_peek_token (parser); + location_t loc = token->location; + switch (token->keyword) { case RID_VOLATILE: - if (is_volatile) - break; - is_volatile = true; - quals = c_parser_peek_token (parser)->value; + if (volatile_loc) + { + error_at (loc, "duplicate asm qualifier %qE", token->value); + inform (volatile_loc, "first seen here"); + } + else + volatile_loc = loc; c_parser_consume_token (parser); continue; case RID_INLINE: - if (is_inline) - break; - is_inline = true; + if (inline_loc) + { + error_at (loc, "duplicate asm qualifier %qE", token->value); + inform (inline_loc, "first seen here"); + } + else + inline_loc = loc; c_parser_consume_token (parser); continue; case RID_GOTO: - if (is_goto) - break; - is_goto = true; + if (goto_loc) + { + error_at (loc, "duplicate asm qualifier %qE", token->value); + inform (goto_loc, "first seen here"); + } + else + goto_loc = loc; c_parser_consume_token (parser); continue; @@ -6405,6 +6417,10 @@ c_parser_asm_statement (c_parser *parser) break; } + bool is_volatile = (volatile_loc != UNKNOWN_LOCATION); + bool is_inline = (inline_loc != UNKNOWN_LOCATION); + bool is_goto = (goto_loc != UNKNOWN_LOCATION); + /* ??? Follow the C++ parser rather than using the lex_untranslated_string kludge. */ parser->lex_untranslated_string = true; @@ -6479,9 +6495,9 @@ c_parser_asm_statement (c_parser *parser) if (!c_parser_require (parser, CPP_SEMICOLON, "expected %<;%>")) c_parser_skip_to_end_of_block_or_statement (parser); - ret = build_asm_stmt (quals, build_asm_expr (asm_loc, str, outputs, inputs, - clobbers, labels, simple, - is_inline)); + ret = build_asm_stmt (is_volatile, + build_asm_expr (asm_loc, str, outputs, inputs, + clobbers, labels, simple, is_inline)); error: parser->lex_untranslated_string = false; diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h index f08a8fc..dc9e3cd 100644 --- a/gcc/c/c-tree.h +++ b/gcc/c/c-tree.h @@ -679,7 +679,7 @@ extern tree c_start_case (location_t, location_t, tree, bool); extern void c_finish_case (tree, tree); extern tree build_asm_expr (location_t, tree, tree, tree, tree, tree, bool, bool); -extern tree build_asm_stmt (tree, tree); +extern tree build_asm_stmt (bool, tree); extern int c_types_compatible_p (tree, tree); extern tree c_begin_compound_stmt (bool); extern tree c_end_compound_stmt (location_t, tree, bool); diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c index 1a89727..abf9b21 100644 --- a/gcc/c/c-typeck.c +++ b/gcc/c/c-typeck.c @@ -10316,9 +10316,9 @@ process_init_element (location_t loc, struct c_expr value, bool implicit, (guaranteed to be 'volatile' or null) and ARGS (represented using an ASM_EXPR node). */ tree -build_asm_stmt (tree cv_qualifier, tree args) +build_asm_stmt (bool is_volatile, tree args) { - if (!ASM_VOLATILE_P (args) && cv_qualifier) + if (is_volatile) ASM_VOLATILE_P (args) = 1; return add_stmt (args); } diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c index 1cc34ba..06a6bb0 100644 --- a/gcc/cp/parser.c +++ b/gcc/cp/parser.c @@ -19630,12 +19630,9 @@ cp_parser_asm_definition (cp_parser* parser) tree clobbers = NULL_TREE; tree labels = NULL_TREE; tree asm_stmt; - bool volatile_p = false; bool extended_p = false; bool invalid_inputs_p = false; bool invalid_outputs_p = false; - bool inline_p = false; - bool goto_p = false; required_token missing = RT_NONE; /* Look for the `asm' keyword. */ @@ -19649,29 +19646,50 @@ cp_parser_asm_definition (cp_parser* parser) } /* Handle the asm-qualifier-list. */ + location_t volatile_loc = UNKNOWN_LOCATION; + location_t inline_loc = UNKNOWN_LOCATION; + location_t goto_loc = UNKNOWN_LOCATION; if (cp_parser_allow_gnu_extensions_p (parser)) for (;;) { + cp_token *token = cp_lexer_peek_token (parser->lexer); + location_t loc = token->location; switch (cp_lexer_peek_token (parser->lexer)->keyword) { case RID_VOLATILE: - if (volatile_p) - break; - volatile_p = true; + if (volatile_loc) + { + error_at (loc, "duplicate asm qualifier %qT", token->u.value); + inform (volatile_loc, "first seen here"); + } + else + volatile_loc = loc; cp_lexer_consume_token (parser->lexer); continue; case RID_INLINE: - if (inline_p || !parser->in_function_body) + if (!parser->in_function_body) break; - inline_p = true; + if (inline_loc) + { + error_at (loc, "duplicate asm qualifier %qT", token->u.value); + inform (inline_loc, "first seen here"); + } + else + inline_loc = loc; cp_lexer_consume_token (parser->lexer); continue; case RID_GOTO: - if (goto_p || !parser->in_function_body) + if (!parser->in_function_body) break; - goto_p = true; + if (goto_loc) + { + error_at (loc, "duplicate asm qualifier %qT", token->u.value); + inform (goto_loc, "first seen here"); + } + else + goto_loc = loc; cp_lexer_consume_token (parser->lexer); continue; @@ -19681,6 +19699,10 @@ cp_parser_asm_definition (cp_parser* parser) break; } + bool volatile_p = (volatile_loc != UNKNOWN_LOCATION); + bool inline_p = (inline_loc != UNKNOWN_LOCATION); + bool goto_p = (goto_loc != UNKNOWN_LOCATION); + /* Look for the opening `('. */ if (!cp_parser_require (parser, CPP_OPEN_PAREN, RT_OPEN_PAREN)) return; @@ -19772,8 +19794,7 @@ cp_parser_asm_definition (cp_parser* parser) CPP_CLOSE_PAREN)) clobbers = cp_parser_asm_clobber_list (parser); } - else if (goto_p - && cp_lexer_next_token_is (parser->lexer, CPP_SCOPE)) + else if (goto_p && cp_lexer_next_token_is (parser->lexer, CPP_SCOPE)) /* The labels are coming next. */ labels_p = true; -- 1.8.3.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers 2018-12-10 22:48 ` [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers Segher Boessenkool @ 2018-12-11 15:35 ` David Malcolm 2018-12-11 15:48 ` David Malcolm 2018-12-12 17:40 ` Segher Boessenkool 2018-12-11 17:31 ` Martin Sebor 1 sibling, 2 replies; 19+ messages in thread From: David Malcolm @ 2018-12-11 15:35 UTC (permalink / raw) To: Segher Boessenkool, gcc-patches; +Cc: jakub, Joseph Myers, jason On Mon, 2018-12-10 at 22:47 +0000, Segher Boessenkool wrote: [...] > diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c > index 121a91c..652e53c 100644 > --- a/gcc/c/c-parser.c > +++ b/gcc/c/c-parser.c > @@ -6360,41 +6360,54 @@ c_parser_for_statement (c_parser *parser, > bool ivdep, unsigned short unroll, > static tree > c_parser_asm_statement (c_parser *parser) > { > - tree quals, str, outputs, inputs, clobbers, labels, ret; > - bool simple, is_volatile, is_inline, is_goto; > + tree str, outputs, inputs, clobbers, labels, ret; > + bool simple; > location_t asm_loc = c_parser_peek_token (parser)->location; > int section, nsections; > > gcc_assert (c_parser_next_token_is_keyword (parser, RID_ASM)); > c_parser_consume_token (parser); > > - quals = NULL_TREE; > - is_volatile = false; > - is_inline = false; > - is_goto = false; > + /* Handle the asm-qualifier-list. */ > + location_t volatile_loc = UNKNOWN_LOCATION; > + location_t inline_loc = UNKNOWN_LOCATION; > + location_t goto_loc = UNKNOWN_LOCATION; > for (;;) > { > - switch (c_parser_peek_token (parser)->keyword) > + c_token *token = c_parser_peek_token (parser); > + location_t loc = token->location; > + switch (token->keyword) > { > case RID_VOLATILE: > - if (is_volatile) > - break; > - is_volatile = true; > - quals = c_parser_peek_token (parser)->value; > + if (volatile_loc) > + { > + error_at (loc, "duplicate asm qualifier %qE", token- > >value); > + inform (volatile_loc, "first seen here"); > + } Thanks for the improvements. Is there test coverage for these errors and notes? A diagnostic nit (new with gcc 9): please add an: auto_diagnostic_group d; to the start of the guarded block, so that the "error" and "note" are known to be related. See: https://gcc.gnu.org/onlinedocs/gccint/Guidelines-for-Diagnostics.html#Group-logically-related-diagnostics > + else > + volatile_loc = loc; > c_parser_consume_token (parser); > continue; > > case RID_INLINE: > - if (is_inline) > - break; > - is_inline = true; > + if (inline_loc) > + { > + error_at (loc, "duplicate asm qualifier %qE", token- > >value); > + inform (inline_loc, "first seen here"); Likewise. > + } > + else > + inline_loc = loc; > c_parser_consume_token (parser); > continue; > > case RID_GOTO: > - if (is_goto) > - break; > - is_goto = true; > + if (goto_loc) > + { > + error_at (loc, "duplicate asm qualifier %qE", token- > >value); > + inform (goto_loc, "first seen here"); > + } Likewise. > + else > + goto_loc = loc; > c_parser_consume_token (parser); > continue; [...] > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > index 1cc34ba..06a6bb0 100644 > --- a/gcc/cp/parser.c > +++ b/gcc/cp/parser.c > @@ -19649,29 +19646,50 @@ cp_parser_asm_definition (cp_parser* > parser) > } > > /* Handle the asm-qualifier-list. */ > + location_t volatile_loc = UNKNOWN_LOCATION; > + location_t inline_loc = UNKNOWN_LOCATION; > + location_t goto_loc = UNKNOWN_LOCATION; > if (cp_parser_allow_gnu_extensions_p (parser)) > for (;;) > { > + cp_token *token = cp_lexer_peek_token (parser->lexer); > + location_t loc = token->location; > switch (cp_lexer_peek_token (parser->lexer)->keyword) > { > case RID_VOLATILE: > - if (volatile_p) > - break; > - volatile_p = true; > + if (volatile_loc) > + { > + error_at (loc, "duplicate asm qualifier %qT", token- > >u.value); > + inform (volatile_loc, "first seen here"); Likewise. > + } > + else > + volatile_loc = loc; > cp_lexer_consume_token (parser->lexer); > continue; > > case RID_INLINE: > - if (inline_p || !parser->in_function_body) > + if (!parser->in_function_body) > break; > - inline_p = true; > + if (inline_loc) > + { > + error_at (loc, "duplicate asm qualifier %qT", token- > >u.value); > + inform (inline_loc, "first seen here"); Likewise. > + } > + else > + inline_loc = loc; > cp_lexer_consume_token (parser->lexer); > continue; > > case RID_GOTO: > - if (goto_p || !parser->in_function_body) > + if (!parser->in_function_body) > break; > - goto_p = true; > + if (goto_loc) > + { > + error_at (loc, "duplicate asm qualifier %qT", token- > >u.value); > + inform (goto_loc, "first seen here"); Likewise. > + } > + else > + goto_loc = loc; > cp_lexer_consume_token (parser->lexer); > continue; > [...] Dave ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers 2018-12-11 15:35 ` David Malcolm @ 2018-12-11 15:48 ` David Malcolm 2018-12-12 17:43 ` Segher Boessenkool 2018-12-12 17:40 ` Segher Boessenkool 1 sibling, 1 reply; 19+ messages in thread From: David Malcolm @ 2018-12-11 15:48 UTC (permalink / raw) To: Segher Boessenkool, gcc-patches; +Cc: jakub, Joseph Myers, jason On Tue, 2018-12-11 at 10:35 -0500, David Malcolm wrote: > On Mon, 2018-12-10 at 22:47 +0000, Segher Boessenkool wrote: > > [...] > > > diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c > > index 121a91c..652e53c 100644 > > --- a/gcc/c/c-parser.c > > +++ b/gcc/c/c-parser.c > > @@ -6360,41 +6360,54 @@ c_parser_for_statement (c_parser *parser, > > bool ivdep, unsigned short unroll, > > static tree > > c_parser_asm_statement (c_parser *parser) > > { > > - tree quals, str, outputs, inputs, clobbers, labels, ret; > > - bool simple, is_volatile, is_inline, is_goto; > > + tree str, outputs, inputs, clobbers, labels, ret; > > + bool simple; > > location_t asm_loc = c_parser_peek_token (parser)->location; > > int section, nsections; > > > > gcc_assert (c_parser_next_token_is_keyword (parser, RID_ASM)); > > c_parser_consume_token (parser); > > > > - quals = NULL_TREE; > > - is_volatile = false; > > - is_inline = false; > > - is_goto = false; > > + /* Handle the asm-qualifier-list. */ > > + location_t volatile_loc = UNKNOWN_LOCATION; > > + location_t inline_loc = UNKNOWN_LOCATION; > > + location_t goto_loc = UNKNOWN_LOCATION; > > for (;;) > > { > > - switch (c_parser_peek_token (parser)->keyword) > > + c_token *token = c_parser_peek_token (parser); > > + location_t loc = token->location; > > + switch (token->keyword) > > { > > case RID_VOLATILE: > > - if (is_volatile) > > - break; > > - is_volatile = true; > > - quals = c_parser_peek_token (parser)->value; > > + if (volatile_loc) > > + { > > + error_at (loc, "duplicate asm qualifier %qE", token- > > > value); > > > > + inform (volatile_loc, "first seen here"); > > + } > > Thanks for the improvements. > > Is there test coverage for these errors and notes? > > A diagnostic nit (new with gcc 9): please add an: > auto_diagnostic_group d; > to the start of the guarded block, so that the "error" and "note" are > known to be related. > > See: > https://gcc.gnu.org/onlinedocs/gccint/Guidelines-for-Diagnostics.html > #Group-logically-related-diagnostics For bonus points, these could offer fix-it hints, so that an IDE can offer to delete the duplicate qualifier token. The above code could be: if (volatile_loc) complain_about_duplicate_asm_qualifier (token->value, loc, volatile_loc); else volatile_loc = loc; void complain_about_duplicate_asm_qualifier (tree value, location_t duplicate_loc, location_t first_loc) { auto_diagnostic_group d; gcc_rich_location richloc (duplicate_loc); richloc.add_fixit_remove (); error_at (&richloc, "duplicate asm qualifier %qE", value); inform (first_loc, "first seen here"); } or somesuch, where rich_location::add_fixit_remove adds a fix-it hint suggesting the removal of all of "loc", the duplicate token; given that it's 5 lines at this point, a subroutine seems justified, to eliminate duplication at the 6 sites it's done. Caveat: haven't tried to compile the above. Dave > > + else > > + volatile_loc = loc; > > c_parser_consume_token (parser); > > continue; > > > > case RID_INLINE: > > - if (is_inline) > > - break; > > - is_inline = true; > > + if (inline_loc) > > + { > > + error_at (loc, "duplicate asm qualifier %qE", token- > > > value); > > > > + inform (inline_loc, "first seen here"); > > Likewise. > > > + } > > + else > > + inline_loc = loc; > > c_parser_consume_token (parser); > > continue; > > > > case RID_GOTO: > > - if (is_goto) > > - break; > > - is_goto = true; > > + if (goto_loc) > > + { > > + error_at (loc, "duplicate asm qualifier %qE", token- > > > value); > > > > + inform (goto_loc, "first seen here"); > > + } > > Likewise. > > > + else > > + goto_loc = loc; > > c_parser_consume_token (parser); > > continue; > > [...] > > > diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c > > index 1cc34ba..06a6bb0 100644 > > --- a/gcc/cp/parser.c > > +++ b/gcc/cp/parser.c > > @@ -19649,29 +19646,50 @@ cp_parser_asm_definition (cp_parser* > > parser) > > } > > > > /* Handle the asm-qualifier-list. */ > > + location_t volatile_loc = UNKNOWN_LOCATION; > > + location_t inline_loc = UNKNOWN_LOCATION; > > + location_t goto_loc = UNKNOWN_LOCATION; > > if (cp_parser_allow_gnu_extensions_p (parser)) > > for (;;) > > { > > + cp_token *token = cp_lexer_peek_token (parser->lexer); > > + location_t loc = token->location; > > switch (cp_lexer_peek_token (parser->lexer)->keyword) > > { > > case RID_VOLATILE: > > - if (volatile_p) > > - break; > > - volatile_p = true; > > + if (volatile_loc) > > + { > > + error_at (loc, "duplicate asm qualifier %qT", > > token- > > > u.value); > > > > + inform (volatile_loc, "first seen here"); > > Likewise. > > > + } > > + else > > + volatile_loc = loc; > > cp_lexer_consume_token (parser->lexer); > > continue; > > > > case RID_INLINE: > > - if (inline_p || !parser->in_function_body) > > + if (!parser->in_function_body) > > break; > > - inline_p = true; > > + if (inline_loc) > > + { > > + error_at (loc, "duplicate asm qualifier %qT", > > token- > > > u.value); > > > > + inform (inline_loc, "first seen here"); > > Likewise. > > > + } > > + else > > + inline_loc = loc; > > cp_lexer_consume_token (parser->lexer); > > continue; > > > > case RID_GOTO: > > - if (goto_p || !parser->in_function_body) > > + if (!parser->in_function_body) > > break; > > - goto_p = true; > > + if (goto_loc) > > + { > > + error_at (loc, "duplicate asm qualifier %qT", > > token- > > > u.value); > > > > + inform (goto_loc, "first seen here"); > > Likewise. > > > + } > > + else > > + goto_loc = loc; > > cp_lexer_consume_token (parser->lexer); > > continue; > > > > [...] > > > Dave ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers 2018-12-11 15:48 ` David Malcolm @ 2018-12-12 17:43 ` Segher Boessenkool 0 siblings, 0 replies; 19+ messages in thread From: Segher Boessenkool @ 2018-12-12 17:43 UTC (permalink / raw) To: David Malcolm; +Cc: gcc-patches, jakub, Joseph Myers, jason On Tue, Dec 11, 2018 at 10:48:15AM -0500, David Malcolm wrote: > For bonus points, these could offer fix-it hints, so that an IDE can > offer to delete the duplicate qualifier token. Yes it could. But have you ever seen this error, in a real program? (It was an error before, just without a nice message). Segher ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers 2018-12-11 15:35 ` David Malcolm 2018-12-11 15:48 ` David Malcolm @ 2018-12-12 17:40 ` Segher Boessenkool 2018-12-12 23:23 ` David Malcolm 1 sibling, 1 reply; 19+ messages in thread From: Segher Boessenkool @ 2018-12-12 17:40 UTC (permalink / raw) To: David Malcolm; +Cc: gcc-patches, jakub, Joseph Myers, jason On Tue, Dec 11, 2018 at 10:35:00AM -0500, David Malcolm wrote: > On Mon, 2018-12-10 at 22:47 +0000, Segher Boessenkool wrote: > > [...] > > > diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c > > index 121a91c..652e53c 100644 > > --- a/gcc/c/c-parser.c > > +++ b/gcc/c/c-parser.c > > @@ -6360,41 +6360,54 @@ c_parser_for_statement (c_parser *parser, > > bool ivdep, unsigned short unroll, > > static tree > > c_parser_asm_statement (c_parser *parser) > > { > > - tree quals, str, outputs, inputs, clobbers, labels, ret; > > - bool simple, is_volatile, is_inline, is_goto; > > + tree str, outputs, inputs, clobbers, labels, ret; > > + bool simple; > > location_t asm_loc = c_parser_peek_token (parser)->location; > > int section, nsections; > > > > gcc_assert (c_parser_next_token_is_keyword (parser, RID_ASM)); > > c_parser_consume_token (parser); > > > > - quals = NULL_TREE; > > - is_volatile = false; > > - is_inline = false; > > - is_goto = false; > > + /* Handle the asm-qualifier-list. */ > > + location_t volatile_loc = UNKNOWN_LOCATION; > > + location_t inline_loc = UNKNOWN_LOCATION; > > + location_t goto_loc = UNKNOWN_LOCATION; > > for (;;) > > { > > - switch (c_parser_peek_token (parser)->keyword) > > + c_token *token = c_parser_peek_token (parser); > > + location_t loc = token->location; > > + switch (token->keyword) > > { > > case RID_VOLATILE: > > - if (is_volatile) > > - break; > > - is_volatile = true; > > - quals = c_parser_peek_token (parser)->value; > > + if (volatile_loc) > > + { > > + error_at (loc, "duplicate asm qualifier %qE", token- > > >value); > > + inform (volatile_loc, "first seen here"); > > + } > > Thanks for the improvements. > > Is there test coverage for these errors and notes? Yes, in this same patch. The error, that is; I have no idea how to test the note, and that belongs in a different test anyhow. Dejagnu ignores notes normally. > A diagnostic nit (new with gcc 9): please add an: > auto_diagnostic_group d; > to the start of the guarded block, so that the "error" and "note" are > known to be related. How would that work for asm volatile goto volatile goto ("uh-oh" :::: lab); lab :; There are two groups, overlapping, but not nested. I suppose something could be done with new() but that is too ugly for words. Is there a procedural interface, too? Something that does not depend on C++ magic. Segher ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers 2018-12-12 17:40 ` Segher Boessenkool @ 2018-12-12 23:23 ` David Malcolm 2018-12-13 0:48 ` Segher Boessenkool 0 siblings, 1 reply; 19+ messages in thread From: David Malcolm @ 2018-12-12 23:23 UTC (permalink / raw) To: Segher Boessenkool; +Cc: gcc-patches, jakub, Joseph Myers, jason On Wed, 2018-12-12 at 11:40 -0600, Segher Boessenkool wrote: > On Tue, Dec 11, 2018 at 10:35:00AM -0500, David Malcolm wrote: > > On Mon, 2018-12-10 at 22:47 +0000, Segher Boessenkool wrote: > > > > [...] > > > > > diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c > > > index 121a91c..652e53c 100644 > > > --- a/gcc/c/c-parser.c > > > +++ b/gcc/c/c-parser.c > > > @@ -6360,41 +6360,54 @@ c_parser_for_statement (c_parser *parser, > > > bool ivdep, unsigned short unroll, > > > static tree > > > c_parser_asm_statement (c_parser *parser) > > > { > > > - tree quals, str, outputs, inputs, clobbers, labels, ret; > > > - bool simple, is_volatile, is_inline, is_goto; > > > + tree str, outputs, inputs, clobbers, labels, ret; > > > + bool simple; > > > location_t asm_loc = c_parser_peek_token (parser)->location; > > > int section, nsections; > > > > > > gcc_assert (c_parser_next_token_is_keyword (parser, RID_ASM)); > > > c_parser_consume_token (parser); > > > > > > - quals = NULL_TREE; > > > - is_volatile = false; > > > - is_inline = false; > > > - is_goto = false; > > > + /* Handle the asm-qualifier-list. */ > > > + location_t volatile_loc = UNKNOWN_LOCATION; > > > + location_t inline_loc = UNKNOWN_LOCATION; > > > + location_t goto_loc = UNKNOWN_LOCATION; > > > for (;;) > > > { > > > - switch (c_parser_peek_token (parser)->keyword) > > > + c_token *token = c_parser_peek_token (parser); > > > + location_t loc = token->location; > > > + switch (token->keyword) > > > { > > > case RID_VOLATILE: > > > - if (is_volatile) > > > - break; > > > - is_volatile = true; > > > - quals = c_parser_peek_token (parser)->value; > > > + if (volatile_loc) > > > + { > > > + error_at (loc, "duplicate asm qualifier %qE", > > > token- > > > > value); > > > > > > + inform (volatile_loc, "first seen here"); > > > + } > > > > Thanks for the improvements. > > > > Is there test coverage for these errors and notes? > > Yes, in this same patch. The error, that is; I have no idea how to > test > the note, and that belongs in a different test anyhow. Dejagnu > ignores > notes normally. You can use dg-message to test for a note. The ignoring of notes is done by prune.exp, which strips out any notes that are unmatched after the dg-message runs. There are numerous examples in the testsuite, see e.g gcc.dg/floatn-errs.c which has: extern float a; /* { dg-message "previous declaration" } */ extern _Float32 a; /* { dg-error "conflicting" } */ > > A diagnostic nit (new with gcc 9): please add an: > > auto_diagnostic_group d; > > to the start of the guarded block, so that the "error" and "note" > > are > > known to be related. > > How would that work for > > asm volatile goto volatile goto ("uh-oh" :::: lab); lab :; > > There are two groups, overlapping, but not nested. I suppose > something > could be done with new() but that is too ugly for words. Is there a > procedural interface, too? Something that does not depend on C++ > magic. If I'm understanding the patch correctly, I'd expect it to output something like: error: duplicate asm qualifier 'volatile' asm volatile goto volatile goto ("uh-oh" :::: lab); lab :; ^~~~~~~~ note: first seen here asm volatile goto volatile goto ("uh-oh" :::: lab); lab :; ^~~~~~~~ error: duplicate asm qualifier 'goto' asm volatile goto volatile goto ("uh-oh" :::: lab); lab :; ^~~~ note: first seen here asm volatile goto volatile goto ("uh-oh" :::: lab); lab :; ^~~~ (with appropriate column numbers) where each error/note pair are in their own diagnostic group. I don't think it matters that the locations "overlap" here. There isn't a procedural interface for diagnostic groups - though there could be if needed. I think the existing auto_diagnostic_group ctor/dtor ought to work here. We definitely don't want/need to be using new, I agree that that would be wrong. Let me know if you want me to update the patch for you. Hope this is constructive Dave ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers 2018-12-12 23:23 ` David Malcolm @ 2018-12-13 0:48 ` Segher Boessenkool 0 siblings, 0 replies; 19+ messages in thread From: Segher Boessenkool @ 2018-12-13 0:48 UTC (permalink / raw) To: David Malcolm; +Cc: gcc-patches, jakub, Joseph Myers, jason On Wed, Dec 12, 2018 at 06:23:08PM -0500, David Malcolm wrote: > On Wed, 2018-12-12 at 11:40 -0600, Segher Boessenkool wrote: > > > Is there test coverage for these errors and notes? > > > > Yes, in this same patch. The error, that is; I have no idea how to > > test > > the note, and that belongs in a different test anyhow. Dejagnu > > ignores > > notes normally. > > You can use dg-message to test for a note. The ignoring of notes is > done by prune.exp, which strips out any notes that are unmatched after > the dg-message runs. > > There are numerous examples in the testsuite, see e.g > gcc.dg/floatn-errs.c > which has: > > extern float a; /* { dg-message "previous declaration" } */ > extern _Float32 a; /* { dg-error "conflicting" } */ Hrm. > > > A diagnostic nit (new with gcc 9): please add an: > > > auto_diagnostic_group d; > > > to the start of the guarded block, so that the "error" and "note" > > > are > > > known to be related. > > > > How would that work for > > > > asm volatile goto volatile goto ("uh-oh" :::: lab); lab :; > > > > There are two groups, overlapping, but not nested. I suppose > > something > > could be done with new() but that is too ugly for words. Is there a > > procedural interface, too? Something that does not depend on C++ > > magic. > > If I'm understanding the patch correctly, I'd expect it to output > something like: > > error: duplicate asm qualifier 'volatile' > asm volatile goto volatile goto ("uh-oh" :::: lab); lab :; > ^~~~~~~~ > note: first seen here > asm volatile goto volatile goto ("uh-oh" :::: lab); lab :; > ^~~~~~~~ > error: duplicate asm qualifier 'goto' > asm volatile goto volatile goto ("uh-oh" :::: lab); lab :; > ^~~~ > note: first seen here > asm volatile goto volatile goto ("uh-oh" :::: lab); lab :; > ^~~~ Yup. > where each error/note pair are in their own diagnostic group. I don't > think it matters that the locations "overlap" here. So how can I do that, "put them in their own group"? As far as I can see the interface allows only nested groups. > There isn't a procedural interface for diagnostic groups - though there > could be if needed. I think the existing auto_diagnostic_group > ctor/dtor ought to work here. We definitely don't want/need to be > using new, I agree that that would be wrong. If you declare the auto_group somewhere inside the switch (or anywhere in the loop even), no two qualifiers will be in the same group; and if you put the auto_group outside the loop, all asm qualifiers will be in one and the same group. Or, what do I misunderstand here? > Let me know if you want me to update the patch for you. Please do a separate patch on top? > Hope this is constructive It is :-) Segher ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers 2018-12-10 22:48 ` [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers Segher Boessenkool 2018-12-11 15:35 ` David Malcolm @ 2018-12-11 17:31 ` Martin Sebor 2018-12-12 17:50 ` Segher Boessenkool 1 sibling, 1 reply; 19+ messages in thread From: Martin Sebor @ 2018-12-11 17:31 UTC (permalink / raw) To: Segher Boessenkool, gcc-patches; +Cc: jakub, Joseph Myers, jason > + { > + error_at (loc, "duplicate asm qualifier %qE", token->value); We have been making an effort to quote keywords, identifiers, option names, and other such things in diagnostics. In the message above and all others like it in this patch kit that mention "asm" the keyword should be quoted the same way as the name of the qualifier. Thanks Martin ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers 2018-12-11 17:31 ` Martin Sebor @ 2018-12-12 17:50 ` Segher Boessenkool 2018-12-12 18:02 ` Martin Sebor 0 siblings, 1 reply; 19+ messages in thread From: Segher Boessenkool @ 2018-12-12 17:50 UTC (permalink / raw) To: Martin Sebor; +Cc: gcc-patches, jakub, Joseph Myers, jason On Tue, Dec 11, 2018 at 10:31:02AM -0700, Martin Sebor wrote: > >+ { > >+ error_at (loc, "duplicate asm qualifier %qE", token->value); > > We have been making an effort to quote keywords, identifiers, > option names, and other such things in diagnostics. In > the message above and all others like it in this patch kit > that mention "asm" the keyword should be quoted the same > way as the name of the qualifier. This message is about "asm qualifiers". It is not about the "asm" statement. You can write this without "asm" keyword, too, anyway (with an "__asm__" or such), making the message even more awkward to quote in that case. The location of the error has nothing to do with the "asm", either. You should only quote keywords that are in the source text. Not random words that *could* be a keyword :-) Segher ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers 2018-12-12 17:50 ` Segher Boessenkool @ 2018-12-12 18:02 ` Martin Sebor 2018-12-12 18:29 ` Segher Boessenkool 0 siblings, 1 reply; 19+ messages in thread From: Martin Sebor @ 2018-12-12 18:02 UTC (permalink / raw) To: Segher Boessenkool; +Cc: gcc-patches, jakub, Joseph Myers, jason On 12/12/18 10:50 AM, Segher Boessenkool wrote: > On Tue, Dec 11, 2018 at 10:31:02AM -0700, Martin Sebor wrote: >>> + { >>> + error_at (loc, "duplicate asm qualifier %qE", token->value); >> >> We have been making an effort to quote keywords, identifiers, >> option names, and other such things in diagnostics. In >> the message above and all others like it in this patch kit >> that mention "asm" the keyword should be quoted the same >> way as the name of the qualifier. > > This message is about "asm qualifiers". It is not about the "asm" > statement. You can write this without "asm" keyword, too, anyway (with > an "__asm__" or such), making the message even more awkward to quote in > that case. The location of the error has nothing to do with the "asm", > either. > > You should only quote keywords that are in the source text. Not random > words that *could* be a keyword :-) asm is not a random word, or even an English word (please look in a dictionary if you're in doubt). It is a C/C++ keyword :-) Martin ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers 2018-12-12 18:02 ` Martin Sebor @ 2018-12-12 18:29 ` Segher Boessenkool 0 siblings, 0 replies; 19+ messages in thread From: Segher Boessenkool @ 2018-12-12 18:29 UTC (permalink / raw) To: Martin Sebor; +Cc: gcc-patches, jakub, Joseph Myers, jason On Wed, Dec 12, 2018 at 11:02:29AM -0700, Martin Sebor wrote: > On 12/12/18 10:50 AM, Segher Boessenkool wrote: > >On Tue, Dec 11, 2018 at 10:31:02AM -0700, Martin Sebor wrote: > >>>+ { > >>>+ error_at (loc, "duplicate asm qualifier %qE", token->value); > >> > >>We have been making an effort to quote keywords, identifiers, > >>option names, and other such things in diagnostics. In > >>the message above and all others like it in this patch kit > >>that mention "asm" the keyword should be quoted the same > >>way as the name of the qualifier. > > > >This message is about "asm qualifiers". It is not about the "asm" > >statement. You can write this without "asm" keyword, too, anyway (with > >an "__asm__" or such), making the message even more awkward to quote in > >that case. The location of the error has nothing to do with the "asm", > >either. > > > >You should only quote keywords that are in the source text. Not random > >words that *could* be a keyword :-) > > asm is not a random word, or even an English word (please look > in a dictionary if you're in doubt). It is a C/C++ keyword :-) An "asm qualifier" is a GCC extension. Segher ^ permalink raw reply [flat|nested] 19+ messages in thread
* Ping: [PATCH 0/4] c/c++, asm: Various updates 2018-12-10 22:47 [PATCH 0/4] c/c++, asm: Various updates Segher Boessenkool ` (3 preceding siblings ...) 2018-12-10 22:48 ` [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers Segher Boessenkool @ 2018-12-17 20:24 ` Segher Boessenkool 4 siblings, 0 replies; 19+ messages in thread From: Segher Boessenkool @ 2018-12-17 20:24 UTC (permalink / raw) To: gcc-patches; +Cc: jakub, Joseph Myers, jason Ping? On Mon, Dec 10, 2018 at 10:47:23PM +0000, Segher Boessenkool wrote: > This ties up some loose ends, and adds some more testcases. > > Tested on powerpc64-linux {-m32,-m64}. Is this okay for trunk? > > > Segher > > > Segher Boessenkool (4): > c/c++, asm: Write the asm-qualifier loop without "done" boolean > c/c++, asm: Use nicer error for duplicate asm qualifiers > c/c++, asm: Use nicer warning for const and restrict > c++, asm: Do not handle any asm-qualifiers in top-level asm > > gcc/c/c-parser.c | 106 ++++++++++++++++++++++--------------- > gcc/c/c-tree.h | 2 +- > gcc/c/c-typeck.c | 4 +- > gcc/cp/parser.c | 107 ++++++++++++++++++++++---------------- > gcc/testsuite/g++.dg/asm-qual-1.C | 13 +++++ > gcc/testsuite/g++.dg/asm-qual-2.C | 46 ++++++++++++++++ > gcc/testsuite/g++.dg/asm-qual-3.C | 12 +++++ > gcc/testsuite/gcc.dg/asm-qual-1.c | 6 +-- > gcc/testsuite/gcc.dg/asm-qual-3.c | 9 ++++ > 9 files changed, 209 insertions(+), 96 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/asm-qual-1.C > create mode 100644 gcc/testsuite/g++.dg/asm-qual-2.C > create mode 100644 gcc/testsuite/g++.dg/asm-qual-3.C > create mode 100644 gcc/testsuite/gcc.dg/asm-qual-3.c > > -- > 1.8.3.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-12-18 20:43 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-10 22:47 [PATCH 0/4] c/c++, asm: Various updates Segher Boessenkool 2018-12-10 22:47 ` [PATCH 1/4] c/c++, asm: Write the asm-qualifier loop without "done" boolean Segher Boessenkool 2018-12-18 20:41 ` Jason Merrill 2018-12-10 22:48 ` [PATCH 4/4] c++, asm: Do not handle any asm-qualifiers in top-level asm Segher Boessenkool 2018-12-18 20:42 ` Jason Merrill 2018-12-10 22:48 ` [PATCH 3/4] c/c++, asm: Use nicer error for const and restrict Segher Boessenkool 2018-12-18 20:43 ` Jason Merrill 2018-12-10 22:48 ` [PATCH 2/4] c/c++, asm: Use nicer error for duplicate asm qualifiers Segher Boessenkool 2018-12-11 15:35 ` David Malcolm 2018-12-11 15:48 ` David Malcolm 2018-12-12 17:43 ` Segher Boessenkool 2018-12-12 17:40 ` Segher Boessenkool 2018-12-12 23:23 ` David Malcolm 2018-12-13 0:48 ` Segher Boessenkool 2018-12-11 17:31 ` Martin Sebor 2018-12-12 17:50 ` Segher Boessenkool 2018-12-12 18:02 ` Martin Sebor 2018-12-12 18:29 ` Segher Boessenkool 2018-12-17 20:24 ` Ping: [PATCH 0/4] c/c++, asm: Various updates Segher Boessenkool
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).