[3/4] libgcc: Split FDE search code from PT_GNU_EH_FRAME lookup

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

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