From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 72A403844019 for ; Mon, 28 Jun 2021 02:10:36 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 72A403844019 Received: from mail-qt1-f198.google.com (mail-qt1-f198.google.com [209.85.160.198]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-497-X9vjZX5FNLmXRZ5Y7DpVDg-1; Sun, 27 Jun 2021 22:10:32 -0400 X-MC-Unique: X9vjZX5FNLmXRZ5Y7DpVDg-1 Received: by mail-qt1-f198.google.com with SMTP id h28-20020ac8777c0000b0290250abe79a79so3688849qtu.14 for ; Sun, 27 Jun 2021 19:10:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=p4bB6c3TezW/V/NI1R2x0MkHuDBzX94LiJssnd67+5A=; b=MtCQzmKJ2Yuw1sl2E6YteYzJ6oXFND+uLPFbTopt+aywpNv4UCjtynQuQYsgsQAgFC RN38pjKDksujAIFXtrKDXs24sakXEA5+NwbJ0hIN02LDXz0duwtDFVCO93E6f0fYUpCQ IrLlIFjJbPojm3TUmzsksJxRyiHatduqkvmGiTlYq0cpjSm9wnV1j6oyolioOZuposnv 1N8a1anBhFwCPz8n/TfQYkf4PeQOvPxUCLJNwvED0R7PpPaKjVuIXzUxN+ZGdhvUvu/4 FRHmuQ/+3TgJWKFCRtZtFE5y7pDZB5vkjKTsI70znZJ+gFK/fvpgeJFsVXswRa+rjOyj PFDA== X-Gm-Message-State: AOAM531gOyH6+NzzF/3JxsGqjpGkFBZN7GFIwfmcSzKacbbGL0BvbE++ XOmI2rThkwaGYHBl9fAW8yepXe0Dup+7c5HIDDU71Po1xEEzQK+MtYuerTCdaSM7lku9a85CpYy igpLN+a/SdFg= X-Received: by 2002:a05:620a:d45:: with SMTP id o5mr22879863qkl.319.1624846231727; Sun, 27 Jun 2021 19:10:31 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxmci62td8nquXL48MQR3TiAd06XPiQ6oMBKUeIF4IRK6IvLENEJetKDLwNJoV65VPxKYfWnA== X-Received: by 2002:a05:620a:d45:: with SMTP id o5mr22879856qkl.319.1624846231491; Sun, 27 Jun 2021 19:10:31 -0700 (PDT) Received: from [192.168.1.16] (198-84-214-74.cpe.teksavvy.com. [198.84.214.74]) by smtp.gmail.com with ESMTPSA id c4sm10014483qkj.81.2021.06.27.19.10.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 27 Jun 2021 19:10:30 -0700 (PDT) Subject: Re: [PATCH] nptl: Export libthread_db-used symbols under GLIBC_PRIVATE To: Florian Weimer , libc-alpha@sourceware.org Cc: gdb@sourceware.org, Kevin Buettner References: <877disuwfq.fsf@oldenburg.str.redhat.com> From: Carlos O'Donell Organization: Red Hat Message-ID: <1058178f-a8d0-4987-cc14-1c9f6da7af5d@redhat.com> Date: Sun, 27 Jun 2021 22:10:29 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: <877disuwfq.fsf@oldenburg.str.redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP, URIBL_BLACK autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Jun 2021 02:10:41 -0000 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: > > 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.