[05/13] string: Remove old TLS usage on strsignal
Commit Message
The per-thread state is refactored two use two strategies:
1. The default one uses a TLS structure, which will be place in the
static TLS space (using __thread keyword).
2. Linux allocates on struct pthread and access it through THREAD_*
macros.
The default strategy has the disadvantage of increasing libc.so static
TLS consumption and thus descreasing the possible surplus used in
some scenarios (which might be mitigated by BZ#25051 fix).
It is used only on Hurd, where accessing the thread point in single
thread case is not straightforward (afaiu, Hurd developers could
correct me here).
The fallback static allocation used for allocation failure is also
removed: defining its size is problematic without synchronize with
translated messages (to avoid partial translation) and the resulting
usage is not thread-safe.
Checked on x86-64-linux-gnu, i686-linux-gnu, powerpc64le-linux-gnu,
and s390x-linux-gnu.
---
include/string.h | 1 +
malloc/thread-freeres.c | 1 +
nptl/allocatestack.c | 2 +
nptl/descr.h | 4 +
string/strsignal.c | 114 ++++++-------------------
sysdeps/generic/Makefile | 1 +
sysdeps/generic/tls-internal-struct.h | 27 ++++++
sysdeps/generic/tls-internal.c | 21 +++++
sysdeps/generic/tls-internal.h | 32 +++++++
sysdeps/unix/sysv/linux/tls-internal.c | 1 +
sysdeps/unix/sysv/linux/tls-internal.h | 30 +++++++
11 files changed, 146 insertions(+), 88 deletions(-)
create mode 100644 sysdeps/generic/tls-internal-struct.h
create mode 100644 sysdeps/generic/tls-internal.c
create mode 100644 sysdeps/generic/tls-internal.h
create mode 100644 sysdeps/unix/sysv/linux/tls-internal.c
create mode 100644 sysdeps/unix/sysv/linux/tls-internal.h
Comments
* Adhemerval Zanella via Libc-alpha:
> The per-thread state is refactored two use two strategies:
>
> 1. The default one uses a TLS structure, which will be place in the
> static TLS space (using __thread keyword).
>
> 2. Linux allocates on struct pthread and access it through THREAD_*
> macros.
Hurd can use the same infrastructure nowadays, since commit
b65a82e4e757c1e6cb7073916a29 ("hurd: Add THREAD_GET/SETMEM/_NC").
I'm not sure if struct tls_internal_t is necessary at this point.
> diff --git a/malloc/thread-freeres.c b/malloc/thread-freeres.c
> index c71ca4fc33..d42eea770b 100644
> --- a/malloc/thread-freeres.c
> +++ b/malloc/thread-freeres.c
> @@ -32,6 +32,7 @@ __libc_thread_freeres (void)
> call_function_static_weak (__rpc_thread_destroy);
> call_function_static_weak (__res_thread_freeres);
> call_function_static_weak (__strerror_thread_freeres);
> + call_function_static_weak (__strsignal_thread_freeres);
I think you can call free directly. I doubt it increases binary
size—the data is in the thread descriptor, so nothing needs to be pulled
into the link for this.
Thanks,
Florian
On 28/05/2020 08:38, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> The per-thread state is refactored two use two strategies:
>>
>> 1. The default one uses a TLS structure, which will be place in the
>> static TLS space (using __thread keyword).
>>
>> 2. Linux allocates on struct pthread and access it through THREAD_*
>> macros.
>
> Hurd can use the same infrastructure nowadays, since commit
> b65a82e4e757c1e6cb7073916a29 ("hurd: Add THREAD_GET/SETMEM/_NC").
Different than Linux, Hurd's THREAD_SELF returns a tcbhead_t. I don't
have a strong preference, I use the generic Hurd mainly to avoid
touching Hurd specific files.
>
> I'm not sure if struct tls_internal_t is necessary at this point.
I added the tls_internal_t as a focal point for extra field when
required. I don't have a strong preference, but I think it is
slight better than update multiple files (for instance the generic
and Linux one).
>
>> diff --git a/malloc/thread-freeres.c b/malloc/thread-freeres.c
>> index c71ca4fc33..d42eea770b 100644
>> --- a/malloc/thread-freeres.c
>> +++ b/malloc/thread-freeres.c
>> @@ -32,6 +32,7 @@ __libc_thread_freeres (void)
>> call_function_static_weak (__rpc_thread_destroy);
>> call_function_static_weak (__res_thread_freeres);
>> call_function_static_weak (__strerror_thread_freeres);
>> + call_function_static_weak (__strsignal_thread_freeres);
>
> I think you can call free directly. I doubt it increases binary
> size—the data is in the thread descriptor, so nothing needs to be pulled
> into the link for this.
Don't we need it to deallocate the memory in the case of a libc
loaded in a different namespace?
* Adhemerval Zanella via Libc-alpha:
>>> diff --git a/malloc/thread-freeres.c b/malloc/thread-freeres.c
>>> index c71ca4fc33..d42eea770b 100644
>>> --- a/malloc/thread-freeres.c
>>> +++ b/malloc/thread-freeres.c
>>> @@ -32,6 +32,7 @@ __libc_thread_freeres (void)
>>> call_function_static_weak (__rpc_thread_destroy);
>>> call_function_static_weak (__res_thread_freeres);
>>> call_function_static_weak (__strerror_thread_freeres);
>>> + call_function_static_weak (__strsignal_thread_freeres);
>>
>> I think you can call free directly. I doubt it increases binary
>> size—the data is in the thread descriptor, so nothing needs to be pulled
>> into the link for this.
>
> Don't we need it to deallocate the memory in the case of a libc
> loaded in a different namespace?
Right, but using call_function_static_weak does not achieve that.
This is a general problem with using TLS that needs separate
deallocation. We don't have per-namespace thread exit hooks, but we
probably need them.
On 01/06/2020 15:13, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>>>> diff --git a/malloc/thread-freeres.c b/malloc/thread-freeres.c
>>>> index c71ca4fc33..d42eea770b 100644
>>>> --- a/malloc/thread-freeres.c
>>>> +++ b/malloc/thread-freeres.c
>>>> @@ -32,6 +32,7 @@ __libc_thread_freeres (void)
>>>> call_function_static_weak (__rpc_thread_destroy);
>>>> call_function_static_weak (__res_thread_freeres);
>>>> call_function_static_weak (__strerror_thread_freeres);
>>>> + call_function_static_weak (__strsignal_thread_freeres);
>>>
>>> I think you can call free directly. I doubt it increases binary
>>> size—the data is in the thread descriptor, so nothing needs to be pulled
>>> into the link for this.
>>
>> Don't we need it to deallocate the memory in the case of a libc
>> loaded in a different namespace?
>
> Right, but using call_function_static_weak does not achieve that.
> This is a general problem with using TLS that needs separate
> deallocation. We don't have per-namespace thread exit hooks, but we
> probably need them.
>
My confusion in fact, this is another issue indeed. I added it just
to follow the small optimization dony by __strerror_thread_freeres
to avoid pulling the free code on static usage.
So do you think it would be better to just call free on
__libc_thread_freeres (it should be ok since code initializes it to
NULL)? If so, maybe also do the same for __strerror_thread_freeres?
* Adhemerval Zanella:
> My confusion in fact, this is another issue indeed. I added it just
> to follow the small optimization dony by __strerror_thread_freeres
> to avoid pulling the free code on static usage.
Yes, it requires a separate fix (eventually, not this series).
> So do you think it would be better to just call free on
> __libc_thread_freeres (it should be ok since code initializes it to
> NULL)? If so, maybe also do the same for __strerror_thread_freeres?
Yes, if it is just a free call that only pulls in malloc, that should
be okay.
@@ -52,6 +52,7 @@ extern char *__strerror_r (int __errnum, char *__buf, size_t __buflen);
/* Called as part of the thread shutdown sequence. */
void __strerror_thread_freeres (void) attribute_hidden;
+void __strsignal_thread_freeres (void) attribute_hidden;
/* Get _STRING_ARCH_unaligned. */
#include <string_private.h>
@@ -32,6 +32,7 @@ __libc_thread_freeres (void)
call_function_static_weak (__rpc_thread_destroy);
call_function_static_weak (__res_thread_freeres);
call_function_static_weak (__strerror_thread_freeres);
+ call_function_static_weak (__strsignal_thread_freeres);
/* This should come last because it shuts down malloc for this
thread and the other shutdown functions might well call free. */
@@ -237,6 +237,8 @@ get_cached_stack (size_t *sizep, void **memp)
/* No pending event. */
result->nextevent = NULL;
+ result->tls_state = (struct tls_internal_t) { 0 };
+
/* Clear the DTV. */
dtv_t *dtv = GET_DTV (TLS_TPADJ (result));
for (size_t cnt = 0; cnt < dtv[-1].counter; ++cnt)
@@ -34,6 +34,7 @@
#include <unwind.h>
#include <bits/types/res_state.h>
#include <kernel-features.h>
+#include <tls-internal-struct.h>
#ifndef TCB_ALIGNMENT
# define TCB_ALIGNMENT sizeof (double)
@@ -398,6 +399,9 @@ struct pthread
/* Indicates whether is a C11 thread created by thrd_creat. */
bool c11;
+ /* Used on strsignal. */
+ struct tls_internal_t tls_state;
+
/* This member must be last. */
char end_padding[];
@@ -20,106 +20,44 @@
#include <stdlib.h>
#include <string.h>
#include <libintl.h>
-#include <libc-lock.h>
-
-static __libc_key_t key;
-
-/* If nonzero the key allocation failed and we should better use a
- static buffer than fail. */
-#define BUFFERSIZ 100
-static char local_buf[BUFFERSIZ];
-static char *static_buf;
-
-/* Destructor for the thread-specific data. */
-static void init (void);
-static void free_key_mem (void *mem);
-static char *getbuffer (void);
-
+#include <tls-internal.h>
+#include <array_length.h>
/* Return a string describing the meaning of the signal number SIGNUM. */
char *
strsignal (int signum)
{
- __libc_once_define (static, once);
- const char *desc;
+ const char *desc = NULL;
- /* If we have not yet initialized the buffer do it now. */
- __libc_once (once, init);
+ if (signum >= 0 && signum <= NSIG
+ && signum < array_length (__sys_siglist_internal))
+ desc = __sys_siglist_internal[signum];
- if (
-#ifdef SIGRTMIN
- (signum >= SIGRTMIN && signum <= SIGRTMAX) ||
-#endif
- signum < 0 || signum >= NSIG
- || (desc = __sys_siglist_internal[signum]) == NULL)
- {
- char *buffer = getbuffer ();
- int len;
-#ifdef SIGRTMIN
- if (signum >= SIGRTMIN && signum <= SIGRTMAX)
- len = __snprintf (buffer, BUFFERSIZ - 1, _("Real-time signal %d"),
- signum - SIGRTMIN);
- else
-#endif
- len = __snprintf (buffer, BUFFERSIZ - 1, _("Unknown signal %d"),
- signum);
- if (len >= BUFFERSIZ)
- buffer = NULL;
- else
- buffer[len] = '\0';
-
- return buffer;
- }
-
- return (char *) _(desc);
-}
+ if (desc != NULL)
+ return (char *) _(desc);
+ struct tls_internal_t *tls_internal = __glibc_tls_internal ();
+ free (tls_internal->strsignal_buf);
-/* Initialize buffer. */
-static void
-init (void)
-{
- if (__libc_key_create (&key, free_key_mem))
- /* Creating the key failed. This means something really went
- wrong. In any case use a static buffer which is better than
- nothing. */
- static_buf = local_buf;
-}
+ int r;
+#ifdef SIGRTMIN
+ if (signum >= SIGRTMIN && signum <= SIGRTMAX)
+ r = __asprintf (&tls_internal->strsignal_buf, _("Real-time signal %d"),
+ signum - SIGRTMIN);
+ else
+#endif
+ r = __asprintf (&tls_internal->strsignal_buf, _("Unknown signal %d"),
+ signum);
+ if (r == -1)
+ tls_internal->strsignal_buf = NULL;
-/* Free the thread specific data, this is done if a thread terminates. */
-static void
-free_key_mem (void *mem)
-{
- free (mem);
- __libc_setspecific (key, NULL);
+ return tls_internal->strsignal_buf;
}
-
-/* Return the buffer to be used. */
-static char *
-getbuffer (void)
+void
+__strsignal_thread_freeres (void)
{
- char *result;
-
- if (static_buf != NULL)
- result = static_buf;
- else
- {
- /* We don't use the static buffer and so we have a key. Use it
- to get the thread-specific buffer. */
- result = __libc_getspecific (key);
- if (result == NULL)
- {
- /* No buffer allocated so far. */
- result = malloc (BUFFERSIZ);
- if (result == NULL)
- /* No more memory available. We use the static buffer. */
- result = local_buf;
- else
- __libc_setspecific (key, result);
- }
- }
-
- return result;
+ free (__glibc_tls_internal()->strsignal_buf);
}
+text_set_element (__libc_subfreeres, __strsignal_thread_freeres);
@@ -16,6 +16,7 @@
# <https://www.gnu.org/licenses/>.
ifeq ($(subdir),string)
+sysdep_routines += tls-internal
CFLAGS-wordcopy.c += -Wno-uninitialized
endif
new file mode 100644
@@ -0,0 +1,27 @@
+/* Per-thread state. Generic version.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library 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
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#ifndef _TLS_INTERNAL_STRUCT_H
+#define _TLS_INTERNAL_STRUCT_H 1
+
+struct tls_internal_t
+{
+ char *strsignal_buf;
+};
+
+#endif
new file mode 100644
@@ -0,0 +1,21 @@
+/* Per-thread state. Generic version.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library 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
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#include <tls-internal.h>
+
+__thread struct tls_internal_t __tls_internal;
new file mode 100644
@@ -0,0 +1,32 @@
+/* Per-thread state. Generic version.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library 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
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#ifndef _TLS_INTERNAL_H
+#define _TLS_INTERNAL_H 1
+
+#include <tls-internal-struct.h>
+
+extern __thread struct tls_internal_t __tls_internal attribute_hidden;
+
+static inline struct tls_internal_t *
+__glibc_tls_internal (void)
+{
+ return &__tls_internal;
+}
+
+#endif
new file mode 100644
@@ -0,0 +1 @@
+/* Empty. */
new file mode 100644
@@ -0,0 +1,30 @@
+/* Per-thread state. Linux version.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library 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
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+#ifndef _TLS_INTERNAL_H
+#define _TLS_INTERNAL_H 1
+
+#include <nptl/pthreadP.h>
+
+static inline struct tls_internal_t *
+__glibc_tls_internal (void)
+{
+ return &THREAD_SELF->tls_state;
+}
+
+#endif