diff mbox series

[RFC,1/5] elf: Add SFrame support to _dl_find_object function

Message ID 20250318130333.18829-2-claudiu.zissulescu-ianculescu@oracle.com (mailing list archive)
State New
Headers
Series glibc: Add SFrame support for stack backtracking |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Claudiu Zissulescu-Ianculescu March 18, 2025, 1:03 p.m. UTC 
  From: Claudiu Zissulescu <claudiu.zissulescu-ianculescu@oracle.com>

The SFrame provides information to be able to do stack trace is now
well defined and implemented in Binutils 2.41.  The format simply
contains enough information to be able to do stack trace given a
program counter (PC) value, the stack pointer, and the frame pointer.
The SFrame information is stored in a .sframe ELF section, which is
loaded into its own PT_GNU_SFRAME segment. We consider for this support
SFrame version 2.

This patch adds the bits to _dl_find_object to recognize and store in
struct dl_find_object the necessary info about SFrame section.

Signed-off-by: Claudiu Zissulescu <claudiu.zissulescu-ianculescu@oracle.com>
---
 bits/dl_find_object.h |  6 +++++
 dlfcn/dlfcn.h         |  3 +++
 elf/dl-find_object.h  | 51 +++++++++++++++++++++++++++++++++++++------
 manual/dynlink.texi   | 24 ++++++++++++++++++++
 4 files changed, 77 insertions(+), 7 deletions(-)

Comments

Joseph Myers March 18, 2025, 4:25 p.m. UTC | #1 
On Tue, 18 Mar 2025, claudiu.zissulescu-ianculescu@oracle.com wrote:

> diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
> index 5747962611..dd4113a757 100644
> --- a/dlfcn/dlfcn.h
> +++ b/dlfcn/dlfcn.h
> @@ -216,6 +216,9 @@ struct dl_find_object
>  # if DLFO_STRUCT_HAS_EH_COUNT
>    int dlfo_eh_count;		/* Number of exception handling entries.  */
>    unsigned int __dlfo_eh_count_pad;
> +# endif
> +# if DLFO_STRUCT_HAS_SFRAME
> +  void *dlfo_sframe;		/* SFrame stack trace data of the object.  */
>  # endif
>    __extension__ unsigned long long int __dflo_reserved[7];
>  };

Don't you need to update __dflo_reserved (taking account of different 
pointer sizes depending on the architecture) to avoid ABI issues?  In 
general, please provide details of the ABI impact of this patch series and 
how you avoid any compatibility issues with existing binaries using this 
structure.
Florian Weimer March 20, 2025, 8:03 a.m. UTC | #2 
* claudiu zissulescu-ianculescu:

> diff --git a/bits/dl_find_object.h b/bits/dl_find_object.h
> index b9f796dc2b..162ef00615 100644
> --- a/bits/dl_find_object.h
> +++ b/bits/dl_find_object.h
> @@ -30,3 +30,9 @@
>  
>  /* The ELF segment which contains the exception handling data.  */
>  #define DLFO_EH_SEGMENT_TYPE PT_GNU_EH_FRAME
> +
> +/* The ELF segment which contains the SFrame data.  */
> +#define DLFO_SFRAME_SEGMENT_TYPE PT_GNU_SFRAME
> +
> +/* This implementation does not have SFrame data.  */
> +#define DLFO_STRUCT_HAS_SFRAME 0

Is there any technical reason why an architecture would not add SFrame
data?

I think we shoul define this unconditionally …

> diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
> index 5747962611..dd4113a757 100644
> --- a/dlfcn/dlfcn.h
> +++ b/dlfcn/dlfcn.h
> @@ -216,6 +216,9 @@ struct dl_find_object
>  # if DLFO_STRUCT_HAS_EH_COUNT
>    int dlfo_eh_count;		/* Number of exception handling entries.  */
>    unsigned int __dlfo_eh_count_pad;
> +# endif
> +# if DLFO_STRUCT_HAS_SFRAME
> +  void *dlfo_sframe;		/* SFrame stack trace data of the object.  */
>  # endif
>    __extension__ unsigned long long int __dflo_reserved[7];
>  };

… and always define this field.

> diff --git a/elf/dl-find_object.h b/elf/dl-find_object.h
> index 0915065be0..e43c3212c2 100644
> --- a/elf/dl-find_object.h
> +++ b/elf/dl-find_object.h
> @@ -43,6 +43,9 @@ struct dl_find_object_internal
>  #if DLFO_STRUCT_HAS_EH_COUNT
>    int eh_count;
>  #endif
> +#if DLFO_STRUCT_HAS_SFRAME
> +  void *sframe;
> +#endif
>  };

And here.

You need to add a flag to dlfo_flags that indicates that the dlfo_sframe
field is valid.

Thanks,
Florian
Claudiu Zissulescu-Ianculescu March 25, 2025, 10:53 a.m. UTC | #3 
Hi Florian,


On 3/20/25 10:03 AM, Florian Weimer wrote:
> * claudiu zissulescu-ianculescu:
> 
>> diff --git a/bits/dl_find_object.h b/bits/dl_find_object.h
>> index b9f796dc2b..162ef00615 100644
>> --- a/bits/dl_find_object.h
>> +++ b/bits/dl_find_object.h
>> @@ -30,3 +30,9 @@
>>  
>>  /* The ELF segment which contains the exception handling data.  */
>>  #define DLFO_EH_SEGMENT_TYPE PT_GNU_EH_FRAME
>> +
>> +/* The ELF segment which contains the SFrame data.  */
>> +#define DLFO_SFRAME_SEGMENT_TYPE PT_GNU_SFRAME
>> +
>> +/* This implementation does not have SFrame data.  */
>> +#define DLFO_STRUCT_HAS_SFRAME 0
> 
> Is there any technical reason why an architecture would not add SFrame
> data?

Not really, unless the maintainers of an architecture don't want to deal
with SFrame.

> 
> I think we shoul define this unconditionally …
> 
>> diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
>> index 5747962611..dd4113a757 100644
>> --- a/dlfcn/dlfcn.h
>> +++ b/dlfcn/dlfcn.h
>> @@ -216,6 +216,9 @@ struct dl_find_object
>>  # if DLFO_STRUCT_HAS_EH_COUNT
>>    int dlfo_eh_count;		/* Number of exception handling entries.  */
>>    unsigned int __dlfo_eh_count_pad;
>> +# endif
>> +# if DLFO_STRUCT_HAS_SFRAME
>> +  void *dlfo_sframe;		/* SFrame stack trace data of the object.  */
>>  # endif
>>    __extension__ unsigned long long int __dflo_reserved[7];
>>  };
> 
> … and always define this field.
> 
>> diff --git a/elf/dl-find_object.h b/elf/dl-find_object.h
>> index 0915065be0..e43c3212c2 100644
>> --- a/elf/dl-find_object.h
>> +++ b/elf/dl-find_object.h
>> @@ -43,6 +43,9 @@ struct dl_find_object_internal
>>  #if DLFO_STRUCT_HAS_EH_COUNT
>>    int eh_count;
>>  #endif
>> +#if DLFO_STRUCT_HAS_SFRAME
>> +  void *sframe;
>> +#endif
>>  };
> 
> And here.
> 
> You need to add a flag to dlfo_flags that indicates that the dlfo_sframe
> field is valid.
> 

Thank you for the input. I'll address all of them in my second round,
Claudiu
diff mbox series

Patch

diff --git a/bits/dl_find_object.h b/bits/dl_find_object.h
index b9f796dc2b..162ef00615 100644
--- a/bits/dl_find_object.h
+++ b/bits/dl_find_object.h
@@ -30,3 +30,9 @@ 
 
 /* The ELF segment which contains the exception handling data.  */
 #define DLFO_EH_SEGMENT_TYPE PT_GNU_EH_FRAME
+
+/* The ELF segment which contains the SFrame data.  */
+#define DLFO_SFRAME_SEGMENT_TYPE PT_GNU_SFRAME
+
+/* This implementation does not have SFrame data.  */
+#define DLFO_STRUCT_HAS_SFRAME 0
diff --git a/dlfcn/dlfcn.h b/dlfcn/dlfcn.h
index 5747962611..dd4113a757 100644
--- a/dlfcn/dlfcn.h
+++ b/dlfcn/dlfcn.h
@@ -216,6 +216,9 @@  struct dl_find_object
 # if DLFO_STRUCT_HAS_EH_COUNT
   int dlfo_eh_count;		/* Number of exception handling entries.  */
   unsigned int __dlfo_eh_count_pad;
+# endif
+# if DLFO_STRUCT_HAS_SFRAME
+  void *dlfo_sframe;		/* SFrame stack trace data of the object.  */
 # endif
   __extension__ unsigned long long int __dflo_reserved[7];
 };
diff --git a/elf/dl-find_object.h b/elf/dl-find_object.h
index 0915065be0..e43c3212c2 100644
--- a/elf/dl-find_object.h
+++ b/elf/dl-find_object.h
@@ -43,6 +43,9 @@  struct dl_find_object_internal
 #if DLFO_STRUCT_HAS_EH_COUNT
   int eh_count;
 #endif
+#if DLFO_STRUCT_HAS_SFRAME
+  void *sframe;
+#endif
 };
 
 /* Create a copy of *SOURCE in *COPY using relaxed MO loads and
@@ -67,6 +70,10 @@  _dl_find_object_internal_copy (const struct dl_find_object_internal *source,
   atomic_store_relaxed (&copy->eh_count,
                         atomic_load_relaxed (&source->eh_count));
 #endif
+#if DLFO_STRUCT_HAS_SFRAME
+  atomic_store_relaxed (&copy->sframe,
+                        atomic_load_relaxed (&source->sframe));
+#endif
 }
 
 static inline void
@@ -84,6 +91,9 @@  _dl_find_object_to_external (struct dl_find_object_internal *internal,
 # if DLFO_STRUCT_HAS_EH_COUNT
   external->dlfo_eh_count = internal->eh_count;
 # endif
+# if DLFO_STRUCT_HAS_SFRAME
+  external->dlfo_sframe = internal->sframe;
+# endif
 }
 
 /* Extract the object location data from a link map and writes it to
@@ -92,6 +102,9 @@  static void __attribute__ ((unused))
 _dl_find_object_from_map (struct link_map *l,
                           struct dl_find_object_internal *result)
 {
+#if DLFO_STRUCT_HAS_SFRAME
+  uint64_t read_seg = 0;
+#endif
   atomic_store_relaxed (&result->map_start, (uintptr_t) l->l_map_start);
   atomic_store_relaxed (&result->map_end, (uintptr_t) l->l_map_end);
   atomic_store_relaxed (&result->map, l);
@@ -100,23 +113,47 @@  _dl_find_object_from_map (struct link_map *l,
   atomic_store_relaxed (&result->eh_dbase, (void *) l->l_info[DT_PLTGOT]);
 #endif
 
+  /* Initialize object's exception handling segment and SFrame segment
+     data.  */
+#if DLFO_STRUCT_HAS_SFRAME
+  atomic_store_relaxed (&result->sframe, NULL);
+#endif
+  atomic_store_relaxed (&result->eh_frame, NULL);
+#if DLFO_STRUCT_HAS_EH_COUNT
+  atomic_store_relaxed (&result->eh_count, 0);
+#endif
+
   for (const ElfW(Phdr) *ph = l->l_phdr, *ph_end = l->l_phdr + l->l_phnum;
        ph < ph_end; ++ph)
-    if (ph->p_type == DLFO_EH_SEGMENT_TYPE)
+    switch (ph->p_type)
       {
+      case DLFO_EH_SEGMENT_TYPE:
         atomic_store_relaxed (&result->eh_frame,
                               (void *) (ph->p_vaddr + l->l_addr));
 #if DLFO_STRUCT_HAS_EH_COUNT
         atomic_store_relaxed (&result->eh_count, ph->p_memsz / 8);
 #endif
-        return;
-      }
 
-  /* Object has no exception handling segment.  */
-  atomic_store_relaxed (&result->eh_frame, NULL);
-#if DLFO_STRUCT_HAS_EH_COUNT
-  atomic_store_relaxed (&result->eh_count, 0);
+#if DLFO_STRUCT_HAS_SFRAME
+        read_seg |= 1;
+        break;
+
+      case DLFO_SFRAME_SEGMENT_TYPE:
+        atomic_store_relaxed (&result->sframe,
+                              (void *) (ph->p_vaddr + l->l_addr));
+        read_seg |= 2;
+        /* Fall through.  */
+      default:
+        if (read_seg == 3)
+          return;
+        break;
+#else
+	return;
+
+      default:
+	break;
 #endif
+      }
 }
 
 /* Called by the dynamic linker to set up the data structures for the
diff --git a/manual/dynlink.texi b/manual/dynlink.texi
index 1500a53de6..86922d9d19 100644
--- a/manual/dynlink.texi
+++ b/manual/dynlink.texi
@@ -459,6 +459,10 @@  This member contains a pointer to the link map of the object.
 This member contains a pointer to the exception handling data of the
 object.  See @code{DLFO_EH_SEGMENT_TYPE} below.
 
+@item void *dlfo_sframe
+This member contains a pointer to the SFrame stack trace data of the
+object.  See @code{DLFO_SFRAME_SEGMENT_TYPE} below.
+
 @end table
 
 This structure is a GNU extension.
@@ -496,6 +500,26 @@  macro expands to the program header type for the unwinding data.
 This macro is a GNU extension.
 @end deftypevr
 
+@deftypevr Macro int DLFO_SFRAME_SEGMENT_TYPE
+@standards{GNU, dlfcn.h}
+On targets using SFrame stack tracer, this macro expands to
+@code{PT_GNU_SFRAME}.  This indicates that @code{dlfo_sframe} in
+@code{struct dl_find_object} points to the @code{PT_GNU_SFRAME}
+segment of the object.  The @code{dlfo_sframe} member is valid only and
+only if @code{DLFO_STRUCT_HAS_SFRAME} is defined as @code{1}.
+
+This macro is a GNU extension.
+@end deftypevr
+
+@deftypevr Macro int DLFO_STRUCT_HAS_SFRAME
+@standards{GNU, dlfcn.h}
+On most targets, this macro is defined as @code{0}.  If it is defined to
+@code{1}, @code{struct dl_find_object} contains an additional member
+@code{dlfo_sframe}.
+
+This macro is a GNU extension.
+@end deftypevr
+
 @deftypefun {int} _dl_find_object (void *@var{address}, struct dl_find_object *@var{result})
 @standards{GNU, dlfcn.h}
 @safety{@mtsafe{}@assafe{}@acsafe{}}