[v5,1/2] elf: Clarify and invert second argument of _dl_allocate_tls_init
Checks
Context |
Check |
Description |
redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Build passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
success
|
Test passed
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
success
|
Build passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-arm |
success
|
Test passed
|
redhat-pt-bot/TryBot-still_applies |
warning
|
Patch no longer applies to master
|
Commit Message
---
elf/dl-tls.c | 10 +++-------
elf/rtld.c | 2 +-
nptl/allocatestack.c | 2 +-
sysdeps/generic/ldsodefs.h | 13 ++++++++++++-
4 files changed, 17 insertions(+), 10 deletions(-)
Comments
On 6/26/24 12:50 PM, Florian Weimer wrote:
> ---
> elf/dl-tls.c | 10 +++-------
> elf/rtld.c | 2 +-
> nptl/allocatestack.c | 2 +-
> sysdeps/generic/ldsodefs.h | 13 ++++++++++++-
> 4 files changed, 17 insertions(+), 10 deletions(-)
This is just a cleanup in preparation for the additional use of main_thread.
LGTM. Minor note about comments and summaries near code.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> diff --git a/elf/dl-tls.c b/elf/dl-tls.c
> index 670dbc42fc..67ce4771e3 100644
> --- a/elf/dl-tls.c
> +++ b/elf/dl-tls.c
> @@ -514,12 +514,8 @@ _dl_resize_dtv (dtv_t *dtv, size_t max_modid)
> }
>
>
> -/* Allocate initial TLS. RESULT should be a non-NULL pointer to storage
> - for the TLS space. The DTV may be resized, and so this function may
> - call malloc to allocate that space. The loader's GL(dl_load_tls_lock)
> - is taken when manipulating global TLS-related data in the loader. */
OK. My preference here is for ldsodefs.h comment to be here.
> void *
> -_dl_allocate_tls_init (void *result, bool init_tls)
> +_dl_allocate_tls_init (void *result, bool main_thread)
> {
> if (result == NULL)
> /* The memory allocation failed. */
> @@ -598,7 +594,7 @@ _dl_allocate_tls_init (void *result, bool init_tls)
> because it would already be set by the audit setup. However,
> subsequent thread creation would need to follow the default
> behaviour. */
> - if (map->l_ns != LM_ID_BASE && !init_tls)
> + if (map->l_ns != LM_ID_BASE && main_thread)
OK.
> continue;
> memset (__mempcpy (dest, map->l_tls_initimage,
> map->l_tls_initimage_size), '\0',
> @@ -626,7 +622,7 @@ _dl_allocate_tls (void *mem)
> {
> return _dl_allocate_tls_init (mem == NULL
> ? _dl_allocate_tls_storage ()
> - : allocate_dtv (mem), true);
> + : allocate_dtv (mem), false);
OK.
> }
> rtld_hidden_def (_dl_allocate_tls)
>
> diff --git a/elf/rtld.c b/elf/rtld.c
> index e9525ea987..da0d115c69 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -2336,7 +2336,7 @@ dl_main (const ElfW(Phdr) *phdr,
> into the main thread's TLS area, which we allocated above.
> Note: thread-local variables must only be accessed after completing
> the next step. */
> - _dl_allocate_tls_init (tcbp, false);
> + _dl_allocate_tls_init (tcbp, true);
OK. Yes, definitely, we're in rtld.
>
> /* And finally install it for the main thread. */
> if (! __rtld_tls_init_tp_called)
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index f35a8369bd..2cb562f8ea 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -139,7 +139,7 @@ get_cached_stack (size_t *sizep, void **memp)
> memset (dtv, '\0', (dtv[-1].counter + 1) * sizeof (dtv_t));
>
> /* Re-initialize the TLS. */
> - _dl_allocate_tls_init (TLS_TPADJ (result), true);
> + _dl_allocate_tls_init (TLS_TPADJ (result), false);
OK.
>
> return result;
> }
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index 50f58a60e3..2bccac3f7c 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -1203,7 +1203,18 @@ extern void _dl_allocate_static_tls (struct link_map *map) attribute_hidden;
> /* These are internal entry points to the two halves of _dl_allocate_tls,
> only used within rtld.c itself at startup time. */
> extern void *_dl_allocate_tls_storage (void) attribute_hidden;
> -extern void *_dl_allocate_tls_init (void *, bool);
> +
> +
> +/* Allocate initial TLS. RESULT should be a non-NULL pointer to storage
> + for the TLS space. The DTV may be resized, and so this function may
> + call malloc to allocate that space. The loader's GL(dl_load_tls_lock)
> + is taken when manipulating global TLS-related data in the loader.
> +
> + If MAIN_THREAD, this is the first call during process
> + initialization. In this case, TLS initialization for secondary
> + (audit) namespaces is skipped because that has already been handled
> + by dlopen. */
OK. I see why we want this here, in that a developer of glibc would want to pull
up the comment as they call the internal API, but my concern is that this starts
to deviate from the implementation. I suggest moving this into the location of the
code and put a summary here.
> +extern void *_dl_allocate_tls_init (void *result, bool main_thread);
> rtld_hidden_proto (_dl_allocate_tls_init)
>
> /* True if the TCB has been set up. */
* Carlos O'Donell:
>> -/* Allocate initial TLS. RESULT should be a non-NULL pointer to storage
>> - for the TLS space. The DTV may be resized, and so this function may
>> - call malloc to allocate that space. The loader's GL(dl_load_tls_lock)
>> - is taken when manipulating global TLS-related data in the loader. */
>
> OK. My preference here is for ldsodefs.h comment to be here.
I'm going to post a v6 with these changes. I don't have a strong
preference (just a slight one), especially it's not always obvious which
header would contain the function comment.
Thanks,
Florian
@@ -514,12 +514,8 @@ _dl_resize_dtv (dtv_t *dtv, size_t max_modid)
}
-/* Allocate initial TLS. RESULT should be a non-NULL pointer to storage
- for the TLS space. The DTV may be resized, and so this function may
- call malloc to allocate that space. The loader's GL(dl_load_tls_lock)
- is taken when manipulating global TLS-related data in the loader. */
void *
-_dl_allocate_tls_init (void *result, bool init_tls)
+_dl_allocate_tls_init (void *result, bool main_thread)
{
if (result == NULL)
/* The memory allocation failed. */
@@ -598,7 +594,7 @@ _dl_allocate_tls_init (void *result, bool init_tls)
because it would already be set by the audit setup. However,
subsequent thread creation would need to follow the default
behaviour. */
- if (map->l_ns != LM_ID_BASE && !init_tls)
+ if (map->l_ns != LM_ID_BASE && main_thread)
continue;
memset (__mempcpy (dest, map->l_tls_initimage,
map->l_tls_initimage_size), '\0',
@@ -626,7 +622,7 @@ _dl_allocate_tls (void *mem)
{
return _dl_allocate_tls_init (mem == NULL
? _dl_allocate_tls_storage ()
- : allocate_dtv (mem), true);
+ : allocate_dtv (mem), false);
}
rtld_hidden_def (_dl_allocate_tls)
@@ -2336,7 +2336,7 @@ dl_main (const ElfW(Phdr) *phdr,
into the main thread's TLS area, which we allocated above.
Note: thread-local variables must only be accessed after completing
the next step. */
- _dl_allocate_tls_init (tcbp, false);
+ _dl_allocate_tls_init (tcbp, true);
/* And finally install it for the main thread. */
if (! __rtld_tls_init_tp_called)
@@ -139,7 +139,7 @@ get_cached_stack (size_t *sizep, void **memp)
memset (dtv, '\0', (dtv[-1].counter + 1) * sizeof (dtv_t));
/* Re-initialize the TLS. */
- _dl_allocate_tls_init (TLS_TPADJ (result), true);
+ _dl_allocate_tls_init (TLS_TPADJ (result), false);
return result;
}
@@ -1203,7 +1203,18 @@ extern void _dl_allocate_static_tls (struct link_map *map) attribute_hidden;
/* These are internal entry points to the two halves of _dl_allocate_tls,
only used within rtld.c itself at startup time. */
extern void *_dl_allocate_tls_storage (void) attribute_hidden;
-extern void *_dl_allocate_tls_init (void *, bool);
+
+
+/* Allocate initial TLS. RESULT should be a non-NULL pointer to storage
+ for the TLS space. The DTV may be resized, and so this function may
+ call malloc to allocate that space. The loader's GL(dl_load_tls_lock)
+ is taken when manipulating global TLS-related data in the loader.
+
+ If MAIN_THREAD, this is the first call during process
+ initialization. In this case, TLS initialization for secondary
+ (audit) namespaces is skipped because that has already been handled
+ by dlopen. */
+extern void *_dl_allocate_tls_init (void *result, bool main_thread);
rtld_hidden_proto (_dl_allocate_tls_init)
/* True if the TCB has been set up. */