2008-08-22 Andrew Haley <aph@redhat.com>

        PR libgcj/8895:

        * interpret-run.cc (REWRITE_INSN): Null this macro.

        * include/jvm.h (class _Jv_Linker): Declare resolve_mutex, init.
        (read_cpool_entry, write_cpool_entry): New functions.
        * link.cc (_Jv_Linker::resolve_mutex): new.
        (_Jv_Linker::init): New function.
        (_Jv_Linker::resolve_pool_entry): Use {read,write}_cpool_entry
        to ensure atomic access to constant pool entries.



git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@139492 138bc75d-0d04-0410-961f-82ee72b054a4

4 files changed, 106 insertions, 31 deletions

diff --git a/libjava/ChangeLog b/libjava/ChangeLog
index 891b4dca588..8aacf4f11d0 100644
--- a/libjava/ChangeLog
+++ b/libjava/ChangeLog
@@ -1,3 +1,16 @@
+2008-08-22  Andrew Haley  <aph@redhat.com>
+
+	PR libgcj/8895:
+	
+	* interpret-run.cc (REWRITE_INSN): Null this macro.
+
+	* include/jvm.h (class _Jv_Linker): Declare resolve_mutex, init.
+	(read_cpool_entry, write_cpool_entry): New functions.
+	* link.cc (_Jv_Linker::resolve_mutex): new.
+	(_Jv_Linker::init): New function.
+	(_Jv_Linker::resolve_pool_entry): Use {read,write}_cpool_entry
+	to ensure atomic access to constant pool entries.
+
 2008-08-07  Andrew Haley  <aph@redhat.com>
 
 	* testsuite/libjava.lang/StackTrace2.java: Rewrite to prevent
diff --git a/libjava/include/jvm.h b/libjava/include/jvm.h
index 64cd6b5d7f9..ec74f295a5f 100644
--- a/libjava/include/jvm.h
+++ b/libjava/include/jvm.h
@@ -308,6 +308,9 @@ private:
     s = signature;
   }  
 
+  static _Jv_Mutex_t resolve_mutex;
+  static void init (void) __attribute__((constructor));
+
 public:
 
   static bool has_field_p (jclass, _Jv_Utf8Const *);
@@ -325,6 +328,27 @@ public:
 					     _Jv_Utf8Const *,
 					     bool check_perms = true);
   static void layout_vtable_methods(jclass);
+
+  static jbyte read_cpool_entry (_Jv_word *data,
+				 const _Jv_Constants *const pool,
+				 int index)
+  {
+    _Jv_MutexLock (&resolve_mutex);
+    jbyte tags = pool->tags[index];
+    *data = pool->data[index];
+    _Jv_MutexUnlock (&resolve_mutex);
+    return tags;
+  }
+
+  static void write_cpool_entry (_Jv_word data, jbyte tags,
+				 _Jv_Constants *pool,
+				 int index)
+  {
+    _Jv_MutexLock (&resolve_mutex);
+    pool->data[index] = data;
+    pool->tags[index] = tags;
+    _Jv_MutexUnlock (&resolve_mutex);
+  }
 };
 
 /* Type of pointer used as finalizer.  */
diff --git a/libjava/interpret-run.cc b/libjava/interpret-run.cc
index f858c971e0b..2934b9b8956 100644
--- a/libjava/interpret-run.cc
+++ b/libjava/interpret-run.cc
@@ -382,12 +382,24 @@ details.  */
 #else // !DEBUG
 #undef NEXT_INSN
 #define NEXT_INSN goto *((pc++)->insn)
-#define REWRITE_INSN(INSN,SLOT,VALUE)		\
-  do {						\
-    pc[-2].insn = INSN;				\
-    pc[-1].SLOT = VALUE;			\
-  }						\
-  while (0)
+
+// REWRITE_INSN does nothing.
+//
+// Rewriting a multi-word instruction in the presence of multiple
+// threads leads to a data race if a thread reads part of an
+// instruction while some other thread is rewriting that instruction.
+// For example, an invokespecial instruction may be rewritten to
+// invokespecial_resolved and its operand changed from an index to a
+// pointer while another thread is executing invokespecial.  This
+// other thread then reads the pointer that is now the operand of
+// invokespecial_resolved and tries to use it as an index.
+//
+// Fixing this requires either spinlocks, a more elaborate data
+// structure, or even per-thread allocated pages.  It's clear from the
+// locking in meth->compile below that the presence of multiple
+// threads was contemplated when this code was written, but the full
+// consequences were not fully appreciated.
+#define REWRITE_INSN(INSN,SLOT,VALUE)
 
 #undef INTERP_REPORT_EXCEPTION
 #define INTERP_REPORT_EXCEPTION(Jthrowable) /* not needed when not debugging */
diff --git a/libjava/link.cc b/libjava/link.cc
index f995531e813..c07b6e15c1c 100644
--- a/libjava/link.cc
+++ b/libjava/link.cc
@@ -380,6 +380,19 @@ _Jv_Linker::resolve_method_entry (jclass klass, jclass &found_class,
   return the_method;
 }
 
+_Jv_Mutex_t _Jv_Linker::resolve_mutex;
+
+void
+_Jv_Linker::init (void)
+{
+  _Jv_MutexInit (&_Jv_Linker::resolve_mutex);
+}
+
+// Locking in resolve_pool_entry is somewhat subtle.  Constant
+// resolution is idempotent, so it doesn't matter if two threads
+// resolve the same entry.  However, it is important that we always
+// write the resolved flag and the data together, atomically.  It is
+// also important that we read them atomically.
 _Jv_word
 _Jv_Linker::resolve_pool_entry (jclass klass, int index, bool lazy)
 {
@@ -387,6 +400,10 @@ _Jv_Linker::resolve_pool_entry (jclass klass, int index, bool lazy)
 
   if (GC_base (klass) && klass->constants.data
       && ! GC_base (klass->constants.data))
+    // If a class is heap-allocated but the constant pool is not this
+    // is a "new ABI" class, i.e. one where the initial constant pool
+    // is in the read-only data section of an object file.  Copy the
+    // initial constant pool from there to a new heap-allocated pool.
     {
       jsize count = klass->constants.size;
       if (count)
@@ -402,14 +419,18 @@ _Jv_Linker::resolve_pool_entry (jclass klass, int index, bool lazy)
 
   _Jv_Constants *pool = &klass->constants;
 
-  if ((pool->tags[index] & JV_CONSTANT_ResolvedFlag) != 0)
-    return pool->data[index];
+  jbyte tags;
+  _Jv_word data;
+  tags = read_cpool_entry (&data, pool, index);
 
-  switch (pool->tags[index] & ~JV_CONSTANT_LazyFlag)
+  if ((tags & JV_CONSTANT_ResolvedFlag) != 0)
+    return data;
+
+  switch (tags & ~JV_CONSTANT_LazyFlag)
     {
     case JV_CONSTANT_Class:
       {
-	_Jv_Utf8Const *name = pool->data[index].utf8;
+	_Jv_Utf8Const *name = data.utf8;
 
 	jclass found;
 	if (name->first() == '[')
@@ -428,8 +449,8 @@ _Jv_Linker::resolve_pool_entry (jclass klass, int index, bool lazy)
 	      {
 		found = _Jv_NewClass(name, NULL, NULL);
 		found->state = JV_STATE_PHANTOM;
-		pool->tags[index] |= JV_CONSTANT_ResolvedFlag;
-		pool->data[index].clazz = found;
+		tags |= JV_CONSTANT_ResolvedFlag;
+		data.clazz = found;
 		break;
 	      }
 	    else
@@ -447,8 +468,8 @@ _Jv_Linker::resolve_pool_entry (jclass klass, int index, bool lazy)
 	    || (_Jv_ClassNameSamePackage (check->name,
 					  klass->name)))
 	  {
-	    pool->data[index].clazz = found;
-	    pool->tags[index] |= JV_CONSTANT_ResolvedFlag;
+	    data.clazz = found;
+	    tags |= JV_CONSTANT_ResolvedFlag;
 	  }
 	else
 	  {
@@ -464,16 +485,16 @@ _Jv_Linker::resolve_pool_entry (jclass klass, int index, bool lazy)
     case JV_CONSTANT_String:
       {
 	jstring str;
-	str = _Jv_NewStringUtf8Const (pool->data[index].utf8);
-	pool->data[index].o = str;
-	pool->tags[index] |= JV_CONSTANT_ResolvedFlag;
+	str = _Jv_NewStringUtf8Const (data.utf8);
+	data.o = str;
+	tags |= JV_CONSTANT_ResolvedFlag;
       }
       break;
 
     case JV_CONSTANT_Fieldref:
       {
 	_Jv_ushort class_index, name_and_type_index;
-	_Jv_loadIndexes (&pool->data[index],
+	_Jv_loadIndexes (&data,
 			 class_index,
 			 name_and_type_index);
 	jclass owner = (resolve_pool_entry (klass, class_index, true)).clazz;
@@ -503,8 +524,8 @@ _Jv_Linker::resolve_pool_entry (jclass klass, int index, bool lazy)
 	// Initialize the field's declaring class, not its qualifying
 	// class.
 	_Jv_InitClass (found_class);
-	pool->data[index].field = the_field;
-	pool->tags[index] |= JV_CONSTANT_ResolvedFlag;
+	data.field = the_field;
+	tags |= JV_CONSTANT_ResolvedFlag;
       }
       break;
 
@@ -512,7 +533,7 @@ _Jv_Linker::resolve_pool_entry (jclass klass, int index, bool lazy)
     case JV_CONSTANT_InterfaceMethodref:
       {
 	_Jv_ushort class_index, name_and_type_index;
-	_Jv_loadIndexes (&pool->data[index],
+	_Jv_loadIndexes (&data,
 			 class_index,
 			 name_and_type_index);
 
@@ -521,18 +542,21 @@ _Jv_Linker::resolve_pool_entry (jclass klass, int index, bool lazy)
 	the_method = resolve_method_entry (klass, found_class,
 					   class_index, name_and_type_index,
 					   true,
-					   pool->tags[index] == JV_CONSTANT_InterfaceMethodref);
+					   tags == JV_CONSTANT_InterfaceMethodref);
       
-	pool->data[index].rmethod
+	data.rmethod
 	  = klass->engine->resolve_method(the_method,
 					  found_class,
 					  ((the_method->accflags
 					    & Modifier::STATIC) != 0));
-	pool->tags[index] |= JV_CONSTANT_ResolvedFlag;
+	tags |= JV_CONSTANT_ResolvedFlag;
       }
       break;
     }
-  return pool->data[index];
+
+  write_cpool_entry (data, tags, pool, index);
+
+  return data;
 }
 
 // This function is used to lazily locate superclasses and
@@ -1728,13 +1752,15 @@ _Jv_Linker::ensure_class_linked (jclass klass)
       // Resolve the remaining constant pool entries.
       for (int index = 1; index < pool->size; ++index)
 	{
-	  if (pool->tags[index] == JV_CONSTANT_String)
-	    {
-	      jstring str;
+	  jbyte tags;
+	  _Jv_word data;
 
-	      str = _Jv_NewStringUtf8Const (pool->data[index].utf8);
-	      pool->data[index].o = str;
-	      pool->tags[index] |= JV_CONSTANT_ResolvedFlag;
+	  tags = read_cpool_entry (&data, pool, index);
+	  if (tags == JV_CONSTANT_String)
+	    {
+	      data.o = _Jv_NewStringUtf8Const (data.utf8);
+	      tags |= JV_CONSTANT_ResolvedFlag;
+	      write_cpool_entry (data, tags, pool, index);
 	    }
 	}