Patchwork libgo patch committed: Include TLS size in stack size

login
register
mail settings
Submitter Ian Taylor
Date June 5, 2012, 6:19 a.m.
Message ID <mcroboyxou5.fsf@dhcp-172-18-216-180.mtv.corp.google.com>
Download mbox | patch
Permalink /patch/163022/
State New
Headers show

Comments

Ian Taylor - June 5, 2012, 6:19 a.m.
This patch to libgo includes the TLS size in the requested stack size of
a new thread, if possible.  This relies on the glibc-specific (and
undocumented) _dl_get_tls_static_info call.  This is particularly
necessary when using glibc, because glibc removes the static TLS size
from the requested stack space, and gives an error if there is not
enough space.  That means that a program that has a lot of TLS variables
will fail bizarrely, or worse may simply get a stack overflow
segmentation violation at runtime.  This patch is far from perfect, but
at least works around that problem for such programs.  Bootstrapped and
ran Go testsuite on x86_64-unknown-linux-gnu.  Committed to mainline and
4.7 branch.

Ian
Jakub Jelinek - June 5, 2012, 7:20 a.m.
On Mon, Jun 04, 2012 at 11:19:46PM -0700, Ian Lance Taylor wrote:
> This patch to libgo includes the TLS size in the requested stack size of
> a new thread, if possible.  This relies on the glibc-specific (and
> undocumented) _dl_get_tls_static_info call.  This is particularly
> necessary when using glibc, because glibc removes the static TLS size
> from the requested stack space, and gives an error if there is not
> enough space.  That means that a program that has a lot of TLS variables
> will fail bizarrely, or worse may simply get a stack overflow
> segmentation violation at runtime.  This patch is far from perfect, but
> at least works around that problem for such programs.  Bootstrapped and
> ran Go testsuite on x86_64-unknown-linux-gnu.  Committed to mainline and
> 4.7 branch.

That is a very bad idea.  _dl_get_tls_static_info is @@GLIBC_PRIVATE symbol,
therefore it must not be used by anything but glibc itself, may change or
may be removed at any time.  Not using of GLIBC_PRIVATE symbols are even enforced
by rpm, so if you build gcc as rpm, it won't install.  So especially
committing that to 4.7 branch is fatal.

Talk to libc-alpha@sourceware.org for what should be done instead.

	Jakub
Richard Guenther - June 5, 2012, 9:15 a.m.
On Tue, Jun 5, 2012 at 9:20 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Jun 04, 2012 at 11:19:46PM -0700, Ian Lance Taylor wrote:
>> This patch to libgo includes the TLS size in the requested stack size of
>> a new thread, if possible.  This relies on the glibc-specific (and
>> undocumented) _dl_get_tls_static_info call.  This is particularly
>> necessary when using glibc, because glibc removes the static TLS size
>> from the requested stack space, and gives an error if there is not
>> enough space.  That means that a program that has a lot of TLS variables
>> will fail bizarrely, or worse may simply get a stack overflow
>> segmentation violation at runtime.  This patch is far from perfect, but
>> at least works around that problem for such programs.  Bootstrapped and
>> ran Go testsuite on x86_64-unknown-linux-gnu.  Committed to mainline and
>> 4.7 branch.
>
> That is a very bad idea.  _dl_get_tls_static_info is @@GLIBC_PRIVATE symbol,
> therefore it must not be used by anything but glibc itself, may change or
> may be removed at any time.  Not using of GLIBC_PRIVATE symbols are even enforced
> by rpm, so if you build gcc as rpm, it won't install.  So especially
> committing that to 4.7 branch is fatal.
>
> Talk to libc-alpha@sourceware.org for what should be done instead.

Ian, can you please revert the patch ASAP as I want to get 4.7.1 out of the
door (well, a release candidate)?  Otherwise we'll ship 4.7.1 with broken Go
(not that we technically care - Go is not a primary language).

Thanks,
Richard.

>        Jakub

Patch

Index: libgo/configure.ac
===================================================================
--- libgo/configure.ac	(revision 187020)
+++ libgo/configure.ac	(working copy)
@@ -481,7 +481,7 @@  fi
 
 AM_CONDITIONAL(HAVE_SYS_MMAN_H, test "$ac_cv_header_sys_mman_h" = yes)
 
-AC_CHECK_FUNCS(strerror_r strsignal wait4 mincore setenv)
+AC_CHECK_FUNCS(strerror_r strsignal wait4 mincore setenv _dl_get_tls_static_info)
 AM_CONDITIONAL(HAVE_STRERROR_R, test "$ac_cv_func_strerror_r" = yes)
 AM_CONDITIONAL(HAVE_WAIT4, test "$ac_cv_func_wait4" = yes)
 
Index: libgo/runtime/proc.c
===================================================================
--- libgo/runtime/proc.c	(revision 187854)
+++ libgo/runtime/proc.c	(working copy)
@@ -1105,6 +1105,7 @@  runtime_newm(void)
 	M *m;
 	pthread_attr_t attr;
 	pthread_t tid;
+	size_t stacksize;
 
 	m = runtime_malloc(sizeof(M));
 	mcommoninit(m);
@@ -1118,7 +1119,31 @@  runtime_newm(void)
 #ifndef PTHREAD_STACK_MIN
 #define PTHREAD_STACK_MIN 8192
 #endif
-	if(pthread_attr_setstacksize(&attr, PTHREAD_STACK_MIN) != 0)
+
+	stacksize = PTHREAD_STACK_MIN;
+
+#ifdef HAVE__DL_GET_TLS_STATIC_INFO
+	{
+		/* On GNU/Linux the static TLS size is taken out of
+		   the stack size, and we get an error or a crash if
+		   there is not enough stack space left.  Add it back
+		   in if we can, in case the program uses a lot of TLS
+		   space.  */
+#ifndef internal_function
+#ifdef __i386__
+#define internal_function __attribute__ ((regparm (3), stdcall))
+#else
+#define internal_function
+#endif
+#endif
+		extern void _dl_get_tls_static_info(size_t*, size_t*) internal_function;
+		size_t tlssize, tlsalign;
+		_dl_get_tls_static_info(&tlssize, &tlsalign);
+		stacksize += tlssize;
+	}
+#endif
+
+	if(pthread_attr_setstacksize(&attr, stacksize) != 0)
 		runtime_throw("pthread_attr_setstacksize");
 
 	if(pthread_create(&tid, &attr, runtime_mstart, m) != 0)