From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by sourceware.org (Postfix) with ESMTP id 7AF9E3858D34 for ; Thu, 2 Jul 2020 19:33:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 7AF9E3858D34 Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-317--IjnuaMtORK6YV1Yp9ZIAw-1; Thu, 02 Jul 2020 15:33:03 -0400 X-MC-Unique: -IjnuaMtORK6YV1Yp9ZIAw-1 Received: by mail-qt1-f199.google.com with SMTP id c22so20096323qtp.9 for ; Thu, 02 Jul 2020 12:33:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=vvybY/00UiIHdiXCbMzmkQU0w9J9VPs0FDHzKI++Eec=; b=VDAoB6zYigMpbQeHuxcys5rE9aQSMP97Z4yxB9afOceTZz61gB0UReGnpglc04BqOe rwa9eT35UhcHnrSwF09hrYdCXDTIfcTBDtNDQmRY5Ey4h1EI83ZhBxDdXnmMQcEPA3EX mNfW6k6GrcqxkKMozilfHw6ssXkgXun0RaGfxS0jhbqNJBDJ0EGwMDP5lwszIeMdmBQr RIMj6uR45NaTi8ofj4AnMMcyn/XqRKCr/POMcb6QxHjOEpVALbqH3k5UHwgX7KjNpDny GQd0X/vWIZeyhNtBvGi4PTeO79IY8KNzJtYrV7SVyvpHzG6aTVbMW2YqhYNGTao+CkJf p22Q== X-Gm-Message-State: AOAM530NZ8KQux+KcPU8EvT3Ix4CzPq9h8B4F2OIG7A2subRSu6nH9hi N3Pgi5ELvJhgvJ9ZcvZqfkNlZJhyp3ev2dQDRXDzpyHSwnNpJZWRDs2M3xMmd0ikFtlmSUGrZ+k zbrP+sgCCa/ScxOvBBaQl X-Received: by 2002:ac8:70da:: with SMTP id g26mr33910078qtp.67.1593718382122; Thu, 02 Jul 2020 12:33:02 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzz4pGc/DFEeHLolK8T+SJEkf5QXRGi6I7iCQDiJv/UQGTPM65wpn2/G5HtQQJ0Mvgv0oi6Ow== X-Received: by 2002:ac8:70da:: with SMTP id g26mr33910045qtp.67.1593718381615; Thu, 02 Jul 2020 12:33:01 -0700 (PDT) Received: from [192.168.1.4] (198-84-170-103.cpe.teksavvy.com. [198.84.170.103]) by smtp.gmail.com with ESMTPSA id c24sm10178159qtd.82.2020.07.02.12.33.00 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 02 Jul 2020 12:33:00 -0700 (PDT) Subject: Re: [PATCH v5 05/13] string: Remove old TLS usage on strsignal To: Adhemerval Zanella , libc-alpha@sourceware.org References: <20200619134352.297146-1-adhemerval.zanella@linaro.org> <20200619134352.297146-5-adhemerval.zanella@linaro.org> From: Carlos O'Donell Organization: Red Hat Message-ID: <15aac00d-dbc6-74d0-0d04-b25102ad6d2f@redhat.com> Date: Thu, 2 Jul 2020 15:32:59 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <20200619134352.297146-5-adhemerval.zanella@linaro.org> Content-Language: en-US X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 02 Jul 2020 19:33:13 -0000 On 6/19/20 9:43 AM, Adhemerval Zanella wrote: > 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). s/place/placed/g OK. > 2. Linux allocates on struct pthread and access it through THREAD_* > macros. s/on/via/g OK. > 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). s/descreasing/decreasing/g > > It is used only on Hurd, where accessing the thread point in single s/point in/storage in the/g > 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 s/synchronize/synchronizing/g > translated messages (to avoid partial translation) and the resulting > usage is not thread-safe. OK for master with fixed up commit message. Framework seems sensible and we might extend it to include more data if we need to. The existing use of __libc_key_create is a little heavy for just a string, and the complex shutdown through a stored function pointer is always difficult to debug. It is easier as you have it to just free the memory during the thread's freeres. No regressions on x86_64 and i686. Tested-by: Carlos O'Donell Reviewed-by: Carlos O'Donell > Checked on x86-64-linux-gnu, i686-linux-gnu, powerpc64le-linux-gnu, > and s390x-linux-gnu. > --- > malloc/thread-freeres.c | 2 + > nptl/allocatestack.c | 2 + > nptl/descr.h | 4 + > string/strsignal.c | 112 +++++-------------------- > sysdeps/generic/Makefile | 1 + > sysdeps/generic/tls-internal-struct.h | 27 ++++++ > sysdeps/generic/tls-internal.c | 21 +++++ > sysdeps/generic/tls-internal.h | 39 +++++++++ > sysdeps/unix/sysv/linux/tls-internal.c | 1 + > sysdeps/unix/sysv/linux/tls-internal.h | 37 ++++++++ > 10 files changed, 155 insertions(+), 91 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 > > diff --git a/malloc/thread-freeres.c b/malloc/thread-freeres.c > index c71ca4fc33..3408bdbefd 100644 > --- a/malloc/thread-freeres.c > +++ b/malloc/thread-freeres.c > @@ -21,6 +21,7 @@ > #include > #include > #include > +#include OK. > > /* Thread shutdown function. Note that this function must be called > for threads during shutdown for correctness reasons. Unlike > @@ -32,6 +33,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); > + __glibc_tls_internal_free (); OK. We remove the use of __libc_key_create and instead of the destructor used there we use the thread shutdown __libc_thread_freeres to free it. > > /* This should come last because it shuts down malloc for this > thread and the other shutdown functions might well call free. */ > diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c > index d16f3d71f8..4ae4b5a986 100644 > --- a/nptl/allocatestack.c > +++ b/nptl/allocatestack.c > @@ -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 }; OK. > + > /* Clear the DTV. */ > dtv_t *dtv = GET_DTV (TLS_TPADJ (result)); > for (size_t cnt = 0; cnt < dtv[-1].counter; ++cnt) > diff --git a/nptl/descr.h b/nptl/descr.h > index e1c7db5473..6a509b6725 100644 > --- a/nptl/descr.h > +++ b/nptl/descr.h > @@ -34,6 +34,7 @@ > #include > #include > #include > +#include OK. > > #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; OK. > + > /* This member must be last. */ > char end_padding[]; > > diff --git a/string/strsignal.c b/string/strsignal.c > index 7e3b262c55..701ce20e6e 100644 > --- a/string/strsignal.c > +++ b/string/strsignal.c > @@ -20,106 +20,36 @@ > #include > #include > #include > -#include > - > -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; OK. Good to see this go. > - > -/* Destructor for the thread-specific data. */ > -static void init (void); > -static void free_key_mem (void *mem); > -static char *getbuffer (void); > - > +#include > +#include > > /* Return a string describing the meaning of the signal number SIGNUM. */ > char * > strsignal (int signum) > { > - __libc_once_define (static, once); > - const char *desc; > - > - /* If we have not yet initialized the buffer do it now. */ > - __libc_once (once, init); > - > - if ( > -#ifdef SIGRTMIN > - (signum >= SIGRTMIN && signum <= SIGRTMAX) || > -#endif > - signum < 0 || signum >= NSIG > - || (desc = __sys_siglist[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); > -} > - > - > -/* 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; > -} > - > + const char *desc = 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); > -} > + if (signum >= 0 && signum <= NSIG && signum < array_length (__sys_siglist)) > + desc = __sys_siglist[signum]; OK. > > + if (desc != NULL) > + return (char *) _(desc); OK. > > -/* Return the buffer to be used. */ > -static char * > -getbuffer (void) > -{ > - char *result; > + struct tls_internal_t *tls_internal = __glibc_tls_internal (); > + free (tls_internal->strsignal_buf); > > - if (static_buf != NULL) > - result = static_buf; > + int r; > +#ifdef SIGRTMIN > + if (signum >= SIGRTMIN && signum <= SIGRTMAX) > + r = __asprintf (&tls_internal->strsignal_buf, _("Real-time signal %d"), > + signum - SIGRTMIN); OK. Use asprintf which will call malloc. > 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); > - } > - } > +#endif > + r = __asprintf (&tls_internal->strsignal_buf, _("Unknown signal %d"), > + signum); OK. Use asptrintf which will call malloc. > + > + if (r == -1) > + tls_internal->strsignal_buf = NULL; > > - return result; > + return tls_internal->strsignal_buf; OK. > } > diff --git a/sysdeps/generic/Makefile b/sysdeps/generic/Makefile > index bd4f9425ca..1240d99436 100644 > --- a/sysdeps/generic/Makefile > +++ b/sysdeps/generic/Makefile > @@ -16,6 +16,7 @@ > # . > > ifeq ($(subdir),string) > +sysdep_routines += tls-internal OK. > CFLAGS-wordcopy.c += -Wno-uninitialized > endif > > diff --git a/sysdeps/generic/tls-internal-struct.h b/sysdeps/generic/tls-internal-struct.h > new file mode 100644 > index 0000000000..33a9079ee9 > --- /dev/null > +++ b/sysdeps/generic/tls-internal-struct.h > @@ -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 > + . */ > + > +#ifndef _TLS_INTERNAL_STRUCT_H > +#define _TLS_INTERNAL_STRUCT_H 1 > + > +struct tls_internal_t > +{ > + char *strsignal_buf; > +}; OK. > + > +#endif > diff --git a/sysdeps/generic/tls-internal.c b/sysdeps/generic/tls-internal.c > new file mode 100644 > index 0000000000..14b914e5f3 > --- /dev/null > +++ b/sysdeps/generic/tls-internal.c > @@ -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 > + . */ > + > +#include > + > +__thread struct tls_internal_t __tls_internal; OK. > diff --git a/sysdeps/generic/tls-internal.h b/sysdeps/generic/tls-internal.h > new file mode 100644 > index 0000000000..1f6a117d76 > --- /dev/null > +++ b/sysdeps/generic/tls-internal.h > @@ -0,0 +1,39 @@ > +/* 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 > + . */ > + > +#ifndef _TLS_INTERNAL_H > +#define _TLS_INTERNAL_H 1 > + > +#include > +#include > + > +extern __thread struct tls_internal_t __tls_internal attribute_hidden; OK. Generic implementation uses __thread to store the structure. > + > +static inline struct tls_internal_t * > +__glibc_tls_internal (void) > +{ > + return &__tls_internal; > +} OK. > + > +static inline void > +__glibc_tls_internal_free (void) > +{ > + free (__tls_internal.strsignal_buf); > +} OK. > + > +#endif > diff --git a/sysdeps/unix/sysv/linux/tls-internal.c b/sysdeps/unix/sysv/linux/tls-internal.c > new file mode 100644 > index 0000000000..6e25b021ab > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/tls-internal.c > @@ -0,0 +1 @@ > +/* Empty. */ > diff --git a/sysdeps/unix/sysv/linux/tls-internal.h b/sysdeps/unix/sysv/linux/tls-internal.h > new file mode 100644 > index 0000000000..5d712abd4a > --- /dev/null > +++ b/sysdeps/unix/sysv/linux/tls-internal.h > @@ -0,0 +1,37 @@ > +/* 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 > + . */ > + > +#ifndef _TLS_INTERNAL_H > +#define _TLS_INTERNAL_H 1 > + > +#include > +#include > + > +static inline struct tls_internal_t * > +__glibc_tls_internal (void) > +{ > + return &THREAD_SELF->tls_state; > +} > + > +static inline void > +__glibc_tls_internal_free (void) > +{ > + free (THREAD_SELF->tls_state.strsignal_buf); > +} OK. > + > +#endif > -- Cheers, Carlos.