This is the mail archive of the java-patches@gcc.gnu.org mailing list for the Java project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Remove data race in libgcj interpreter


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]