(no subject)

[Andi Kleen]

This version addresses all the feedback so far (Thanks!).  The largest
change is support for using [[musttail]] in C23, not just C++.

-Andi

[PATCH v2 1/5] Improve must tail in RTL backend

[Andi Kleen]

- Give error messages for all causes of non sibling call generation
- Don't override choices of other non sibling call checks with
must tail. This causes ICEs. The must tail attribute now only
overrides flag_optimize_sibling_calls locally.
- Error out when tree-tailcall failed to mark a must-tail call
sibcall. In this case it doesn't know the true reason and only gives
a vague message (this could be improved, but it's already useful without
that) tree-tailcall usually fails without optimization, so must
adjust the existing must-tail plugin test to specify -O2.
---
 gcc/calls.cc                                  | 31 +++++++++++++------
 .../gcc.dg/plugin/must-tail-call-1.c          |  1 +
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/gcc/calls.cc b/gcc/calls.cc
index 01f447347437..3115807b7788 100644
--- a/gcc/calls.cc
+++ b/gcc/calls.cc
@@ -2650,7 +2650,9 @@ expand_call (tree exp, rtx target, int ignore)
   /* The type of the function being called.  */
   tree fntype;
   bool try_tail_call = CALL_EXPR_TAILCALL (exp);
-  bool must_tail_call = CALL_EXPR_MUST_TAIL_CALL (exp);
+  /* tree-tailcall decided not to do tail calls. Error for the musttail case.  */
+  if (!try_tail_call)
+      maybe_complain_about_tail_call (exp, "cannot tail-call: other reasons");
   int pass;
 
   /* Register in which non-BLKmode value will be returned,
@@ -3021,10 +3023,22 @@ expand_call (tree exp, rtx target, int ignore)
      pushed these optimizations into -O2.  Don't try if we're already
      expanding a call, as that means we're an argument.  Don't try if
      there's cleanups, as we know there's code to follow the call.  */
-  if (currently_expanding_call++ != 0
-      || (!flag_optimize_sibling_calls && !CALL_FROM_THUNK_P (exp))
-      || args_size.var
-      || dbg_cnt (tail_call) == false)
+  if (currently_expanding_call++ != 0)
+    {
+      maybe_complain_about_tail_call (exp, "cannot tail-call: inside another call");
+      try_tail_call = 0;
+    }
+  if (!flag_optimize_sibling_calls
+	&& !CALL_FROM_THUNK_P (exp)
+	&& !CALL_EXPR_MUST_TAIL_CALL (exp))
+    try_tail_call = 0;
+  if (args_size.var)
+    {
+      /* ??? correct message?  */
+      maybe_complain_about_tail_call (exp, "cannot tail-call: stack space needed");
+      try_tail_call = 0;
+    }
+  if (dbg_cnt (tail_call) == false)
     try_tail_call = 0;
 
   /* Workaround buggy C/C++ wrappers around Fortran routines with
@@ -3045,15 +3059,12 @@ expand_call (tree exp, rtx target, int ignore)
 	    if (MEM_P (*iter))
 	      {
 		try_tail_call = 0;
+		maybe_complain_about_tail_call (exp,
+				"cannot tail-call: hidden string length argument");
 		break;
 	      }
 	}
 
-  /* If the user has marked the function as requiring tail-call
-     optimization, attempt it.  */
-  if (must_tail_call)
-    try_tail_call = 1;
-
   /*  Rest of purposes for tail call optimizations to fail.  */
   if (try_tail_call)
     try_tail_call = can_implement_as_sibling_call_p (exp,
diff --git a/gcc/testsuite/gcc.dg/plugin/must-tail-call-1.c b/gcc/testsuite/gcc.dg/plugin/must-tail-call-1.c
index 3a6d4cceaba7..44af361e2925 100644
--- a/gcc/testsuite/gcc.dg/plugin/must-tail-call-1.c
+++ b/gcc/testsuite/gcc.dg/plugin/must-tail-call-1.c
@@ -1,4 +1,5 @@
 /* { dg-do compile { target tail_call } } */
+/* { dg-options "-O2" } */
 /* { dg-options "-fdelayed-branch" { target sparc*-*-* } } */
 
 extern void abort (void);
-- 
2.43.0

[PATCH v2 2/5] C++: Support clang compatible [[musttail]] (PR83324)

[Andi Kleen]

This patch implements a clang compatible [[musttail]] attribute for
returns.

musttail is useful as an alternative to computed goto for interpreters.
With computed goto the interpreter function usually ends up very big
which causes problems with register allocation and other per function
optimizations not scaling. With musttail the interpreter can be instead
written as a sequence of smaller functions that call each other. To
avoid unbounded stack growth this requires forcing a sibling call, which
this attribute does. It guarantees an error if the call cannot be tail
called which allows the programmer to fix it instead of risking a stack
overflow. Unlike computed goto it is also type-safe.

It turns out that David Malcolm had already implemented middle/backend
support for a musttail attribute back in 2016, but it wasn't exposed
to any frontend other than a special plugin.

This patch adds a [[gnu::musttail]] attribute for C++ that can be added
to return statements. The return statement must be a direct call
(it does not follow dependencies), which is similar to what clang
implements. It then uses the existing must tail infrastructure.

For compatibility it also detects clang::musttail

One problem is that tree-tailcall usually fails when optimization
is disabled, which implies the attribute only really works with
optimization on. But that seems to be a reasonable limitation.

Passes bootstrap and full test
---
 gcc/cp/cp-tree.h    |  4 ++--
 gcc/cp/parser.cc    | 28 +++++++++++++++++++++++-----
 gcc/cp/semantics.cc |  6 +++---
 gcc/cp/typeck.cc    | 20 ++++++++++++++++++--
 4 files changed, 46 insertions(+), 12 deletions(-)

diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index 60e6dafc5494..bed52e860a00 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7763,7 +7763,7 @@ extern void finish_while_stmt			(tree);
 extern tree begin_do_stmt			(void);
 extern void finish_do_body			(tree);
 extern void finish_do_stmt		(tree, tree, bool, tree, bool);
-extern tree finish_return_stmt			(tree);
+extern tree finish_return_stmt			(tree, bool = false);
 extern tree begin_for_scope			(tree *);
 extern tree begin_for_stmt			(tree, tree);
 extern void finish_init_stmt			(tree);
@@ -8275,7 +8275,7 @@ extern tree composite_pointer_type		(const op_location_t &,
 						 tsubst_flags_t);
 extern tree merge_types				(tree, tree);
 extern tree strip_array_domain			(tree);
-extern tree check_return_expr			(tree, bool *, bool *);
+extern tree check_return_expr			(tree, bool *, bool *, bool);
 extern tree spaceship_type			(tree, tsubst_flags_t = tf_warning_or_error);
 extern tree genericize_spaceship		(location_t, tree, tree, tree);
 extern tree cp_build_binary_op                  (const op_location_t &,
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 3748ccd49ff3..5a32804c0201 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -2462,7 +2462,7 @@ static tree cp_parser_perform_range_for_lookup
 static tree cp_parser_range_for_member_function
   (tree, tree);
 static tree cp_parser_jump_statement
-  (cp_parser *);
+  (cp_parser *, bool = false);
 static void cp_parser_declaration_statement
   (cp_parser *);
 
@@ -12719,9 +12719,27 @@ cp_parser_statement (cp_parser* parser, tree in_statement_expr,
 						     NULL_TREE, false);
 	  break;
 
+	case RID_RETURN:
+	  {
+	    bool musttail_p = false;
+	    std_attrs = process_stmt_hotness_attribute (std_attrs, attrs_loc);
+	    if (lookup_attribute ("", "musttail", std_attrs))
+	      {
+		musttail_p = true;
+		std_attrs = remove_attribute ("", "musttail", std_attrs);
+	      }
+	    // support this for compatibility
+	    if (lookup_attribute ("clang", "musttail", std_attrs))
+	      {
+		musttail_p = true;
+		std_attrs = remove_attribute ("clang", "musttail", std_attrs);
+	      }
+	    statement = cp_parser_jump_statement (parser, musttail_p);
+	  }
+	  break;
+
 	case RID_BREAK:
 	case RID_CONTINUE:
-	case RID_RETURN:
 	case RID_CO_RETURN:
 	case RID_GOTO:
 	  std_attrs = process_stmt_hotness_attribute (std_attrs, attrs_loc);
@@ -14767,7 +14785,7 @@ cp_parser_init_statement (cp_parser *parser, tree *decl)
   return false;
 }
 
-/* Parse a jump-statement.
+/* Parse a jump-statement. MUSTTAIL_P indicates a musttail attribute.
 
    jump-statement:
      break ;
@@ -14785,7 +14803,7 @@ cp_parser_init_statement (cp_parser *parser, tree *decl)
    Returns the new BREAK_STMT, CONTINUE_STMT, RETURN_EXPR, or GOTO_EXPR.  */
 
 static tree
-cp_parser_jump_statement (cp_parser* parser)
+cp_parser_jump_statement (cp_parser* parser, bool musttail_p)
 {
   tree statement = error_mark_node;
   cp_token *token;
@@ -14869,7 +14887,7 @@ cp_parser_jump_statement (cp_parser* parser)
 	else if (FNDECL_USED_AUTO (current_function_decl) && in_discarded_stmt)
 	  /* Don't deduce from a discarded return statement.  */;
 	else
-	  statement = finish_return_stmt (expr);
+	  statement = finish_return_stmt (expr, musttail_p);
 	/* Look for the final `;'.  */
 	cp_parser_require (parser, CPP_SEMICOLON, RT_SEMICOLON);
       }
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index 3299e2704465..a277f70ea0fd 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -1324,16 +1324,16 @@ finish_do_stmt (tree cond, tree do_stmt, bool ivdep, tree unroll,
 }
 
 /* Finish a return-statement.  The EXPRESSION returned, if any, is as
-   indicated.  */
+   indicated.  MUSTTAIL_P indicates a mustcall attribute.  */
 
 tree
-finish_return_stmt (tree expr)
+finish_return_stmt (tree expr, bool musttail_p)
 {
   tree r;
   bool no_warning;
   bool dangling;
 
-  expr = check_return_expr (expr, &no_warning, &dangling);
+  expr = check_return_expr (expr, &no_warning, &dangling, musttail_p);
 
   if (error_operand_p (expr)
       || (flag_openmp && !check_omp_return ()))
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index a15eda3f5f8c..8c116e3b4f4c 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -11028,10 +11028,12 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
    the DECL_RESULT for the function.  Set *NO_WARNING to true if
    code reaches end of non-void function warning shouldn't be issued
    on this RETURN_EXPR.  Set *DANGLING to true if code returns the
-   address of a local variable.  */
+   address of a local variable.  MUSTTAIL_P indicates a musttail
+   return.  */
 
 tree
-check_return_expr (tree retval, bool *no_warning, bool *dangling)
+check_return_expr (tree retval, bool *no_warning, bool *dangling,
+		   bool musttail_p)
 {
   tree result;
   /* The type actually returned by the function.  */
@@ -11045,6 +11047,20 @@ check_return_expr (tree retval, bool *no_warning, bool *dangling)
   *no_warning = false;
   *dangling = false;
 
+  if (musttail_p)
+    {
+      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 if (TREE_CODE (retval) != CALL_EXPR)
+	{
+	  error_at (loc, "cannot tail-call: return value must be a call");
+	  return error_mark_node;
+	}
+      else
+	CALL_EXPR_MUST_TAIL_CALL (retval) = 1;
+    }
+
   /* A `volatile' function is one that isn't supposed to return, ever.
      (This is a G++ extension, used to get better code for functions
      that call the `volatile' function.)  */
-- 
2.43.0

[PATCH v2 3/5] C: Implement musttail attribute for returns

[Andi Kleen]

Implement a C23 clang compatible musttail attribute similar to the earlier
C++ implementation in the C parser.
---
 gcc/c/c-parser.cc | 59 +++++++++++++++++++++++++++++++++++++----------
 gcc/c/c-tree.h    |  2 +-
 gcc/c/c-typeck.cc | 15 ++++++++++--
 3 files changed, 61 insertions(+), 15 deletions(-)

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;
-- 
2.43.0

Re: [PATCH v2 3/5] C: Implement musttail attribute for returns

[Joseph Myers]

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).

-- 
Joseph S. Myers
josmyers@redhat.com

Re: [PATCH v2 3/5] C: Implement musttail attribute for returns

[Andi Kleen]

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

Re: [PATCH v2 3/5] C: Implement musttail attribute for returns

[Joseph Myers]

[-- Attachment #1: Type: text/plain, Size: 1617 bytes --]

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.

-- 
Joseph S. Myers
josmyers@redhat.com

Re: [PATCH v2 3/5] C: Implement musttail attribute for returns

[Andi Kleen]

> > 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++;

Re: [PATCH v2 3/5] C: Implement musttail attribute for returns

[Joseph Myers]

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]".

-- 
Joseph S. Myers
josmyers@redhat.com

[PATCH v2 4/5] Add tests for C/C++ musttail attributes

[Andi Kleen]

Mostly adopted from the existing C musttail plugin tests.
---
 gcc/testsuite/c-c++-common/musttail1.c | 17 ++++++++++++
 gcc/testsuite/c-c++-common/musttail2.c | 36 ++++++++++++++++++++++++++
 gcc/testsuite/c-c++-common/musttail3.c | 31 ++++++++++++++++++++++
 gcc/testsuite/c-c++-common/musttail4.c | 19 ++++++++++++++
 4 files changed, 103 insertions(+)
 create mode 100644 gcc/testsuite/c-c++-common/musttail1.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail2.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail3.c
 create mode 100644 gcc/testsuite/c-c++-common/musttail4.c

diff --git a/gcc/testsuite/c-c++-common/musttail1.c b/gcc/testsuite/c-c++-common/musttail1.c
new file mode 100644
index 000000000000..476185e3ed4b
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail1.c
@@ -0,0 +1,17 @@
+/* { dg-do compile { target tail_call } } */
+/* { dg-options "-O2" } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+/* { dg-additional-options "-std=c23" { target c } } */
+/* { dg-additional-options "-fdelayed-branch" { target sparc*-*-* } } */
+
+int __attribute__((noinline,noclone))
+callee (int i)
+{
+  return i * i;
+}
+
+int __attribute__((noinline,noclone))
+caller (int i)
+{
+  [[gnu::musttail]] return callee (i + 1);
+}
diff --git a/gcc/testsuite/c-c++-common/musttail2.c b/gcc/testsuite/c-c++-common/musttail2.c
new file mode 100644
index 000000000000..28f2f68ef13d
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail2.c
@@ -0,0 +1,36 @@
+/* { dg-do compile { target tail_call } } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+/* { dg-additional-options "-std=c23" { target c } } */
+
+struct box { char field[256]; int i; };
+
+int __attribute__((noinline,noclone))
+test_2_callee (int i, struct box b)
+{
+  if (b.field[0])
+    return 5;
+  return i * i;
+}
+
+int __attribute__((noinline,noclone))
+test_2_caller (int i)
+{
+  struct box b;
+  [[gnu::musttail]] return test_2_callee (i + 1, b); /* { dg-error "cannot tail-call: " } */
+}
+
+extern void setjmp (void);
+void
+test_3 (void)
+{
+  [[gnu::musttail]] return setjmp (); /* { dg-error "cannot tail-call: " } */
+}
+
+typedef void (fn_ptr_t) (void);
+volatile fn_ptr_t fn_ptr;
+
+void
+test_5 (void)
+{
+  [[gnu::musttail]] return fn_ptr (); /* { dg-error "cannot tail-call: " } */
+}
diff --git a/gcc/testsuite/c-c++-common/musttail3.c b/gcc/testsuite/c-c++-common/musttail3.c
new file mode 100644
index 000000000000..fdbb292944ad
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail3.c
@@ -0,0 +1,31 @@
+/* { dg-do compile { target tail_call } } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+/* { dg-additional-options "-std=c23" { target c } } */
+
+extern int foo2 (int x, ...);
+
+struct str
+{
+  int a, b;
+};
+
+struct str
+cstruct (int x)
+{
+  if (x < 10)
+    [[clang::musttail]] return cstruct (x + 1);
+  return ((struct str){ x, 0 });
+}
+
+int
+foo (int x)
+{
+  if (x < 10)
+    [[clang::musttail]] return foo2 (x, 29);
+  if (x < 100)
+    {
+      int k = foo (x + 1);
+      [[clang::musttail]] return k;	/* { dg-error "cannot tail-call: " } */
+    }
+  return x;
+}
diff --git a/gcc/testsuite/c-c++-common/musttail4.c b/gcc/testsuite/c-c++-common/musttail4.c
new file mode 100644
index 000000000000..7bf44816f14a
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/musttail4.c
@@ -0,0 +1,19 @@
+/* { dg-do compile { target tail_call } } */
+/* { dg-additional-options "-std=c++11" { target c++ } } */
+/* { dg-additional-options "-std=c23" { target c } } */
+
+struct box { char field[64]; int i; };
+
+struct box __attribute__((noinline,noclone))
+returns_struct (int i)
+{
+  struct box b;
+  b.i = i * i;
+  return b;
+}
+
+int __attribute__((noinline,noclone))
+test_1 (int i)
+{
+  [[gnu::musttail]] return returns_struct (i * 5).i; /* { dg-error "cannot tail-call: " } */
+}
-- 
2.43.0

[PATCH v2 5/5] Add documentation for musttail attribute

[Andi Kleen]

---
 gcc/doc/extend.texi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 0bc586d120e7..c68d32bed8de 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -9867,6 +9867,22 @@ foo (int x, int y)
 @code{y} is not actually incremented and the compiler can but does not
 have to optimize it to just @code{return 42 + 42;}.
 
+@cindex @code{musttail} statement attribute
+@item musttail
+
+The @code{gnu::musttail} or @code{clang::hottail} attribute
+can be applied to a return statement that returns the value
+of a call to indicate that the call must be a tail call
+that does not allocate extra stack space.
+
+@smallexample
+[[gnu::musttail]] return foo();
+@end smallexample
+
+If the compiler cannot generate a tail call it will generate
+an error. Tail calls generally require enabling optimization.
+On some targets they may not be supported.
+
 @end table
 
 @node Attribute Syntax
-- 
2.43.0

Re: [PATCH v2 5/5] Add documentation for musttail attribute

[rep.dot.nop]

On 24 January 2024 20:30:45 CET, Andi Kleen <ak@linux.intel.com> wrote:
>---
> gcc/doc/extend.texi | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
>diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
>index 0bc586d120e7..c68d32bed8de 100644
>--- a/gcc/doc/extend.texi
>+++ b/gcc/doc/extend.texi
>@@ -9867,6 +9867,22 @@ foo (int x, int y)
> @code{y} is not actually incremented and the compiler can but does not
> have to optimize it to just @code{return 42 + 42;}.
> 
>+@cindex @code{musttail} statement attribute
>+@item musttail
>+
>+The @code{gnu::musttail} or @code{clang::hottail} attribute

AFAICS this patchset does not handle hottail ?

thanks,

>+can be applied to a return statement that returns the value
>+of a call to indicate that the call must be a tail call
>+that does not allocate extra stack space.
>+
>+@smallexample
>+[[gnu::musttail]] return foo();
>+@end smallexample
>+
>+If the compiler cannot generate a tail call it will generate
>+an error. Tail calls generally require enabling optimization.
>+On some targets they may not be supported.
>+
> @end table
> 
> @node Attribute Syntax

Re: [PATCH v2 5/5] Add documentation for musttail attribute

[Andi Kleen]

On Thu, Jan 25, 2024 at 07:51:21AM +0100, rep.dot.nop@gmail.com wrote:
> On 24 January 2024 20:30:45 CET, Andi Kleen <ak@linux.intel.com> wrote:
> >---
> > gcc/doc/extend.texi | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> >diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> >index 0bc586d120e7..c68d32bed8de 100644
> >--- a/gcc/doc/extend.texi
> >+++ b/gcc/doc/extend.texi
> >@@ -9867,6 +9867,22 @@ foo (int x, int y)
> > @code{y} is not actually incremented and the compiler can but does not
> > have to optimize it to just @code{return 42 + 42;}.
> > 
> >+@cindex @code{musttail} statement attribute
> >+@item musttail
> >+
> >+The @code{gnu::musttail} or @code{clang::hottail} attribute
> 
> AFAICS this patchset does not handle hottail ?

Thanks. Fixed the typo.
-Andi