re PR libgcj/8995 (race cases in interpreter)

2008-09-17  Andrew Haley  <aph@redhat.com>

        PR libgcj/8995:

        * defineclass.cc (_Jv_ClassReader::handleCodeAttribute):
        Initialize thread_count.
        * include/java-interp.h (_Jv_InterpMethod::thread_count): New
        field.
         (_Jv_InterpMethod::rewrite_insn_mutex): New mutex.
        (_Jv_InterpFrame:: _Jv_InterpFrame): Pass frame_type.
        * interpret.cc
        (ThreadCountAdjuster): New class.
        (_Jv_InterpMethod::thread_count): New field.
        (_Jv_InitInterpreter): Initialize rewrite_insn_mutex.
        Increment and decrement thread_count field in methods.
        * interpret-run.cc (REWRITE_INSN): Check thread_count <= 1.
        (REWRITE_INSN): Likewise.
        Declare a ThreadCountAdjuster.
        * java/lang/reflect/natVMProxy.cc (run_proxy): Initialize frame
        type as frame_proxy.

From-SVN: r140593

libjava/ChangeLog

+2008-09-17  Andrew Haley  <aph@redhat.com>
+
+	PR libgcj/8995:
+	
+	* defineclass.cc (_Jv_ClassReader::handleCodeAttribute):
+	Initialize thread_count.
+	* include/java-interp.h (_Jv_InterpMethod::thread_count): New
+	field.
+	 (_Jv_InterpMethod::rewrite_insn_mutex): New mutex.
+	(_Jv_InterpFrame:: _Jv_InterpFrame): Pass frame_type.
+	* interpret.cc
+	(ThreadCountAdjuster): New class.
+	(_Jv_InterpMethod::thread_count): New field.
+	(_Jv_InitInterpreter): Initialize rewrite_insn_mutex.
+	Increment and decrement thread_count field in methods.
+	* interpret-run.cc (REWRITE_INSN): Check thread_count <= 1.
+	(REWRITE_INSN): Likewise.
+	Declare a ThreadCountAdjuster.
+	* java/lang/reflect/natVMProxy.cc (run_proxy): Initialize frame
+	type as frame_proxy.
+
 2008-09-05  David Daney  <ddaney@avtrex.com>
 
 	* configure.ac (reduced-reflection): New AC_ARG_ENABLE.

libjava/defineclass.cc

@@ -1682,7 +1682,9 @@ void _Jv_ClassReader::handleCodeAttribute
   method->prepared       = NULL;
   method->line_table_len = 0;
   method->line_table     = NULL;
-
+#ifdef DIRECT_THREADED
+  method->thread_count   = 0;
+#endif
 
   // grab the byte code!
   memcpy ((void*) method->bytecode (),

libjava/include/java-interp.h

@@ -175,6 +175,17 @@ class _Jv_InterpMethod : public _Jv_MethodBase
   static pc_t breakpoint_insn;
 #ifdef DIRECT_THREADED
   static insn_slot bp_insn_slot;
+
+public:
+  // Mutex to prevent a data race between threads when rewriting
+  // instructions.  See interpret-run.cc for an explanation of its use.
+  static _Jv_Mutex_t rewrite_insn_mutex;
+
+  // The count of threads executing this method.
+  long thread_count;
+
+private:
+
 #else
   static unsigned char bp_insn_opcode;
 #endif
@@ -455,9 +466,10 @@ public:
   jobject obj_ptr;
 
   _Jv_InterpFrame (void *meth, java::lang::Thread *thr, jclass proxyCls = NULL,
-                   pc_t *pc = NULL)
+                   pc_t *pc = NULL, 
+		   _Jv_FrameType frame_type = frame_interpreter)
   : _Jv_Frame (reinterpret_cast<_Jv_MethodBase *> (meth), thr,
-	             frame_interpreter)
+	             frame_type)
   {
     next_interp = (_Jv_InterpFrame *) thr->interp_frame;
     proxyClass = proxyCls;
@@ -501,6 +513,76 @@ public:
   }
 };
 
+#ifdef DIRECT_THREADED
+// This class increments and decrements the thread_count field in an
+// interpreted method.  On entry to the interpreter a
+// ThreadCountAdjuster is created when increments the thread_count in
+// the current method and uses the next_interp field in the frame to
+// find the previous method and decrement its thread_count.
+class ThreadCountAdjuster
+{
+
+  // A class used to handle the rewrite_insn_mutex while we're
+  // adjusting the thread_count in a method.  Unlocking the mutex in a
+  // destructor ensures that it's unlocked even if (for example) a
+  // segfault occurs in the critical section.
+  class MutexLock
+  {
+  private:
+    _Jv_Mutex_t *mutex;
+  public:
+    MutexLock (_Jv_Mutex_t *m)
+    {
+      mutex = m;
+      _Jv_MutexLock (mutex);
+    }
+    ~MutexLock ()
+    {
+      _Jv_MutexUnlock (mutex);
+    }
+  };
+
+  _Jv_InterpMethod *method;
+  _Jv_InterpMethod *next_method;
+
+public:
+
+  ThreadCountAdjuster (_Jv_InterpMethod *m, _Jv_InterpFrame *fr)
+  {
+    MutexLock lock (&::_Jv_InterpMethod::rewrite_insn_mutex);
+
+    method = m;
+    next_method = NULL;
+
+    _Jv_InterpFrame *next_interp = fr->next_interp;
+
+    // Record the fact that we're executing this method and that
+    // we're no longer executing the method that called us.
+    method->thread_count++;
+
+    if (next_interp && next_interp->frame_type == frame_interpreter)
+      {
+	next_method 
+	  = reinterpret_cast<_Jv_InterpMethod *> (next_interp->meth);
+	next_method->thread_count--;
+      }
+  }
+
+  ~ThreadCountAdjuster ()
+  {
+    MutexLock lock (&::_Jv_InterpMethod::rewrite_insn_mutex);
+
+    // We're going to return to the method that called us, so bump its
+    // thread_count and decrement our own.
+
+    method->thread_count--;
+
+    if (next_method)
+      next_method->thread_count++;
+  }
+};
+#endif // DIRECT_THREADED
+
 #endif /* INTERPRETER */
 
 #endif /* __JAVA_INTERP_H__ */

libjava/interpret-run.cc

@@ -29,6 +29,10 @@ details.  */
   _Jv_InterpFrame frame_desc (meth, thread);
 #endif
 
+#ifdef DIRECT_THREADED
+  ThreadCountAdjuster adj (meth, &frame_desc);
+#endif // DIRECT_THREADED
+
   _Jv_word stack[meth->max_stack];
   _Jv_word *sp = stack;
 
@@ -361,20 +365,29 @@ details.  */
     }									\
   while (0)
 
+// We fail to rewrite a breakpoint if there is another thread
+// currently executing this method.  This is a bug, but there's
+// nothing else we can do that doesn't cause a data race.
 #undef REWRITE_INSN
 #define REWRITE_INSN(INSN,SLOT,VALUE)					\
-  do {									\
-    if (pc[-2].insn == breakpoint_insn->insn)				\
-      {									\
-	using namespace ::gnu::gcj::jvmti;				\
-	jlocation location = meth->insn_index (pc - 2);			\
-	_Jv_RewriteBreakpointInsn (meth->self, location, (pc_t) INSN);	\
-      }									\
-    else								\
-      pc[-2].insn = INSN;						\
+  do									\
+    {									\
+      _Jv_MutexLock (&rewrite_insn_mutex);				\
+      if (meth->thread_count <= 1)					\
+	{								\
+	  if (pc[-2].insn == breakpoint_insn->insn)			\
+	    {								\
+	      using namespace ::gnu::gcj::jvmti;			\
+	      jlocation location = meth->insn_index (pc - 2);		\
+	      _Jv_RewriteBreakpointInsn (meth->self, location, (pc_t) INSN); \
+	    }								\
+	  else								\
+	    pc[-2].insn = INSN;						\
 									\
-    pc[-1].SLOT = VALUE;						\
-  }									\
+	  pc[-1].SLOT = VALUE;						\
+	}								\
+      _Jv_MutexUnlock (&rewrite_insn_mutex);				\
+    }									\
   while (0)
 
 #undef INTERP_REPORT_EXCEPTION
@@ -383,23 +396,23 @@ details.  */
 #undef NEXT_INSN
 #define NEXT_INSN goto *((pc++)->insn)
 
-// 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)
+// threads is a data race if a thread reads part of an instruction
+// while some other thread is rewriting that instruction.  We detect
+// more than one thread executing a method and don't rewrite the
+// instruction.  A thread entering a method blocks on
+// rewrite_insn_mutex until the write is complete.
+#define REWRITE_INSN(INSN,SLOT,VALUE)		\
+  do {						\
+    _Jv_MutexLock (&rewrite_insn_mutex);	\
+    if (meth->thread_count <= 1)		\
+      {						\
+	pc[-2].insn = INSN;			\
+	pc[-1].SLOT = VALUE;			\
+      }						\
+    _Jv_MutexUnlock (&rewrite_insn_mutex);	\
+  }						\
+  while (0)
 
 #undef INTERP_REPORT_EXCEPTION
 #define INTERP_REPORT_EXCEPTION(Jthrowable) /* not needed when not debugging */

libjava/interpret.cc

@@ -78,10 +78,15 @@ static void find_catch_location (jthrowable, jthread, jmethodID *, jlong *);
 // the Class monitor as user code in another thread could hold it.
 static _Jv_Mutex_t compile_mutex;
 
+// See class ThreadCountAdjuster and REWRITE_INSN for how this is
+// used.
+_Jv_Mutex_t _Jv_InterpMethod::rewrite_insn_mutex;
+
 void
 _Jv_InitInterpreter()
 {
   _Jv_MutexInit (&compile_mutex);
+  _Jv_MutexInit (&_Jv_InterpMethod::rewrite_insn_mutex);
 }
 #else
 void _Jv_InitInterpreter() {}

libjava/java/lang/reflect/natVMProxy.cc

@@ -350,7 +350,8 @@ run_proxy (ffi_cif *cif,
   // than about Proxy.class itself.  FRAME_DESC has a destructor so it
   // cleans up automatically when this proxy invocation returns.
   Thread *thread = Thread::currentThread();
-  _Jv_InterpFrame frame_desc (self->self, thread, proxyClass);
+  _Jv_InterpFrame frame_desc (self->self, thread, proxyClass,
+			      NULL, frame_proxy);
 
   // The method to invoke is saved in $Proxy0.m[method_index].
   // FIXME: We could somewhat improve efficiency by storing a pointer