Remove data race in libgcj interpreter

Remove data race in libgcj interpreter

[Andrew Haley]

I've discovered a nasty data race in libgcj's interpreter.

It is the cause of several bug reports, in particular
https://bugzilla.redhat.com/show_bug.cgi?id=458921

An optimization rerwites instructions of the form

  invokespecial <constant pool index>

to

  invokespecial_resolved <address>

the first time that each invokespecial is encountered.

However, in the presence of multiple threads this breaks.

If Thread A is rewriting the instruction while Thread B is reading it,
it is possible for Thread B to read half of the instruction before
rewriting and half of the instruction after rewriting.  Typically, it
will read

  invokespecial <address>

and it will crash, as the address is out of range for a constant pool
index.  Unfortunately we don't check the validity of constant pool
indexes in the interpreter.

This optimization was fairly extensively tested, but when it was written
most of the test machines were uniprocessors, and this race is very
unlikely to trigger a failure on a uniprocessor.  As Dijkstra put it,
testing can be used to show the presence of bugs, but never to show
their absence!

This patch, which I'm going to commit to all active branches, removes
all uses of the instruction-rewriting optimization from libgcj's
interpreter.  It will make the interpreter considerably slower.

Restoring the optimization will require some use of locking and
careful analysis.  I don't think it will be very difficult to fix, but
it will need thought.

Andrew.

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

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

--- interpret-run.cc~	2007-12-07 16:55:50.000000000 +0000
+++ interpret-run.cc	2008-08-21 13:58:50.000000000 +0100
@@ -382,12 +382,24 @@
 #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 */

Re: Remove data race in libgcj interpreter

[Andrew Haley]

Andrew Haley wrote:
> I've discovered a nasty data race in libgcj's interpreter.
> 
> It is the cause of several bug reports, in particular
> https://bugzilla.redhat.com/show_bug.cgi?id=458921

I spoke too soon: there may be another cause of that bug.

Andrew.

gij: more thread-safety fixes

[Andrew Haley]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]

There were more problems in gij related to the lack of locking when
resolving constants.  This patch fixes the problems by using
fine-grained locking whenever accessing the constant pool.

This lack of synchronization did not affect precompiled code because
gcj resolves constants by emitting calls _Jv_ResolvePoolEntry, and
that function locks the class whenever updating the constant pool.
While coarse, this does guarantee that the data races in the
interpreter cannot occur, which is why we've never seen this problem
with precompiled code.

In theory the class lock in _Jv_ResolvePoolEntry could now be removed
because the new fine-grained locking makes it unnececessary.  However,
I'm not going to remove it yet.

Andrew.



[-- Attachment #2: pp --]
[-- Type: text/plain, Size: 8208 bytes --]

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

	* 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.

*** include/jvm.h~	2007-12-07 16:55:41.000000000 +0000
--- include/jvm.h	2008-08-21 18:11:01.000000000 +0100
***************
*** 307,312 ****
--- 307,315 ----
      s = signature;
    }  
  
+   static _Jv_Mutex_t resolve_mutex;
+   static void init (void) __attribute__((constructor));
+ 
  public:
  
    static bool has_field_p (jclass, _Jv_Utf8Const *);
***************
*** 324,329 ****
--- 327,353 ----
  					     _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.  */
*** link.cc~	2008-03-14 17:07:31.000000000 +0000
--- link.cc	2008-08-22 10:55:26.000000000 +0100
***************
*** 362,367 ****
--- 362,380 ----
    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)
  {
***************
*** 369,374 ****
--- 382,391 ----
  
    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)
***************
*** 384,397 ****
  
    _Jv_Constants *pool = &klass->constants;
  
!   if ((pool->tags[index] & JV_CONSTANT_ResolvedFlag) != 0)
!     return pool->data[index];
  
!   switch (pool->tags[index] & ~JV_CONSTANT_LazyFlag)
      {
      case JV_CONSTANT_Class:
        {
! 	_Jv_Utf8Const *name = pool->data[index].utf8;
  
  	jclass found;
  	if (name->first() == '[')
--- 401,418 ----
  
    _Jv_Constants *pool = &klass->constants;
  
!   jbyte tags;
!   _Jv_word data;
!   tags = read_cpool_entry (&data, pool, index);
  
!   if ((tags & JV_CONSTANT_ResolvedFlag) != 0)
!     return data;
! 
!   switch (tags & ~JV_CONSTANT_LazyFlag)
      {
      case JV_CONSTANT_Class:
        {
! 	_Jv_Utf8Const *name = data.utf8;
  
  	jclass found;
  	if (name->first() == '[')
***************
*** 410,417 ****
  	      {
  		found = _Jv_NewClass(name, NULL, NULL);
  		found->state = JV_STATE_PHANTOM;
! 		pool->tags[index] |= JV_CONSTANT_ResolvedFlag;
! 		pool->data[index].clazz = found;
  		break;
  	      }
  	    else
--- 431,438 ----
  	      {
  		found = _Jv_NewClass(name, NULL, NULL);
  		found->state = JV_STATE_PHANTOM;
! 		tags |= JV_CONSTANT_ResolvedFlag;
! 		data.clazz = found;
  		break;
  	      }
  	    else
***************
*** 429,436 ****
  	    || (_Jv_ClassNameSamePackage (check->name,
  					  klass->name)))
  	  {
! 	    pool->data[index].clazz = found;
! 	    pool->tags[index] |= JV_CONSTANT_ResolvedFlag;
  	  }
  	else
  	  {
--- 450,457 ----
  	    || (_Jv_ClassNameSamePackage (check->name,
  					  klass->name)))
  	  {
! 	    data.clazz = found;
! 	    tags |= JV_CONSTANT_ResolvedFlag;
  	  }
  	else
  	  {
***************
*** 446,461 ****
      case JV_CONSTANT_String:
        {
  	jstring str;
! 	str = _Jv_NewStringUtf8Const (pool->data[index].utf8);
! 	pool->data[index].o = str;
! 	pool->tags[index] |= JV_CONSTANT_ResolvedFlag;
        }
        break;
  
      case JV_CONSTANT_Fieldref:
        {
  	_Jv_ushort class_index, name_and_type_index;
! 	_Jv_loadIndexes (&pool->data[index],
  			 class_index,
  			 name_and_type_index);
  	jclass owner = (resolve_pool_entry (klass, class_index, true)).clazz;
--- 467,482 ----
      case JV_CONSTANT_String:
        {
  	jstring str;
! 	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 (&data,
  			 class_index,
  			 name_and_type_index);
  	jclass owner = (resolve_pool_entry (klass, class_index, true)).clazz;
***************
*** 485,492 ****
  	// 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;
        }
        break;
  
--- 506,513 ----
  	// Initialize the field's declaring class, not its qualifying
  	// class.
  	_Jv_InitClass (found_class);
! 	data.field = the_field;
! 	tags |= JV_CONSTANT_ResolvedFlag;
        }
        break;
  
***************
*** 494,500 ****
      case JV_CONSTANT_InterfaceMethodref:
        {
  	_Jv_ushort class_index, name_and_type_index;
! 	_Jv_loadIndexes (&pool->data[index],
  			 class_index,
  			 name_and_type_index);
  
--- 515,521 ----
      case JV_CONSTANT_InterfaceMethodref:
        {
  	_Jv_ushort class_index, name_and_type_index;
! 	_Jv_loadIndexes (&data,
  			 class_index,
  			 name_and_type_index);
  
***************
*** 503,520 ****
  	the_method = resolve_method_entry (klass, found_class,
  					   class_index, name_and_type_index,
  					   true,
! 					   pool->tags[index] == JV_CONSTANT_InterfaceMethodref);
        
! 	pool->data[index].rmethod
  	  = klass->engine->resolve_method(the_method,
  					  found_class,
  					  ((the_method->accflags
  					    & Modifier::STATIC) != 0));
! 	pool->tags[index] |= JV_CONSTANT_ResolvedFlag;
        }
        break;
      }
!   return pool->data[index];
  }
  
  // This function is used to lazily locate superclasses and
--- 524,544 ----
  	the_method = resolve_method_entry (klass, found_class,
  					   class_index, name_and_type_index,
  					   true,
! 					   tags == JV_CONSTANT_InterfaceMethodref);
        
! 	data.rmethod
  	  = klass->engine->resolve_method(the_method,
  					  found_class,
  					  ((the_method->accflags
  					    & Modifier::STATIC) != 0));
! 	tags |= JV_CONSTANT_ResolvedFlag;
        }
        break;
      }
! 
!   write_cpool_entry (data, tags, pool, index);
! 
!   return data;
  }
  
  // This function is used to lazily locate superclasses and
***************
*** 1701,1713 ****
        // Resolve the remaining constant pool entries.
        for (int index = 1; index < pool->size; ++index)
  	{
! 	  if (pool->tags[index] == JV_CONSTANT_String)
! 	    {
! 	      jstring str;
  
! 	      str = _Jv_NewStringUtf8Const (pool->data[index].utf8);
! 	      pool->data[index].o = str;
! 	      pool->tags[index] |= JV_CONSTANT_ResolvedFlag;
  	    }
  	}
  
--- 1725,1739 ----
        // Resolve the remaining constant pool entries.
        for (int index = 1; index < pool->size; ++index)
  	{
! 	  jbyte tags;
! 	  _Jv_word data;
  
! 	  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);
  	    }
  	}
  

Re: Remove data race in libgcj interpreter

[Bryce McKinlay]

On Thu, Aug 21, 2008 at 2:03 PM, Andrew Haley <aph@redhat.com> wrote:

> I've discovered a nasty data race in libgcj's interpreter.
>
> It is the cause of several bug reports, in particular
> https://bugzilla.redhat.com/show_bug.cgi?id=458921
>
> An optimization rerwites instructions of the form
>
>  invokespecial <constant pool index>
>
> to
>
>  invokespecial_resolved <address>
>
> the first time that each invokespecial is encountered.
>
> However, in the presence of multiple threads this breaks.

Ahh, yeah. I remember this bug:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8995

As Tom suggests, the proper fix may be to do all the rewriting in the
compile phase, which is protected by a lock.

There is at least one other interpreter bug which shows up only on
multi-core systems:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=16902

Re: Remove data race in libgcj interpreter

[Andrew Haley]

Bryce McKinlay wrote:
> On Thu, Aug 21, 2008 at 2:03 PM, Andrew Haley <aph@redhat.com> wrote:
> 
>> I've discovered a nasty data race in libgcj's interpreter.
>>
>> It is the cause of several bug reports, in particular
>> https://bugzilla.redhat.com/show_bug.cgi?id=458921
>>
>> An optimization rerwites instructions of the form
>>
>>  invokespecial <constant pool index>
>>
>> to
>>
>>  invokespecial_resolved <address>
>>
>> the first time that each invokespecial is encountered.
>>
>> However, in the presence of multiple threads this breaks.
> 
> Ahh, yeah. I remember this bug:
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=8995
> 
> As Tom suggests, the proper fix may be to do all the rewriting in the
> compile phase, which is protected by a lock.

Ah right, thanks.  It's rather appalling that we once knew about this
bug but didn't fix it, preferring a fast interpreter to a correct one.
Thankfully, my name doesn't appear on any of the comments!  :-)

> There is at least one other interpreter bug which shows up only on
> multi-core systems:
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=16902

Thanks.

Andrew.

Re: Remove data race in libgcj interpreter

[Andrew Haley]

Here's a patch for the bug.  It shouldn't impact performance
in the fast case, but uses fine-grained locks when rewriting the
instructions.

It's rather complicated, but I think it's correct.  Comments welcome,
from Tom Tromey in particular.

Andrew.

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

	PR libgcj/8995:

	* interpret.cc (rewrite_mutex): New variable.
	(_Jv_InitInterpreter): Initialize rewrite_mutex.
	* interpret-run.cc (NEXT_INSN): Add a read barrier.
	(REWRITE_INSN): Add locking.
	(CHECK_INSN): New macro.
	(interpreter): Add CHECK_INSN at every point an instruction is
	rewritten.

Index: interpret-run.cc
===================================================================
*** interpret-run.cc	(revision 139491)
--- interpret-run.cc	(working copy)
***************
*** 343,353 ****
--- 343,385 ----

  #ifdef DIRECT_THREADED

+ /***********************************************************************
+   Locking in the direct-threaded case is quite subtle.  See PR
+   libgcj/8995 for more details.
+
+   We must ensure that no-one ever reads an inconsistent {insn,value}
+   pair.  Rather than trying to lock everything we detect when some
+   other thread has modified the insn we are trying to modify and back
+   out.
+
+   When we read a pair before rewriting it we do this:
+
+     i1 = insn
+     v = value
+     lock
+       i2 = insn
+     unlock
+     if (i2 != i1) try again
+
+   and when we write a pair we do this:
+
+      lock
+        value = v
+        write barrier
+        insn = i
+      unlock
+
+   There is a read barrier in NEXT_INSN which ensures that we don't
+   read a rewritten value with a stale insn.
+
+ *************************************************************************/
+
  #ifdef DEBUG
  #undef NEXT_INSN
  #define NEXT_INSN							\
    do									\
      {									\
+       read_barrier ();							\
        pc_t insn = pc++;							\
        if (JVMTI_REQUESTED_EVENT (SingleStep))				\
  	{								\
***************
*** 364,369 ****
--- 396,406 ----
  #undef REWRITE_INSN
  #define REWRITE_INSN(INSN,SLOT,VALUE)					\
    do {									\
+     _Jv_MutexLock (&rewrite_mutex);					\
+ 									\
+     pc[-1].SLOT = VALUE;						\
+     write_barrier ();							\
+ 									\
      if (pc[-2].insn == breakpoint_insn->insn)				\
        {									\
  	using namespace ::gnu::gcj::jvmti;				\
***************
*** 373,379 ****
      else								\
        pc[-2].insn = INSN;						\
  									\
!     pc[-1].SLOT = VALUE;						\
    }									\
    while (0)

--- 410,416 ----
      else								\
        pc[-2].insn = INSN;						\
  									\
!     _Jv_MutexUnlock (&rewrite_mutex);					\
    }									\
    while (0)

***************
*** 381,398 ****
  #define INTERP_REPORT_EXCEPTION(Jthrowable) REPORT_EXCEPTION (Jthrowable)
  #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)

  #undef INTERP_REPORT_EXCEPTION
  #define INTERP_REPORT_EXCEPTION(Jthrowable) /* not needed when not debugging */
  #endif // !DEBUG

  #define INTVAL() ((pc++)->int_val)
  #define AVAL() ((pc++)->datum)

--- 418,464 ----
  #define INTERP_REPORT_EXCEPTION(Jthrowable) REPORT_EXCEPTION (Jthrowable)
  #else // !DEBUG
  #undef NEXT_INSN
! #define NEXT_INSN				\
! do						\
!   {						\
!     read_barrier ();				\
!     goto *((pc++)->insn);			\
    }						\
+  while (0)
+
+ #define REWRITE_INSN(INSN,SLOT,VALUE)					\
+   do {									\
+     _Jv_MutexLock (&rewrite_mutex);					\
+     pc[-1].SLOT = VALUE;						\
+     /*  Barrier here to ensure that the insn is not written until	\
+ 	after its operand.  There is a corresponding read barrier in	\
+ 	NEXT_INSN.  */							\
+     write_barrier ();							\
+     pc[-2].insn = INSN;							\
+     _Jv_MutexUnlock (&rewrite_mutex);					\
+   }									\
    while (0)

  #undef INTERP_REPORT_EXCEPTION
  #define INTERP_REPORT_EXCEPTION(Jthrowable) /* not needed when not debugging */
  #endif // !DEBUG

+ #define CHECK_INSN(EXPECTED)						\
+   do {									\
+     /*  Lock here to ensure that rewriting the instruction is not in	\
+ 	progress.  */							\
+     _Jv_MutexLock (&rewrite_mutex);					\
+     insn_slot slot = pc[-2];						\
+     _Jv_MutexUnlock (&rewrite_mutex);					\
+     if (slot.insn != EXPECTED)						\
+       {									\
+ 	/* The instruction has been changed; try again.  */		\
+ 	pc -=2;								\
+ 	NEXT_INSN;							\
+       }									\
+   }									\
+   while (0)
+
  #define INTVAL() ((pc++)->int_val)
  #define AVAL() ((pc++)->datum)

***************
*** 509,514 ****
--- 575,584 ----
  	SAVE_PC();
  	int index = GET2U ();

+ #ifdef DIRECT_THREADED
+ 	CHECK_INSN (&&insn_invokevirtual);
+ #endif /* DIRECT_THREADED */
+
  	/* _Jv_Linker::resolve_pool_entry returns immediately if the
  	 * value already is resolved.  If we want to clutter up the
  	 * code here to gain a little performance, then we can check
***************
*** 1822,1827 ****
--- 1892,1902 ----
      insn_getstatic:
        {
  	jint fieldref_index = GET2U ();
+
+ #ifdef DIRECT_THREADED
+ 	CHECK_INSN (&&insn_getstatic);
+ #endif /* DIRECT_THREADED */
+
          SAVE_PC(); // Constant pool resolution could throw.
  	_Jv_Linker::resolve_pool_entry (meth->defining_class, fieldref_index);
  	_Jv_Field *field = pool_data[fieldref_index].field;
***************
*** 1910,1915 ****
--- 1985,1995 ----
        {
  	SAVE_PC();
  	jint fieldref_index = GET2U ();
+
+ #ifdef DIRECT_THREADED
+ 	CHECK_INSN (&&insn_getfield);
+ #endif /* DIRECT_THREADED */
+
  	_Jv_Linker::resolve_pool_entry (meth->defining_class, fieldref_index);
  	_Jv_Field *field = pool_data[fieldref_index].field;

***************
*** 2024,2029 ****
--- 2104,2114 ----
        {
  	SAVE_PC();
  	jint fieldref_index = GET2U ();
+
+ #ifdef DIRECT_THREADED
+ 	CHECK_INSN (&&insn_putstatic);
+ #endif /* DIRECT_THREADED */
+
  	_Jv_Linker::resolve_pool_entry (meth->defining_class, fieldref_index);
  	_Jv_Field *field = pool_data[fieldref_index].field;

***************
*** 2111,2116 ****
--- 2196,2206 ----
        {
  	SAVE_PC();
  	jint fieldref_index = GET2U ();
+
+ #ifdef DIRECT_THREADED
+ 	CHECK_INSN (&&insn_putfield);
+ #endif /* DIRECT_THREADED */
+
  	_Jv_Linker::resolve_pool_entry (meth->defining_class, fieldref_index);
  	_Jv_Field *field = pool_data[fieldref_index].field;

***************
*** 2235,2240 ****
--- 2325,2334 ----
  	SAVE_PC();
  	int index = GET2U ();

+ #ifdef DIRECT_THREADED
+ 	CHECK_INSN (&&insn_invokespecial);
+ #endif /* DIRECT_THREADED */
+
  	rmeth = (_Jv_Linker::resolve_pool_entry (meth->defining_class,
  						   index)).rmethod;

***************
*** 2280,2285 ****
--- 2374,2383 ----
  	SAVE_PC();
  	int index = GET2U ();

+ #ifdef DIRECT_THREADED
+ 	CHECK_INSN (&&insn_invokestatic);
+ #endif /* DIRECT_THREADED */
+
  	rmeth = (_Jv_Linker::resolve_pool_entry (meth->defining_class,
  						   index)).rmethod;

***************
*** 2311,2316 ****
--- 2409,2418 ----
  	SAVE_PC();
  	int index = GET2U ();

+ #ifdef DIRECT_THREADED
+ 	CHECK_INSN (&&insn_invokeinterface);
+ #endif /* DIRECT_THREADED */
+
  	rmeth = (_Jv_Linker::resolve_pool_entry (meth->defining_class,
  						   index)).rmethod;

***************
*** 2356,2361 ****
--- 2458,2468 ----
        {
  	SAVE_PC();
  	int index = GET2U ();
+
+ #ifdef DIRECT_THREADED
+ 	CHECK_INSN (&&insn_new);
+ #endif /* DIRECT_THREADED */
+
  	jclass klass = (_Jv_Linker::resolve_pool_entry (meth->defining_class,
  							  index)).clazz;
  	/* VM spec, section 3.11.5 */
***************
*** 2398,2403 ****
--- 2505,2515 ----
        {
  	SAVE_PC();
  	int index = GET2U ();
+
+ #ifdef DIRECT_THREADED
+ 	CHECK_INSN (&&insn_anewarray);
+ #endif /* DIRECT_THREADED */
+
  	jclass klass = (_Jv_Linker::resolve_pool_entry (meth->defining_class,
  							  index)).clazz;
  	int size  = POPI();
***************
*** 2443,2448 ****
--- 2555,2565 ----
          SAVE_PC();
  	jobject value = POPA();
  	jint index = GET2U ();
+
+ #ifdef DIRECT_THREADED
+ 	CHECK_INSN (&&insn_checkcast);
+ #endif /* DIRECT_THREADED */
+
  	jclass to = (_Jv_Linker::resolve_pool_entry (meth->defining_class,
  						       index)).clazz;

***************
*** 2473,2478 ****
--- 2590,2600 ----
          SAVE_PC();
  	jobject value = POPA();
  	jint index = GET2U ();
+
+ #ifdef DIRECT_THREADED
+ 	CHECK_INSN (&&insn_instanceof);
+ #endif /* DIRECT_THREADED */
+
  	jclass to = (_Jv_Linker::resolve_pool_entry (meth->defining_class,
  						       index)).clazz;
  	PUSHI (to->isInstance (value));
Index: interpret.cc
===================================================================
*** interpret.cc	(revision 139469)
--- interpret.cc	(working copy)
***************
*** 77,87 ****
--- 77,89 ----
  // We could use a finer-grained lock here, however it is not safe to use
  // the Class monitor as user code in another thread could hold it.
  static _Jv_Mutex_t compile_mutex;
+ static _Jv_Mutex_t rewrite_mutex;

  void
  _Jv_InitInterpreter()
  {
    _Jv_MutexInit (&compile_mutex);
+   _Jv_MutexInit (&rewrite_mutex);
  }
  #else
  void _Jv_InitInterpreter() {}

Re: Remove data race in libgcj interpreter

[David Daney]

Andrew Haley wrote:
> +
> + #ifdef DIRECT_THREADED
> + 	CHECK_INSN (&&insn_instanceof);
> + #endif /* DIRECT_THREADED */
> +

Just a little style pedantry...

Should we re-write the CHECK_INSN macro so that it is a nop ifndef 
DIRECT_THREADED?

That would make the many places you have this construct look much cleaner.

David Daney

Re: Remove data race in libgcj interpreter

[Andrew Haley]

David Daney wrote:
> Andrew Haley wrote:
>> +
>> + #ifdef DIRECT_THREADED
>> +     CHECK_INSN (&&insn_instanceof);
>> + #endif /* DIRECT_THREADED */
>> +
> 
> Just a little style pedantry...
> 
> Should we re-write the CHECK_INSN macro so that it is a nop ifndef
> DIRECT_THREADED?
> 
> That would make the many places you have this construct look much cleaner.

OK, but would it be easier to understand when reading the code?  That's
why I didn't do that.  Sometimes clarity and cleanliness don't go
together.  What do you think?

Andrew.

Re: Remove data race in libgcj interpreter

[David Daney]

Andrew Haley wrote:
> David Daney wrote:
>> Andrew Haley wrote:
>>> +
>>> + #ifdef DIRECT_THREADED
>>> +     CHECK_INSN (&&insn_instanceof);
>>> + #endif /* DIRECT_THREADED */
>>> +
>> Just a little style pedantry...
>>
>> Should we re-write the CHECK_INSN macro so that it is a nop ifndef
>> DIRECT_THREADED?
>>
>> That would make the many places you have this construct look much cleaner.
> 
> OK, but would it be easier to understand when reading the code?  That's
> why I didn't do that.  Sometimes clarity and cleanliness don't go
> together.  What do you think?

For me there are two issues:

1) Mutex free synchronization scares me.  But That part of the patch 
seems plausible.

2) Of much less importance is the question of littering the code with 
#ifdef.  I stated my preference, but don't feel strongly enough about it 
that I would be offended if your original patch were committed.

David Daney

RE: Remove data race in libgcj interpreter

[Boehm, Hans]

>
> +
> /*************************************************************
> **********
> +   Locking in the direct-threaded case is quite subtle.  See PR
> +   libgcj/8995 for more details.
> +
> +   We must ensure that no-one ever reads an inconsistent {insn,value}
> +   pair.  Rather than trying to lock everything we detect when some
> +   other thread has modified the insn we are trying to
> modify and back
> +   out.
> +
> +   When we read a pair before rewriting it we do this:
> +
> +     i1 = insn
> +     v = value
> +     lock
> +       i2 = insn
> +     unlock
> +     if (i2 != i1) try again
This is probably statistically likely to generate correct code on some machines, but it doesn't look correct to me.  I think you're counting on the three loads to be performed in the order written.  But the compiler gets to reshuffle the first two.  And there is generally no prohibition against moving loads INTO a critical section, so they can in fact also be arbitrarily reordered with the i2 assignment as well.  (On IA64 and SPARC, the hardware might conceivably do that, depending on the lock implementation.  There is a long story about when this is Posix compatible, but it doesn't really matter here.)

There is also another important issue:  Posix and the upcoming C++ standard (among others) give undefined behavior to any program involving a data race.  Clearly this program involves data races on the initial insn and value reads.  The problem is that there are several good reasons for the Posix and C++ positions:

1) Compilers, including I believe gcc, implicitly assume that there are no asynchronous changes on non-volatile variables.  If you invalidate this assumption, weird things may happen.  These weird things don't always correspond to reading a different value.  For example, the compiler may reread a register copy of the value from the shared variable when it has to spill the register, making it appear that a local variable (which is a copy of the shared variable) suddenly changes without an assignment to it.  It's not hard to come up with cases in which this would provoke a crash.  Empirically, this sort of thing doesn't happen much, but we were told at some point that gcc may behave this way.

2) I would guess that gcc supports some platforms on which things like pointer loads are not always atomic.

3) In the not-too-distant future, we'd really like to have reliable data-race detectors.  This is much easier if code doesn't contain intentional data races.

4) None of us has managed to come up with an entirely satisfactory definition of what programs with data races mean.  The JSR133 definition is reasonably close, but still has some issues that have come up recently.  And it would probably have more serious performance implications on something like C++.

I haven't thought though this through enough.  Does it work to instead keep a shared (possibly per method?) count of the number of running interpreters, together with a shared bit (tested on backward branches) asking them to stop?  You only modify the code if no interpreters are running (in that method).  Getting this right is still nontrivial, but the races are restricted to well-defined atomic variables, which is fine.

Hans

> +
> +   and when we write a pair we do this:
> +
> +      lock
> +        value = v
> +        write barrier
> +        insn = i
> +      unlock
> +
> +   There is a read barrier in NEXT_INSN which ensures that we don't
> +   read a rewritten value with a stale insn.
> +
> +

Re: Remove data race in libgcj interpreter

[Andrew Haley]

Boehm, Hans wrote:
>> +
>> /*************************************************************
>> **********
>> +   Locking in the direct-threaded case is quite subtle.  See PR
>> +   libgcj/8995 for more details.
>> +
>> +   We must ensure that no-one ever reads an inconsistent {insn,value}
>> +   pair.  Rather than trying to lock everything we detect when some
>> +   other thread has modified the insn we are trying to
>> modify and back
>> +   out.
>> +
>> +   When we read a pair before rewriting it we do this:
>> +
>> +     i1 = insn
>> +     v = value
>> +     lock
>> +       i2 = insn
>> +     unlock
>> +     if (i2 != i1) try again

Ah, thanks for replying.  I was hoping you'd see this.

> This is probably statistically likely to generate correct code on
> some machines, but it doesn't look correct to me.  I think you're
> counting on the three loads to be performed in the order written.
> But the compiler gets to reshuffle the first two.  And there is
> generally no prohibition against moving loads INTO a critical
> section, so they can in fact also be arbitrarily reordered with the
> i2 assignment as well.  (On IA64 and SPARC, the hardware might
> conceivably do that, depending on the lock implementation.  There is
> a long story about when this is Posix compatible, but it doesn't
> really matter here.)

You're right, I was assuming exactly that.  OK, but that is presumably
simply a lack of volatility in the declaration of PC.

> There is also another important issue: Posix and the upcoming C++
> standard (among others) give undefined behavior to any program
> involving a data race.  Clearly this program involves data races on
> the initial insn and value reads.  The problem is that there are
> several good reasons for the Posix and C++ positions:

> 1) Compilers, including I believe gcc, implicitly assume that there
> are no asynchronous changes on non-volatile variables.  If you
> invalidate this assumption, weird things may happen.  These weird
> things don't always correspond to reading a different value.  For
> example, the compiler may reread a register copy of the value from
> the shared variable when it has to spill the register, making it
> appear that a local variable (which is a copy of the shared
> variable) suddenly changes without an assignment to it.  It's not
> hard to come up with cases in which this would provoke a crash.
> Empirically, this sort of thing doesn't happen much, but we were
> told at some point that gcc may behave this way.

> 2) I would guess that gcc supports some platforms on which things
> like pointer loads are not always atomic.

Sure, but on such platforms gcj won't run so I don't care about them.

> 3) In the not-too-distant future, we'd really like to have reliable
> data-race detectors.  This is much easier if code doesn't contain
> intentional data races.

Point taken.

> 4) None of us has managed to come up with an entirely satisfactory
> definition of what programs with data races mean.  The JSR133
> definition is reasonably close, but still has some issues that have
> come up recently.  And it would probably have more serious
> performance implications on something like C++.

> I haven't thought though this through enough.  Does it work to
> instead keep a shared (possibly per method?) count of the number of
> running interpreters, together with a shared bit (tested on backward
> branches) asking them to stop?  You only modify the code if no
> interpreters are running (in that method).  Getting this right is
> still nontrivial, but the races are restricted to well-defined
> atomic variables, which is fine.

Thank you for that suggestion.  I'll think some more.

Andrew.

Re: Remove data race in libgcj interpreter

[Hans Boehm]

On Fri, 5 Sep 2008, Andrew Haley wrote:

> You're right, I was assuming exactly that.  OK, but that is presumably
> simply a lack of volatility in the declaration of PC.
As a practical matter, that would probably help at the expense
of slowing things down a bit on some architectures.  Officially this
doesn't help much, since I believe Posix allows undefined behavior
for data races, even if they involve volatiles.  Even in terms of
current implementations, I don't believe it prevents hardware reordering
on architectures like PowerPC.

The C++ working paper provides atomic operations to address these issues. 
I think they're even somewhat implemented in the gcc trunk.  But I'm not 
sure you can avoid operations that generate fences on some architectures 
in what should be the fast path.

Hans

Re: Remove data race in libgcj interpreter

[Andrew Haley]

Hans Boehm wrote:
> 
> 
> On Fri, 5 Sep 2008, Andrew Haley wrote:
> 
>> You're right, I was assuming exactly that.  OK, but that is presumably
>> simply a lack of volatility in the declaration of PC.
> As a practical matter, that would probably help at the expense
> of slowing things down a bit on some architectures.  Officially this
> doesn't help much, since I believe Posix allows undefined behavior
> for data races, even if they involve volatiles.  Even in terms of
> current implementations, I don't believe it prevents hardware reordering
> on architectures like PowerPC.
> 
> The C++ working paper provides atomic operations to address these
> issues. I think they're even somewhat implemented in the gcc trunk.  But
> I'm not sure you can avoid operations that generate fences on some
> architectures in what should be the fast path.

Indeed.  The solution of detecting the case where two threads are concurrently
executing a method seems far the best: it's obviously correct without requiring
deep analysis of memory behaviour.  I'm investigating that.

Andrew.

Re: Remove data race in libgcj interpreter

[Andrew Haley]

I've thought about this a good deal.

Following Hans's suggestion I've added a field thread_count to
class _Jv_InterpMethod and a mutex that's used whenever we rewrite
instructions.  Whenever we enter a method context we create an
instance of ThreadCountAdjuster.  In the ThreadCountAdjuster constructor
we lock the mutex and increment the thread_count in the method.
We also look at the previous method we were executing and decrement
its thread count.  Iff the thread count of a method is <= 1 we
are allowed to rewrite instructions in the method.

We don't have a data race if another thread enters a method while we
are rewriting an instruction because the new thread will block on
the mutex on entry.

There is a small wrinkle in that proxy frames and JNI frames appear
in the method stack but don't have an associated _Jv_InterpMethod.
We detect this case and don't attempt to adjust the thread count.
This means that we might miss an optimization opportunity, but
this is an unusual case and is of little importance.

Andrew.


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..
	
Index: interpret.cc
===================================================================
*** interpret.cc	(revision 139469)
--- interpret.cc	(working copy)
*************** static void find_catch_location (jthrowa
*** 78,87 ****
--- 78,92 ----
  // 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() {}
Index: include/java-interp.h
===================================================================
*** include/java-interp.h	(revision 139469)
--- include/java-interp.h	(working copy)
*************** class _Jv_InterpMethod : public _Jv_Meth
*** 175,180 ****
--- 175,191 ----
    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
*************** public:
*** 455,463 ****
    jobject obj_ptr;

    _Jv_InterpFrame (void *meth, java::lang::Thread *thr, jclass proxyCls = NULL,
!                    pc_t *pc = NULL)
    : _Jv_Frame (reinterpret_cast<_Jv_MethodBase *> (meth), thr,
! 	             frame_interpreter)
    {
      next_interp = (_Jv_InterpFrame *) thr->interp_frame;
      proxyClass = proxyCls;
--- 466,475 ----
    jobject obj_ptr;

    _Jv_InterpFrame (void *meth, java::lang::Thread *thr, jclass proxyCls = NULL,
!                    pc_t *pc = NULL,
! 		   _Jv_FrameType frame_type = frame_interpreter)
    : _Jv_Frame (reinterpret_cast<_Jv_MethodBase *> (meth), thr,
! 	             frame_type)
    {
      next_interp = (_Jv_InterpFrame *) thr->interp_frame;
      proxyClass = proxyCls;
*************** public:
*** 501,506 ****
--- 513,588 ----
    }
  };

+ #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__ */
Index: ChangeLog
===================================================================
*** ChangeLog	(revision 139493)
--- ChangeLog	(working copy)
***************
*** 1,3 ****
--- 1,24 ----
+ 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-08-22  Andrew Haley  <aph@redhat.com>

  	PR libgcj/8995:
Index: interpret-run.cc
===================================================================
*** interpret-run.cc	(revision 139978)
--- interpret-run.cc	(working copy)
*************** details.  */
*** 29,34 ****
--- 29,38 ----
    _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;

*************** details.  */
*** 361,380 ****
      }									\
    while (0)

  #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;						\
  									\
!     pc[-1].SLOT = VALUE;						\
!   }									\
    while (0)

  #undef INTERP_REPORT_EXCEPTION
--- 365,393 ----
      }									\
    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									\
!     {									\
!       _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;						\
! 	}								\
!       _Jv_MutexUnlock (&rewrite_insn_mutex);				\
!     }									\
    while (0)

  #undef INTERP_REPORT_EXCEPTION
*************** details.  */
*** 383,405 ****
  #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)

  #undef INTERP_REPORT_EXCEPTION
  #define INTERP_REPORT_EXCEPTION(Jthrowable) /* not needed when not debugging */
--- 396,418 ----
  #undef NEXT_INSN
  #define NEXT_INSN goto *((pc++)->insn)

  // Rewriting a multi-word instruction in the presence of multiple
! // 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 */
Index: java/lang/reflect/natVMProxy.cc
===================================================================
*** java/lang/reflect/natVMProxy.cc	(revision 139469)
--- java/lang/reflect/natVMProxy.cc	(working copy)
*************** run_proxy (ffi_cif *cif,
*** 350,356 ****
    // 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);

    // The method to invoke is saved in $Proxy0.m[method_index].
    // FIXME: We could somewhat improve efficiency by storing a pointer
--- 350,357 ----
    // 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,
! 			      NULL, frame_proxy);

    // The method to invoke is saved in $Proxy0.m[method_index].
    // FIXME: We could somewhat improve efficiency by storing a pointer
Index: defineclass.cc
===================================================================
*** defineclass.cc	(revision 139469)
--- defineclass.cc	(working copy)
*************** void _Jv_ClassReader::handleCodeAttribut
*** 1682,1688 ****
    method->prepared       = NULL;
    method->line_table_len = 0;
    method->line_table     = NULL;
!

    // grab the byte code!
    memcpy ((void*) method->bytecode (),
--- 1682,1690 ----
    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 (),