From patchwork Tue Jan 10 13:43:05 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Torvald Riegel X-Patchwork-Id: 135246 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 826CEB6FBB for ; Wed, 11 Jan 2012 01:33:30 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1326810810; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Subject:From:To:Cc:In-Reply-To:References:Content-Type:Date: Message-ID:Mime-Version:Mailing-List:Precedence:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=c/UKJ4+9nBUVmZvKup5FViWwAKs=; b=PPa0hU5ks8KLD+G T73kUs8E9QKwLlnRbiyvn6ySofsh0W7NQ0BZ/KFRA2pHIimyLG9VP/YqwZ/DVhES iQRiMMendZPyXDnUicRL+ZeMIDza8b7EoW3S9NOXnbAsUeWhNgcB2+LN/IzUBfmr 0s0WnBSVL+nv3m6TB7pT/u8uj1BE= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Subject:From:To:Cc:In-Reply-To:References:Content-Type:Date:Message-ID:Mime-Version:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=TbGTbwMDpsPw1YOBRBjhpzsK2Al+7I3JqPDiBnOs6LFBQK33aKIhQLZqdimQrv eFy5GnUzwFQeTKrY96n+qbcQmZoQ+seqxeZxkgmUvt0XmLDqOi378lZpkUb7ss5D 4/AtdTWo13n0iLwzC3T3J0NSIZ2nxVlVQ+oogOVS6SlzA=; Received: (qmail 32459 invoked by alias); 10 Jan 2012 14:33:16 -0000 Received: (qmail 32145 invoked by uid 22791); 10 Jan 2012 14:33:15 -0000 X-SWARE-Spam-Status: No, hits=-6.7 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_HI, SPF_HELO_PASS, TW_CP, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 10 Jan 2012 14:32:59 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q0AEWxp7028536 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 10 Jan 2012 09:32:59 -0500 Received: from [10.36.6.139] (vpn1-6-139.ams2.redhat.com [10.36.6.139]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q0ADh6uC014273; Tue, 10 Jan 2012 08:43:06 -0500 Subject: Re: [RFC, patch] libitm: Filter out undo writes that overlap with the libitm stack. From: Torvald Riegel To: Richard Henderson Cc: GCC Patches , Aldy Hernandez In-Reply-To: <4F0BCE1D.8050700@redhat.com> References: <1326050802.7636.4787.camel@triegel.csb> <4F0BCE1D.8050700@redhat.com> Date: Tue, 10 Jan 2012 14:43:05 +0100 Message-ID: <1326202985.23708.1320.camel@triegel.csb> Mime-Version: 1.0 X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org On Tue, 2012-01-10 at 16:35 +1100, Richard Henderson wrote: > On 01/09/2012 06:26 AM, Torvald Riegel wrote: > > libitm: Filter out undo writes that overlap with the libitm stack. > > > > libitm/ > > * config/generic/tls.h (GTM::mask_stack_top, > > GTM::mask_stack_bottom): New. > > * local.cc (gtm_undolog::rollback): Filter out any updates that > > overlap the libitm stack. Add current transaction as parameter. > > * libitm_i.h (GTM::gtm_undolog::rollback): Adapt. > > * beginend.cc (GTM::gtm_thread::rollback): Adapt. > > * testsuite/libitm.c/stackundo.c: New test. > > One could steal code from bohem-gc for this. > See GC_get_stack_base in os_dep.c. Thanks for the pointer. I looked at this code, and it seems fairly complex given the dependencies on OS/libc and OS/libc behavior. From a maintenance point-of-view, does it make sense to copy that complexity into libitm? boehm-gc is used in GCC, so perhaps that's not much of a problem, however. I also looked at glibc's memcpy implementations, and copying those plus a simple byte-wise copy for the generic case could be also a fairly clean solution. Also, is the license compatible with the GPL wrt. mixing sources? What about keeping the patch/hack that I posted for now, creating a PR, and looking at this again for another release? Attached a slightly updated version with just comments in local.cc changed. commit 02357c5f11138f512e714d1740491abc86c61388 Author: Torvald Riegel Date: Sun Jan 8 20:12:33 2012 +0100 libitm: Filter out undo writes that overlap with the libitm stack. libitm/ * config/generic/tls.h (GTM::mask_stack_top, GTM::mask_stack_bottom): New. * local.cc (gtm_undolog::rollback): Filter out any updates that overlap the libitm stack. Add current transaction as parameter. * libitm_i.h (GTM::gtm_undolog::rollback): Adapt. * beginend.cc (GTM::gtm_thread::rollback): Adapt. * testsuite/libitm.c/stackundo.c: New test. diff --git a/libitm/beginend.cc b/libitm/beginend.cc index fe14f32..08c2174 100644 --- a/libitm/beginend.cc +++ b/libitm/beginend.cc @@ -327,7 +327,7 @@ GTM::gtm_thread::rollback (gtm_transaction_cp *cp, bool aborting) // data. Because of the latter, we have to roll it back before any // dispatch-specific rollback (which handles synchronization with other // transactions). - undolog.rollback (cp ? cp->undolog_size : 0); + undolog.rollback (this, cp ? cp->undolog_size : 0); // Perform dispatch-specific rollback. abi_disp()->rollback (cp); diff --git a/libitm/config/generic/tls.h b/libitm/config/generic/tls.h index 6bbdccf..07efef3 100644 --- a/libitm/config/generic/tls.h +++ b/libitm/config/generic/tls.h @@ -60,6 +60,25 @@ static inline abi_dispatch * abi_disp() { return _gtm_thr_tls.disp; } static inline void set_abi_disp(abi_dispatch *x) { _gtm_thr_tls.disp = x; } #endif +#ifndef HAVE_ARCH_GTM_MASK_STACK +// To filter out any updates that overlap the libitm stack, we define +// gtm_mask_stack_top to the entry point to the library and +// gtm_mask_stack_bottom to below current function. This +// definition should be fine for all stack-grows-down architectures. +// FIXME We fake the bottom to be lower so that we are safe even if we might +// call further functions (compared to where we called gtm_mask_stack_bottom +// in the call hierarchy) to actually undo or redo writes (e.g., memcpy). +// This is a completely arbitrary value; can we instead ensure that there are +// no such calls, or can we determine a future-proof value otherwise? +static inline void * +mask_stack_top(gtm_thread *tx) { return tx->jb.cfa; } +static inline void * +mask_stack_bottom(gtm_thread *tx) +{ + return (uint8_t*)__builtin_dwarf_cfa() - 128; +} +#endif + } // namespace GTM #endif // LIBITM_TLS_H diff --git a/libitm/libitm_i.h b/libitm/libitm_i.h index f922d22..f849654 100644 --- a/libitm/libitm_i.h +++ b/libitm/libitm_i.h @@ -138,7 +138,7 @@ struct gtm_undolog size_t size() const { return undolog.size(); } // In local.cc - void rollback (size_t until_size = 0); + void rollback (gtm_thread* tx, size_t until_size = 0); }; // Contains all thread-specific data required by the entire library. diff --git a/libitm/local.cc b/libitm/local.cc index 39b6da3..8123063 100644 --- a/libitm/local.cc +++ b/libitm/local.cc @@ -26,11 +26,20 @@ namespace GTM HIDDEN { - -void -gtm_undolog::rollback (size_t until_size) +// This function needs to be noinline because we need to prevent that it gets +// inlined into another function that calls further functions. This could +// break our assumption that we only call memcpy and thus only need to +// additionally protect the memcpy stack (see the hack in mask_stack_bottom()). +// Even if that isn't an issue because those other calls don't happen during +// copying, we still need mask_stack_bottom() to be called "close" to the +// memcpy in terms of stack frames, so just ensure that for now using the +// noinline. +void __attribute__((noinline)) +gtm_undolog::rollback (gtm_thread* tx, size_t until_size) { size_t i, n = undolog.size(); + void *top = mask_stack_top(tx); + void *bot = mask_stack_bottom(tx); if (n > 0) { @@ -40,7 +49,17 @@ gtm_undolog::rollback (size_t until_size) size_t len = undolog[i]; size_t words = (len + sizeof(gtm_word) - 1) / sizeof(gtm_word); i -= words; - __builtin_memcpy (ptr, &undolog[i], len); + // Filter out any updates that overlap the libitm stack. We don't + // bother filtering out just the overlapping bytes because we don't + // merge writes and thus any overlapping write is either bogus or + // would restore data on stack frames that are not in use anymore. + // FIXME The memcpy can/will end up as another call but we + // calculated BOT based on the current function. Can we inline or + // reimplement this without too much trouble due to unaligned calls + // and still have good performance, so that we can remove the hack + // in mask_stack_bottom()? + if (likely(ptr > top || (uint8_t*)ptr + len <=bot)) + __builtin_memcpy (ptr, &undolog[i], len); } } } diff --git a/libitm/testsuite/libitm.c/stackundo.c b/libitm/testsuite/libitm.c/stackundo.c new file mode 100644 index 0000000..02759d7 --- /dev/null +++ b/libitm/testsuite/libitm.c/stackundo.c @@ -0,0 +1,23 @@ +int __attribute__((noinline)) test2(int x[1000]) +{ + int i; + return x[12]; +} + +int __attribute__((noinline)) test1() +{ + int x[1000], i; + + for (i = 0; i < 1000; i++) + x[i] = i; + return test2(x); +} + +int main() +{ + __transaction_atomic { + if (test1() !=0) + __transaction_cancel; + } + return 0; +}