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 [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id C329A3858D28 for ; Tue, 23 Nov 2021 17:56:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org C329A3858D28 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-239-wJlomerFMEK66vadTOu5Yg-1; Tue, 23 Nov 2021 12:56:25 -0500 X-MC-Unique: wJlomerFMEK66vadTOu5Yg-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4B672100CCCC; Tue, 23 Nov 2021 17:56:24 +0000 (UTC) Received: from oldenburg.str.redhat.com (unknown [10.39.192.29]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E26D05DF5F; Tue, 23 Nov 2021 17:56:16 +0000 (UTC) From: Florian Weimer To: Jakub Jelinek Cc: gcc-patches@gcc.gnu.org, libc-alpha@sourceware.org, Jason Merrill Subject: Re: [PATCH 3/4] libgcc: Split FDE search code from PT_GNU_EH_FRAME lookup References: <53daedec153e3bf9b1a9c14f61cfe23385de80c9.1635955148.git.fweimer@redhat.com> <20211118152117.GE2646553@tucnak> Date: Tue, 23 Nov 2021 18:56:14 +0100 In-Reply-To: <20211118152117.GE2646553@tucnak> (Jakub Jelinek's message of "Thu, 18 Nov 2021 16:21:17 +0100") Message-ID: <87bl2arc1t.fsf@oldenburg.str.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain 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, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=unavailable autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 23 Nov 2021 17:56:30 -0000 * Jakub Jelinek: > On Wed, Nov 03, 2021 at 05:28:48PM +0100, Florian Weimer wrote: >> @@ -383,12 +376,34 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) >> # endif >> #endif >> >> - _Unwind_Ptr dbase = unw_eh_callback_data_dbase (data); >> + return 1; >> +} >> + >> +/* Result type of find_fde_tail below. */ >> +struct find_fde_tail_result >> +{ >> + const fde *entry; >> + void *func; >> +}; >> + >> +/* Find the FDE for the program counter PC, in a previously located >> + PT_GNU_EH_FRAME data region. */ >> +static struct find_fde_tail_result >> +find_fde_tail (_Unwind_Ptr pc, >> + const struct unw_eh_frame_hdr *hdr, >> + _Unwind_Ptr dbase) > > I think returning a struct like find_fde_tail_result can work nicely > on certain targets, but on many others the psABI forces such returns through > stack etc. > Wouldn't it be better to return const fde * instead of > struct find_fde_tail_result, pass in struct dwarf_eh_bases *bases > as another argument to find_fde_tail, just return NULL on the failure > cases and return some fde pointer and set bases->func on success? I've refactored it further in the version below. I introduced the struct to consolidate the *bases struct update among the success cases, but I think it's okay to do this inline. I didn't introduce a separate function for that. I tested this in isolation on x86-64 and i386, with no apparent regressions. I think the changes are compatible with the fourth patch (which I still have to rebase on top of that). Thanks, Florian 8<------------------------------------------------------------------8< This allows switching to a different implementation for PT_GNU_EH_FRAME lookup in a subsequent commit. This moves some of the PT_GNU_EH_FRAME parsing out of the glibc loader lock that is implied by dl_iterate_phdr. However, the FDE is already parsed outside the lock before this change, so this does not introduce additional crashes in case of a concurrent dlclose. libunwind/ChangeLog * unwind-dw2-fde-dip.c (struct unw_eh_callback_data): Add hdr. Remove func, ret. (find_fde_tail): New function. Split from _Unwind_IteratePhdrCallback. Move the result initialization from _Unwind_Find_FDE. (_Unwind_Find_FDE): Updated to call find_fde_tail. --- libgcc/unwind-dw2-fde-dip.c | 92 ++++++++++++++++++++++++--------------------- 1 file changed, 50 insertions(+), 42 deletions(-) diff --git a/libgcc/unwind-dw2-fde-dip.c b/libgcc/unwind-dw2-fde-dip.c index 3f302826d2d..fbb0fbdebb9 100644 --- a/libgcc/unwind-dw2-fde-dip.c +++ b/libgcc/unwind-dw2-fde-dip.c @@ -113,8 +113,7 @@ struct unw_eh_callback_data #if NEED_DBASE_MEMBER void *dbase; #endif - void *func; - const fde *ret; + const struct unw_eh_frame_hdr *hdr; int check_cache; }; @@ -197,10 +196,6 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) #else _Unwind_Ptr load_base; #endif - const unsigned char *p; - const struct unw_eh_frame_hdr *hdr; - _Unwind_Ptr eh_frame; - struct object ob; _Unwind_Ptr pc_low = 0, pc_high = 0; struct ext_dl_phdr_info @@ -348,10 +343,8 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) return 0; /* Read .eh_frame_hdr header. */ - hdr = (const struct unw_eh_frame_hdr *) + data->hdr = (const struct unw_eh_frame_hdr *) __RELOC_POINTER (p_eh_frame_hdr->p_vaddr, load_base); - if (hdr->version != 1) - return 1; #ifdef CRT_GET_RFIB_DATA # if defined __i386__ || defined __nios2__ @@ -383,12 +376,30 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) # endif #endif - _Unwind_Ptr dbase = unw_eh_callback_data_dbase (data); + return 1; +} + +/* Find the FDE for the program counter PC, in a previously located + PT_GNU_EH_FRAME data region. *BASES is updated if an FDE to return is + found. */ + +static const fde * +find_fde_tail (_Unwind_Ptr pc, + const struct unw_eh_frame_hdr *hdr, + _Unwind_Ptr dbase, + struct dwarf_eh_bases *bases) +{ + const unsigned char *p = (const unsigned char *) (hdr + 1); + _Unwind_Ptr eh_frame; + struct object ob; + + if (hdr->version != 1) + return NULL; + p = read_encoded_value_with_base (hdr->eh_frame_ptr_enc, base_from_cb_data (hdr->eh_frame_ptr_enc, dbase), - (const unsigned char *) (hdr + 1), - &eh_frame); + p, &eh_frame); /* We require here specific table encoding to speed things up. Also, DW_EH_PE_datarel here means using PT_GNU_EH_FRAME start @@ -404,7 +415,7 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) p, &fde_count); /* Shouldn't happen. */ if (fde_count == 0) - return 1; + return NULL; if ((((_Unwind_Ptr) p) & 3) == 0) { struct fde_table { @@ -419,9 +430,9 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) _Unwind_Ptr range; mid = fde_count - 1; - if (data->pc < table[0].initial_loc + data_base) - return 1; - else if (data->pc < table[mid].initial_loc + data_base) + if (pc < table[0].initial_loc + data_base) + return NULL; + else if (pc < table[mid].initial_loc + data_base) { lo = 0; hi = mid; @@ -429,9 +440,9 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) while (lo < hi) { mid = (lo + hi) / 2; - if (data->pc < table[mid].initial_loc + data_base) + if (pc < table[mid].initial_loc + data_base) hi = mid; - else if (data->pc >= table[mid + 1].initial_loc + data_base) + else if (pc >= table[mid + 1].initial_loc + data_base) lo = mid + 1; else break; @@ -445,10 +456,16 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) f_enc_size = size_of_encoded_value (f_enc); read_encoded_value_with_base (f_enc & 0x0f, 0, &f->pc_begin[f_enc_size], &range); - if (data->pc < table[mid].initial_loc + data_base + range) - data->ret = f; - data->func = (void *) (table[mid].initial_loc + data_base); - return 1; + _Unwind_Ptr func = table[mid].initial_loc + data_base; + if (pc < table[mid].initial_loc + data_base + range) + { + bases->tbase = NULL; + bases->dbase = (void *) dbase; + bases->func = (void *) func; + return f; + } + else + return NULL; } } @@ -461,18 +478,20 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) ob.u.single = (fde *) eh_frame; ob.s.i = 0; ob.s.b.mixed_encoding = 1; /* Need to assume worst case. */ - data->ret = linear_search_fdes (&ob, (fde *) eh_frame, (void *) data->pc); - if (data->ret != NULL) + const fde *entry = linear_search_fdes (&ob, (fde *) eh_frame, (void *) pc); + if (entry != NULL) { _Unwind_Ptr func; - unsigned int encoding = get_fde_encoding (data->ret); + unsigned int encoding = get_fde_encoding (entry); read_encoded_value_with_base (encoding, base_from_cb_data (encoding, dbase), - data->ret->pc_begin, &func); - data->func = (void *) func; + entry->pc_begin, &func); + bases->tbase = NULL; + bases->dbase = (void *) dbase; + bases->func = (void *) func; } - return 1; + return entry; } const fde * @@ -489,24 +508,13 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) #if NEED_DBASE_MEMBER data.dbase = NULL; #endif - data.func = NULL; - data.ret = NULL; data.check_cache = 1; - if (dl_iterate_phdr (_Unwind_IteratePhdrCallback, &data) < 0) + if (dl_iterate_phdr (_Unwind_IteratePhdrCallback, &data) <= 0) return NULL; - if (data.ret) - { - bases->tbase = NULL; -#if NEED_DBASE_MEMBER - bases->dbase = data.dbase; -#else - bases->dbase = NULL; -#endif - bases->func = data.func; - } - return data.ret; + _Unwind_Ptr dbase = unw_eh_callback_data_dbase (&data); + return find_fde_tail ((_Unwind_Ptr) pc, data.hdr, dbase, bases); } #else