Patchwork [C++] PR 38634 (Take 2)

login
register
mail settings
Submitter Paolo Carlini
Date July 3, 2013, 10:17 p.m.
Message ID <51D4A302.6010608@oracle.com>
Download mbox | patch
Permalink /patch/256767/
State New
Headers show

Comments

Paolo Carlini - July 3, 2013, 10:17 p.m.
Hi,

today I was going through some pending issues, and decided to rework my 
first try at fixing this very old ICE on invalid:

     http://gcc.gnu.org/ml/gcc-patches/2013-06/msg00480.html

In the first try, bailing out early in case of error without undoing the 
committed changes to decl1 made me a little nervous. The below variant 
works at first on newdecl and only if push_template_decl goes well, 
copies it back to decl1. Still passes testing on x86_64-linux.

Thanks,
Paolo.

//////////////////////
/cp
2013-07-04  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/38634
	* decl.c (start_preparsed_function): Return a bool, false if
	push_template_decl fails.
	(start_function): Adjust.
	* cp-tree.h: Update.

/testsuite
2013-07-04  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/38634
	* g++.dg/template/crash116.C: New.
Jason Merrill - July 4, 2013, 3:58 p.m.
On 07/03/2013 03:17 PM, Paolo Carlini wrote:
> In the first try, bailing out early in case of error without undoing the
> committed changes to decl1 made me a little nervous. The below variant
> works at first on newdecl and only if push_template_decl goes well,
> copies it back to decl1. Still passes testing on x86_64-linux.

Since newdecl points to the same tree node as decl1, I don't see what 
difference this would make.  And I'm not too worried about making 
changes to a decl that's erroneous anyway.

Your earlier patch, plus removing the FIXME, is OK.

Jason
Paolo Carlini - July 4, 2013, 6:08 p.m.
Hi

Jason Merrill <jason@redhat.com> ha scritto:

>Since newdecl points to the same tree node as decl1, I don't see what
>difference this would make.

Yeah ;) I think it would if instead of tree newdecl = decl1 I had copy_node (decl1), right? But I understand it's normally not needed...

 And I'm not too worried about making
>changes to a decl that's erroneous anyway.

I see.

>
>Your earlier patch, plus removing the FIXME, is OK.

Will do, thanks!

Paolo

Patch

Index: cp/cp-tree.h
===================================================================
--- cp/cp-tree.h	(revision 200645)
+++ cp/cp-tree.h	(working copy)
@@ -5206,8 +5206,9 @@  extern void finish_enum_value_list		(tree);
 extern void finish_enum				(tree);
 extern void build_enumerator			(tree, tree, tree, location_t);
 extern tree lookup_enumerator			(tree, tree);
-extern void start_preparsed_function		(tree, tree, int);
-extern int start_function			(cp_decl_specifier_seq *, const cp_declarator *, tree);
+extern bool start_preparsed_function		(tree, tree, int);
+extern bool start_function			(cp_decl_specifier_seq *,
+						 const cp_declarator *, tree);
 extern tree begin_function_body			(void);
 extern void finish_function_body		(tree);
 extern tree outer_curly_brace_block		(tree);
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 200645)
+++ cp/decl.c	(working copy)
@@ -12993,9 +12993,10 @@  check_function_type (tree decl, tree current_funct
    error_mark_node if the function has never been defined, or
    a BLOCK if the function has been defined somewhere.  */
 
-void
+bool
 start_preparsed_function (tree decl1, tree attrs, int flags)
 {
+  tree newdecl = decl1;
   tree ctype = NULL_TREE;
   tree fntype;
   tree restype;
@@ -13003,22 +13004,22 @@  start_preparsed_function (tree decl1, tree attrs,
   cp_binding_level *bl;
   tree current_function_parms;
   struct c_fileinfo *finfo
-    = get_fileinfo (LOCATION_FILE (DECL_SOURCE_LOCATION (decl1)));
+    = get_fileinfo (LOCATION_FILE (DECL_SOURCE_LOCATION (newdecl)));
   bool honor_interface;
 
   /* Sanity check.  */
   gcc_assert (VOID_TYPE_P (TREE_VALUE (void_list_node)));
   gcc_assert (TREE_CHAIN (void_list_node) == NULL_TREE);
 
-  fntype = TREE_TYPE (decl1);
+  fntype = TREE_TYPE (newdecl);
   if (TREE_CODE (fntype) == METHOD_TYPE)
     ctype = TYPE_METHOD_BASETYPE (fntype);
 
   /* ISO C++ 11.4/5.  A friend function defined in a class is in
      the (lexical) scope of the class in which it is defined.  */
-  if (!ctype && DECL_FRIEND_P (decl1))
+  if (!ctype && DECL_FRIEND_P (newdecl))
     {
-      ctype = DECL_FRIEND_CONTEXT (decl1);
+      ctype = DECL_FRIEND_CONTEXT (newdecl);
 
       /* CTYPE could be null here if we're dealing with a template;
 	 for example, `inline friend float foo()' inside a template
@@ -13029,60 +13030,61 @@  start_preparsed_function (tree decl1, tree attrs,
 	doing_friend = 1;
     }
 
-  if (DECL_DECLARED_INLINE_P (decl1)
+  if (DECL_DECLARED_INLINE_P (newdecl)
       && lookup_attribute ("noinline", attrs))
-    warning (0, "inline function %q+D given attribute noinline", decl1);
+    warning (0, "inline function %q+D given attribute noinline", newdecl);
 
   /* Handle gnu_inline attribute.  */
-  if (GNU_INLINE_P (decl1))
+  if (GNU_INLINE_P (newdecl))
     {
-      DECL_EXTERNAL (decl1) = 1;
-      DECL_NOT_REALLY_EXTERN (decl1) = 0;
-      DECL_INTERFACE_KNOWN (decl1) = 1;
-      DECL_DISREGARD_INLINE_LIMITS (decl1) = 1;
+      DECL_EXTERNAL (newdecl) = 1;
+      DECL_NOT_REALLY_EXTERN (newdecl) = 0;
+      DECL_INTERFACE_KNOWN (newdecl) = 1;
+      DECL_DISREGARD_INLINE_LIMITS (newdecl) = 1;
     }
 
-  if (DECL_MAYBE_IN_CHARGE_CONSTRUCTOR_P (decl1))
+  if (DECL_MAYBE_IN_CHARGE_CONSTRUCTOR_P (newdecl))
     /* This is a constructor, we must ensure that any default args
        introduced by this definition are propagated to the clones
        now. The clones are used directly in overload resolution.  */
-    adjust_clone_args (decl1);
+    adjust_clone_args (newdecl);
 
   /* Sometimes we don't notice that a function is a static member, and
      build a METHOD_TYPE for it.  Fix that up now.  */
-  gcc_assert (!(ctype != NULL_TREE && DECL_STATIC_FUNCTION_P (decl1)
-		&& TREE_CODE (TREE_TYPE (decl1)) == METHOD_TYPE));
+  gcc_assert (!(ctype != NULL_TREE && DECL_STATIC_FUNCTION_P (newdecl)
+		&& TREE_CODE (TREE_TYPE (newdecl)) == METHOD_TYPE));
 
   /* Set up current_class_type, and enter the scope of the class, if
      appropriate.  */
   if (ctype)
     push_nested_class (ctype);
-  else if (DECL_STATIC_FUNCTION_P (decl1))
-    push_nested_class (DECL_CONTEXT (decl1));
+  else if (DECL_STATIC_FUNCTION_P (newdecl))
+    push_nested_class (DECL_CONTEXT (newdecl));
 
   /* Now that we have entered the scope of the class, we must restore
-     the bindings for any template parameters surrounding DECL1, if it
+     the bindings for any template parameters surrounding NEWDECL, if it
      is an inline member template.  (Order is important; consider the
      case where a template parameter has the same name as a field of
      the class.)  It is not until after this point that
      PROCESSING_TEMPLATE_DECL is guaranteed to be set up correctly.  */
   if (flags & SF_INCLASS_INLINE)
-    maybe_begin_member_template_processing (decl1);
+    maybe_begin_member_template_processing (newdecl);
 
   /* Effective C++ rule 15.  */
   if (warn_ecpp
-      && DECL_OVERLOADED_OPERATOR_P (decl1) == NOP_EXPR
+      && DECL_OVERLOADED_OPERATOR_P (newdecl) == NOP_EXPR
       && VOID_TYPE_P (TREE_TYPE (fntype)))
-    warning (OPT_Weffc__, "%<operator=%> should return a reference to %<*this%>");
+    warning (OPT_Weffc__,
+	     "%<operator=%> should return a reference to %<*this%>");
 
   /* Make the init_value nonzero so pushdecl knows this is not tentative.
      error_mark_node is replaced below (in poplevel) with the BLOCK.  */
-  if (!DECL_INITIAL (decl1))
-    DECL_INITIAL (decl1) = error_mark_node;
+  if (!DECL_INITIAL (newdecl))
+    DECL_INITIAL (newdecl) = error_mark_node;
 
   /* This function exists in static storage.
      (This does not mean `static' in the C sense!)  */
-  TREE_STATIC (decl1) = 1;
+  TREE_STATIC (newdecl) = 1;
 
   /* We must call push_template_decl after current_class_type is set
      up.  (If we are processing inline definitions after exiting a
@@ -13090,10 +13092,14 @@  start_preparsed_function (tree decl1, tree attrs,
      by push_nested_class.)  */
   if (processing_template_decl)
     {
-      /* FIXME: Handle error_mark_node more gracefully.  */
-      tree newdecl1 = push_template_decl (decl1);
-      if (newdecl1 != error_mark_node)
-	decl1 = newdecl1;
+      newdecl = push_template_decl (newdecl);
+      if (newdecl == error_mark_node)
+	{
+	  if (ctype || DECL_STATIC_FUNCTION_P (decl1))
+	    pop_nested_class ();
+	  return false;
+	}
+      decl1 = newdecl;
     }
 
   /* We are now in the scope of the function being defined.  */
@@ -13204,7 +13210,7 @@  start_preparsed_function (tree decl1, tree attrs,
   /* This function may already have been parsed, in which case just
      return; our caller will skip over the body without parsing.  */
   if (DECL_INITIAL (decl1) != error_mark_node)
-    return;
+    return true;
 
   /* Initialize RTL machinery.  We cannot do this until
      CURRENT_FUNCTION_DECL and DECL_RESULT are set up.  We do this
@@ -13366,17 +13372,19 @@  start_preparsed_function (tree decl1, tree attrs,
   start_fname_decls ();
 
   store_parm_decls (current_function_parms);
+
+  return true;
 }
 
 
 /* Like start_preparsed_function, except that instead of a
    FUNCTION_DECL, this function takes DECLSPECS and DECLARATOR.
 
-   Returns 1 on success.  If the DECLARATOR is not suitable for a function
-   (it defines a datum instead), we return 0, which tells
-   yyparse to report a parse error.  */
+   Returns true on success.  If the DECLARATOR is not suitable
+   for a function, we return false, which tells the parser to
+   skip the entire function.  */
 
-int
+bool
 start_function (cp_decl_specifier_seq *declspecs,
 		const cp_declarator *declarator,
 		tree attrs)
@@ -13385,13 +13393,13 @@  start_function (cp_decl_specifier_seq *declspecs,
 
   decl1 = grokdeclarator (declarator, declspecs, FUNCDEF, 1, &attrs);
   if (decl1 == error_mark_node)
-    return 0;
+    return false;
   /* If the declarator is not suitable for a function definition,
      cause a syntax error.  */
   if (decl1 == NULL_TREE || TREE_CODE (decl1) != FUNCTION_DECL)
     {
       error ("invalid function declaration");
-      return 0;
+      return false;
     }
 
   if (DECL_MAIN_P (decl1))
@@ -13400,9 +13408,7 @@  start_function (cp_decl_specifier_seq *declspecs,
     gcc_assert (same_type_p (TREE_TYPE (TREE_TYPE (decl1)),
 			     integer_type_node));
 
-  start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT);
-
-  return 1;
+  return start_preparsed_function (decl1, attrs, /*flags=*/SF_DEFAULT);
 }
 
 /* Returns true iff an EH_SPEC_BLOCK should be created in the body of