Fix crash with -fdump-ada-spec

Message ID 2508992.5cBtdV5qiJ@arcturus.home
State New
Headers show
Series
  • Fix crash with -fdump-ada-spec
Related show

Commit Message

Eric Botcazou Aug. 21, 2019, 9:54 a.m.
This fixes a crash of the C compiler when -fdump-ada-spec is passed for:

extern void (*signal (int __sig, void (*__handler)(int)))(int);

It appears that the C parser is somewhat confused by this declaration and 
builds a FUNCTION_DECL whose type has got 2 arguments (TYPE_ARG_TYPES list) 
but which itself has got only 1 argument (DECL_ARGUMENTS list) corresponding 
to the anonymous (int) at the end.  The C++ parser doesn't have the issue.

Joseph, is that a known issue of the C parser?

The attached patch changes -fdump-ada-spec to using TYPE_ARG_TYPES for it.


2019-08-21  Eric Botcazou  <ebotcazou@adacore.com>

c-family/
	* c-ada-spec.c (dump_ada_function_declaration): Be prepared for broken
	function declarations where arguments are missing.  Rename variables.


2019-08-21  Eric Botcazou  <ebotcazou@adacore.com>

	* c-c++-common/dump-ada-spec-15.c: New test.

-- 
Eric Botcazou

Comments

Joseph Myers Aug. 21, 2019, 11:52 a.m. | #1
On Wed, 21 Aug 2019, Eric Botcazou wrote:

> This fixes a crash of the C compiler when -fdump-ada-spec is passed for:

> 

> extern void (*signal (int __sig, void (*__handler)(int)))(int);

> 

> It appears that the C parser is somewhat confused by this declaration and 

> builds a FUNCTION_DECL whose type has got 2 arguments (TYPE_ARG_TYPES list) 

> but which itself has got only 1 argument (DECL_ARGUMENTS list) corresponding 

> to the anonymous (int) at the end.  The C++ parser doesn't have the issue.

> 

> Joseph, is that a known issue of the C parser?


The problem is that the logic in c_parser_declaration_or_fndef for setting 
DECL_ARGUMENTS (not previously meaningful in non-definition declarations 
at all), as introduced by

r253411 | dmalcolm | 2017-10-04 14:10:59 +0000 (Wed, 04 Oct 2017) | 85 lines

C: underline parameters in mismatching function calls

(and subsequently changed by r268574, but that change isn't relevant here) 
is incorrect.  Rather than checking if the outermost declarator is 
cdk_function and using its parameters if so, it's necessary to check if 
the *innermost declarator that isn't cdk_id or cdk_attrs* is cdk_function 
and use its parameters if so, as that's what actually determines if the 
declarator for the entity being declared has the form of a function 
declarator (see how grokdeclarator determines funcdef_syntax).  In your 
example, the outermost declarator is the one for the target type of the 
pointer that is the return type.

-- 
Joseph S. Myers
joseph@codesourcery.com
Eric Botcazou Aug. 21, 2019, 9:19 p.m. | #2
> The problem is that the logic in c_parser_declaration_or_fndef for setting

> DECL_ARGUMENTS (not previously meaningful in non-definition declarations

> at all), as introduced by

> 

> r253411 | dmalcolm | 2017-10-04 14:10:59 +0000 (Wed, 04 Oct 2017) | 85 lines

> 

> C: underline parameters in mismatching function calls

> 

> (and subsequently changed by r268574, but that change isn't relevant here)

> is incorrect.


As a matter of fact, this logic helped -fdump-ada-spec too by making the name 
of the parameters available in non-definition declarations.

> Rather than checking if the outermost declarator is

> cdk_function and using its parameters if so, it's necessary to check if

> the *innermost declarator that isn't cdk_id or cdk_attrs* is cdk_function

> and use its parameters if so, as that's what actually determines if the

> declarator for the entity being declared has the form of a function

> declarator (see how grokdeclarator determines funcdef_syntax).


Thanks for the hint.  Tentative patch attached based on it, which certainly 
makes -fdump-ada-spec happy, as witnessed by the dump-ada-spec-15.c change.

Is it something that you would approve for mainline?


c/
	* c-parser.c (c_parser_declaration_or_fndef): Set DECL_ARGUMENTS of a
	FUNCTION_DECL to the right value in the presence of nested declarators.

testsuite/
	* c-c++-common/dump-ada-spec-15.c: Check that the parameters are named.

-- 
Eric Botcazou
Index: c/c-parser.c
===================================================================
--- c/c-parser.c	(revision 274744)
+++ c/c-parser.c	(working copy)
@@ -2161,10 +2161,40 @@ c_parser_declaration_or_fndef (c_parser *parser, b
 					    all_prefix_attrs));
 	      if (d
 		  && TREE_CODE (d) == FUNCTION_DECL
-		  && declarator->kind == cdk_function
 		  && DECL_ARGUMENTS (d) == NULL_TREE
 		  && DECL_INITIAL (d) == NULL_TREE)
-		DECL_ARGUMENTS (d) = declarator->u.arg_info->parms;
+		{
+		  /* Check if the innermost declarator that isn't cdk_id or
+		     cdk_attrs is cdk_function.  */
+		  const struct c_declarator *decl = declarator;
+		  const struct c_declarator *last_non_id_attrs = NULL;
+
+		  while (decl)
+		    switch (decl->kind)
+		      {
+		      case cdk_array:
+		      case cdk_function:
+		      case cdk_pointer:
+			last_non_id_attrs = decl;
+			decl = decl->declarator;
+			break;
+
+		      case cdk_attrs:
+			decl = decl->declarator;
+			break;
+
+		      case cdk_id:
+			decl = 0;
+			break;
+
+		      default:
+			gcc_unreachable ();
+		      }
+		  /* If so, use its parameters for the function.  */
+		  if (last_non_id_attrs
+		      && last_non_id_attrs->kind == cdk_function)
+		    DECL_ARGUMENTS (d) = last_non_id_attrs->u.arg_info->parms;
+		}
 	      if (omp_declare_simd_clauses.exists ())
 		{
 		  tree parms = NULL_TREE;
Index: testsuite/c-c++-common/dump-ada-spec-15.c
===================================================================
--- testsuite/c-c++-common/dump-ada-spec-15.c	(revision 274794)
+++ testsuite/c-c++-common/dump-ada-spec-15.c	(working copy)
@@ -3,4 +3,6 @@
 
 extern void (*signal (int __sig, void (*__handler)(int)))(int);
 
+/* { dg-final { scan-ada-spec "uu_sig" } } */
+/* { dg-final { scan-ada-spec "uu_handler" } } */
 /* { dg-final { cleanup-ada-spec } } */
Joseph Myers Aug. 21, 2019, 9:22 p.m. | #3
On Wed, 21 Aug 2019, Eric Botcazou wrote:

> > Rather than checking if the outermost declarator is

> > cdk_function and using its parameters if so, it's necessary to check if

> > the *innermost declarator that isn't cdk_id or cdk_attrs* is cdk_function

> > and use its parameters if so, as that's what actually determines if the

> > declarator for the entity being declared has the form of a function

> > declarator (see how grokdeclarator determines funcdef_syntax).

> 

> Thanks for the hint.  Tentative patch attached based on it, which certainly 

> makes -fdump-ada-spec happy, as witnessed by the dump-ada-spec-15.c change.

> 

> Is it something that you would approve for mainline?


This patch is OK.

-- 
Joseph S. Myers
joseph@codesourcery.com

Patch

Index: c-family/c-ada-spec.c
===================================================================
--- c-family/c-ada-spec.c	(revision 274744)
+++ c-family/c-ada-spec.c	(working copy)
@@ -1589,14 +1589,13 @@  dump_ada_function_declaration (pretty_printer *buf
 			       bool is_method, bool is_constructor,
 			       bool is_destructor, int spc)
 {
-  tree arg;
-  const tree node = TREE_TYPE (func);
+  tree type = TREE_TYPE (func);
+  tree arg = TYPE_ARG_TYPES (type);
+  tree t;
   char buf[17];
-  int num = 0, num_args = 0, have_args = true, have_ellipsis = false;
+  int num, num_args = 0, have_args = true, have_ellipsis = false;
 
   /* Compute number of arguments.  */
-  arg = TYPE_ARG_TYPES (node);
-
   if (arg)
     {
       while (TREE_CHAIN (arg) && arg != error_mark_node)
@@ -1627,25 +1626,29 @@  dump_ada_function_declaration (pretty_printer *buf
       pp_left_paren (buffer);
     }
 
+  /* For a function, see if we have the corresponding arguments.  */
   if (TREE_CODE (func) == FUNCTION_DECL)
-    arg = DECL_ARGUMENTS (func);
+    {
+      arg = DECL_ARGUMENTS (func);
+      for (t = arg, num = 0; t; t = DECL_CHAIN (t))
+	num++;
+      if (num < num_args)
+	arg = NULL_TREE;
+    }
   else
     arg = NULL_TREE;
 
-  if (arg == NULL_TREE)
+  /* Otherwise, only print the types.  */
+  if (!arg)
     {
       have_args = false;
-      arg = TYPE_ARG_TYPES (node);
-
-      if (arg && TREE_CODE (TREE_VALUE (arg)) == VOID_TYPE)
-	arg = NULL_TREE;
+      arg = TYPE_ARG_TYPES (type);
     }
 
   if (is_constructor)
     arg = TREE_CHAIN (arg);
 
-  /* Print the argument names (if available) & types.  */
-
+  /* Print the argument names (if available) and types.  */
   for (num = 1; num <= num_args; num++)
     {
       if (have_args)
@@ -1663,13 +1666,13 @@  dump_ada_function_declaration (pretty_printer *buf
 	      pp_string (buffer, buf);
 	    }
 
-	  dump_ada_node (buffer, TREE_TYPE (arg), node, spc, false, true);
+	  dump_ada_node (buffer, TREE_TYPE (arg), type, spc, false, true);
 	}
       else
 	{
 	  sprintf (buf, "arg%d : ", num);
 	  pp_string (buffer, buf);
-	  dump_ada_node (buffer, TREE_VALUE (arg), node, spc, false, true);
+	  dump_ada_node (buffer, TREE_VALUE (arg), type, spc, false, true);
 	}
 
       /* If the type is a pointer to a tagged type, we need to differentiate
@@ -1707,11 +1710,11 @@  dump_ada_function_declaration (pretty_printer *buf
   if (num_args > 0)
     pp_right_paren (buffer);
 
-  if (is_constructor || !VOID_TYPE_P (TREE_TYPE (node)))
+  if (is_constructor || !VOID_TYPE_P (TREE_TYPE (type)))
     {
       pp_string (buffer, " return ");
-      tree type = is_constructor ? DECL_CONTEXT (func) : TREE_TYPE (node);
-      dump_ada_node (buffer, type, type, spc, false, true);
+      tree rtype = is_constructor ? DECL_CONTEXT (func) : TREE_TYPE (type);
+      dump_ada_node (buffer, rtype, rtype, spc, false, true);
     }
 }