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

[Carlos O'Donell <carlos@redhat.com>]

On 6/28/21 5:09 AM, Florian Weimer via Libc-alpha wrote:
> This allows distributions to strip debugging information from
> libc.so.6 without impacting the debugging experience.
> 
> The commit also enhances the checks in nptl/db-symbols.awk to cover
> both the _thread_db_* descriptor symbols and the actual symbols
> themselves.

Please repost a v3 of this patch with the additional redundant checks
dropped. Thank you.

> ---
>  nptl/Versions          | 58 ++++++++++++++++++++++++++++++++++++++++++
>  nptl/pthread_create.c  | 13 ++++++----
>  nptl_db/Makefile       |  2 +-
>  nptl_db/db-symbols.awk | 16 ++++++++++--
>  4 files changed, 81 insertions(+), 8 deletions(-)
> 
> diff --git a/nptl/Versions b/nptl/Versions
> index e4fae73c0b..060d8d5dec 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.

>      __nptl_nthreads;
> +    __nptl_rtld_global;

OK.

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

OK.

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

OK. Add __nptl_nthreads.

> +    _thread_db___nptl_rtld_global;
> +    _thread_db___pthread_keys;

OK. Add __pthread_keys.

> +    _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;
> +    _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)
>  
>  /* 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)
>  
>  #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;
>  #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 1f79c018a1..c04aa6140a 100644
> --- a/nptl_db/Makefile
> +++ b/nptl_db/Makefile
> @@ -55,7 +55,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 ef1d91b167..a9289b4686 100644
> --- a/nptl_db/db-symbols.awk
> +++ b/nptl_db/db-symbols.awk
> @@ -1,7 +1,8 @@
> -# This script processes the output of 'readelf -W -s' on the libpthread.so
> +# This script processes the output of 'readelf -W -D -s' on the libc.so

OK.

>  # we've just built.  It checks for all the symbols used in td_symbol_list.
>  
>  BEGIN {
> +# This processes the _thread_db_* names.

This comment is not quite accurate. We process both.

>  %define DB_MAIN_VARIABLE(name) /* Nothing. */
>  %define DB_MAIN_SYMBOL(name) /* Nothing. */
>  %define DB_MAIN_ARRAY_VARIABLE(name) /* Nothing. */
> @@ -9,10 +10,19 @@ BEGIN {
>  %define DB_LOOKUP_NAME_TH_UNIQUE(idx, name)	th_unique[STRINGIFY (name)] = 1;
>  %include "db-symbols.h"

This also includes structs.def via db-symbols.h and also processes
the whole thing via the preprocessor and so so at each DB_LOOKUP_NAME
instance we get an expansion to 'requireded[STRINGIFY (name)] = 1;'

So where we have DB_VARIABLE in db-symbols.h we get two expansions:

 33 #define DB_VARIABLE(name) \
 34   DB_LOOKUP_NAME (SYM_##name, name) \
 35   DB_LOOKUP_NAME (SYM_DESC_##name, _thread_db_##name)

That cover checking both the non-_thread_db_* and _thread_db_* symbols.

From a build with db-symbols.v.i I see duplicate checks for __pthread_keys.

I think you can drop the additional checks below.
  
> +# And this processes the symbol names themselves.
> +%define DB_STRUCT(...) /* Nothing.  */
> +%define DB_STRUCT_FIELD(...) /* Nothing.  */
> +%define DB_STRUCT_FLEXIBLE_ARRAY(...) /* Nothing.  */
> +%define DB_SYMBOL(name) required[STRINGIFY (name)] = 1;
> +%define DB_FUNCTION(name) required[STRINGIFY (name)] = 1;
> +%define DB_VARIABLE(name) required[STRINGIFY (name)] = 1;
> +%include "structs.def"
> +
>     in_symtab = 0;
>  }
>  
> -/Symbol table '.symtab'/ { in_symtab=1; next }
> +/Symbol table for image/ { in_symtab=1; next }
>  NF == 0 { in_symtab=0; next }
>  
>  !in_symtab { next }
> @@ -23,6 +33,7 @@ END {
>    status = 0;
>  
>    for (s in required) {
> +    s = s "@@GLIBC_PRIVATE"
>      if (s in seen) print s, "ok";
>      else {
>        status = 1;
> @@ -32,6 +43,7 @@ END {
>  
>    any = "";
>    for (s in th_unique) {
> +    s = s "@@GLIBC_PRIVATE"
>      if (s in seen) {
>        any = s;
>        break;
> 


-- 
Cheers,
Carlos.