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

Message ID 29cf4df8950a06184aa510fec6f2ac1333fd5c0d.1620838411.git.fweimer@redhat.com
State New
Headers show
Series
  • nptl: Move almost all remaining functions into libc
Related show

Commit Message

Mark Brown via Libc-alpha May 12, 2021, 4:57 p.m.
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(-)

-- 
2.31.1

Comments

Mark Brown via Libc-alpha May 13, 2021, 5:51 p.m. | #1
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.
Mark Brown via Libc-alpha May 17, 2021, 8:06 a.m. | #2
* 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
Mark Brown via Libc-alpha May 18, 2021, 5:28 p.m. | #3
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.

Patch

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
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);
 
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);
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
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)
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
     {
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