| Message ID | mcroboyxou5.fsf@dhcp-172-18-216-180.mtv.corp.google.com |
|---|---|
| State | New |
| Headers | show |
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
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
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)