On Wed, Jul 17, 2024 at 09:30:00PM -0700, Andi Kleen wrote:
> Implement a C23 clang compatible musttail attribute similar to the earlier
> C++ implementation in the C parser.
>
> gcc/c/ChangeLog:
>
> PR c/83324
> * c-parser.cc (struct attr_state): Define with musttail_p.
> (c_parser_statement_after_labels): Handle [[musttail]].
> (c_parser_std_attribute): Dito.
> (c_parser_handle_musttail): Dito.
> (c_parser_compound_statement_nostart): Dito.
> (c_parser_all_labels): Dito.
> (c_parser_statement): Dito.
> * c-tree.h (c_finish_return): Add musttail_p flag.
> * c-typeck.cc (c_finish_return): Handle musttail_p flag.
> ---
> gcc/c/c-parser.cc | 70 ++++++++++++++++++++++++++++++++++++++---------
> gcc/c/c-tree.h | 2 +-
> gcc/c/c-typeck.cc | 7 +++--
> 3 files changed, 63 insertions(+), 16 deletions(-)
>
> diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
> index 12c5ed5d92c7..a8848d01f21a 100644
> --- a/gcc/c/c-parser.cc
> +++ b/gcc/c/c-parser.cc
> @@ -1621,6 +1621,12 @@ struct omp_for_parse_data {
> bool fail : 1;
> };
>
> +struct attr_state
> +{
> + /* True if we parsed a musttail attribute for return. */
> + bool musttail_p;
> +};
> +
> static bool c_parser_nth_token_starts_std_attributes (c_parser *,
> unsigned int);
> static tree c_parser_std_attribute_specifier_sequence (c_parser *);
> @@ -1665,7 +1671,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 =
> {});
Nit: the line seems to go over 80 columns.
> static tree c_parser_c99_block_statement (c_parser *, bool *,
> location_t * = NULL);
> static void c_parser_if_statement (c_parser *, bool *, vec<tree> *);
> @@ -6982,6 +6988,29 @@ c_parser_handle_directive_omp_attributes (tree &attrs,
> }
> }
>
> +/* Check if STD_ATTR contains a musttail attribute and remove if it
> + precedes a return. PARSER is the parser and ATTR is the output
> + attr_state. */
> +
> +static tree
> +c_parser_handle_musttail (c_parser *parser, tree std_attrs, attr_state &attr)
> +{
> + 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);
> + attr.musttail_p = true;
> + }
> + if (lookup_attribute ("clang", "musttail", std_attrs))
> + {
> + std_attrs = remove_attribute ("clang", "musttail", std_attrs);
> + attr.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). */
> @@ -6998,6 +7027,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))
> {
> @@ -7138,7 +7168,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)
> @@ -7286,7 +7319,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
> @@ -7328,15 +7361,20 @@ 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 attr = {};
> 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_std_attribute_specifier_sequence (parser);
> + std_attrs = c_parser_handle_musttail (parser, std_attrs, attr);
> + }
> +
> 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)
> @@ -7346,7 +7384,10 @@ c_parser_all_labels (c_parser *parser)
> 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_std_attribute_specifier_sequence (parser);
> + std_attrs = c_parser_handle_musttail (parser, std_attrs, attr);
> + }
Thanks, I believe this addresses the testcase I mentioned earlier:
struct str
{
int a, b;
};
struct str
cstruct (int x)
{
if (x < 10)
L: // <====
[[gnu::musttail]] return cstruct (x + 1);
return ((struct str){ x, 0 });
}
but I didn't see that being tested in your testsuite patch; apologies if
I missed it.
> 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;
> @@ -11742,6 +11743,8 @@ c_finish_return (location_t loc, tree retval, tree
> origtype)
> warning_at (xloc, 0,
> "function declared %<noreturn%> has a %<return%> statement");
>
> + set_musttail_on_return (retval, xloc, musttail_p);
> +
> if (retval)
> {
> tree semantic_type = NULL_TREE;
Is it deliberate that set_musttail_on_return is called outside the
if (retval) block? If it can be moved into it, set_musttail_on_return
can be simplified to assume that retval is always non-null.
Marek
