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

Message ID 20240124193134.622934-4-ak@linux.intel.com
State Unresolved
Headers
Series [v2,1/5] Improve must tail in RTL backend |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Andi Kleen Jan. 24, 2024, 7:30 p.m. UTC
  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(-)
  

Comments

Joseph Myers Jan. 25, 2024, 8:08 p.m. UTC | #1
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).
  
Andi Kleen Jan. 25, 2024, 8:39 p.m. UTC | #2
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
  
Joseph Myers Jan. 26, 2024, 8:48 a.m. UTC | #3
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.
  
Andi Kleen Jan. 26, 2024, 9:13 a.m. UTC | #4
> > 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++;
  
Joseph Myers Jan. 26, 2024, 9:35 a.m. UTC | #5
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]".
  

Patch

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;