From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6404 invoked by alias); 21 Aug 2008 13:04:36 -0000 Received: (qmail 6389 invoked by uid 22791); 21 Aug 2008 13:04:34 -0000 X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (66.187.233.31) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 21 Aug 2008 13:03:51 +0000 Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id m7LD3n3l021394 for ; Thu, 21 Aug 2008 09:03:49 -0400 Received: from zebedee.pink (vpn-12-59.rdu.redhat.com [10.11.12.59]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id m7LD3kie021144; Thu, 21 Aug 2008 09:03:47 -0400 Message-ID: <48AD67B2.4040308@redhat.com> Date: Thu, 21 Aug 2008 13:31:00 -0000 From: Andrew Haley User-Agent: Thunderbird 2.0.0.16 (X11/20080707) MIME-Version: 1.0 To: Java Patch List , Jakub Jelinek , Lillian Angel Subject: Remove data race in libgcj interpreter Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes Mailing-List: contact java-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: java-patches-owner@gcc.gnu.org X-SW-Source: 2008-q3/txt/msg00047.txt.bz2 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 to invokespecial_resolved
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
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 * 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 */