Patchwork gccgo patch committed: Fix initialization order bug

login
register
mail settings
Submitter Ian Taylor
Date Jan. 29, 2013, 12:28 a.m.
Message ID <mcr4ni0x277.fsf@google.com>
Download mbox | patch
Permalink /patch/216405/
State New
Headers show

Comments

Ian Taylor - Jan. 29, 2013, 12:28 a.m.
This patch to the Go frontend fixes a bug determining the initialization
order.  I've committed a test case for the bug to the master Go
repository:

https://code.google.com/p/go/source/detail?spec=svn921e53d4863c8827756c0e7b228ab210441e4032&r=c3155f9f1bb64c8b3adb2b6f5527895d51f83b74

The old algorithm was overly clever and frankly I don't know what I was
thinking.  The new algorithm is simpler and probably more efficient.
Bootstrapped and ran Go testsuite on x86_64-unknown-linux-gnu.
Committed to mainline.

Ian

Patch

diff -r ee18ff1199b6 go/expressions.h
--- a/go/expressions.h	Fri Jan 25 16:13:13 2013 -0800
+++ b/go/expressions.h	Mon Jan 28 16:23:26 2013 -0800
@@ -983,6 +983,11 @@ 
       statement_(statement), is_lvalue_(false)
   { }
 
+  // The temporary that this expression refers to.
+  Temporary_statement*
+  statement() const
+  { return this->statement_; }
+
   // Indicate that this reference appears on the left hand side of an
   // assignment statement.
   void
diff -r ee18ff1199b6 go/gogo-tree.cc
--- a/go/gogo-tree.cc	Fri Jan 25 16:13:13 2013 -0800
+++ b/go/gogo-tree.cc	Mon Jan 28 16:23:26 2013 -0800
@@ -499,7 +499,7 @@ 
   // A hash table we use to avoid looping.  The index is the name of a
   // named object.  We only look through objects defined in this
   // package.
-  typedef Unordered_set(std::string) Seen_objects;
+  typedef Unordered_set(const void*) Seen_objects;
 
   Find_var(Named_object* var, Seen_objects* seen_objects)
     : Traverse(traverse_expressions),
@@ -547,7 +547,7 @@ 
 	  if (init != NULL)
 	    {
 	      std::pair<Seen_objects::iterator, bool> ins =
-		this->seen_objects_->insert(v->name());
+		this->seen_objects_->insert(v);
 	      if (ins.second)
 		{
 		  // This is the first time we have seen this name.
@@ -568,7 +568,7 @@ 
       if (f->is_function() && f->package() == NULL)
 	{
 	  std::pair<Seen_objects::iterator, bool> ins =
-	    this->seen_objects_->insert(f->name());
+	    this->seen_objects_->insert(f);
 	  if (ins.second)
 	    {
 	      // This is the first time we have seen this name.
@@ -578,6 +578,25 @@ 
 	}
     }
 
+  Temporary_reference_expression* tre = e->temporary_reference_expression();
+  if (tre != NULL)
+    {
+      Temporary_statement* ts = tre->statement();
+      Expression* init = ts->init();
+      if (init != NULL)
+	{
+	  std::pair<Seen_objects::iterator, bool> ins =
+	    this->seen_objects_->insert(ts);
+	  if (ins.second)
+	    {
+	      // This is the first time we have seen this temporary
+	      // statement.
+	      if (Expression::traverse(&init, this) == TRAVERSE_EXIT)
+		return TRAVERSE_EXIT;
+	    }
+	}
+    }
+
   return TRAVERSE_CONTINUE;
 }
 
@@ -613,11 +632,11 @@ 
 {
  public:
   Var_init()
-    : var_(NULL), init_(NULL_TREE), waiting_(0)
+    : var_(NULL), init_(NULL_TREE)
   { }
 
   Var_init(Named_object* var, tree init)
-    : var_(var), init_(init), waiting_(0)
+    : var_(var), init_(init)
   { }
 
   // Return the variable.
@@ -630,24 +649,11 @@ 
   init() const
   { return this->init_; }
 
-  // Return the number of variables waiting for this one to be
-  // initialized.
-  size_t
-  waiting() const
-  { return this->waiting_; }
-
-  // Increment the number waiting.
-  void
-  increment_waiting()
-  { ++this->waiting_; }
-
  private:
   // The variable being initialized.
   Named_object* var_;
   // The initialization expression to run.
   tree init_;
-  // The number of variables which are waiting for this one.
-  size_t waiting_;
 };
 
 typedef std::list<Var_init> Var_inits;
@@ -660,6 +666,10 @@ 
 static void
 sort_var_inits(Gogo* gogo, Var_inits* var_inits)
 {
+  typedef std::pair<Named_object*, Named_object*> No_no;
+  typedef std::map<No_no, bool> Cache;
+  Cache cache;
+
   Var_inits ready;
   while (!var_inits->empty())
     {
@@ -670,23 +680,30 @@ 
       Named_object* dep = gogo->var_depends_on(var->var_value());
 
       // Start walking through the list to see which variables VAR
-      // needs to wait for.  We can skip P1->WAITING variables--that
-      // is the number we've already checked.
+      // needs to wait for.
       Var_inits::iterator p2 = p1;
       ++p2;
-      for (size_t i = p1->waiting(); i > 0; --i)
-	++p2;
 
       for (; p2 != var_inits->end(); ++p2)
 	{
 	  Named_object* p2var = p2->var();
-	  if (expression_requires(init, preinit, dep, p2var))
+	  No_no key(var, p2var);
+	  std::pair<Cache::iterator, bool> ins =
+	    cache.insert(std::make_pair(key, false));
+	  if (ins.second)
+	    ins.first->second = expression_requires(init, preinit, dep, p2var);
+	  if (ins.first->second)
 	    {
 	      // Check for cycles.
-	      if (expression_requires(p2var->var_value()->init(),
+	      key = std::make_pair(p2var, var);
+	      ins = cache.insert(std::make_pair(key, false));
+	      if (ins.second)
+		ins.first->second =
+		  expression_requires(p2var->var_value()->init(),
 				      p2var->var_value()->preinit(),
 				      gogo->var_depends_on(p2var->var_value()),
-				      var))
+				      var);
+	      if (ins.first->second)
 		{
 		  error_at(var->location(),
 			   ("initialization expressions for %qs and "
@@ -700,12 +717,8 @@ 
 	      else
 		{
 		  // We can't emit P1 until P2 is emitted.  Move P1.
-		  // Note that the WAITING loop always executes at
-		  // least once, which is what we want.
-		  p2->increment_waiting();
 		  Var_inits::iterator p3 = p2;
-		  for (size_t i = p2->waiting(); i > 0; --i)
-		    ++p3;
+		  ++p3;
 		  var_inits->splice(p3, *var_inits, p1);
 		}
 	      break;
diff -r ee18ff1199b6 go/statements.h
--- a/go/statements.h	Fri Jan 25 16:13:13 2013 -0800
+++ b/go/statements.h	Mon Jan 28 16:23:26 2013 -0800
@@ -490,6 +490,11 @@ 
   Type*
   type() const;
 
+  // Return the initializer if there is one.
+  Expression*
+  init() const
+  { return this->init_; }
+
   // Note that it is OK for this statement to set hidden fields.
   void
   set_hidden_fields_are_ok()