From: Andrew Haley <aph@redhat.com>
To: Java Patch List <java-patches@gcc.gnu.org>
Subject: Re: Remove data race in libgcj interpreter
Date: Thu, 04 Sep 2008 16:00:00 -0000 [thread overview]
Message-ID: <48C0042C.5080804@redhat.com> (raw)
In-Reply-To: <48AD67B2.4040308@redhat.com>
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() {}
next prev parent reply other threads:[~2008-09-04 15:52 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-08-21 13:31 Andrew Haley
2008-08-21 21:54 ` Andrew Haley
2008-08-22 12:12 ` gij: more thread-safety fixes Andrew Haley
2008-08-22 12:51 ` Remove data race in libgcj interpreter Bryce McKinlay
2008-08-24 1:46 ` Andrew Haley
2008-09-04 16:00 ` Andrew Haley [this message]
2008-09-04 16:12 ` David Daney
2008-09-04 16:25 ` Andrew Haley
2008-09-04 18:37 ` David Daney
2008-09-05 9:39 ` Boehm, Hans
2008-09-05 11:28 ` Andrew Haley
2008-09-06 9:26 ` Hans Boehm
2008-09-08 17:42 ` Andrew Haley
2008-09-22 14:25 ` Andrew Haley
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=48C0042C.5080804@redhat.com \
--to=aph@redhat.com \
--cc=java-patches@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).