On Thu, Jul 18, 2024 at 03:11:56PM -0700, Andi Kleen wrote:
> 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?
Yes, thanks.
Marek
