Re: [PATCH] nptl: Export libthread_db-used symbols under GLIBC_PRIVATE

On 6/17/21 3:23 PM, Florian Weimer via Libc-alpha wrote:
> Kevin, this *almost* gives us the perfect debugging experience with your
> patched GDB in Fedora 35, according to my preliminary tests.  A build
> with the patch is running here:
> 
>   <https://koji.fedoraproject.org/koji/buildinfo?buildID=1773177>

Please send v2 with comment update and some discussion about the missing
3 symbols that could be added (or not added).

> However, it seems that GDB still needs pthread_create in the .symtab to
> trigger loading of libpthread_db.  A fully stripped libc.so.6 without
> .symtab does not trigger loading in my experiments.  (The other symbols
> we preserve for valgrind's sake and are immaterial to GDB.)  In other
> words,
> 
>   cp /lib64/libc.so.6 . ; strip libc.so.6 ; LD_LIBRARY_PATH=. gdb …
> 
> does not result in working thread debugging.

This has to do with lookup_minimal_symbol() API in gdb as I noted to Simon.

I don't think gdb has loaded .dynsyms yet, but it could have.

I think we might ask Kevin or Simon to verify what is going on there.

> If the pthread_create .symtab requirement is too difficult to work
> around, we might as well put the _thread_db_* symbols into .symtab and
> tell distributions to use:
> 
>   strip -w -K pthread_create -K _thread_db_\* libc.so.6
> 
> But then we'd need two categories of symbols in the internal consistency
> check for nptl_db (the db-symbol.awk file), so maybe that's overdoing
> things for a slightly smaller .dynsym size.
> 
> Thanks,
> Florian
> 
> 8<------------------------------------------------------------------8<

Note: Patch starts here.

> This allows distributions to strip debugging information from
> libc.so.6 without impacting the debugging experience.
> 
> nptl_version had to be renamed to __nptl_version to avoid
> namespace issues.
> ---
>  nptl/Versions          | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  nptl/pthread_create.c  | 13 +++++++-----
>  nptl_db/Makefile       |  2 +-
>  nptl_db/db-symbols.awk |  4 +++-
>  nptl_db/structs.def    |  2 +-
>  nptl_db/td_ta_new.c    |  2 +-
>  6 files changed, 70 insertions(+), 9 deletions(-)
> 
> diff --git a/nptl/Versions b/nptl/Versions
> index 62bb939d54..c03ed92848 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -403,10 +403,14 @@ libc {
>      __nptl_deallocate_tsd;
>      __nptl_death_event;
>      __nptl_free_tcb;
> +    __nptl_last_event;

OK. Defined with DB_VARIABLE.

>      __nptl_nthreads;
> +    __nptl_rtld_global;

OK. Defined with DB_VARIABLE.

>      __nptl_setxid_sighandler;
>      __nptl_stack_list_add;
>      __nptl_stack_list_del;
> +    __nptl_threads_events;
> +    __nptl_version;

OK. Both defined with DB_SYMBOL.

>      __pthread_attr_copy;
>      __pthread_attr_destroy;
>      __pthread_attr_init;
> @@ -430,6 +434,58 @@ libc {
>      __pthread_unwind;
>      __sched_fifo_max_prio;
>      __sched_fifo_min_prio;

I audited this against the current pristine upstream.

I see 3 additional symbols not in your list:

_thread_db___nptl_initial_report_events (DB_RTLD_VARIABLE)
_thread_db___nptl_nthreads (DB_MAIN_VARIABLE)
_thread_db___pthread_keys (DB_MAIN_ARRAY_VARIABLE)

Given that they *are* party of the db, is there any reason
not to include them for completeness?

So something like this...

diff --git a/nptl/Versions b/nptl/Versions
index 0a366a8a29..bd0b56dc7c 100644
--- a/nptl/Versions
+++ b/nptl/Versions
@@ -434,6 +434,8 @@ libc {
     __pthread_unwind;
     __sched_fifo_max_prio;
     __sched_fifo_min_prio;
+    _thread_db___nptl_nthreads;
+    _thread_db___pthread_keys;
     _thread_db___nptl_last_event;
     _thread_db___nptl_rtld_global;
     _thread_db_const_thread_area;
@@ -572,5 +574,6 @@ ld {
   GLIBC_PRIVATE {
      __nptl_initial_report_events;
      __nptl_set_robust_list_avail;
+     _thread_db___nptl_initial_report_events;
   }
 }
diff --git a/nptl_db/db-symbols.awk b/nptl_db/db-symbols.awk
index 2033f95e38..257a213d5c 100644
--- a/nptl_db/db-symbols.awk
+++ b/nptl_db/db-symbols.awk
@@ -3,9 +3,7 @@
 
 BEGIN {
 %define DB_RTLD_VARIABLE(name) /* Nothing. */
-%define DB_MAIN_VARIABLE(name) /* Nothing. */
-%define DB_MAIN_SYMBOL(name) /* Nothing. */
-%define DB_MAIN_ARRAY_VARIABLE(name) /* Nothing. */
+%define DB_RTLD_GLOBAL_FIELD(name) /* Nothing. */
 %define DB_LOOKUP_NAME(idx, name)              required[STRINGIFY (name)] = 1;
 %define DB_LOOKUP_NAME_TH_UNIQUE(idx, name)    th_unique[STRINGIFY (name)] = 1;
 %include "db-symbols.h"
---

Adds _thread_db___nptl_nthreads, and _thread_db___pthread_keys, which
are there already, and because we drop DB_MAIN_VARIABLE and DB_MAIN_SYMBOL
from the db-symbolc.awk exclusion list we have testing which covers *both*
verifying the _thread_db* symbol is there and the underlying variable or
symbol is there.

e.g.

cat nptl_db/db-symbols.out | grep __pthread_keys
_thread_db___pthread_keys@@GLIBC_PRIVATE ok
__pthread_keys@@GLIBC_PRIVATE ok

cat nptl_db/db-symbols.out | grep __nptl_nthreads
__nptl_nthreads@@GLIBC_PRIVATE ok
_thread_db___nptl_nthreads@@GLIBC_PRIVATE ok

So we increased coverage.

But we had remove the DB_RTLD_GLOBAL_FIELD entries because they are just
offsets into __nptl_rtld_global and we can't test for those easily.

	> +    _thread_db___nptl_last_event;
	> +    _thread_db___nptl_rtld_global;
	> +    _thread_db_const_thread_area;
	> +    _thread_db_dtv_dtv;
	> +    _thread_db_dtv_slotinfo_gen;
	> +    _thread_db_dtv_slotinfo_list_len;
	> +    _thread_db_dtv_slotinfo_list_next;
	> +    _thread_db_dtv_slotinfo_list_slotinfo;
	> +    _thread_db_dtv_slotinfo_map;
	> +    _thread_db_dtv_t_counter;
	> +    _thread_db_dtv_t_pointer_val;
	> +    _thread_db_link_map_l_tls_modid;
	> +    _thread_db_link_map_l_tls_offset;
	> +    _thread_db_list_t_next;
	> +    _thread_db_list_t_prev;
	> +    _thread_db_pthread_cancelhandling;
	> +    _thread_db_pthread_dtvp;
	> +    _thread_db_pthread_eventbuf;
	> +    _thread_db_pthread_eventbuf_eventmask;
	> +    _thread_db_pthread_eventbuf_eventmask_event_bits;
	> +    _thread_db_pthread_key_data_data;
	> +    _thread_db_pthread_key_data_level2_data;
	> +    _thread_db_pthread_key_data_seq;
	> +    _thread_db_pthread_key_struct_destr;
	> +    _thread_db_pthread_key_struct_seq;
	> +    _thread_db_pthread_list;
	> +    _thread_db_pthread_nextevent;
	> +    _thread_db_pthread_report_events;
	> +    _thread_db_pthread_schedparam_sched_priority;
	> +    _thread_db_pthread_schedpolicy;
	> +    _thread_db_pthread_specific;
	> +    _thread_db_pthread_start_routine;
	> +    _thread_db_pthread_tid;

> +    _thread_db_register32;
> +    _thread_db_register32_thread_area;
> +    _thread_db_register64;
> +    _thread_db_register64_thread_area;

These four are defined in db-symbols.h. 

	> +    _thread_db_rtld_global__dl_stack_used;
	> +    _thread_db_rtld_global__dl_stack_user;
	> +    _thread_db_rtld_global__dl_tls_dtv_slotinfo_list;
	> +    _thread_db_sizeof_dtv_slotinfo;
	> +    _thread_db_sizeof_dtv_slotinfo_list;
	> +    _thread_db_sizeof_list_t;
	> +    _thread_db_sizeof_pthread;
	> +    _thread_db_sizeof_pthread_key_data;
	> +    _thread_db_sizeof_pthread_key_data_level2;
	> +    _thread_db_sizeof_pthread_key_struct;
	> +    _thread_db_sizeof_td_eventbuf_t;
	> +    _thread_db_sizeof_td_thr_events_t;
	> +    _thread_db_td_eventbuf_t_eventdata;
	> +    _thread_db_td_eventbuf_t_eventnum;
	> +    _thread_db_td_thr_events_t_event_bits;
>    }
>  }
>  
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index 3f017f1e26..d1b6817a81 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -43,21 +43,24 @@
>  
>  
>  /* Globally enabled events.  */
> -static td_thr_events_t __nptl_threads_events __attribute_used__;
> +td_thr_events_t __nptl_threads_events __attribute__ ((nocommon));
> +libc_hidden_proto (__nptl_threads_events)
> +libc_hidden_data_def (__nptl_threads_events)

OK.

>  
>  /* Pointer to descriptor with the last event.  */
> -static struct pthread *__nptl_last_event __attribute_used__;
> +struct pthread *__nptl_last_event __attribute__ ((nocommon));
> +libc_hidden_proto (__nptl_last_event)
> +libc_hidden_data_def (__nptl_last_event)

OK.

>  
>  #ifdef SHARED
>  /* This variable is used to access _rtld_global from libthread_db.  If
>     GDB loads libpthread before ld.so, it is not possible to resolve
>     _rtld_global directly during libpthread initialization.  */
> -static struct rtld_global *__nptl_rtld_global __attribute_used__
> -  = &_rtld_global;
> +struct rtld_global *__nptl_rtld_global = &_rtld_global;

OK.

>  #endif
>  
>  /* Version of the library, used in libthread_db to detect mismatches.  */
> -static const char nptl_version[] __attribute_used__ = VERSION;
> +const char __nptl_version[] = VERSION;

OK.

>  
>  /* This performs the initialization necessary when going from
>     single-threaded to multi-threaded mode for the first time.  */
> diff --git a/nptl_db/Makefile b/nptl_db/Makefile
> index ea721c1dcf..4cc51c0e7b 100644
> --- a/nptl_db/Makefile
> +++ b/nptl_db/Makefile
> @@ -57,7 +57,7 @@ include ../Rules
>  
>  $(objpfx)db-symbols.out: $(objpfx)db-symbols.v.i \
>  			 $(common-objpfx)libc.so
> -	LC_ALL=C $(READELF) -W -s $(filter %.so,$^) | $(AWK) -f $< > $@; \
> +	LC_ALL=C $(READELF) -W -D -s $(filter %.so,$^) | $(AWK) -f $< > $@; \

OK. Use .dynsyms.

>  	$(evaluate-test)
>  
>  $(objpfx)db-symbols.v.i: db-symbols.awk
> diff --git a/nptl_db/db-symbols.awk b/nptl_db/db-symbols.awk
> index 6f326cf379..2033f95e38 100644
> --- a/nptl_db/db-symbols.awk
> +++ b/nptl_db/db-symbols.awk

Comment at the start of db-symbols.awk needs fixing.

Suggest:
# This script processes the output of 'readelf -W -D -s' on the libc.so
# we've just built.  It checks for all the symbols used in td_symbol_list.


> @@ -13,7 +13,7 @@ BEGIN {
>     in_symtab = 0;
>  }
>  
> -/Symbol table '.symtab'/ { in_symtab=1; next }
> +/Symbol table for image/ { in_symtab=1; next }

OK. Changed due to -D usage.

>  NF == 0 { in_symtab=0; next }
>  
>  !in_symtab { next }
> @@ -24,6 +24,7 @@ END {
>    status = 0;
>  
>    for (s in required) {
> +    s = s "@@GLIBC_PRIVATE"

OK. Agreed, should be private.

>      if (s in seen) print s, "ok";
>      else {
>        status = 1;
> @@ -33,6 +34,7 @@ END {
>  
>    any = "";
>    for (s in th_unique) {
> +    s = s "@@GLIBC_PRIVATE"

OK. Agreed, should be private.

>      if (s in seen) {
>        any = s;
>        break;
> diff --git a/nptl_db/structs.def b/nptl_db/structs.def
> index 6a726f207e..e2e51acc39 100644
> --- a/nptl_db/structs.def
> +++ b/nptl_db/structs.def
> @@ -77,7 +77,7 @@ DB_STRUCT (td_eventbuf_t)
>  DB_STRUCT_FIELD (td_eventbuf_t, eventnum)
>  DB_STRUCT_FIELD (td_eventbuf_t, eventdata)
>  
> -DB_SYMBOL (nptl_version)
> +DB_SYMBOL (__nptl_version)

OK. Namespace clean using leading __.

>  DB_MAIN_SYMBOL (__nptl_create_event)
>  DB_MAIN_SYMBOL (__nptl_death_event)
>  DB_SYMBOL (__nptl_threads_events)
> diff --git a/nptl_db/td_ta_new.c b/nptl_db/td_ta_new.c
> index 501d922ea2..eeca29d5a0 100644
> --- a/nptl_db/td_ta_new.c
> +++ b/nptl_db/td_ta_new.c
> @@ -39,7 +39,7 @@ td_ta_new (struct ps_prochandle *ps, td_thragent_t **ta)
>    LOG ("td_ta_new");
>  
>    /* Check whether the versions match.  */
> -  if (td_lookup (ps, SYM_nptl_version, &versaddr) != PS_OK)> +  if (td_lookup (ps, SYM___nptl_version, &versaddr) != PS_OK)

OK. Internally consistent. Matches internal definition.

>      return TD_NOLIBTHREAD;
>    if (ps_pdread (ps, versaddr, versbuf, sizeof (versbuf)) != PS_OK)
>      return TD_ERR;
> 


-- 
Cheers,
Carlos.