nptl: Export libthread_db-used symbols under GLIBC_PRIVATE

  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>

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.

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

On 2021-06-17 3:23 p.m., Florian Weimer via Gdb 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>
> 
> 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,

Stupid question, but: since pthread_create is a function exposed by the
libc.so shared library, won't there still be an entry for it in .dynsym?
And if so, shouldn't GDB see it?

Simon
  

On 6/20/21 8:38 AM, Simon Marchi via Libc-alpha wrote:
> On 2021-06-17 3:23 p.m., Florian Weimer via Gdb 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>
>>
>> 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,
> 
> Stupid question, but: since pthread_create is a function exposed by the
> libc.so shared library, won't there still be an entry for it in .dynsym?

No question is stupid. Thank you for asking.

Yes, there *must* be a .dynsym entry for pthread_create in libc.so.6 in
order for the dynamic loader to bind the reference to the definition.

> And if so, shouldn't GDB see it?

It should. Today gdb is only using the internal lookup_minimal_symbol()
API in order to trigger libthread_db loading, and it's not clear to me
if that will load *all* of the dynamic symbols during gdb startup.

Someone would need to debug the minsyms.c API to see if something isn't
working as expected.

gdb/minsyms.c:

1267    When files contain multiple sources of symbol information, it is
1268    possible for the minimal symbol table to contain many duplicate entries.
1269    As an example, SVR4 systems use ELF formatted object files, which
1270    usually contain at least two different types of symbol tables (a
1271    standard ELF one and a smaller dynamic linking table), as well as
1272    DWARF debugging information for files compiled with -g.

So it sounds like the data is loadable, and mergeable, but it appears that
it's not all there at early startup in gdb.
  

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

* Carlos O'Donell:

> 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?

I'm going to send a new series which brings this to the maximum set of
symbols.

Thanks,
Florian
  

On 2021-06-27 6:25 p.m., Carlos O'Donell wrote:> On 6/20/21 8:38 AM, Simon Marchi via Libc-alpha wrote:
>> On 2021-06-17 3:23 p.m., Florian Weimer via Gdb 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>
>>>
>>> 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,
>>
>> Stupid question, but: since pthread_create is a function exposed by the
>> libc.so shared library, won't there still be an entry for it in .dynsym?
> 
> No question is stupid. Thank you for asking.
> 
> Yes, there *must* be a .dynsym entry for pthread_create in libc.so.6 in
> order for the dynamic loader to bind the reference to the definition.
> 
>> And if so, shouldn't GDB see it?
> 
> It should. Today gdb is only using the internal lookup_minimal_symbol()
> API in order to trigger libthread_db loading, and it's not clear to me
> if that will load *all* of the dynamic symbols during gdb startup.
> 
> Someone would need to debug the minsyms.c API to see if something isn't
> working as expected.
> 
> gdb/minsyms.c:
> 
> 1267    When files contain multiple sources of symbol information, it is
> 1268    possible for the minimal symbol table to contain many duplicate entries.
> 1269    As an example, SVR4 systems use ELF formatted object files, which
> 1270    usually contain at least two different types of symbol tables (a
> 1271    standard ELF one and a smaller dynamic linking table), as well as
> 1272    DWARF debugging information for files compiled with -g.
> 
> So it sounds like the data is loadable, and mergeable, but it appears that
> it's not all there at early startup in gdb.

My intuition is that the .dynsym symbols are normally loaded and result
in minimal symbols in GDB:

    https://gitlab.com/gnutools/gdb/-/blob/master/gdb/elfread.c#L1102-1128

But there may be some filtering that prevents the symbols like
pthread_create from making their way into the minimal symbols database.

I'll have to try using the container that Florian pointed to
(registry.fedoraproject.org/fedora:rawhide).

Simon
  

* Simon Marchi:

> I'll have to try using the container that Florian pointed to
> (registry.fedoraproject.org/fedora:rawhide).

You may have to fully strip /usr/lib64/libc.so.6 first because we have a
fairly complete .symtab for other tools (not just GDB).  Currently it's
glibc-2.33.9000-29.fc35.x86_64, and it does not have pthread_create, but
Fedora rawhide glibc is currently under very heavy development.

Thanks,
Florian