Patchwork Go patch committed: Fix bug converting unnamed types

login
register
mail settings
Submitter Ian Taylor
Date Dec. 1, 2012, 12:15 a.m.
Message ID <mcrfw3qiq4x.fsf@google.com>
Download mbox | patch
Permalink /patch/203087/
State New
Headers show

Comments

Ian Taylor - Dec. 1, 2012, 12:15 a.m.
This patch to the Go frontend fixes a bug converting unnamed types to
GIMPLE.  It's important to convert all identical unnamed Go types to the
same GIMPLE type, to avoid middle-end errors in assignments and type
conversions.  This is done via a hash table.  Unfortunately
Type::get_backend_placeholder was not using the hash table, so it was
possible to get two different placeholder types for two identical
unnamed Go types, leading in some cases to getting two different GIMPLE
types.  This would then cause an ICE if the two types appeared in an
assignment of some sort.

This patch fixes the problem by recording placeholder information in the
hash table, so that get_backend_placeholder can use it.  Bootstrapped
and ran Go testsuite on x86_64-unknown-linux-gnu.  Committed to mainline
and 4.7 branch.

Ian

Patch

diff -r b7d0a78bc2df go/types.cc
--- a/go/types.cc	Thu Nov 29 23:01:48 2012 -0800
+++ b/go/types.cc	Fri Nov 30 15:36:47 2012 -0800
@@ -45,8 +45,7 @@ 
 // Class Type.
 
 Type::Type(Type_classification classification)
-  : classification_(classification), btype_is_placeholder_(false),
-    btype_(NULL), type_descriptor_var_(NULL)
+  : classification_(classification), btype_(NULL), type_descriptor_var_(NULL)
 {
 }
 
@@ -910,11 +909,7 @@ 
 Type::get_backend(Gogo* gogo)
 {
   if (this->btype_ != NULL)
-    {
-      if (this->btype_is_placeholder_ && gogo->named_types_are_converted())
-	this->finish_backend(gogo);
-      return this->btype_;
-    }
+    return this->btype_;
 
   if (this->forward_declaration_type() != NULL
       || this->named_type() != NULL)
@@ -928,20 +923,36 @@ 
   // that.  There is no need to use the hash table for named types, as
   // named types are only identical to themselves.
 
-  std::pair<Type*, Btype*> val(this, NULL);
+  std::pair<Type*, Type_btype_entry> val;
+  val.first = this;
+  val.second.btype = NULL;
+  val.second.is_placeholder = false;
   std::pair<Type_btypes::iterator, bool> ins =
     Type::type_btypes.insert(val);
-  if (!ins.second && ins.first->second != NULL)
-    {
-      if (gogo != NULL && gogo->named_types_are_converted())
-	this->btype_ = ins.first->second;
-      return ins.first->second;
+  if (!ins.second && ins.first->second.btype != NULL)
+    {
+      // Note that GOGO can be NULL here, but only when the GCC
+      // middle-end is asking for a frontend type.  That will only
+      // happen for simple types, which should never require
+      // placeholders.
+      if (!ins.first->second.is_placeholder)
+	this->btype_ = ins.first->second.btype;
+      else if (gogo->named_types_are_converted())
+	{
+	  this->finish_backend(gogo, ins.first->second.btype);
+	  ins.first->second.is_placeholder = false;
+	}
+
+      return ins.first->second.btype;
     }
 
   Btype* bt = this->get_btype_without_hash(gogo);
 
-  if (ins.first->second == NULL)
-    ins.first->second = bt;
+  if (ins.first->second.btype == NULL)
+    {
+      ins.first->second.btype = bt;
+      ins.first->second.is_placeholder = false;
+    }
   else
     {
       // We have already created a backend representation for this
@@ -949,10 +960,9 @@ 
       // a named type which in turns uses an identical unnamed type.
       // Use the tree we created earlier and ignore the one we just
       // built.
-      bt = ins.first->second;
-      if (gogo == NULL || !gogo->named_types_are_converted())
-	return bt;
-      this->btype_ = bt;
+      if (this->btype_ == bt)
+	this->btype_ = ins.first->second.btype;
+      bt = ins.first->second.btype;
     }
 
   return bt;
@@ -1019,6 +1029,37 @@ 
       // These are simple types that can just be created directly.
       return this->get_backend(gogo);
 
+    case TYPE_MAP:
+    case TYPE_CHANNEL:
+      // All maps and channels have the same backend representation.
+      return this->get_backend(gogo);
+
+    case TYPE_NAMED:
+    case TYPE_FORWARD:
+      // Named types keep track of their own dependencies and manage
+      // their own placeholders.
+      return this->get_backend(gogo);
+
+    case TYPE_INTERFACE:
+      if (this->interface_type()->is_empty())
+	return Interface_type::get_backend_empty_interface_type(gogo);
+      break;
+
+    default:
+      break;
+    }
+
+  std::pair<Type*, Type_btype_entry> val;
+  val.first = this;
+  val.second.btype = NULL;
+  val.second.is_placeholder = false;
+  std::pair<Type_btypes::iterator, bool> ins =
+    Type::type_btypes.insert(val);
+  if (!ins.second && ins.first->second.btype != NULL)
+    return ins.first->second.btype;
+
+  switch (this->classification_)
+    {
     case TYPE_FUNCTION:
       {
 	Location loc = this->function_type()->location();
@@ -1061,37 +1102,36 @@ 
 	}
       break;
 	
-    case TYPE_MAP:
-    case TYPE_CHANNEL:
-      // All maps and channels have the same backend representation.
-      return this->get_backend(gogo);
-
     case TYPE_INTERFACE:
-      if (this->interface_type()->is_empty())
-	return Interface_type::get_backend_empty_interface_type(gogo);
-      else
-	{
-	  std::vector<Backend::Btyped_identifier> bfields;
-	  get_backend_interface_fields(gogo, this->interface_type(), true,
-				       &bfields);
-	  bt = gogo->backend()->struct_type(bfields);
-	}
+      {
+	go_assert(!this->interface_type()->is_empty());
+	std::vector<Backend::Btyped_identifier> bfields;
+	get_backend_interface_fields(gogo, this->interface_type(), true,
+				     &bfields);
+	bt = gogo->backend()->struct_type(bfields);
+      }
       break;
 
-    case TYPE_NAMED:
-    case TYPE_FORWARD:
-      // Named types keep track of their own dependencies and manage
-      // their own placeholders.
-      return this->get_backend(gogo);
-
     case TYPE_SINK:
     case TYPE_CALL_MULTIPLE_RESULT:
+      /* Note that various classifications were handled in the earlier
+	 switch.  */
     default:
       go_unreachable();
     }
 
-  this->btype_ = bt;
-  this->btype_is_placeholder_ = true;
+  if (ins.first->second.btype == NULL)
+    {
+      ins.first->second.btype = bt;
+      ins.first->second.is_placeholder = true;
+    }
+  else
+    {
+      // A placeholder for this type got created along the way.  Use
+      // that one and ignore the one we just built.
+      bt = ins.first->second.btype;
+    }
+
   return bt;
 }
 
@@ -1099,12 +1139,8 @@ 
 // using a placeholder type.
 
 void
-Type::finish_backend(Gogo* gogo)
-{
-  go_assert(this->btype_ != NULL);
-  if (!this->btype_is_placeholder_)
-    return;
-
+Type::finish_backend(Gogo* gogo, Btype *placeholder)
+{
   switch (this->classification_)
     {
     case TYPE_ERROR:
@@ -1120,7 +1156,7 @@ 
     case TYPE_FUNCTION:
       {
 	Btype* bt = this->do_get_backend(gogo);
-	if (!gogo->backend()->set_placeholder_function_type(this->btype_, bt))
+	if (!gogo->backend()->set_placeholder_function_type(placeholder, bt))
 	  go_assert(saw_errors());
       }
       break;
@@ -1128,7 +1164,7 @@ 
     case TYPE_POINTER:
       {
 	Btype* bt = this->do_get_backend(gogo);
-	if (!gogo->backend()->set_placeholder_pointer_type(this->btype_, bt))
+	if (!gogo->backend()->set_placeholder_pointer_type(placeholder, bt))
 	  go_assert(saw_errors());
       }
       break;
@@ -1165,7 +1201,7 @@ 
       go_unreachable();
     }
 
-  this->btype_is_placeholder_ = false;
+  this->btype_ = placeholder;
 }
 
 // Return a pointer to the type descriptor for this type.
@@ -2968,8 +3004,8 @@ 
       // backend representation, so force it to be finished now.
       if (!gogo->named_types_are_converted())
 	{
-	  pb->get_backend_placeholder(gogo);
-	  pb->finish_backend(gogo);
+	  Btype* bt = pb->get_backend_placeholder(gogo);
+	  pb->finish_backend(gogo, bt);
 	}
 
       fields[0].name = "__data";
diff -r b7d0a78bc2df go/types.h
--- a/go/types.h	Thu Nov 29 23:01:48 2012 -0800
+++ b/go/types.h	Fri Nov 30 15:36:47 2012 -0800
@@ -888,7 +888,7 @@ 
 
   // Finish the backend representation of a placeholder.
   void
-  finish_backend(Gogo*);
+  finish_backend(Gogo*, Btype*);
 
   // Build a type descriptor entry for this type.  Return a pointer to
   // it.  The location is the location which causes us to need the
@@ -1210,10 +1210,18 @@ 
   Btype*
   get_btype_without_hash(Gogo*);
 
+  // A backend type that may be a placeholder.
+  struct Type_btype_entry
+  {
+    Btype *btype;
+    bool is_placeholder;
+  };
+
   // A mapping from Type to Btype*, used to ensure that the backend
-  // representation of identical types is identical.
-  typedef Unordered_map_hash(const Type*, Btype*, Type_hash_identical,
-			     Type_identical) Type_btypes;
+  // representation of identical types is identical.  This is only
+  // used for unnamed types.
+  typedef Unordered_map_hash(const Type*, Type_btype_entry,
+			     Type_hash_identical, Type_identical) Type_btypes;
 
   static Type_btypes type_btypes;
 
@@ -1230,9 +1238,6 @@ 
 
   // The type classification.
   Type_classification classification_;
-  // Whether btype_ is a placeholder type used while named types are
-  // being converted.
-  bool btype_is_placeholder_;
   // The backend representation of the type, once it has been
   // determined.
   Btype* btype_;