[01/23] nptl: Move __nptl_initial_report_events into ld.so/startup code
Checks
Commit Message
The initialization of the report_events TCB field is now performed
in __tls_init_tp instead of __pthread_initialize_minimal_internal
(in libpthread).
The events interface is difficult to test becaue GDB stopped using it
in 2015. The td_thr_get_info change to ingore lookup issues is
enough to support GDB with this change.
---
nptl/Versions | 1 +
nptl/nptl-init.c | 12 ------------
nptl/pthreadP.h | 5 +++++
nptl_db/db_info.c | 3 ---
nptl_db/structs.def | 2 +-
nptl_db/td_thr_get_info.c | 11 +++++++++--
sysdeps/nptl/dl-tls_init_tp.c | 8 ++++++++
7 files changed, 24 insertions(+), 18 deletions(-)
Comments
On 12/05/2021 13:57, Florian Weimer via Libc-alpha wrote:
> The initialization of the report_events TCB field is now performed
> in __tls_init_tp instead of __pthread_initialize_minimal_internal
> (in libpthread).
>
> The events interface is difficult to test becaue GDB stopped using it
s/becaue/because
> in 2015. The td_thr_get_info change to ingore lookup issues is
s/ingore/ignore
> enough to support GDB with this change.
Should we start to deprecate and remove such interfaces? Your patch is
already ignoring the possible error on td_thr_get_info.
Not sure how backwards compatible regarding debugging interface we
should be.
Regardless, it looks good to me.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> ---
> nptl/Versions | 1 +
> nptl/nptl-init.c | 12 ------------
> nptl/pthreadP.h | 5 +++++
> nptl_db/db_info.c | 3 ---
> nptl_db/structs.def | 2 +-
> nptl_db/td_thr_get_info.c | 11 +++++++++--
> sysdeps/nptl/dl-tls_init_tp.c | 8 ++++++++
> 7 files changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/nptl/Versions b/nptl/Versions
> index 1dd3fbc18c..fb3379b788 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -497,6 +497,7 @@ libpthread {
>
> ld {
> GLIBC_PRIVATE {
> + __nptl_initial_report_events;
> __nptl_set_robust_list_avail;
> }
> }
> \ No newline at end of file
Ok.
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index 16fb66bdf5..f4b86fbfaf 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -96,21 +96,9 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
> extern void **__libc_dl_error_tsd (void) __attribute__ ((const));
>
>
> -/* This can be set by the debugger before initialization is complete. */
> -static bool __nptl_initial_report_events __attribute_used__;
> -
> void
> __pthread_initialize_minimal_internal (void)
> {
> - /* Partial initialization of the TCB already happened in TLS_INIT_TP
> - and __tls_init_tp. */
> - struct pthread *pd = THREAD_SELF;
> -
> - /* Before initializing GL (dl_stack_user), the debugger could not
> - find us and had to set __nptl_initial_report_events. Propagate
> - its setting. */
> - THREAD_SETMEM (pd, report_events, __nptl_initial_report_events);
> -
> struct sigaction sa;
> __sigemptyset (&sa.sa_mask);
>
Ok.
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 5b844f331a..dc2aece37e 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -328,6 +328,11 @@ extern int __pthread_attr_init (pthread_attr_t *attr);
> libc_hidden_proto (__pthread_attr_init)
> extern int __pthread_attr_init_2_0 (pthread_attr_t *attr);
>
> +/* Part of the legacy thread events interface (which has been
> + superseded by PTRACE_O_TRACECLONE). This can be set by the
> + debugger before initialization is complete. */
> +extern bool __nptl_initial_report_events;
> +rtld_hidden_proto (__nptl_initial_report_events)
>
> /* Event handlers for libthread_db interface. */
> extern void __nptl_create_event (void);
Ok.
> diff --git a/nptl_db/db_info.c b/nptl_db/db_info.c
> index bed6ed486e..b39aebb78c 100644
> --- a/nptl_db/db_info.c
> +++ b/nptl_db/db_info.c
> @@ -42,9 +42,6 @@ typedef struct rtld_global rtld_global;
> typedef struct dtv_slotinfo_list dtv_slotinfo_list;
> typedef struct dtv_slotinfo dtv_slotinfo;
>
> -/* Actually static in nptl/init.c, but we only need it for typeof. */
> -extern bool __nptl_initial_report_events;
> -
> #define schedparam_sched_priority schedparam.sched_priority
>
> #define eventbuf_eventmask eventbuf.eventmask
Ok.
> diff --git a/nptl_db/structs.def b/nptl_db/structs.def
> index 9173e1ab8f..275c12bc7a 100644
> --- a/nptl_db/structs.def
> +++ b/nptl_db/structs.def
> @@ -80,7 +80,7 @@ DB_FUNCTION (__nptl_death_event)
> DB_SYMBOL (__nptl_threads_events)
> DB_MAIN_VARIABLE (__nptl_nthreads)
> DB_VARIABLE (__nptl_last_event)
> -DB_VARIABLE (__nptl_initial_report_events)
> +DB_RTLD_VARIABLE (__nptl_initial_report_events)
>
> DB_MAIN_ARRAY_VARIABLE (__pthread_keys)
> DB_STRUCT (pthread_key_struct)
Ok.
> diff --git a/nptl_db/td_thr_get_info.c b/nptl_db/td_thr_get_info.c
> index 002a22a5be..01af021d2a 100644
> --- a/nptl_db/td_thr_get_info.c
> +++ b/nptl_db/td_thr_get_info.c
> @@ -41,8 +41,15 @@ td_thr_get_info (const td_thrhandle_t *th, td_thrinfo_t *infop)
> schedpolicy = SCHED_OTHER;
> schedprio = 0;
> tid = 0;
> - err = DB_GET_VALUE (report_events, th->th_ta_p,
> - __nptl_initial_report_events, 0);
> +
> + /* Ignore errors to obtain the __nptl_initial_report_events
> + value because GDB no longer uses the events interface, and
> + other libthread_db consumers hopefully can handle different
> + libpthread/lds.o load orders. */
> + report_events = 0;
> + (void) DB_GET_VALUE (report_events, th->th_ta_p,
> + __nptl_initial_report_events, 0);
> + err = TD_OK;
> }
> else
> {
Ok.
> diff --git a/sysdeps/nptl/dl-tls_init_tp.c b/sysdeps/nptl/dl-tls_init_tp.c
> index f1aaa5aa9d..1f7790297f 100644
> --- a/sysdeps/nptl/dl-tls_init_tp.c
> +++ b/sysdeps/nptl/dl-tls_init_tp.c
> @@ -27,6 +27,9 @@ bool __nptl_set_robust_list_avail __attribute__ ((nocommon));
> rtld_hidden_data_def (__nptl_set_robust_list_avail)
> #endif
>
> +bool __nptl_initial_report_events __attribute__ ((nocommon));
> +rtld_hidden_def (__nptl_initial_report_events)
> +
> #ifdef SHARED
> /* Dummy implementation. See __rtld_mutex_init. */
> static int
> @@ -63,6 +66,11 @@ __tls_init_tp (void)
> THREAD_SETMEM (pd, specific[0], &pd->specific_1stblock[0]);
> THREAD_SETMEM (pd, user_stack, true);
>
> + /* Before initializing GL (dl_stack_user), the debugger could not
> + find us and had to set __nptl_initial_report_events. Propagate
> + its setting. */
> + THREAD_SETMEM (pd, report_events, __nptl_initial_report_events);
> +
> /* Initialize the robust mutex data. */
> {
> #if __PTHREAD_MUTEX_HAVE_PREV
>
Ok.
* Adhemerval Zanella via Libc-alpha:
>> enough to support GDB with this change.
>
> Should we start to deprecate and remove such interfaces? Your patch is
> already ignoring the possible error on td_thr_get_info.
I think we should deprecate these interfaces. The dyninst framework may
still be using them (it's hard to tell based on the source code).
There are other libthread_db interfaces that aren't used by GDB (they
pthread_key_t handling comes to my mind).
Thanks,
Florian
On 17/05/2021 05:06, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>>> enough to support GDB with this change.
>>
>> Should we start to deprecate and remove such interfaces? Your patch is
>> already ignoring the possible error on td_thr_get_info.
>
> I think we should deprecate these interfaces. The dyninst framework may
> still be using them (it's hard to tell based on the source code).
>
> There are other libthread_db interfaces that aren't used by GDB (they
> pthread_key_t handling comes to my mind).
OK, I think we can start to deprecate them on new version. Maybe we
can even remove it, since I hardly think this interface is worth
to keep as a compatibility layer.
@@ -497,6 +497,7 @@ libpthread {
ld {
GLIBC_PRIVATE {
+ __nptl_initial_report_events;
__nptl_set_robust_list_avail;
}
}
\ No newline at end of file
@@ -96,21 +96,9 @@ sigcancel_handler (int sig, siginfo_t *si, void *ctx)
extern void **__libc_dl_error_tsd (void) __attribute__ ((const));
-/* This can be set by the debugger before initialization is complete. */
-static bool __nptl_initial_report_events __attribute_used__;
-
void
__pthread_initialize_minimal_internal (void)
{
- /* Partial initialization of the TCB already happened in TLS_INIT_TP
- and __tls_init_tp. */
- struct pthread *pd = THREAD_SELF;
-
- /* Before initializing GL (dl_stack_user), the debugger could not
- find us and had to set __nptl_initial_report_events. Propagate
- its setting. */
- THREAD_SETMEM (pd, report_events, __nptl_initial_report_events);
-
struct sigaction sa;
__sigemptyset (&sa.sa_mask);
@@ -328,6 +328,11 @@ extern int __pthread_attr_init (pthread_attr_t *attr);
libc_hidden_proto (__pthread_attr_init)
extern int __pthread_attr_init_2_0 (pthread_attr_t *attr);
+/* Part of the legacy thread events interface (which has been
+ superseded by PTRACE_O_TRACECLONE). This can be set by the
+ debugger before initialization is complete. */
+extern bool __nptl_initial_report_events;
+rtld_hidden_proto (__nptl_initial_report_events)
/* Event handlers for libthread_db interface. */
extern void __nptl_create_event (void);
@@ -42,9 +42,6 @@ typedef struct rtld_global rtld_global;
typedef struct dtv_slotinfo_list dtv_slotinfo_list;
typedef struct dtv_slotinfo dtv_slotinfo;
-/* Actually static in nptl/init.c, but we only need it for typeof. */
-extern bool __nptl_initial_report_events;
-
#define schedparam_sched_priority schedparam.sched_priority
#define eventbuf_eventmask eventbuf.eventmask
@@ -80,7 +80,7 @@ DB_FUNCTION (__nptl_death_event)
DB_SYMBOL (__nptl_threads_events)
DB_MAIN_VARIABLE (__nptl_nthreads)
DB_VARIABLE (__nptl_last_event)
-DB_VARIABLE (__nptl_initial_report_events)
+DB_RTLD_VARIABLE (__nptl_initial_report_events)
DB_MAIN_ARRAY_VARIABLE (__pthread_keys)
DB_STRUCT (pthread_key_struct)
@@ -41,8 +41,15 @@ td_thr_get_info (const td_thrhandle_t *th, td_thrinfo_t *infop)
schedpolicy = SCHED_OTHER;
schedprio = 0;
tid = 0;
- err = DB_GET_VALUE (report_events, th->th_ta_p,
- __nptl_initial_report_events, 0);
+
+ /* Ignore errors to obtain the __nptl_initial_report_events
+ value because GDB no longer uses the events interface, and
+ other libthread_db consumers hopefully can handle different
+ libpthread/lds.o load orders. */
+ report_events = 0;
+ (void) DB_GET_VALUE (report_events, th->th_ta_p,
+ __nptl_initial_report_events, 0);
+ err = TD_OK;
}
else
{
@@ -27,6 +27,9 @@ bool __nptl_set_robust_list_avail __attribute__ ((nocommon));
rtld_hidden_data_def (__nptl_set_robust_list_avail)
#endif
+bool __nptl_initial_report_events __attribute__ ((nocommon));
+rtld_hidden_def (__nptl_initial_report_events)
+
#ifdef SHARED
/* Dummy implementation. See __rtld_mutex_init. */
static int
@@ -63,6 +66,11 @@ __tls_init_tp (void)
THREAD_SETMEM (pd, specific[0], &pd->specific_1stblock[0]);
THREAD_SETMEM (pd, user_stack, true);
+ /* Before initializing GL (dl_stack_user), the debugger could not
+ find us and had to set __nptl_initial_report_events. Propagate
+ its setting. */
+ THREAD_SETMEM (pd, report_events, __nptl_initial_report_events);
+
/* Initialize the robust mutex data. */
{
#if __PTHREAD_MUTEX_HAVE_PREV