[3/4] libgcc: Split FDE search code from PT_GNU_EH_FRAME lookup
Commit Message
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(-)
Comments
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
* 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
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
@@ -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