| Message ID | 20240124193134.622934-4-ak@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/5] Improve must tail in RTL backend | expand |
On Wed, 24 Jan 2024, Andi Kleen wrote: > Implement a C23 clang compatible musttail attribute similar to the earlier > C++ implementation in the C parser. I'd expect diagnostics, and associated tests of those diagnostics, for: * musttail attribute used with any arguments, even empty [[gnu::musttail()]], much like e.g. [[fallthrough()]] or [[maybe_unused()]] gets diagnosed. * musttail attribute used on a declaration, or as part of an attribute declaration (attributes on their own before a semicolon - whether musttail on its own, or together with the fallthrough attribute that is valid in that case). * musttail attribute used on any statement other than a return statement. All of these should definitely apply to the gnu:: form and probably to clang:: as well. Some of these might already be diagnosed, but I don't see them in the added testcases. For the first one of these, it may help to include the attribute in the c_common_gnu_attributes table so the common attribute parsing code knows that this one doesn't accept arguments (and with an attribute handler that always rejects it on declarations, much like handle_fallthrough_attribute).
On Thu, Jan 25, 2024 at 08:08:23PM +0000, Joseph Myers wrote: > On Wed, 24 Jan 2024, Andi Kleen wrote: > > > Implement a C23 clang compatible musttail attribute similar to the earlier > > C++ implementation in the C parser. > > I'd expect diagnostics, and associated tests of those diagnostics, for: > > * musttail attribute used with any arguments, even empty > [[gnu::musttail()]], much like e.g. [[fallthrough()]] or > [[maybe_unused()]] gets diagnosed. These happen naturally because the attribute doesn't get removed when not in front of return, and it gets warned about like any other unknown attribute: tattr.c:5:9: warning: ‘musttail’ attribute ignored [-Wattributes] 5 | [[gnu::musttail]] i++; | ^ I don't have tests for that but since it's not new behavior I suppose that's sufficient. > For the first one of these, it may help to include the attribute in the > c_common_gnu_attributes table so the common attribute parsing code knows > that this one doesn't accept arguments (and with an attribute handler that > always rejects it on declarations, much like > handle_fallthrough_attribute). I just removed it there based on earlier feedback, which gives the intended "attribute is ignored" warning for these cases too. -Andi
On Thu, 25 Jan 2024, Andi Kleen wrote: > On Thu, Jan 25, 2024 at 08:08:23PM +0000, Joseph Myers wrote: > > On Wed, 24 Jan 2024, Andi Kleen wrote: > > > > > Implement a C23 clang compatible musttail attribute similar to the earlier > > > C++ implementation in the C parser. > > > > I'd expect diagnostics, and associated tests of those diagnostics, for: > > > > * musttail attribute used with any arguments, even empty > > [[gnu::musttail()]], much like e.g. [[fallthrough()]] or > > [[maybe_unused()]] gets diagnosed. > > These happen naturally because the attribute doesn't get removed when > not in front of return, and it gets warned about like any other unknown attribute: > > tattr.c:5:9: warning: ‘musttail’ attribute ignored [-Wattributes] > 5 | [[gnu::musttail]] i++; > | ^ > > I don't have tests for that but since it's not new behavior I suppose > that's sufficient. Each attribute should have tests that invalid uses are appropriately diagnosed. See gcc.dg/c23-attr-fallthrough-2.c for examples of such tests in the case of the [[fallthrough]] attribute. Some invalid uses may be diagnosed by existing code that's generic across attributes, others require specific code for the individual attribute. The default parsing of an attribute without an entry in the table of attribute handlers is that arbitrary balanced token sequences are parsed and discarded as arguments. To diagnose such arguments (in contexts when the attribute is otherwise valid), an entry in the table of attribute handlers is appropriate.
> > I don't have tests for that but since it's not new behavior I suppose > > that's sufficient. > > Each attribute should have tests that invalid uses are appropriately > diagnosed. See gcc.dg/c23-attr-fallthrough-2.c for examples of such tests > in the case of the [[fallthrough]] attribute. Some invalid uses may be > diagnosed by existing code that's generic across attributes, others > require specific code for the individual attribute. Okay I can add a test for the other statement and declaration cases like below. Any other change you need for approval? > > The default parsing of an attribute without an entry in the table of > attribute handlers is that arbitrary balanced token sequences are parsed > and discarded as arguments. And it triggers a warning too (see below) > To diagnose such arguments (in contexts when > the attribute is otherwise valid), an entry in the table of attribute > handlers is appropriate. The only valid usage is [[musttail]] return and there is already the default warning in the other cases. So I don't think an entry in the table is needed. BTW I noticed that [[musttail]] ; (empty statement with attribute) gives an error, which is probably a (unrelated) bug, afaik that should be legal for C23. -Andi t.c: [[musttail]] int j; __attribute__((musttail)) int k; void foo(void) { [[musttail]] j++; [[musttail]] if (k > 0) [[musttail]] k++; } t.c:2:1: warning: ‘musttail’ attribute ignored [-Wattributes] 2 | [[musttail]] int j; | ^ t.c:3:1: warning: ‘musttail’ attribute directive ignored [-Wattributes] 3 | __attribute__((musttail)) int k; | ^~~~~~~~~~~~~ t.c: In function ‘foo’: t.c:7:9: warning: ‘musttail’ attribute ignored [-Wattributes] 7 | [[musttail]] j++; | ^ t.c:8:9: warning: ‘musttail’ attribute ignored [-Wattributes] 8 | [[musttail]] if (k > 0) | ^ t.c:9:17: warning: ‘musttail’ attribute ignored [-Wattributes] 9 | [[musttail]] k++;
On Fri, 26 Jan 2024, Andi Kleen wrote: > > > I don't have tests for that but since it's not new behavior I suppose > > > that's sufficient. > > > > Each attribute should have tests that invalid uses are appropriately > > diagnosed. See gcc.dg/c23-attr-fallthrough-2.c for examples of such tests > > in the case of the [[fallthrough]] attribute. Some invalid uses may be > > diagnosed by existing code that's generic across attributes, others > > require specific code for the individual attribute. > > Okay I can add a test for the other statement and declaration cases like > below. > > Any other change you need for approval? I use testcases as a key part of the review of a patch, to see if the behavior is as I'd expect, so will need to see the updated patch series. As we're in regression-fixing mode for GCC 14, a new feature like this will need to wait for consideration until after GCC 14 branches. > > The default parsing of an attribute without an entry in the table of > > attribute handlers is that arbitrary balanced token sequences are parsed > > and discarded as arguments. > > And it triggers a warning too (see below) For attribute arguments, the key test is [[gnu::musttail()]] on a return statement where the attribute would be valid were it not for the attribute arguments. > BTW I noticed that [[musttail]] ; (empty statement with attribute) gives an error, which > is probably a (unrelated) bug, afaik that should be legal for C23. That's defined in the standard as an attribute declaration, not an attribute on a statement (empty statements can't have attributes). The only currently supported attribute valid in an attribute declaration is [[fallthrough]]. When you give an attribute in C23 syntax without a namespace (so [[musttail]] as opposed to [[gnu::musttail]]), if it's not a known standard attribute then it fails the constraint in 6.7.12.1p2, "The identifier in a standard attribute shall be one of: [list]".
diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc index c31349dae2ff..30f3fe042a2b 100644 --- a/gcc/c/c-parser.cc +++ b/gcc/c/c-parser.cc @@ -1616,6 +1616,11 @@ struct omp_for_parse_data { bool fail : 1; }; +struct attr_state +{ + bool musttail_p; // parsed a musttail for return +}; + static bool c_parser_nth_token_starts_std_attributes (c_parser *, unsigned int); static tree c_parser_std_attribute_specifier_sequence (c_parser *); @@ -1660,7 +1665,7 @@ static location_t c_parser_compound_statement_nostart (c_parser *); static void c_parser_label (c_parser *, tree); static void c_parser_statement (c_parser *, bool *, location_t * = NULL); static void c_parser_statement_after_labels (c_parser *, bool *, - vec<tree> * = NULL); + vec<tree> * = NULL, attr_state = {}); static tree c_parser_c99_block_statement (c_parser *, bool *, location_t * = NULL); static void c_parser_if_statement (c_parser *, bool *, vec<tree> *); @@ -6943,6 +6948,28 @@ c_parser_handle_directive_omp_attributes (tree &attrs, } } +/* Check if STD_ATTR contains a musttail attribute and handle it + PARSER is the parser and A is the output attr_state. */ + +static tree +c_parser_handle_musttail (c_parser *parser, tree std_attrs, attr_state &a) +{ + if (c_parser_next_token_is_keyword (parser, RID_RETURN)) + { + if (lookup_attribute ("gnu", "musttail", std_attrs)) + { + std_attrs = remove_attribute ("gnu", "musttail", std_attrs); + a.musttail_p = true; + } + if (lookup_attribute ("clang", "musttail", std_attrs)) + { + std_attrs = remove_attribute ("clang", "musttail", std_attrs); + a.musttail_p = true; + } + } + return std_attrs; +} + /* Parse a compound statement except for the opening brace. This is used for parsing both compound statements and statement expressions (which follow different paths to handling the opening). */ @@ -6959,6 +6986,7 @@ c_parser_compound_statement_nostart (c_parser *parser) bool in_omp_loop_block = omp_for_parse_state ? omp_for_parse_state->want_nested_loop : false; tree sl = NULL_TREE; + attr_state a = {}; if (c_parser_next_token_is (parser, CPP_CLOSE_BRACE)) { @@ -7097,7 +7125,10 @@ c_parser_compound_statement_nostart (c_parser *parser) = c_parser_nth_token_starts_std_attributes (parser, 1); tree std_attrs = NULL_TREE; if (have_std_attrs) - std_attrs = c_parser_std_attribute_specifier_sequence (parser); + { + std_attrs = c_parser_std_attribute_specifier_sequence (parser); + std_attrs = c_parser_handle_musttail (parser, std_attrs, a); + } if (c_parser_next_token_is_keyword (parser, RID_CASE) || c_parser_next_token_is_keyword (parser, RID_DEFAULT) || (c_parser_next_token_is (parser, CPP_NAME) @@ -7245,7 +7276,7 @@ c_parser_compound_statement_nostart (c_parser *parser) last_stmt = true; mark_valid_location_for_stdc_pragma (false); if (!omp_for_parse_state) - c_parser_statement_after_labels (parser, NULL); + c_parser_statement_after_labels (parser, NULL, NULL, a); else { /* In canonical loop nest form, nested loops can only appear @@ -7287,15 +7318,18 @@ c_parser_compound_statement_nostart (c_parser *parser) /* Parse all consecutive labels, possibly preceded by standard attributes. In this context, a statement is required, not a declaration, so attributes must be followed by a statement that is - not just a semicolon. */ + not just a semicolon. Returns an attr_state. */ -static void +static attr_state c_parser_all_labels (c_parser *parser) { + attr_state a = {}; bool have_std_attrs; tree std_attrs = NULL; if ((have_std_attrs = c_parser_nth_token_starts_std_attributes (parser, 1))) - std_attrs = c_parser_std_attribute_specifier_sequence (parser); + std_attrs = c_parser_handle_musttail (parser, + c_parser_std_attribute_specifier_sequence (parser), a); + while (c_parser_next_token_is_keyword (parser, RID_CASE) || c_parser_next_token_is_keyword (parser, RID_DEFAULT) || (c_parser_next_token_is (parser, CPP_NAME) @@ -7317,6 +7351,7 @@ c_parser_all_labels (c_parser *parser) } else if (have_std_attrs && c_parser_next_token_is (parser, CPP_SEMICOLON)) c_parser_error (parser, "expected statement"); + return a; } /* Parse a label (C90 6.6.1, C99 6.8.1, C11 6.8.1). @@ -7560,11 +7595,11 @@ c_parser_label (c_parser *parser, tree std_attrs) static void c_parser_statement (c_parser *parser, bool *if_p, location_t *loc_after_labels) { - c_parser_all_labels (parser); + attr_state a = c_parser_all_labels (parser); if (loc_after_labels) *loc_after_labels = c_parser_peek_token (parser)->location; parser->omp_attrs_forbidden_p = false; - c_parser_statement_after_labels (parser, if_p, NULL); + c_parser_statement_after_labels (parser, if_p, NULL, a); } /* Parse a statement, other than a labeled statement. CHAIN is a vector @@ -7573,11 +7608,11 @@ c_parser_statement (c_parser *parser, bool *if_p, location_t *loc_after_labels) IF_P is used to track whether there's a (possibly labeled) if statement which is not enclosed in braces and has an else clause. This is used to - implement -Wparentheses. */ + implement -Wparentheses. A has an earlier parsed attribute state. */ static void c_parser_statement_after_labels (c_parser *parser, bool *if_p, - vec<tree> *chain) + vec<tree> *chain, attr_state a) { location_t loc = c_parser_peek_token (parser)->location; tree stmt = NULL_TREE; @@ -7645,7 +7680,7 @@ c_parser_statement_after_labels (c_parser *parser, bool *if_p, c_parser_consume_token (parser); if (c_parser_next_token_is (parser, CPP_SEMICOLON)) { - stmt = c_finish_return (loc, NULL_TREE, NULL_TREE); + stmt = c_finish_return (loc, NULL_TREE, NULL_TREE, a.musttail_p); c_parser_consume_token (parser); } else @@ -7654,7 +7689,7 @@ c_parser_statement_after_labels (c_parser *parser, bool *if_p, struct c_expr expr = c_parser_expression_conv (parser); mark_exp_read (expr.value); stmt = c_finish_return (EXPR_LOC_OR_LOC (expr.value, xloc), - expr.value, expr.original_type); + expr.value, expr.original_type, a.musttail_p); goto expect_semicolon; } break; diff --git a/gcc/c/c-tree.h b/gcc/c/c-tree.h index cf29534c0915..902cc8f6aa49 100644 --- a/gcc/c/c-tree.h +++ b/gcc/c/c-tree.h @@ -824,7 +824,7 @@ extern tree c_begin_stmt_expr (void); extern tree c_finish_stmt_expr (location_t, tree); extern tree c_process_expr_stmt (location_t, tree); extern tree c_finish_expr_stmt (location_t, tree); -extern tree c_finish_return (location_t, tree, tree); +extern tree c_finish_return (location_t, tree, tree, bool = false); extern tree c_finish_bc_stmt (location_t, tree, bool); extern tree c_finish_goto_label (location_t, tree); extern tree c_finish_goto_ptr (location_t, c_expr val); diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc index 66c6abc9f076..144b001e3a6f 100644 --- a/gcc/c/c-typeck.cc +++ b/gcc/c/c-typeck.cc @@ -11422,10 +11422,10 @@ c_finish_goto_ptr (location_t loc, c_expr val) to return, or a null pointer for `return;' with no value. LOC is the location of the return statement, or the location of the expression, if the statement has any. If ORIGTYPE is not NULL_TREE, it - is the original type of RETVAL. */ + is the original type of RETVAL. MUSTTAIL_P indicates a musttail attribute. */ tree -c_finish_return (location_t loc, tree retval, tree origtype) +c_finish_return (location_t loc, tree retval, tree origtype, bool musttail_p) { tree valtype = TREE_TYPE (TREE_TYPE (current_function_decl)), ret_stmt; bool no_warning = false; @@ -11439,6 +11439,17 @@ c_finish_return (location_t loc, tree retval, tree origtype) warning_at (xloc, 0, "function declared %<noreturn%> has a %<return%> statement"); + if (retval && musttail_p) + { + if (TREE_CODE (retval) == CALL_EXPR) + CALL_EXPR_MUST_TAIL_CALL (retval) = 1; + else if (TREE_CODE (retval) == TARGET_EXPR + && TREE_CODE (TARGET_EXPR_INITIAL (retval)) == CALL_EXPR) + CALL_EXPR_MUST_TAIL_CALL (TARGET_EXPR_INITIAL (retval)) = 1; + else + error_at (xloc, "cannot tail-call: return value must be call"); + } + if (retval) { tree semantic_type = NULL_TREE;