[COMMITTED] libsframe: use uint8_t data type for FRE info related stubs

[COMMITTED] libsframe: use uint8_t data type for FRE info related stubs

[Indu Bhagat]

libsframe/
	* sframe.c: Use uint8_t for FRE offset count and FRE offset
	size.  Use uint8_t for FRE info word as well.
---
 libsframe/sframe.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/libsframe/sframe.c b/libsframe/sframe.c
index d65662484cb..a79d4de96da 100644
--- a/libsframe/sframe.c
+++ b/libsframe/sframe.c
@@ -112,20 +112,20 @@ sframe_get_hdr_size (sframe_header *sfh)
 
 /* Access functions for frame row entry data.  */
 
-static unsigned int
-sframe_fre_get_offset_count (unsigned char fre_info)
+static uint8_t
+sframe_fre_get_offset_count (uint8_t fre_info)
 {
   return SFRAME_V1_FRE_OFFSET_COUNT (fre_info);
 }
 
-static unsigned int
-sframe_fre_get_offset_size (unsigned char fre_info)
+static uint8_t
+sframe_fre_get_offset_size (uint8_t fre_info)
 {
   return SFRAME_V1_FRE_OFFSET_SIZE (fre_info);
 }
 
 static bool
-sframe_get_fre_ra_mangled_p (unsigned char fre_info)
+sframe_get_fre_ra_mangled_p (uint8_t fre_info)
 {
   return SFRAME_V1_FRE_MANGLED_RA_P (fre_info);
 }
@@ -237,8 +237,7 @@ flip_fre_start_address (char *fp, unsigned int fre_type)
 }
 
 static void
-flip_fre_stack_offsets (char *fp, unsigned char offset_size,
-			unsigned char offset_cnt)
+flip_fre_stack_offsets (char *fp, uint8_t offset_size, uint8_t offset_cnt)
 {
   int j;
   void *offsets = (void *)fp;
@@ -287,8 +286,8 @@ sframe_fre_start_addr_size (unsigned int fre_type)
 static bool
 sframe_fre_sanity_check_p (sframe_frame_row_entry *frep)
 {
-  unsigned int offset_size, offset_cnt;
-  unsigned int fre_info;
+  uint8_t offset_size, offset_cnt;
+  uint8_t fre_info;
 
   if (frep == NULL)
     return false;
@@ -311,9 +310,9 @@ sframe_fre_sanity_check_p (sframe_frame_row_entry *frep)
 /* Get FRE_INFO's offset size in bytes.  */
 
 static size_t
-sframe_fre_offset_bytes_size (unsigned char fre_info)
+sframe_fre_offset_bytes_size (uint8_t fre_info)
 {
-  unsigned int offset_size, offset_cnt;
+  uint8_t offset_size, offset_cnt;
 
   offset_size = sframe_fre_get_offset_size (fre_info);
 
@@ -337,7 +336,7 @@ sframe_fre_entry_size (sframe_frame_row_entry *frep, unsigned int fre_type)
   if (frep == NULL)
     return 0;
 
-  unsigned char fre_info = frep->fre_info;
+  uint8_t fre_info = frep->fre_info;
   size_t addr_size = sframe_fre_start_addr_size (fre_type);
 
   return (addr_size + sizeof (frep->fre_info)
@@ -347,8 +346,8 @@ sframe_fre_entry_size (sframe_frame_row_entry *frep, unsigned int fre_type)
 static int
 flip_fre (char *fp, unsigned int fre_type, size_t *fre_size)
 {
-  unsigned char fre_info;
-  unsigned int offset_size, offset_cnt;
+  uint8_t fre_info;
+  uint8_t offset_size, offset_cnt;
   size_t addr_size, fre_info_size = 0;
   int err = 0;
 
@@ -361,13 +360,13 @@ flip_fre (char *fp, unsigned int fre_type, size_t *fre_size)
   addr_size = sframe_fre_start_addr_size (fre_type);
   fp += addr_size;
 
-  /* FRE info is unsigned char.  No need to flip.  */
-  fre_info = *(unsigned char*)fp;
+  /* FRE info is uint8_t.  No need to flip.  */
+  fre_info = *(uint8_t*)fp;
   offset_size = sframe_fre_get_offset_size (fre_info);
   offset_cnt = sframe_fre_get_offset_count (fre_info);
 
   /* Advance the buffer pointer to where the stack offsets are.  */
-  fre_info_size = sizeof (unsigned char);
+  fre_info_size = sizeof (uint8_t);
   fp += fre_info_size;
   flip_fre_stack_offsets (fp, offset_size, offset_cnt);
 
@@ -500,7 +499,7 @@ fde_func (const void *p1, const void *p2)
 static int32_t
 sframe_get_fre_offset (sframe_frame_row_entry *fre, int idx, int *errp)
 {
-  int offset_cnt, offset_size;
+  uint8_t offset_cnt, offset_size;
 
   if (fre == NULL || !sframe_fre_sanity_check_p (fre))
     return sframe_set_errno (errp, SFRAME_ERR_FRE_INVAL);
@@ -606,7 +605,7 @@ sframe_fre_get_base_reg_id (sframe_frame_row_entry *fre, int *errp)
   if (fre == NULL)
     return sframe_set_errno (errp, SFRAME_ERR_FRE_INVAL);
 
-  unsigned int fre_info = fre->fre_info;
+  uint8_t fre_info = fre->fre_info;
   return SFRAME_V1_FRE_CFA_BASE_REG_ID (fre_info);
 }
 
@@ -750,9 +749,9 @@ sframe_decode_fre (const char *fre_buf, sframe_frame_row_entry *fre,
   sframe_decode_fre_start_address (fre_buf, &fre->fre_start_addr, fre_type);
 
   addr_size = sframe_fre_start_addr_size (fre_type);
-  fre->fre_info = *(unsigned char *)(fre_buf + addr_size);
+  fre->fre_info = *(uint8_t *)(fre_buf + addr_size);
   /* Sanity check as the API works closely with the binary format.  */
-  sframe_assert (sizeof (fre->fre_info) == sizeof (unsigned char));
+  sframe_assert (sizeof (fre->fre_info) == sizeof (uint8_t));
 
   /* Cleanup the space for fre_offsets first, then copy over the valid
      bytes.  */
-- 
2.39.2

[COMMITTED] libsframe: use const char * consistently for immutable FRE buffers

[Indu Bhagat]

libsframe/
        * sframe.c (sframe_decode_fre): Use const char * datatype when
	handling buffer containing the FREs.
	(sframe_fre_get_end_ip_offset): Likewise.
	(sframe_find_fre): Likewise.
	(sframe_decoder_get_fre): Likewise.
---
 libsframe/sframe.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/libsframe/sframe.c b/libsframe/sframe.c
index a79d4de96da..72b221349ad 100644
--- a/libsframe/sframe.c
+++ b/libsframe/sframe.c
@@ -733,11 +733,10 @@ sframe_decode_fre_start_address (const char *fre_buf,
 
 static int
 sframe_decode_fre (const char *fre_buf, sframe_frame_row_entry *fre,
-		   unsigned int fre_type,
-		   size_t *esz)
+		   unsigned int fre_type, size_t *esz)
 {
   int err = 0;
-  void *stack_offsets = NULL;
+  const char *stack_offsets = NULL;
   size_t stack_offsets_sz;
   size_t addr_size;
   size_t fre_size;
@@ -758,7 +757,7 @@ sframe_decode_fre (const char *fre_buf, sframe_frame_row_entry *fre,
   memset (fre->fre_offsets, 0, MAX_OFFSET_BYTES);
   /* Get offsets size.  */
   stack_offsets_sz = sframe_fre_offset_bytes_size (fre->fre_info);
-  stack_offsets = (unsigned char *)fre_buf + addr_size + sizeof (fre->fre_info);
+  stack_offsets = fre_buf + addr_size + sizeof (fre->fre_info);
   memcpy (fre->fre_offsets, stack_offsets, stack_offsets_sz);
 
   /* The FRE has been decoded.  Use it to perform one last sanity check.  */
@@ -991,7 +990,7 @@ sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
   sframe_func_desc_entry *fdep;
   uint32_t start_address, i;
   sframe_frame_row_entry cur_fre, next_fre;
-  unsigned char *sp;
+  const char *fres;
   unsigned int fre_type, fde_type;
   size_t esz;
   int err = 0;
@@ -1022,10 +1021,10 @@ sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
   if (fde_type == SFRAME_FDE_TYPE_PCMASK)
     bitmask = 0xff;
 
-  sp = (unsigned char *) ctx->sfd_fres + fdep->sfde_func_start_fre_off;
+  fres = ctx->sfd_fres + fdep->sfde_func_start_fre_off;
   for (i = 0; i < fdep->sfde_func_num_fres; i++)
    {
-     err = sframe_decode_fre ((const char *)sp, &next_fre, fre_type, &esz);
+     err = sframe_decode_fre (fres, &next_fre, fre_type, &esz);
      start_address = next_fre.fre_start_addr;
 
      if (((fdep->sfde_func_start_address
@@ -1037,8 +1036,7 @@ sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
 	 if (i < fdep->sfde_func_num_fres - 1)
 	   {
 	     sp += esz;
-	     err = sframe_decode_fre ((const char*)sp, &next_fre,
-					 fre_type, &esz);
+	     err = sframe_decode_fre (fres, &next_fre, fre_type, &esz);
 
 	     /* Sanity check the next FRE.  */
 	     if (!sframe_fre_sanity_check_p (&next_fre))
@@ -1141,7 +1139,7 @@ sframe_decoder_get_fre (sframe_decoder_ctx *ctx,
 {
   sframe_func_desc_entry *fdep;
   sframe_frame_row_entry ifre;
-  unsigned char *sp;
+  const char *fres;
   uint32_t i;
   unsigned int fre_type;
   size_t esz = 0;
@@ -1158,11 +1156,11 @@ sframe_decoder_get_fre (sframe_decoder_ctx *ctx,
 
   fre_type = sframe_get_fre_type (fdep);
   /* Now scan the FRE entries.  */
-  sp = (unsigned char *) ctx->sfd_fres + fdep->sfde_func_start_fre_off;
+  fres = ctx->sfd_fres + fdep->sfde_func_start_fre_off;
   for (i = 0; i < fdep->sfde_func_num_fres; i++)
    {
      /* Decode the FRE at the current position.  Return it if valid.  */
-     err = sframe_decode_fre ((const char *)sp, &ifre, fre_type, &esz);
+     err = sframe_decode_fre (fres, &ifre, fre_type, &esz);
      if (i == fre_idx)
        {
 	 if (!sframe_fre_sanity_check_p (&ifre))
@@ -1179,7 +1177,7 @@ sframe_decoder_get_fre (sframe_decoder_ctx *ctx,
 	 return 0;
        }
      /* Next FRE.  */
-     sp += esz;
+     fres += esz;
    }
 
   return sframe_set_errno (&err, SFRAME_ERR_FDE_NOTFOUND);
-- 
2.39.2

[COMMITTED] libsframe: revisit sframe_find_fre API

[Indu Bhagat]

Inspite of implementing a rather simple functionality, this function was
relatively difficult to follow, and maintain.  Some changes are done now
to address that - refactor the function and use better names to make it
more readable.

The changes to the implementation do not cause any change in the
contract of the API.

libsframe/
        * sframe.c (sframe_fre_get_end_ip_offset): to here...
        (sframe_find_fre): Refactor some bits from...
---
 libsframe/sframe.c | 83 +++++++++++++++++++++++++++-------------------
 1 file changed, 49 insertions(+), 34 deletions(-)

diff --git a/libsframe/sframe.c b/libsframe/sframe.c
index 72b221349ad..dadce2c1262 100644
--- a/libsframe/sframe.c
+++ b/libsframe/sframe.c
@@ -980,6 +980,32 @@ sframe_get_funcdesc_with_addr (sframe_decoder_ctx *ctx,
   return sframe_ret_set_errno (errp, SFRAME_ERR_FDE_NOTFOUND);
 }
 
+/* Get the end IP offset for the FRE at index i in the FDEP.  The buffer FRES
+   is the starting location for the FRE.  */
+
+static uint32_t
+sframe_fre_get_end_ip_offset (sframe_func_desc_entry *fdep, unsigned int i,
+			      const char *fres)
+{
+  uint32_t end_ip_offset;
+  uint32_t fre_type;
+
+  fre_type = sframe_get_fre_type (fdep);
+
+  /* Get the start address of the next FRE in sequence.  */
+  if (i < fdep->sfde_func_num_fres - 1)
+    {
+      sframe_decode_fre_start_address (fres, &end_ip_offset, fre_type);
+      end_ip_offset -= 1;
+    }
+  else
+    /* The end IP offset for the FRE needs to be deduced from the function
+       size.  */
+    end_ip_offset = fdep->sfde_func_size - 1;
+
+  return end_ip_offset;
+}
+
 /* Find the SFrame Row Entry which contains the PC.  Returns
    SFRAME_ERR if failure.  */
 
@@ -987,14 +1013,15 @@ int
 sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
 		 sframe_frame_row_entry *frep)
 {
+  sframe_frame_row_entry cur_fre;
   sframe_func_desc_entry *fdep;
-  uint32_t start_address, i;
-  sframe_frame_row_entry cur_fre, next_fre;
-  const char *fres;
   unsigned int fre_type, fde_type;
-  size_t esz;
-  int err = 0;
+  uint32_t end_ip_offset, i;
+  int32_t start_ip, end_ip;
+  int32_t func_start_addr;
+  const char *fres;
   size_t size = 0;
+  int err = 0;
   /* For regular FDEs (i.e. fde_type SFRAME_FDE_TYPE_PCINC),
      where the start address in the FRE is an offset from start pc,
      use a bitmask with all bits set so that none of the address bits are
@@ -1022,40 +1049,28 @@ sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
     bitmask = 0xff;
 
   fres = ctx->sfd_fres + fdep->sfde_func_start_fre_off;
+  func_start_addr = fdep->sfde_func_start_address;
+
   for (i = 0; i < fdep->sfde_func_num_fres; i++)
    {
-     err = sframe_decode_fre (fres, &next_fre, fre_type, &esz);
-     start_address = next_fre.fre_start_addr;
+     err = sframe_decode_fre (fres, &cur_fre, fre_type, &size);
+     if (err)
+       return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
 
-     if (((fdep->sfde_func_start_address
-	   + (int32_t) start_address) & bitmask) <= (pc & bitmask))
+     start_ip = func_start_addr + cur_fre.fre_start_addr;
+     end_ip_offset = sframe_fre_get_end_ip_offset (fdep, i, fres + size);
+     end_ip = func_start_addr + end_ip_offset;
+
+     if ((start_ip & bitmask) > (pc & bitmask))
+       return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
+
+     if (((start_ip & bitmask) <= (pc & bitmask))
+	 && (end_ip & bitmask) >= (pc & bitmask))
        {
-	 sframe_frame_row_entry_copy (&cur_fre, &next_fre);
-
-	 /* Get the next FRE in sequence.  */
-	 if (i < fdep->sfde_func_num_fres - 1)
-	   {
-	     sp += esz;
-	     err = sframe_decode_fre (fres, &next_fre, fre_type, &esz);
-
-	     /* Sanity check the next FRE.  */
-	     if (!sframe_fre_sanity_check_p (&next_fre))
-	       return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
-
-	     size = next_fre.fre_start_addr;
-	   }
-	 else size = fdep->sfde_func_size;
-
-	 /* If the cur FRE is the one that contains the PC, return it.  */
-	 if (((fdep->sfde_func_start_address
-	       + (int32_t)size) & bitmask) > (pc & bitmask))
-	   {
-	     sframe_frame_row_entry_copy (frep, &cur_fre);
-	     return 0;
-	   }
+	 sframe_frame_row_entry_copy (frep, &cur_fre);
+	 return 0;
        }
-     else
-       return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
+     fres += size;
    }
   return sframe_set_errno (&err, SFRAME_ERR_FDE_INVAL);
 }
-- 
2.39.2

[COMMITTED] sframe/doc: minor improvements for readability

[Indu Bhagat]

libsframe/
	* sframe-spec.texi: Cosmetic fixes.
---
 libsframe/doc/sframe-spec.texi | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/libsframe/doc/sframe-spec.texi b/libsframe/doc/sframe-spec.texi
index 6987b6fee13..5155410bdc8 100644
--- a/libsframe/doc/sframe-spec.texi
+++ b/libsframe/doc/sframe-spec.texi
@@ -46,8 +46,8 @@ Frame Pointer (FP).
 Return Address (RA).
 @end itemize
 
-The reason for existence of the SFrame format is to support fast, online
-generation of stack traces using simple means.
+The reason for existence of the SFrame format is to provide a simple, fast and
+low-overhead mechanism to generate stack traces.
 
 @menu
 * Overview::
@@ -81,7 +81,7 @@ The associated API to decode, probe and encode the SFrame section, provided via
 later.
 
 This document is intended to be in sync with the C code in @file{sframe.h}.
-Please report descrepancies between the two, if any.
+Please report discrepancies between the two, if any.
 
 @node SFrame section
 @chapter SFrame section
@@ -234,14 +234,14 @@ of the return address is @code{CFA - 8}.  Since this offset is in close
 vicinity with the CFA in most ABIs, @code{sfh_cfa_fixed_fp_offset} and
 @code{sfh_cfa_fixed_ra_offset} are limited to signed 8-bit integers.
 
-SFrame format has provisioned for future ABIs/architectures that it may
-support.  The @code{sframe_header} structure provides an unsigned 8-bit
-integral field to denote the size of an auxilliary SFrame header.  The
-auxilliary SFrame header follows right after the @code{sframe_header}
+SFrame format has made some provisions for supporting more ABIs/architectures
+in the future.  The @code{sframe_header} structure provides an unsigned 8-bit
+integral field to denote the size of an auxiliary SFrame header.  The
+auxiliary SFrame header follows right after the @code{sframe_header}
 structure.  As for the offset calculations, the @emph{end} of SFrame header
-must be the end of the auxilliary SFrame header, if the latter is present.
+must be the end of the auxiliary SFrame header, if the latter is present.
 
-Tieing it all together:
+Putting it all together:
 
 @multitable {Offset} {@code{int8_t sfh_cfa_fixed_fp_offset}} {The number of SFrame FREs in the section.}
 @headitem Offset @tab Name @tab Description
@@ -263,7 +263,7 @@ Tieing it all together:
 
 @item 0x07
 @tab @code{uint8_t sfh_auxhdr_len}
-@tab Size in bytes of the auxilliary header that follows the
+@tab Size in bytes of the auxiliary header that follows the
 @code{sframe_header} structure.
 
 @item 0x08
-- 
2.39.2

Re: [COMMITTED] libsframe: revisit sframe_find_fre API

[Jan Beulich]

On 26.05.2023 09:01, Indu Bhagat via Binutils wrote:
> @@ -1022,40 +1049,28 @@ sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
>      bitmask = 0xff;
>  
>    fres = ctx->sfd_fres + fdep->sfde_func_start_fre_off;
> +  func_start_addr = fdep->sfde_func_start_address;
> +
>    for (i = 0; i < fdep->sfde_func_num_fres; i++)
>     {
> -     err = sframe_decode_fre (fres, &next_fre, fre_type, &esz);
> -     start_address = next_fre.fre_start_addr;
> +     err = sframe_decode_fre (fres, &cur_fre, fre_type, &size);
> +     if (err)
> +       return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
>  
> -     if (((fdep->sfde_func_start_address
> -	   + (int32_t) start_address) & bitmask) <= (pc & bitmask))
> +     start_ip = func_start_addr + cur_fre.fre_start_addr;
> +     end_ip_offset = sframe_fre_get_end_ip_offset (fdep, i, fres + size);
> +     end_ip = func_start_addr + end_ip_offset;
> +
> +     if ((start_ip & bitmask) > (pc & bitmask))
> +       return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
> +
> +     if (((start_ip & bitmask) <= (pc & bitmask))
> +	 && (end_ip & bitmask) >= (pc & bitmask))
>         {
> -	 sframe_frame_row_entry_copy (&cur_fre, &next_fre);
> -
> -	 /* Get the next FRE in sequence.  */
> -	 if (i < fdep->sfde_func_num_fres - 1)
> -	   {
> -	     sp += esz;

The buildbot failure just reported was found on the commit prior to this
one, which removed the declaration of sp. Please try to make sure that
every patch builds (and works) okay individually.

Jan

Re: [COMMITTED] libsframe: revisit sframe_find_fre API

[Indu Bhagat]

On 5/26/23 00:30, Jan Beulich wrote:
> On 26.05.2023 09:01, Indu Bhagat via Binutils wrote:
>> @@ -1022,40 +1049,28 @@ sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
>>       bitmask = 0xff;
>>   
>>     fres = ctx->sfd_fres + fdep->sfde_func_start_fre_off;
>> +  func_start_addr = fdep->sfde_func_start_address;
>> +
>>     for (i = 0; i < fdep->sfde_func_num_fres; i++)
>>      {
>> -     err = sframe_decode_fre (fres, &next_fre, fre_type, &esz);
>> -     start_address = next_fre.fre_start_addr;
>> +     err = sframe_decode_fre (fres, &cur_fre, fre_type, &size);
>> +     if (err)
>> +       return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
>>   
>> -     if (((fdep->sfde_func_start_address
>> -	   + (int32_t) start_address) & bitmask) <= (pc & bitmask))
>> +     start_ip = func_start_addr + cur_fre.fre_start_addr;
>> +     end_ip_offset = sframe_fre_get_end_ip_offset (fdep, i, fres + size);
>> +     end_ip = func_start_addr + end_ip_offset;
>> +
>> +     if ((start_ip & bitmask) > (pc & bitmask))
>> +       return sframe_set_errno (&err, SFRAME_ERR_FRE_INVAL);
>> +
>> +     if (((start_ip & bitmask) <= (pc & bitmask))
>> +	 && (end_ip & bitmask) >= (pc & bitmask))
>>          {
>> -	 sframe_frame_row_entry_copy (&cur_fre, &next_fre);
>> -
>> -	 /* Get the next FRE in sequence.  */
>> -	 if (i < fdep->sfde_func_num_fres - 1)
>> -	   {
>> -	     sp += esz;
> 
> The buildbot failure just reported was found on the commit prior to this
> one, which removed the declaration of sp. Please try to make sure that
> every patch builds (and works) okay individually.
> 
> Jan

[(Re)Sending to binutils@sourceware, I earlier sent my apologies to the 
builder@ ;-) ]

Yes, my apologies. Turns out the 4 commits I just did were not 
bisectable truly. I split a change into two logical commits, and missed 
testing them one by one.

Revision: 812d868850126d8e791795c7e248ffbf580925f6 causes build 
failures, but the subsequent commit (Revision: 
83c219872b2131546ccec7edc57eb47c64b8911d) fixes the issue.

Will take care in future.

Indu