* [PATCH 0/4] Use _dl_find_eh_frame to locate DWARF EH data in the unwinder @ 2021-11-03 16:28 Florian Weimer 2021-11-03 16:28 ` [PATCH 1/4] libgcc: Remove tbase member from struct unw_eh_callback_data Florian Weimer ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Florian Weimer @ 2021-11-03 16:28 UTC (permalink / raw) To: gcc-patches; +Cc: libc-alpha, Jakub Jelinek, Jason Merrill This is the GCC side of the patch series. To simplify testing, a weak reference to _dl_find_eh_frame is used to enable this feature when running on newer glibc even if built for older glibc. The first three patches are cleanups/refactorings to simplify the actual change in the last patch. Benchmarking-wise, the new unwinder is slightly faster even in the optimal case for the old implementation (single-threaded, 100% cache hit rate). The old implementation performs really poorly once the cache hit rate drops and the number of shared objects participating in unwinding increases, so that's not a fair comparison. Old performance with multiple threads is also poor due to the global loader lock implied by dl_iterate_phdr (which is necessary to serialize access to the libgcc unwinder cache), and I haven't bother to benchmark that. Thanks, Florian Florian Weimer (4): libgcc: Remove tbase member from struct unw_eh_callback_data libgcc: Remove dbase member from struct unw_eh_callback_data if NULL libgcc: Split FDE search code from PT_GNU_EH_FRAME lookup libgcc: Use _dl_find_eh_frame in _Unwind_Find_FDE libgcc/unwind-dw2-fde-dip.c | 185 +++++++++++++++++++++++++++--------- 1 file changed, 139 insertions(+), 46 deletions(-) base-commit: 6b8b25575570ffde37cc8997af096514b929779d -- 2.31.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/4] libgcc: Remove tbase member from struct unw_eh_callback_data 2021-11-03 16:28 [PATCH 0/4] Use _dl_find_eh_frame to locate DWARF EH data in the unwinder Florian Weimer @ 2021-11-03 16:28 ` Florian Weimer 2021-11-18 13:47 ` Jakub Jelinek 2021-11-03 16:28 ` [PATCH 2/4] libgcc: Remove dbase member from struct unw_eh_callback_data if NULL Florian Weimer ` (2 subsequent siblings) 3 siblings, 1 reply; 13+ messages in thread From: Florian Weimer @ 2021-11-03 16:28 UTC (permalink / raw) To: gcc-patches; +Cc: libc-alpha, Jakub Jelinek, Jason Merrill It is always a null pointer. libgcc/ChangeLog * unwind-dw2-fde-dip.c (struct unw_eh_callback_data): Remove tbase member. (base_from_cb_data): Adjust. (_Unwind_IteratePhdrCallback): Likewise. (_Unwind_Find_FDE): Likewise. --- libgcc/unwind-dw2-fde-dip.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/libgcc/unwind-dw2-fde-dip.c b/libgcc/unwind-dw2-fde-dip.c index 5095b6830bf..4a4d990f455 100644 --- a/libgcc/unwind-dw2-fde-dip.c +++ b/libgcc/unwind-dw2-fde-dip.c @@ -104,7 +104,6 @@ static const fde * _Unwind_Find_registered_FDE (void *pc, struct dwarf_eh_bases struct unw_eh_callback_data { _Unwind_Ptr pc; - void *tbase; void *dbase; void *func; const fde *ret; @@ -154,7 +153,7 @@ base_from_cb_data (unsigned char encoding, struct unw_eh_callback_data *data) return 0; case DW_EH_PE_textrel: - return (_Unwind_Ptr) data->tbase; + return 0; case DW_EH_PE_datarel: return (_Unwind_Ptr) data->dbase; default: @@ -431,7 +430,7 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) As soon as GLIBC will provide API so to notify that a library has been removed, we could cache this (and thus use search_object). */ ob.pc_begin = NULL; - ob.tbase = data->tbase; + ob.tbase = NULL; ob.dbase = data->dbase; ob.u.single = (fde *) eh_frame; ob.s.i = 0; @@ -461,7 +460,6 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) return ret; data.pc = (_Unwind_Ptr) pc; - data.tbase = NULL; data.dbase = NULL; data.func = NULL; data.ret = NULL; @@ -472,7 +470,7 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) if (data.ret) { - bases->tbase = data.tbase; + bases->tbase = NULL; bases->dbase = data.dbase; bases->func = data.func; } -- 2.31.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] libgcc: Remove tbase member from struct unw_eh_callback_data 2021-11-03 16:28 ` [PATCH 1/4] libgcc: Remove tbase member from struct unw_eh_callback_data Florian Weimer @ 2021-11-18 13:47 ` Jakub Jelinek 0 siblings, 0 replies; 13+ messages in thread From: Jakub Jelinek @ 2021-11-18 13:47 UTC (permalink / raw) To: Florian Weimer; +Cc: gcc-patches, libc-alpha, Jason Merrill On Wed, Nov 03, 2021 at 05:28:35PM +0100, Florian Weimer wrote: > It is always a null pointer. > > libgcc/ChangeLog > > * unwind-dw2-fde-dip.c (struct unw_eh_callback_data): Remove > tbase member. > (base_from_cb_data): Adjust. > (_Unwind_IteratePhdrCallback): Likewise. > (_Unwind_Find_FDE): Likewise. Ok. Jakub ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/4] libgcc: Remove dbase member from struct unw_eh_callback_data if NULL 2021-11-03 16:28 [PATCH 0/4] Use _dl_find_eh_frame to locate DWARF EH data in the unwinder Florian Weimer 2021-11-03 16:28 ` [PATCH 1/4] libgcc: Remove tbase member from struct unw_eh_callback_data Florian Weimer @ 2021-11-03 16:28 ` Florian Weimer 2021-11-18 14:33 ` Jakub Jelinek 2021-11-03 16:28 ` [PATCH 3/4] libgcc: Split FDE search code from PT_GNU_EH_FRAME lookup Florian Weimer 2021-11-03 16:28 ` [PATCH 4/4] libgcc: Use _dl_find_eh_frame in _Unwind_Find_FDE Florian Weimer 3 siblings, 1 reply; 13+ messages in thread From: Florian Weimer @ 2021-11-03 16:28 UTC (permalink / raw) To: gcc-patches; +Cc: libc-alpha, Jakub Jelinek, Jason Merrill Only i386 and nios2 need this member at present. libgcc/ChangeLog * unwind-dw2-fde-dip.c (NEED_DBASE_MEMBER): Define. (struct unw_eh_callback_data): Make dbase member conditional. (unw_eh_callback_data_dbase): New function. (base_from_cb_data): Simplify for the non-dbase case. (_Unwind_IteratePhdrCallback): Adjust. (_Unwind_Find_FDE): Likewise. --- libgcc/unwind-dw2-fde-dip.c | 46 +++++++++++++++++++++++++++++++------ 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/libgcc/unwind-dw2-fde-dip.c b/libgcc/unwind-dw2-fde-dip.c index 4a4d990f455..3f302826d2d 100644 --- a/libgcc/unwind-dw2-fde-dip.c +++ b/libgcc/unwind-dw2-fde-dip.c @@ -101,15 +101,35 @@ static const fde * _Unwind_Find_registered_FDE (void *pc, struct dwarf_eh_bases #define PT_GNU_EH_FRAME (PT_LOOS + 0x474e550) #endif +#ifdef CRT_GET_RFIB_DATA +#define NEED_DBASE_MEMBER 1 +#else +#define NEED_DBASE_MEMBER 0 +#endif + struct unw_eh_callback_data { _Unwind_Ptr pc; +#if NEED_DBASE_MEMBER void *dbase; +#endif void *func; const fde *ret; int check_cache; }; +/* Returns DATA->dbase if available, else NULL. */ +static inline _Unwind_Ptr +unw_eh_callback_data_dbase (const struct unw_eh_callback_data *data + __attribute__ ((unused))) +{ +#if NEED_DBASE_MEMBER + return (_Unwind_Ptr) data->dbase; +#else + return 0; +#endif +} + struct unw_eh_frame_hdr { unsigned char version; @@ -139,9 +159,11 @@ static struct frame_hdr_cache_element *frame_hdr_cache_head; /* Like base_of_encoded_value, but take the base from a struct unw_eh_callback_data instead of an _Unwind_Context. */ -static _Unwind_Ptr -base_from_cb_data (unsigned char encoding, struct unw_eh_callback_data *data) +static inline _Unwind_Ptr +base_from_cb_data (unsigned char encoding __attribute__ ((unused)), + _Unwind_Ptr dbase __attribute__ ((unused))) { +#if NEED_DBASE_MEMBER if (encoding == DW_EH_PE_omit) return 0; @@ -155,10 +177,13 @@ base_from_cb_data (unsigned char encoding, struct unw_eh_callback_data *data) case DW_EH_PE_textrel: return 0; case DW_EH_PE_datarel: - return (_Unwind_Ptr) data->dbase; + return dbase; default: gcc_unreachable (); } +#else /* !NEED_DBASE_MEMBER */ + return 0; +#endif } static int @@ -358,9 +383,10 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) # endif #endif + _Unwind_Ptr dbase = unw_eh_callback_data_dbase (data); p = read_encoded_value_with_base (hdr->eh_frame_ptr_enc, base_from_cb_data (hdr->eh_frame_ptr_enc, - data), + dbase), (const unsigned char *) (hdr + 1), &eh_frame); @@ -374,7 +400,7 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) p = read_encoded_value_with_base (hdr->fde_count_enc, base_from_cb_data (hdr->fde_count_enc, - data), + dbase), p, &fde_count); /* Shouldn't happen. */ if (fde_count == 0) @@ -431,7 +457,7 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) removed, we could cache this (and thus use search_object). */ ob.pc_begin = NULL; ob.tbase = NULL; - ob.dbase = data->dbase; + ob.dbase = (void *) dbase; ob.u.single = (fde *) eh_frame; ob.s.i = 0; ob.s.b.mixed_encoding = 1; /* Need to assume worst case. */ @@ -442,7 +468,7 @@ _Unwind_IteratePhdrCallback (struct dl_phdr_info *info, size_t size, void *ptr) unsigned int encoding = get_fde_encoding (data->ret); read_encoded_value_with_base (encoding, - base_from_cb_data (encoding, data), + base_from_cb_data (encoding, dbase), data->ret->pc_begin, &func); data->func = (void *) func; } @@ -460,7 +486,9 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) return ret; data.pc = (_Unwind_Ptr) pc; +#if NEED_DBASE_MEMBER data.dbase = NULL; +#endif data.func = NULL; data.ret = NULL; data.check_cache = 1; @@ -471,7 +499,11 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) 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; -- 2.31.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] libgcc: Remove dbase member from struct unw_eh_callback_data if NULL 2021-11-03 16:28 ` [PATCH 2/4] libgcc: Remove dbase member from struct unw_eh_callback_data if NULL Florian Weimer @ 2021-11-18 14:33 ` Jakub Jelinek 0 siblings, 0 replies; 13+ messages in thread From: Jakub Jelinek @ 2021-11-18 14:33 UTC (permalink / raw) To: Florian Weimer; +Cc: gcc-patches, libc-alpha, Jason Merrill On Wed, Nov 03, 2021 at 05:28:41PM +0100, Florian Weimer wrote: > Only i386 and nios2 need this member at present. Only i386, nios2, frv and bfin to be precise. > > libgcc/ChangeLog > > * unwind-dw2-fde-dip.c (NEED_DBASE_MEMBER): Define. > (struct unw_eh_callback_data): Make dbase member conditional. > (unw_eh_callback_data_dbase): New function. > (base_from_cb_data): Simplify for the non-dbase case. > (_Unwind_IteratePhdrCallback): Adjust. > (_Unwind_Find_FDE): Likewise. LGTM. Jakub ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/4] libgcc: Split FDE search code from PT_GNU_EH_FRAME lookup 2021-11-03 16:28 [PATCH 0/4] Use _dl_find_eh_frame to locate DWARF EH data in the unwinder Florian Weimer 2021-11-03 16:28 ` [PATCH 1/4] libgcc: Remove tbase member from struct unw_eh_callback_data Florian Weimer 2021-11-03 16:28 ` [PATCH 2/4] libgcc: Remove dbase member from struct unw_eh_callback_data if NULL Florian Weimer @ 2021-11-03 16:28 ` Florian Weimer 2021-11-18 15:21 ` Jakub Jelinek 2021-11-03 16:28 ` [PATCH 4/4] libgcc: Use _dl_find_eh_frame in _Unwind_Find_FDE Florian Weimer 3 siblings, 1 reply; 13+ messages in thread From: Florian Weimer @ 2021-11-03 16:28 UTC (permalink / raw) To: gcc-patches; +Cc: libc-alpha, Jakub Jelinek, Jason Merrill 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. (struct find_fde_tail_result): New. (find_fde_tail): New function. Split from _Unwind_IteratePhdrCallback. (_Unwind_Find_FDE): Add call to find_fde_tail. --- libgcc/unwind-dw2-fde-dip.c | 91 +++++++++++++++++++++---------------- 1 file changed, 52 insertions(+), 39 deletions(-) diff --git a/libgcc/unwind-dw2-fde-dip.c b/libgcc/unwind-dw2-fde-dip.c index 3f302826d2d..272c0ec46c0 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,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) +{ + const unsigned char *p = (const unsigned char *) (hdr + 1); + _Unwind_Ptr eh_frame; + struct object ob; + + if (hdr->version != 1) + return (struct find_fde_tail_result) { 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 +419,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 (struct find_fde_tail_result) { NULL, }; if ((((_Unwind_Ptr) p) & 3) == 0) { struct fde_table { @@ -419,9 +434,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 (struct find_fde_tail_result) { NULL, }; + else if (pc < table[mid].initial_loc + data_base) { lo = 0; hi = mid; @@ -429,9 +444,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 +460,11 @@ _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; + void *func = (void *) (table[mid].initial_loc + data_base); + if (pc < table[mid].initial_loc + data_base + range) + return (struct find_fde_tail_result) { f, func }; + else + return (struct find_fde_tail_result) { NULL, func }; } } @@ -461,18 +477,18 @@ _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); + return (struct find_fde_tail_result) { entry, (void *) func }; } - return 1; + return (struct find_fde_tail_result) { NULL, }; } const fde * @@ -489,24 +505,21 @@ _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) + _Unwind_Ptr dbase = unw_eh_callback_data_dbase (&data); + struct find_fde_tail_result result = find_fde_tail ((_Unwind_Ptr) pc, + data.hdr, dbase); + if (result.entry != NULL) { bases->tbase = NULL; -#if NEED_DBASE_MEMBER - bases->dbase = data.dbase; -#else - bases->dbase = NULL; -#endif - bases->func = data.func; + bases->dbase = (void *) dbase; + bases->func = result.func; } - return data.ret; + return result.entry; } #else -- 2.31.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] libgcc: Split FDE search code from PT_GNU_EH_FRAME lookup 2021-11-03 16:28 ` [PATCH 3/4] libgcc: Split FDE search code from PT_GNU_EH_FRAME lookup Florian Weimer @ 2021-11-18 15:21 ` Jakub Jelinek 2021-11-23 17:56 ` Florian Weimer 0 siblings, 1 reply; 13+ messages in thread From: Jakub Jelinek @ 2021-11-18 15:21 UTC (permalink / raw) To: Florian Weimer; +Cc: gcc-patches, libc-alpha, Jason Merrill 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? > +{ > + const unsigned char *p = (const unsigned char *) (hdr + 1); > + _Unwind_Ptr eh_frame; > + struct object ob; > + > + if (hdr->version != 1) > + return (struct find_fde_tail_result) { NULL, }; If really returning a struct, I would have preferred { NULL, NULL } in these cases, but see above. > + if (pc < table[mid].initial_loc + data_base + range) > + return (struct find_fde_tail_result) { f, func }; > + else > + return (struct find_fde_tail_result) { NULL, func }; This case was returning NULL fde and non-NULL func? What it would be good for, the caller just throws that away. Otherwise LGTM. Jakub ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] libgcc: Split FDE search code from PT_GNU_EH_FRAME lookup 2021-11-18 15:21 ` Jakub Jelinek @ 2021-11-23 17:56 ` Florian Weimer 2021-11-25 17:16 ` Jakub Jelinek 0 siblings, 1 reply; 13+ messages in thread From: Florian Weimer @ 2021-11-23 17:56 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches, libc-alpha, Jason Merrill * 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/4] libgcc: Split FDE search code from PT_GNU_EH_FRAME lookup 2021-11-23 17:56 ` Florian Weimer @ 2021-11-25 17:16 ` Jakub Jelinek 0 siblings, 0 replies; 13+ messages in thread From: Jakub Jelinek @ 2021-11-25 17:16 UTC (permalink / raw) To: Florian Weimer; +Cc: gcc-patches, libc-alpha, Jason Merrill On Tue, Nov 23, 2021 at 06:56:14PM +0100, Florian Weimer wrote: > 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. LGTM, thanks. Jakub ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 4/4] libgcc: Use _dl_find_eh_frame in _Unwind_Find_FDE 2021-11-03 16:28 [PATCH 0/4] Use _dl_find_eh_frame to locate DWARF EH data in the unwinder Florian Weimer ` (2 preceding siblings ...) 2021-11-03 16:28 ` [PATCH 3/4] libgcc: Split FDE search code from PT_GNU_EH_FRAME lookup Florian Weimer @ 2021-11-03 16:28 ` Florian Weimer 2021-11-18 15:45 ` Jakub Jelinek 3 siblings, 1 reply; 13+ messages in thread From: Florian Weimer @ 2021-11-03 16:28 UTC (permalink / raw) To: gcc-patches; +Cc: libc-alpha, Jakub Jelinek, Jason Merrill libgcc/ChangeLog * unwind-dw2-fde-dip.c (USE_DL_FIND_EH_FRAME) (DL_FIND_EH_FRAME_CONDITION): New macros. [__GLIBC__ && !DL_FIND_EH_FRAME_DBASE] (_dl_find_eh_frame): Declare weak function. (_Unwind_Find_FDE): Call _dl_find_eh_frame if available. --- libgcc/unwind-dw2-fde-dip.c | 50 +++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/libgcc/unwind-dw2-fde-dip.c b/libgcc/unwind-dw2-fde-dip.c index 272c0ec46c0..b5b4a23dc56 100644 --- a/libgcc/unwind-dw2-fde-dip.c +++ b/libgcc/unwind-dw2-fde-dip.c @@ -129,6 +129,30 @@ unw_eh_callback_data_dbase (const struct unw_eh_callback_data *data #endif } +#ifdef DL_FIND_EH_FRAME_DBASE +#if DL_FIND_EH_FRAME_DBASE != NEED_DBASE_MEMBER +#error "DL_FIND_EH_FRAME_DBASE != NEED_DBASE_MEMBER" +#endif +#define USE_DL_FIND_EH_FRAME 1 +#define DL_FIND_EH_FRAME_CONDITION 1 +#endif + +/* Fallback declaration for old glibc headers. DL_FIND_EH_FRAME_DBASE is used + as a proxy to determine if <dlfcn.h> declares _dl_find_eh_frame. */ +#if defined __GLIBC__ && !defined DL_FIND_EH_FRAME_DBASE +#if NEED_DBASE_MEMBER +void *_dl_find_eh_frame (void *__pc, void **__dbase) __attribute__ ((weak)); +#else +void *_dl_find_eh_frame (void *__pc) __attribute__ ((weak)); +#endif +#define USE_DL_FIND_EH_FRAME 1 +#define DL_FIND_EH_FRAME_CONDITION (_dl_find_eh_frame != NULL) +#endif + +#ifndef USE_DL_FIND_EH_FRAME +#define USE_DL_FIND_EH_FRAME 0 +#endif + struct unw_eh_frame_hdr { unsigned char version; @@ -501,6 +525,32 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) if (ret != NULL) return ret; +#if USE_DL_FIND_EH_FRAME + if (DL_FIND_EH_FRAME_CONDITION) + { + void *dbase; + void *eh_frame; +#if NEED_DBASE_MEMBER + eh_frame = _dl_find_eh_frame (pc, &dbase); +#else + dbase = NULL; + eh_frame = _dl_find_eh_frame (pc); +#endif + if (eh_frame == NULL) + return NULL; + + struct find_fde_tail_result result + = find_fde_tail ((_Unwind_Ptr) pc, eh_frame, (_Unwind_Ptr) dbase); + if (result.entry != NULL) + { + bases->tbase = NULL; + bases->dbase = (void *) dbase; + bases->func = result.func; + } + return result.entry; + } +#endif + data.pc = (_Unwind_Ptr) pc; #if NEED_DBASE_MEMBER data.dbase = NULL; -- 2.31.1 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] libgcc: Use _dl_find_eh_frame in _Unwind_Find_FDE 2021-11-03 16:28 ` [PATCH 4/4] libgcc: Use _dl_find_eh_frame in _Unwind_Find_FDE Florian Weimer @ 2021-11-18 15:45 ` Jakub Jelinek 2021-11-25 20:42 ` Florian Weimer 0 siblings, 1 reply; 13+ messages in thread From: Jakub Jelinek @ 2021-11-18 15:45 UTC (permalink / raw) To: Florian Weimer; +Cc: gcc-patches, libc-alpha, Jason Merrill On Wed, Nov 03, 2021 at 05:28:55PM +0100, Florian Weimer wrote: > --- a/libgcc/unwind-dw2-fde-dip.c > +++ b/libgcc/unwind-dw2-fde-dip.c > @@ -129,6 +129,30 @@ unw_eh_callback_data_dbase (const struct unw_eh_callback_data *data > #endif > } > > +#ifdef DL_FIND_EH_FRAME_DBASE > +#if DL_FIND_EH_FRAME_DBASE != NEED_DBASE_MEMBER > +#error "DL_FIND_EH_FRAME_DBASE != NEED_DBASE_MEMBER" Instead of #error just don't define USE_DL_FIND_EH_FRAME? I.e. #ifdef DL_FIND_EH_FRAME_DBASE #if DL_FIND_EH_FRAME_DBASE == NEED_DBASE_MEMBER > +#define USE_DL_FIND_EH_FRAME 1 > +#define DL_FIND_EH_FRAME_CONDITION 1 > +#endif ? Is it a good idea to have different arguments of the function for i386/nios2/frv/bfind vs. the rest, wouldn't just always passing void **__dbase argument be cleaner? Internally it is fine to differentiate based on NEED_DBASE_MEMBER, but doing that on a public interface? > +/* Fallback declaration for old glibc headers. DL_FIND_EH_FRAME_DBASE is used > + as a proxy to determine if <dlfcn.h> declares _dl_find_eh_frame. */ > +#if defined __GLIBC__ && !defined DL_FIND_EH_FRAME_DBASE > +#if NEED_DBASE_MEMBER > +void *_dl_find_eh_frame (void *__pc, void **__dbase) __attribute__ ((weak)); > +#else > +void *_dl_find_eh_frame (void *__pc) __attribute__ ((weak)); > +#endif > +#define USE_DL_FIND_EH_FRAME 1 > +#define DL_FIND_EH_FRAME_CONDITION (_dl_find_eh_frame != NULL) > +#endif I'd prefer not to do this. If we find glibc with the support in the headers, let's use it, otherwise let's keep using what we were doing before. > +#if NEED_DBASE_MEMBER > + eh_frame = _dl_find_eh_frame (pc, &dbase); > +#else > + dbase = NULL; > + eh_frame = _dl_find_eh_frame (pc); > +#endif > + if (eh_frame == NULL) > + return NULL; > + > + struct find_fde_tail_result result > + = find_fde_tail ((_Unwind_Ptr) pc, eh_frame, (_Unwind_Ptr) dbase); > + if (result.entry != NULL) > + { > + bases->tbase = NULL; > + bases->dbase = (void *) dbase; > + bases->func = result.func; > + } > + return result.entry; > + } > +#endif > + > data.pc = (_Unwind_Ptr) pc; > #if NEED_DBASE_MEMBER > data.dbase = NULL; > -- > 2.31.1 Otherwise LGTM. Jakub ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] libgcc: Use _dl_find_eh_frame in _Unwind_Find_FDE 2021-11-18 15:45 ` Jakub Jelinek @ 2021-11-25 20:42 ` Florian Weimer 2021-11-26 15:49 ` Jakub Jelinek 0 siblings, 1 reply; 13+ messages in thread From: Florian Weimer @ 2021-11-25 20:42 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches, Adhemerval Zanella, libc-alpha * Jakub Jelinek: >> +/* Fallback declaration for old glibc headers. DL_FIND_EH_FRAME_DBASE is used >> + as a proxy to determine if <dlfcn.h> declares _dl_find_eh_frame. */ >> +#if defined __GLIBC__ && !defined DL_FIND_EH_FRAME_DBASE >> +#if NEED_DBASE_MEMBER >> +void *_dl_find_eh_frame (void *__pc, void **__dbase) __attribute__ ((weak)); >> +#else >> +void *_dl_find_eh_frame (void *__pc) __attribute__ ((weak)); >> +#endif >> +#define USE_DL_FIND_EH_FRAME 1 >> +#define DL_FIND_EH_FRAME_CONDITION (_dl_find_eh_frame != NULL) >> +#endif > > I'd prefer not to do this. If we find glibc with the support in the > headers, let's use it, otherwise let's keep using what we were doing before. I've included a simplified version below, based on the _dl_find_object patch for glibc. This is a bit difficult to test, but I ran a full toolchain bootstrap with GCC + glibc on all glibc-supported architectures (except Hurd and one m68k variant; they do not presnetly build, see Joseph's testers). I also tested this by copying the respective GCC-built libgcc_s into a glibc build tree for run-time testing on i686-linux-gnu and x86_64-linux-gnu. There weren't any issues. There are a buch of unwinder tests in glibc, giving at least some coverage. Thanks, Florian Subject: libgcc: Use _dl_find_object in _Unwind_Find_FDE libgcc/ChangeLog: * unwind-dw2-fde-dip.c (_Unwind_Find_FDE): Call _dl_find_object if available. --- libgcc/unwind-dw2-fde-dip.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/libgcc/unwind-dw2-fde-dip.c b/libgcc/unwind-dw2-fde-dip.c index fbb0fbdebb9..b837d8e4904 100644 --- a/libgcc/unwind-dw2-fde-dip.c +++ b/libgcc/unwind-dw2-fde-dip.c @@ -504,6 +504,24 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) if (ret != NULL) return ret; + /* Use DLFO_STRUCT_HAS_EH_DBASE as a proxy for the existence of a glibc-style + _dl_find_object function. */ +#ifdef DLFO_STRUCT_HAS_EH_DBASE + { + struct dl_find_object dlfo; + if (_dl_find_object (pc, &dlfo) == 0) + return find_fde_tail ((_Unwind_Ptr) pc, dlfo.dlfo_eh_frame, +# if DLFO_STRUCT_HAS_EH_DBASE + (_Unwind_Ptr) dlfo.dlfo_eh_dbase, +# else + NULL, +# endif + bases); + else + return NULL; + } +#endif /* DLFO_STRUCT_HAS_EH_DBASE */ + data.pc = (_Unwind_Ptr) pc; #if NEED_DBASE_MEMBER data.dbase = NULL; ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] libgcc: Use _dl_find_eh_frame in _Unwind_Find_FDE 2021-11-25 20:42 ` Florian Weimer @ 2021-11-26 15:49 ` Jakub Jelinek 0 siblings, 0 replies; 13+ messages in thread From: Jakub Jelinek @ 2021-11-26 15:49 UTC (permalink / raw) To: Florian Weimer; +Cc: gcc-patches, Adhemerval Zanella, libc-alpha On Thu, Nov 25, 2021 at 09:42:53PM +0100, Florian Weimer wrote: > I've included a simplified version below, based on the _dl_find_object > patch for glibc. > > This is a bit difficult to test, but I ran a full toolchain bootstrap > with GCC + glibc on all glibc-supported architectures (except Hurd and > one m68k variant; they do not presnetly build, see Joseph's testers). > > I also tested this by copying the respective GCC-built libgcc_s into a > glibc build tree for run-time testing on i686-linux-gnu and > x86_64-linux-gnu. There weren't any issues. There are a buch of > unwinder tests in glibc, giving at least some coverage. See the comment I've just sent on this patch. If the DLFO_STRUCT_HAS_EH_DBASE and DLFO_STRUCT_HAS_EH_COUNT macros are gone, we'd need to use __GLIBC_PREREQ or configure test or combination of the two. Otherwise LGTM. > Subject: libgcc: Use _dl_find_object in _Unwind_Find_FDE > > libgcc/ChangeLog: > > * unwind-dw2-fde-dip.c (_Unwind_Find_FDE): Call _dl_find_object > if available. Jakub ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-11-26 15:49 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-03 16:28 [PATCH 0/4] Use _dl_find_eh_frame to locate DWARF EH data in the unwinder Florian Weimer 2021-11-03 16:28 ` [PATCH 1/4] libgcc: Remove tbase member from struct unw_eh_callback_data Florian Weimer 2021-11-18 13:47 ` Jakub Jelinek 2021-11-03 16:28 ` [PATCH 2/4] libgcc: Remove dbase member from struct unw_eh_callback_data if NULL Florian Weimer 2021-11-18 14:33 ` Jakub Jelinek 2021-11-03 16:28 ` [PATCH 3/4] libgcc: Split FDE search code from PT_GNU_EH_FRAME lookup Florian Weimer 2021-11-18 15:21 ` Jakub Jelinek 2021-11-23 17:56 ` Florian Weimer 2021-11-25 17:16 ` Jakub Jelinek 2021-11-03 16:28 ` [PATCH 4/4] libgcc: Use _dl_find_eh_frame in _Unwind_Find_FDE Florian Weimer 2021-11-18 15:45 ` Jakub Jelinek 2021-11-25 20:42 ` Florian Weimer 2021-11-26 15:49 ` Jakub Jelinek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).