gold patch committed: Avoid some race conditions

[Ian Lance Taylor <iant at google dot com>]

When using threads, gold has had a long-standing race condition,
originally analyzed by Kris Van Hees.  A target may be allocated
simultaneously in two different threads, leading to an "incompatible
target" warning as the two instances of the same target appear to be
different.  While looking into that, I realized that there are two other
similar race conditions: allocating the Descriptors lock field and
allocating the Errors lock field.  This was unfortunately rather sloppy
coding on my part.  I committed this patch to use pthread_once to avoid
these problems.  It is used in a somewhat indirect manner to retain
efficiency when not threaded.

Ian


2009-03-23  Ian Lance Taylor  <iant@google.com>

	* gold-threads.h (class Initialize_lock): Define.
	* gold-threads.cc (class Initialize_lock_once): Define.
	(initialize_lock_control): New static variable.
	(initialize_lock_pointer): New static variable.
	(initialize_lock_once): New static function.
	(Initialize_lock::Initialize_lock): Define.
	(Initialize_lock::initialize): Define.
	* target-select.h: Include "gold-threads.h".
	(class Target_selector): Add lock_ and initialize_lock_ fields.
	Don't define instantiate_target, just declare it.
	* target-select.cc (Target_selector::Target_selector): Initialize
	new fields.
	(Target_selector::instantiate_target): Define.
	* descriptors.h: Include "gold-threads.h".
	(class Descriptors): Add initialize_lock_ field.
	* descriptors.cc (Descriptors::Descriptors): Initialize new
	field.
	(Descriptors::open): Use initialize_lock_ field
	* errors.h (class Errors): Add initialize_lock_ field.
	* errors.cc (Errors::Errors): Initialize new field.
	(Errors::initialize_lock): Use initialize_lock_ field.
	* powerpc.cc (class Target_selector_powerpc): Remove
	instantiated_target_ field.  In do_recognize call
	instantiate_target rather than do_instantiate_target.  In
	do_instantiate_target just allocate a new target.
	* sparc.cc (class Target_selector_sparc): Likewise.


Index: descriptors.cc
===================================================================
RCS file: /cvs/src/src/gold/descriptors.cc,v
retrieving revision 1.6
diff -p -u -r1.6 descriptors.cc
--- descriptors.cc	19 Mar 2009 19:02:53 -0000	1.6
+++ descriptors.cc	24 Mar 2009 04:41:41 -0000
@@ -51,8 +51,8 @@ namespace gold
 // adjusted downward if we run out of file descriptors.
 
 Descriptors::Descriptors()
-  : lock_(NULL), open_descriptors_(), stack_top_(-1), current_(0),
-    limit_(8192 - 16)
+  : lock_(NULL), initialize_lock_(&this->lock_), open_descriptors_(),
+    stack_top_(-1), current_(0), limit_(8192 - 16)
 {
   this->open_descriptors_.reserve(128);
 }
@@ -66,13 +66,9 @@ Descriptors::open(int descriptor, const 
   // initialize a Lock until we have parsed the options to find out
   // whether we are running with threads.  We can be called before
   // options are valid when reading a linker script.
-  if (this->lock_ == NULL)
-    {
-      if (parameters->options_valid())
-	this->lock_ = new Lock();
-      else
-	gold_assert(descriptor < 0);
-    }
+  bool lock_initialized = this->initialize_lock_.initialize();
+
+  gold_assert(lock_initialized || descriptor < 0);
 
   if (descriptor >= 0)
     {
Index: descriptors.h
===================================================================
RCS file: /cvs/src/src/gold/descriptors.h,v
retrieving revision 1.4
diff -p -u -r1.4 descriptors.h
--- descriptors.h	28 Feb 2009 03:05:08 -0000	1.4
+++ descriptors.h	24 Mar 2009 04:41:41 -0000
@@ -25,11 +25,11 @@
 
 #include <vector>
 
+#include "gold-threads.h"
+
 namespace gold
 {
 
-class Lock;
-
 // This class manages file descriptors for gold.
 
 class Descriptors
@@ -78,6 +78,8 @@ class Descriptors
 
   // We need to lock before accessing any fields.
   Lock* lock_;
+  // Used to initialize the lock_ field exactly once.
+  Initialize_lock initialize_lock_;
   // Information for descriptors.
   std::vector<Open_descriptor> open_descriptors_;
   // Top of stack.
Index: errors.cc
===================================================================
RCS file: /cvs/src/src/gold/errors.cc,v
retrieving revision 1.8
diff -p -u -r1.8 errors.cc
--- errors.cc	6 Feb 2009 19:20:09 -0000	1.8
+++ errors.cc	24 Mar 2009 04:41:41 -0000
@@ -39,8 +39,8 @@ namespace gold
 const int Errors::max_undefined_error_report;
 
 Errors::Errors(const char* program_name)
-  : program_name_(program_name), lock_(NULL), error_count_(0),
-    warning_count_(0), undefined_symbols_()
+  : program_name_(program_name), lock_(NULL), initialize_lock_(&this->lock_),
+    error_count_(0), warning_count_(0), undefined_symbols_()
 {
 }
 
@@ -53,9 +53,7 @@ Errors::Errors(const char* program_name)
 bool
 Errors::initialize_lock()
 {
-  if (this->lock_ == NULL && parameters->options_valid())
-    this->lock_ = new Lock;
-  return this->lock_ != NULL;
+  return this->initialize_lock_.initialize();
 }
 
 // Increment a counter, holding the lock if available.
Index: errors.h
===================================================================
RCS file: /cvs/src/src/gold/errors.h,v
retrieving revision 1.7
diff -p -u -r1.7 errors.h
--- errors.h	6 Feb 2009 19:20:09 -0000	1.7
+++ errors.h	24 Mar 2009 04:41:41 -0000
@@ -116,6 +116,8 @@ class Errors
   // This class can be accessed from multiple threads.  This lock is
   // used to control access to the data structures.
   Lock* lock_;
+  // Used to initialize the lock_ field exactly once.
+  Initialize_lock initialize_lock_;
   // Numbers of errors reported.
   int error_count_;
   // Number of warnings reported.
Index: gold-threads.cc
===================================================================
RCS file: /cvs/src/src/gold/gold-threads.cc,v
retrieving revision 1.7
diff -p -u -r1.7 gold-threads.cc
--- gold-threads.cc	13 Mar 2008 21:04:21 -0000	1.7
+++ gold-threads.cc	24 Mar 2009 04:41:41 -0000
@@ -276,4 +276,128 @@ Condvar::~Condvar()
   delete this->condvar_;
 }
 
+#ifdef ENABLE_THREADS
+
+// Class Initialize_lock_once.  This exists to hold a pthread_once_t
+// structure for Initialize_lock.
+
+class Initialize_lock_once
+{
+ public:
+  Initialize_lock_once()
+    : once_(PTHREAD_ONCE_INIT)
+  { }
+
+  // Return a pointer to the pthread_once_t variable.
+  pthread_once_t*
+  once_control()
+  { return &this->once_; }
+
+ private:
+  pthread_once_t once_;
+};
+
+#endif // !defined(ENABLE_THREADS)
+
+#ifdef ENABLE_THREADS
+
+// A single lock which controls access to initialize_lock_pointer.
+// This is used because we can't pass parameters to functions passed
+// to pthread_once.
+
+static pthread_mutex_t initialize_lock_control = PTHREAD_MUTEX_INITIALIZER;
+
+// A pointer to a pointer to the lock which we need to initialize
+// once.  Access to this is controlled by initialize_lock_pointer.
+
+static Lock** initialize_lock_pointer;
+
+// A routine passed to pthread_once which initializes the lock which
+// initialize_lock_pointer points to.
+
+extern "C"
+{
+
+static void
+initialize_lock_once()
+{
+  *initialize_lock_pointer = new Lock();
+}
+
+}
+
+#endif // !defined(ENABLE_THREADS)
+
+// Class Initialize_lock.
+
+Initialize_lock::Initialize_lock(Lock** pplock)
+  : pplock_(pplock)
+{
+#ifndef ENABLE_THREADS
+  this->once_ = NULL;
+#else
+  this->once_ = new Initialize_lock_once();
+#endif
+}
+
+// Initialize the lock.
+
+bool
+Initialize_lock::initialize()
+{
+  // If the lock has already been initialized, we don't need to do
+  // anything.  Note that this assumes that the pointer value will be
+  // set completely or not at all.  I hope this is always safe.  We
+  // want to do this for efficiency.
+  if (*this->pplock_ != NULL)
+    return true;
+
+  // We can't initialize the lock until we have read the options.
+  if (!parameters->options_valid())
+    return false;
+
+  // If the user did not use --threads, then we can initialize
+  // directly.
+  if (!parameters->options().threads())
+    {
+      *this->pplock_ = new Lock();
+      return true;
+    }
+
+#ifndef ENABLE_THREADS
+
+  // If there is no threads support, we don't need to use
+  // pthread_once.
+  *this->pplock_ = new Lock();
+
+#else // !defined(ENABLE_THREADS)
+
+  // Since we can't pass parameters to routines called by
+  // pthread_once, we use a static variable: initialize_lock_pointer.
+  // That in turns means that we need to use a mutex to control access
+  // to initialize_lock_pointer.
+
+  int err = pthread_mutex_lock(&initialize_lock_control);
+  if (err != 0)
+    gold_fatal(_("pthread_mutex_lock failed: %s"), strerror(err));
+
+  initialize_lock_pointer = this->pplock_;
+
+  err = pthread_once(this->once_->once_control(), initialize_lock_once);
+  if (err != 0)
+    gold_fatal(_("pthread_once failed: %s"), strerror(err));
+
+  initialize_lock_pointer = NULL;
+
+  err = pthread_mutex_unlock(&initialize_lock_control);
+  if (err != 0)
+    gold_fatal(_("pthread_mutex_unlock failed: %s"), strerror(err));
+
+  gold_assert(*this->pplock_ != NULL);
+
+#endif // !defined(ENABLE_THREADS)
+
+  return true;
+}
+
 } // End namespace gold.
Index: gold-threads.h
===================================================================
RCS file: /cvs/src/src/gold/gold-threads.h,v
retrieving revision 1.5
diff -p -u -r1.5 gold-threads.h
--- gold-threads.h	25 Jul 2008 04:25:49 -0000	1.5
+++ gold-threads.h	24 Mar 2009 04:41:41 -0000
@@ -35,6 +35,7 @@ namespace gold
 {
 
 class Condvar;
+class Initialize_lock_once;
 
 // The interface for the implementation of a Lock.
 
@@ -190,6 +191,33 @@ class Condvar
   Condvar_impl* condvar_;
 };
 
+// A class used to initialize a lock exactly once, after the options
+// have been read.  This is needed because the implementation of locks
+// depends on whether we've seen the --threads option.  Before the
+// options have been read, we know we are single-threaded, so we can
+// get by without using a lock.  This class should be an instance
+// variable of the class which has a lock which needs to be
+// initialized.
+
+class Initialize_lock
+{
+ public:
+  // The class which uses this will have a pointer to a lock.  This
+  // must be constructed with a pointer to that pointer.
+  Initialize_lock(Lock** pplock);
+
+  // Initialize the lock.  Return true if the lock is now initialized,
+  // false if it is not (because the options have not yet been read).
+  bool
+  initialize();
+
+ private:
+  // A pointer to the lock pointer which must be initialized.
+  Lock** const pplock_;
+  // If needed, a pointer to a pthread_once_t structure.
+  Initialize_lock_once* once_;
+};
+
 } // End namespace gold.
 
 #endif // !defined(GOLD_THREADS_H)
Index: powerpc.cc
===================================================================
RCS file: /cvs/src/src/gold/powerpc.cc,v
retrieving revision 1.10
diff -p -u -r1.10 powerpc.cc
--- powerpc.cc	4 Mar 2009 06:46:27 -0000	1.10
+++ powerpc.cc	24 Mar 2009 04:41:41 -0000
@@ -1982,8 +1982,6 @@ public:
 		       (big_endian ? "elf32-powerpc" : "elf32-powerpcle")))
   { }
 
-  Target* instantiated_target_;
-
   Target* do_recognize(int machine, int, int)
   {
     switch (size)
@@ -2002,15 +2000,11 @@ public:
 	return NULL;
       }
 
-    return do_instantiate_target();
+    return this->instantiate_target();
   }
 
   Target* do_instantiate_target()
-  {
-    if (this->instantiated_target_ == NULL)
-      this->instantiated_target_ = new Target_powerpc<size, big_endian>();
-    return this->instantiated_target_;
-  }
+  { return new Target_powerpc<size, big_endian>(); }
 };
 
 Target_selector_powerpc<32, true> target_selector_ppc32;
Index: sparc.cc
===================================================================
RCS file: /cvs/src/src/gold/sparc.cc,v
retrieving revision 1.14
diff -p -u -r1.14 sparc.cc
--- sparc.cc	17 Mar 2009 07:19:10 -0000	1.14
+++ sparc.cc	24 Mar 2009 04:41:42 -0000
@@ -3240,8 +3240,6 @@ public:
 		      (size == 64 ? "elf64-sparc" : "elf32-sparc"))
   { }
 
-  Target* instantiated_target_;
-
   Target* do_recognize(int machine, int, int)
   {
     switch (size)
@@ -3261,15 +3259,11 @@ public:
 	return NULL;
       }
 
-    return do_instantiate_target();
+    return this->instantiate_target();
   }
 
   Target* do_instantiate_target()
-  {
-    if (this->instantiated_target_ == NULL)
-      this->instantiated_target_ = new Target_sparc<size, big_endian>();
-    return this->instantiated_target_;
-  }
+  { return new Target_sparc<size, big_endian>(); }
 };
 
 Target_selector_sparc<32, true> target_selector_sparc32;
Index: target-select.cc
===================================================================
RCS file: /cvs/src/src/gold/target-select.cc,v
retrieving revision 1.8
diff -p -u -r1.8 target-select.cc
--- target-select.cc	26 Mar 2008 23:36:46 -0000	1.8
+++ target-select.cc	24 Mar 2009 04:41:42 -0000
@@ -46,13 +46,27 @@ namespace gold
 Target_selector::Target_selector(int machine, int size, bool is_big_endian,
 				 const char* bfd_name)
   : machine_(machine), size_(size), is_big_endian_(is_big_endian),
-    bfd_name_(bfd_name), instantiated_target_(NULL)
+    bfd_name_(bfd_name), instantiated_target_(NULL), lock_(NULL),
+    initialize_lock_(&this->lock_)
     
 {
   this->next_ = target_selectors;
   target_selectors = this;
 }
 
+// Instantiate the target and return it.  Use a lock to avoid
+// instantiating two instances of the same target.
+
+Target*
+Target_selector::instantiate_target()
+{
+  this->initialize_lock_.initialize();
+  Hold_optional_lock hl(this->lock_);
+  if (this->instantiated_target_ == NULL)
+    this->instantiated_target_ = this->do_instantiate_target();
+  return this->instantiated_target_;
+}
+
 // Find the target for an ELF file.
 
 Target*
Index: target-select.h
===================================================================
RCS file: /cvs/src/src/gold/target-select.h,v
retrieving revision 1.8
diff -p -u -r1.8 target-select.h
--- target-select.h	24 Mar 2009 00:31:29 -0000	1.8
+++ target-select.h	24 Mar 2009 04:41:42 -0000
@@ -25,6 +25,8 @@
 
 #include <vector>
 
+#include "gold-threads.h"
+
 namespace gold
 {
 
@@ -136,12 +138,7 @@ class Target_selector
 
   // Instantiate the target and return it.
   Target*
-  instantiate_target()
-  {
-    if (this->instantiated_target_ == NULL)
-      this->instantiated_target_ = this->do_instantiate_target();
-    return this->instantiated_target_;
-  }
+  instantiate_target();
 
  private:
   // ELF machine code.
@@ -157,6 +154,11 @@ class Target_selector
   // The singleton Target structure--this points to an instance of the
   // real implementation.
   Target* instantiated_target_;
+  // Lock to make sure that we don't instantiate the target more than
+  // once.
+  Lock* lock_;
+  // We only want to initialize the lock_ pointer once.
+  Initialize_lock initialize_lock_;
 };
 
 // Select the target for an ELF file.