Fix crash with -fdump-ada-spec
diff mbox series

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

Comments

Joseph Myers Aug. 21, 2019, 11:52 a.m. UTC | #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.
Eric Botcazou Aug. 21, 2019, 9:19 p.m. UTC | #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.
Joseph Myers Aug. 21, 2019, 9:22 p.m. UTC | #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.

Patch
diff mbox series

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);
     }
 }