[01/23] nptl: Move __nptl_initial_report_events into ld.so/startup code

  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(-)
  

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.