[PATCH 0/4] Use _dl_find_eh_frame to locate DWARF EH data in the unwinder

[PATCH 0/4] Use _dl_find_eh_frame to locate DWARF EH data in the unwinder

[Florian Weimer via Libc-alpha]

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

[PATCH 1/4] libgcc: Remove tbase member from struct unw_eh_callback_data

[Florian Weimer via Libc-alpha]

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

[PATCH 2/4] libgcc: Remove dbase member from struct unw_eh_callback_data if NULL

[Florian Weimer via Libc-alpha]

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

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

[Florian Weimer via Libc-alpha]

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

[PATCH 4/4] libgcc: Use _dl_find_eh_frame in _Unwind_Find_FDE

[Florian Weimer via Libc-alpha]

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

Re: [PATCH 1/4] libgcc: Remove tbase member from struct unw_eh_callback_data

[Jakub Jelinek via Libc-alpha]

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

Re: [PATCH 2/4] libgcc: Remove dbase member from struct unw_eh_callback_data if NULL

[Jakub Jelinek via Libc-alpha]

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

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

[Jakub Jelinek via Libc-alpha]

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

Re: [PATCH 4/4] libgcc: Use _dl_find_eh_frame in _Unwind_Find_FDE

[Jakub Jelinek via Libc-alpha]

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

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

[Florian Weimer via Libc-alpha]

* 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

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

[Jakub Jelinek via Libc-alpha]

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

Re: [PATCH 4/4] libgcc: Use _dl_find_eh_frame in _Unwind_Find_FDE

[Florian Weimer via 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;

Re: [PATCH 4/4] libgcc: Use _dl_find_eh_frame in _Unwind_Find_FDE

[Jakub Jelinek via 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