On Thu, Jul 18, 2024 at 02:19:21PM -0400, Marek Polacek wrote:
> 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.
Ok.
> > || 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.
It wasn't there. I will add 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.
Yes it can be removed.
Is the patchk ok with these changes?
-Andi
