[trans-mem] Discard dead commits to stack memory

[trans-mem] Discard dead commits to stack memory

[Richard Henderson]

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

The problem being addressed here is that somewhere in the call stack
of a transaction, we can wind up instrumenting local variables.  When
those subroutines return, the stack gets re-used.  It can happen that
that re-use happens to be within the stack frames of libitm itself.

If we do nothing, we risk clobbering libitm's stack frames and 
crashing on the way back out to the main application.


r~

[-- Attachment #2: commit-528b666 --]
[-- Type: text/plain, Size: 16175 bytes --]

commit 528b6661ad21c00f23b6e47abf99bddae0e06fb2
Author: Richard Henderson <rth@twiddle.net>
Date:   Wed Jun 16 15:29:58 2010 -0700

    Discard obsolete changes that overlap the libitm stack.

diff --git a/libitm/ChangeLog b/libitm/ChangeLog
index d384a05..0b427b7 100644
--- a/libitm/ChangeLog
+++ b/libitm/ChangeLog
@@ -1,5 +1,23 @@
 2010-06-16  Richard Henderson  <rth@redhat.com>
 
+	* method-wbetl.cc (wbetl_dispatch::trycommit): Discard changes
+	that overlap the libitm stack.
+	* barrier.tpl: Mark incoming stack.
+	* beginend.cc (_ITM_rollbackTransaction, _ITM_abortTransaction,
+	_ITM_commitTransaction, _ITM_commitTransactionEH): Likewise.
+	* clone.cc (_ITM_getTMCloneOrIrrevocable): Likewise.
+	* memcpy.cc, memset.cc: Likewise.
+	* method-serial.cc (_ITM_changeTransactionMode): Likewise.
+	* config/generic/tls.h (gtm_thread): Add stack_top member.
+	(gtm_stack_top, set_gtm_stack_top, struct gtm_stack_marker): New.
+	* libitm_i.h (gtm_mask_stack): Declare.
+	* config/generic/tls.cc: New file.
+	* Makefile.am (libitm_la_SOURCES): Add it.
+	(AM_CXXFLAGS): Turn off exceptions.
+	* Makefile.in: Rebuild.
+	
+2010-06-16  Richard Henderson  <rth@redhat.com>
+
 	* alloc.cc (struct gtm_alloc_action): Move definition ...
 	* libitm_i.h: ... here.
 	(class gtm_transaction): Declare new and delete.
diff --git a/libitm/Makefile.am b/libitm/Makefile.am
index 31a75c8..fef2a13 100644
--- a/libitm/Makefile.am
+++ b/libitm/Makefile.am
@@ -18,7 +18,7 @@ vpath % $(strip $(search_path))
 
 AM_CPPFLAGS = $(addprefix -I, $(search_path))
 AM_CFLAGS = $(XCFLAGS)
-AM_CXXFLAGS = -std=gnu++0x -fno-rtti $(XCFLAGS) $(abi_version)
+AM_CXXFLAGS = -std=gnu++0x -fno-exceptions -fno-rtti $(XCFLAGS) $(abi_version)
 AM_CCASFLAGS = $(XCFLAGS)
 AM_LDFLAGS = $(XLDFLAGS) $(SECTION_LDFLAGS) $(OPT_LDFLAGS)
 
@@ -32,8 +32,8 @@ libitm_version_script =
 endif
 libitm_version_info = -version-info $(libtool_VERSION)
 
-## Force link with C, not C++.  For now, while we're using C++ we don't
-## want or need libstdc++.
+# Force link with C, not C++.  For now, while we're using C++ we don't
+# want or need libstdc++.
 libitm_la_LINK = $(LINK)
 libitm_la_LDFLAGS = $(libitm_version_info) $(libitm_version_script) \
         -no-undefined
@@ -42,7 +42,7 @@ libitm_la_SOURCES = \
 	aatree.cc alloc.cc alloc_c.cc alloc_cpp.cc barrier.cc beginend.cc \
 	clone.cc cacheline.cc cachepage.cc eh_cpp.cc local.cc memcpy.cc \
 	memset.cc query.cc retry.cc rwlock.cc useraction.cc util.cc \
-	sjlj.S method-serial.cc method-readonly.cc method-wbetl.cc
+	sjlj.S tls.cc method-serial.cc method-readonly.cc method-wbetl.cc
 
 if ARCH_X86
 libitm_la_SOURCES += x86_sse.cc x86_avx.cc
diff --git a/libitm/Makefile.in b/libitm/Makefile.in
index ed77a38..723669a 100644
--- a/libitm/Makefile.in
+++ b/libitm/Makefile.in
@@ -94,15 +94,16 @@ libitm_la_LIBADD =
 am__libitm_la_SOURCES_DIST = aatree.cc alloc.cc alloc_c.cc \
 	alloc_cpp.cc barrier.cc beginend.cc clone.cc cacheline.cc \
 	cachepage.cc eh_cpp.cc local.cc memcpy.cc memset.cc query.cc \
-	retry.cc rwlock.cc useraction.cc util.cc sjlj.S \
+	retry.cc rwlock.cc useraction.cc util.cc sjlj.S tls.cc \
 	method-serial.cc method-readonly.cc method-wbetl.cc x86_sse.cc \
 	x86_avx.cc
 @ARCH_X86_TRUE@am__objects_1 = x86_sse.lo x86_avx.lo
 am_libitm_la_OBJECTS = aatree.lo alloc.lo alloc_c.lo alloc_cpp.lo \
 	barrier.lo beginend.lo clone.lo cacheline.lo cachepage.lo \
 	eh_cpp.lo local.lo memcpy.lo memset.lo query.lo retry.lo \
-	rwlock.lo useraction.lo util.lo sjlj.lo method-serial.lo \
-	method-readonly.lo method-wbetl.lo $(am__objects_1)
+	rwlock.lo useraction.lo util.lo sjlj.lo tls.lo \
+	method-serial.lo method-readonly.lo method-wbetl.lo \
+	$(am__objects_1)
 libitm_la_OBJECTS = $(am_libitm_la_OBJECTS)
 DEFAULT_INCLUDES = -I.@am__isrc@
 depcomp = $(SHELL) $(top_srcdir)/../depcomp
@@ -339,7 +340,7 @@ fincludedir = $(libdir)/gcc/$(target_alias)/$(gcc_version)/finclude
 libsubincludedir = $(libdir)/gcc/$(target_alias)/$(gcc_version)/include
 AM_CPPFLAGS = $(addprefix -I, $(search_path))
 AM_CFLAGS = $(XCFLAGS)
-AM_CXXFLAGS = -std=gnu++0x -fno-rtti $(XCFLAGS) $(abi_version)
+AM_CXXFLAGS = -std=gnu++0x -fno-exceptions -fno-rtti $(XCFLAGS) $(abi_version)
 AM_CCASFLAGS = $(XCFLAGS)
 AM_LDFLAGS = $(XLDFLAGS) $(SECTION_LDFLAGS) $(OPT_LDFLAGS)
 toolexeclib_LTLIBRARIES = libitm.la
@@ -347,6 +348,9 @@ nodist_toolexeclib_HEADERS = libitm.spec
 @LIBITM_BUILD_VERSIONED_SHLIB_FALSE@libitm_version_script = 
 @LIBITM_BUILD_VERSIONED_SHLIB_TRUE@libitm_version_script = -Wl,--version-script,$(top_srcdir)/libitm.map
 libitm_version_info = -version-info $(libtool_VERSION)
+
+# Force link with C, not C++.  For now, while we're using C++ we don't
+# want or need libstdc++.
 libitm_la_LINK = $(LINK)
 libitm_la_LDFLAGS = $(libitm_version_info) $(libitm_version_script) \
         -no-undefined
@@ -354,7 +358,7 @@ libitm_la_LDFLAGS = $(libitm_version_info) $(libitm_version_script) \
 libitm_la_SOURCES = aatree.cc alloc.cc alloc_c.cc alloc_cpp.cc \
 	barrier.cc beginend.cc clone.cc cacheline.cc cachepage.cc \
 	eh_cpp.cc local.cc memcpy.cc memset.cc query.cc retry.cc \
-	rwlock.cc useraction.cc util.cc sjlj.S method-serial.cc \
+	rwlock.cc useraction.cc util.cc sjlj.S tls.cc method-serial.cc \
 	method-readonly.cc method-wbetl.cc $(am__append_1)
 all: config.h
 	$(MAKE) $(AM_MAKEFLAGS) all-recursive
@@ -474,6 +478,7 @@ distclean-compile:
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/retry.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/rwlock.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/sjlj.Plo@am__quote@
+@AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/tls.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/useraction.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/util.Plo@am__quote@
 @AMDEP_TRUE@@am__include@ @am__quote@./$(DEPDIR)/x86_avx.Plo@am__quote@
diff --git a/libitm/barrier.tpl b/libitm/barrier.tpl
index a0a5940..b174791 100644
--- a/libitm/barrier.tpl
+++ b/libitm/barrier.tpl
@@ -122,11 +122,17 @@ void do_write (T *ptr, T val, gtm_dispatch::lock_type lock)
 
 #define ITM_READ(T, LOCK)						\
   _ITM_TYPE_##T ITM_REGPARM _ITM_##LOCK##T (const _ITM_TYPE_##T *ptr)	\
-  { return do_read (ptr, gtm_dispatch::LOCK); }
+  {									\
+    gtm_stack_marker marker;						\
+    return do_read (ptr, gtm_dispatch::LOCK);				\
+  }
 
 #define ITM_WRITE(T, LOCK)						\
   void ITM_REGPARM _ITM_##LOCK##T (_ITM_TYPE_##T *ptr, _ITM_TYPE_##T val) \
-  { do_write (ptr, val, gtm_dispatch::LOCK); }
+  {									\
+    gtm_stack_marker marker;						\
+    do_write (ptr, val, gtm_dispatch::LOCK);				\
+  }
 
 #define ITM_BARRIERS(T)		\
   ITM_READ(T, R)		\
diff --git a/libitm/beginend.cc b/libitm/beginend.cc
index f50503e..4bebb1e 100644
--- a/libitm/beginend.cc
+++ b/libitm/beginend.cc
@@ -84,7 +84,6 @@ GTM::gtm_transaction::operator delete(void *tx)
   thr->free_tx[idx] = tx;
 }
 
-
 uint32_t
 GTM::gtm_transaction::begin_transaction (uint32_t prop, const gtm_jmpbuf *jb)
 {
@@ -162,6 +161,8 @@ _ITM_rollbackTransaction (void)
   assert ((tx->prop & pr_hasNoAbort) == 0);
   assert ((tx->state & gtm_transaction::STATE_ABORTING) == 0);
 
+  gtm_stack_marker marker;
+
   tx->rollback ();
   tx->state |= gtm_transaction::STATE_ABORTING;
 }
@@ -178,6 +179,8 @@ _ITM_abortTransaction (_ITM_abortReason reason)
   if (tx->state & gtm_transaction::STATE_IRREVOCABLE)
     abort ();
 
+  gtm_stack_marker marker;
+
   tx->rollback ();
   gtm_disp()->fini ();
 
@@ -251,6 +254,8 @@ void ITM_REGPARM
 _ITM_commitTransaction(void)
 {
   gtm_transaction *tx = gtm_tx();
+  gtm_stack_marker marker;
+
   if (!tx->trycommit_and_finalize ())
     tx->restart (RESTART_VALIDATE_COMMIT);
 }
@@ -259,6 +264,8 @@ void ITM_REGPARM
 _ITM_commitTransactionEH(void *exc_ptr)
 {
   gtm_transaction *tx = gtm_tx();
+  gtm_stack_marker marker;
+
   if (!tx->trycommit_and_finalize ())
     {
       tx->eh_in_flight = exc_ptr;
diff --git a/libitm/clone.cc b/libitm/clone.cc
index 2b4e9a8..46d6328 100644
--- a/libitm/clone.cc
+++ b/libitm/clone.cc
@@ -92,7 +92,9 @@ _ITM_getTMCloneOrIrrevocable (void *ptr)
   if (ret)
     return ret;
 
+  gtm_stack_marker marker;
   gtm_tx()->serialirr_mode ();
+
   return ptr;
 }
 
diff --git a/libitm/config/generic/tls.cc b/libitm/config/generic/tls.cc
new file mode 100644
index 0000000..1e541b6
--- /dev/null
+++ b/libitm/config/generic/tls.cc
@@ -0,0 +1,76 @@
+/* Copyright (C) 2010 Free Software Foundation, Inc.
+   Contributed by Richard Henderson <rth@redhat.com>.
+
+   This file is part of the GNU Transactional Memory Library (libitm).
+
+   Libitm is free software; you can redistribute it and/or modify it
+   under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   Libitm is distributed in the hope that it will be useful, but WITHOUT ANY
+   WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+   FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+   more details.
+
+   Under Section 7 of GPL version 3, you are granted additional
+   permissions described in the GCC Runtime Library Exception, version
+   3.1, as published by the Free Software Foundation.
+
+   You should have received a copy of the GNU General Public License and
+   a copy of the GCC Runtime Library Exception along with this program;
+   see the files COPYING3 and COPYING.RUNTIME respectively.  If not, see
+   <http://www.gnu.org/licenses/>.  */
+
+#include "libitm_i.h"
+
+namespace GTM HIDDEN {
+
+// Filter out any updates that overlap the libitm stack, as defined by
+// TOP (entry point to library) and BOT (below current function).  This
+// definition should be fine for all stack-grows-down architectures.
+
+gtm_cacheline_mask __attribute__((noinline))
+gtm_mask_stack(gtm_cacheline *line, gtm_cacheline_mask mask)
+{
+  void *top = gtm_stack_top();
+  void *bot = __builtin_dwarf_cfa();
+
+  // We must have come through an entry point that set TOP.
+  assert (top != NULL);
+
+  if (line + 1 < bot)
+    {
+      // Since we don't have the REAL stack boundaries for this thread,
+      // we cannot know if this is a dead write to a stack address below
+      // the current function or if it is write to another VMA.  In either
+      // case allowing the write should not affect correctness.
+    }
+  else if (line >= top)
+    {
+      // A valid write to an address in an outer stack frame, or a write
+      // to another VMA.
+    }
+  else
+    {
+      uintptr_t diff = (uintptr_t)top - (uintptr_t)line;
+      if (diff >= CACHELINE_SIZE)
+	{
+	  // The write is either fully within the proscribed area, or the tail
+	  // of the cacheline overlaps the proscribed area.  Assume that all
+	  // stacks are at least cacheline aligned and declare the head of the
+	  // cacheline dead.
+	  mask = 0;
+	}
+      else
+	{
+	  // The head of the cacheline is within the proscribed area, but the
+	  // tail of the cacheline is live.  Eliminate the dead writes.
+	  mask &= (gtm_cacheline_mask)-1 << diff;
+	}
+    }
+
+  return mask;
+}
+
+} // namespace GTM
diff --git a/libitm/config/generic/tls.h b/libitm/config/generic/tls.h
index d1504e2..3bfad74 100644
--- a/libitm/config/generic/tls.h
+++ b/libitm/config/generic/tls.h
@@ -40,6 +40,10 @@ struct gtm_thread
   // if the target provides some efficient mechanism for storing this.
   gtm_dispatch *disp;
 #endif
+#ifndef HAVE_ARCH_GTM_THREAD_STACK_TOP
+  // The top of stack on entry to the STM library.
+  void *stack_top;
+#endif
 
   // The maximum number of free gtm_transaction structs to be kept.
   // This number must be greater than 1 in order for transaction abort
@@ -67,24 +71,35 @@ extern __thread gtm_thread _gtm_thr;
 #ifndef HAVE_ARCH_GTM_THREAD
 // If the target does not provide optimized access to the thread-local
 // data, simply access the TLS variable defined above.
-static inline void setup_gtm_thr(void) { }
-static inline gtm_thread *gtm_thr(void) { return &_gtm_thr; }
+static inline void setup_gtm_thr() { }
+static inline gtm_thread *gtm_thr() { return &_gtm_thr; }
 #endif
 
 #ifndef HAVE_ARCH_GTM_THREAD_TX
 // If the target does not provide optimized access to the currently
 // active transaction, simply access via GTM_THR.
-static inline gtm_transaction * gtm_tx(void) { return gtm_thr()->tx; }
+static inline gtm_transaction * gtm_tx() { return gtm_thr()->tx; }
 static inline void set_gtm_tx(gtm_transaction *x) { gtm_thr()->tx = x; }
 #endif
 
 #ifndef HAVE_ARCH_GTM_THREAD_DISP
 // If the target does not provide optimized access to the currently
 // active dispatch table, simply access via GTM_THR.
-static inline gtm_dispatch * gtm_disp(void) { return gtm_thr()->disp; }
+static inline gtm_dispatch * gtm_disp() { return gtm_thr()->disp; }
 static inline void set_gtm_disp(gtm_dispatch *x) { gtm_thr()->disp = x; }
 #endif
 
+#ifndef HAVE_ARCH_GTM_THREAD_STACK_TOP
+static inline void *gtm_stack_top() { return gtm_thr()->stack_top; }
+static inline void set_gtm_stack_top(void *t) { gtm_thr()->stack_top = t; }
+#endif
+
+struct gtm_stack_marker
+{
+  gtm_stack_marker(void *t = __builtin_dwarf_cfa()) { set_gtm_stack_top (t); }
+  ~gtm_stack_marker() { set_gtm_stack_top (0); }
+};
+
 } // namespace GTM
 
 #endif // LIBITM_TLS_H
diff --git a/libitm/libitm_i.h b/libitm/libitm_i.h
index 48b9bb3..3469857 100644
--- a/libitm/libitm_i.h
+++ b/libitm/libitm_i.h
@@ -285,6 +285,8 @@ extern gtm_dispatch *dispatch_wbetl();
 extern gtm_dispatch *dispatch_readonly();
 extern gtm_dispatch *dispatch_serial();
 
+extern gtm_cacheline_mask gtm_mask_stack(gtm_cacheline *, gtm_cacheline_mask);
+
 } // namespace GTM
 
 #endif // LIBITM_I_H
diff --git a/libitm/memcpy.cc b/libitm/memcpy.cc
index 672af98..c435294 100644
--- a/libitm/memcpy.cc
+++ b/libitm/memcpy.cc
@@ -302,11 +302,13 @@ do_memmove (uintptr_t idst, uintptr_t isrc, size_t size,
 #define ITM_MEM_DEF(NAME, READ, WRITE) \
 void ITM_REGPARM _ITM_memcpy##NAME(void *dst, const void *src, size_t size)  \
 {									     \
+  gtm_stack_marker marker;						     \
   do_memcpy ((uintptr_t)dst, (uintptr_t)src, size,			     \
 	     gtm_dispatch::WRITE, gtm_dispatch::READ);			     \
 }									     \
 void ITM_REGPARM _ITM_memmove##NAME(void *dst, const void *src, size_t size) \
 {									     \
+  gtm_stack_marker marker;						     \
   do_memmove ((uintptr_t)dst, (uintptr_t)src, size,			     \
 	      gtm_dispatch::WRITE, gtm_dispatch::READ);			     \
 }
diff --git a/libitm/memset.cc b/libitm/memset.cc
index d77b626..05584ec 100644
--- a/libitm/memset.cc
+++ b/libitm/memset.cc
@@ -69,7 +69,10 @@ do_memset(uintptr_t idst, int c, size_t size, gtm_dispatch::lock_type W)
 
 #define ITM_MEM_DEF(WRITE) \
 void ITM_REGPARM _ITM_memset##WRITE(void *dst, int c, size_t size)	\
-{ do_memset ((uintptr_t)dst, c, size, gtm_dispatch::WRITE); }
+{									\
+  gtm_stack_marker marker;						\
+  do_memset ((uintptr_t)dst, c, size, gtm_dispatch::WRITE);		\
+}
 
 ITM_MEM_DEF(W)
 ITM_MEM_DEF(WaR)
diff --git a/libitm/method-serial.cc b/libitm/method-serial.cc
index 2a69d48..c7c79f0 100644
--- a/libitm/method-serial.cc
+++ b/libitm/method-serial.cc
@@ -117,6 +117,7 @@ GTM::gtm_transaction::serialirr_mode ()
 void ITM_REGPARM
 _ITM_changeTransactionMode (_ITM_transactionState state)
 {
+  gtm_stack_marker marker;
   assert (state == modeSerialIrrevocable);
   gtm_tx()->serialirr_mode ();
 }
diff --git a/libitm/method-wbetl.cc b/libitm/method-wbetl.cc
index 98ce1e4..89ca815 100644
--- a/libitm/method-wbetl.cc
+++ b/libitm/method-wbetl.cc
@@ -426,8 +426,13 @@ wbetl_dispatch::trycommit ()
       for (size_t i = 0; i < n; ++i)
 	{
 	  w_entry *w = &m_wset_entries[i];
-	  gtm_cacheline::copy_mask (w->addr, w->value,
-		*gtm_cacheline_page::mask_for_page_line (w->value));
+	  gtm_cacheline_mask mask
+	    = *gtm_cacheline_page::mask_for_page_line (w->value);
+
+	  /* Filter out any updates that overlap the libitm stack.  */
+	  mask = gtm_mask_stack (w->addr, mask);
+
+	  gtm_cacheline::copy_mask (w->addr, w->value, mask);
 	}
 
       /* Only emit barrier after all cachelines are copied.  */