[PATCH 0/6] Small improvements around SFrame support

[PATCH 0/6] Small improvements around SFrame support

[Indu Bhagat]

Hello,

This series contains small patches that make minor improvements in the SFrame
support.  Some of these can be committed as obvious, but it won't hurt to send
them as a part of this set either.

Testing notes:
- Regression testing using a script that tests a variety of targets on host
x86_64-linux-gnu, and host aarch64-linux-gnu.
- try bot run showed no regressions.
- unwinder testsuite based on SFrame (not upstream'd yet) continues to work
on x86_64 and aarch64.

Thanks,

Indu Bhagat (6):
  libsframe: minor formatting nits
  sframe.h: make some macros more precise
  sframe: gas: libsframe: define constants and remove magic numbers
  gas: sframe: fine tune the fragment fixup for SFrame func info
  libsframe: rename API sframe_fde_func_info to
    sframe_fde_create_func_info
  objdump: sframe: fix memory leaks

 bfd/elfxx-x86.c                               |  6 +-
 binutils/objdump.c                            |  8 ++-
 gas/gen-sframe.c                              | 55 +++++++++++++++----
 gas/sframe-opt.c                              | 46 ++++++++++------
 include/sframe-api.h                          |  4 +-
 include/sframe.h                              | 19 ++++++-
 libsframe/sframe.c                            | 42 +++++++-------
 .../testsuite/libsframe.encode/encode-1.c     |  8 +--
 8 files changed, 127 insertions(+), 61 deletions(-)

-- 
2.37.2

[PATCH 1/6] libsframe: minor formatting nits

[Indu Bhagat]

ChangeLog:

	* libsframe/sframe.c: Fix formatting nits.
---
 libsframe/sframe.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/libsframe/sframe.c b/libsframe/sframe.c
index fce821e4414..6e0eb7b6511 100644
--- a/libsframe/sframe.c
+++ b/libsframe/sframe.c
@@ -202,8 +202,7 @@ sframe_header_sanity_check_p (sframe_header *hp)
   /* Check preamble is valid.  */
   if ((hp->sfh_preamble.sfp_magic != SFRAME_MAGIC)
       || (hp->sfh_preamble.sfp_version != SFRAME_VERSION)
-      || ((hp->sfh_preamble.sfp_flags | all_flags)
-	  != all_flags))
+      || ((hp->sfh_preamble.sfp_flags | all_flags) != all_flags))
     return 0;
 
   /* Check offsets are valid.  */
@@ -723,7 +722,7 @@ sframe_decode_fre (const char *fre_buf, sframe_frame_row_entry *fre,
   /* The FRE has been decoded.  Use it to perform one last sanity check.  */
   fre_size = sframe_fre_entry_size (fre, fre_type);
   sframe_assert (fre_size == (addr_size + sizeof (fre->fre_info)
-				 + stack_offsets_sz));
+			      + stack_offsets_sz));
   *esz = fre_size;
 
   return 0;
@@ -980,8 +979,7 @@ sframe_find_fre (sframe_decoder_ctx *ctx, int32_t pc,
   sp = (unsigned char *) 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 ((const char *)sp, &next_fre, fre_type, &esz);
      start_address = next_fre.fre_start_addr;
 
      if (((fdep->sfde_func_start_address
@@ -1074,7 +1072,7 @@ sframe_decoder_get_funcdesc (sframe_decoder_ctx *ctx,
 
 static sframe_func_desc_entry *
 sframe_decoder_get_funcdesc_at_index (sframe_decoder_ctx *ctx,
-					 uint32_t func_idx)
+				      uint32_t func_idx)
 {
   /* Invalid argument.  No FDE will be found.  */
   if (func_idx >= sframe_decoder_get_num_fidx (ctx))
@@ -1091,9 +1089,9 @@ sframe_decoder_get_funcdesc_at_index (sframe_decoder_ctx *ctx,
 
 int
 sframe_decoder_get_fre (sframe_decoder_ctx *ctx,
-			   unsigned int func_idx,
-			   unsigned int fre_idx,
-			   sframe_frame_row_entry *fre)
+			unsigned int func_idx,
+			unsigned int fre_idx,
+			sframe_frame_row_entry *fre)
 {
   sframe_func_desc_entry *fdep;
   sframe_frame_row_entry ifre;
@@ -1157,7 +1155,7 @@ sframe_encoder_get_header (sframe_encoder_ctx *encoder)
 
 static sframe_func_desc_entry *
 sframe_encoder_get_funcdesc_at_index (sframe_encoder_ctx *encoder,
-					 uint32_t func_idx)
+				      uint32_t func_idx)
 {
   sframe_func_desc_entry *fde = NULL;
   if (func_idx < sframe_encoder_get_num_fidx (encoder))
@@ -1273,8 +1271,8 @@ sframe_encoder_get_num_fidx (sframe_encoder_ctx *encoder)
 
 int
 sframe_encoder_add_fre (sframe_encoder_ctx *encoder,
-			   unsigned int func_idx,
-			   sframe_frame_row_entry *frep)
+			unsigned int func_idx,
+			sframe_frame_row_entry *frep)
 {
   sframe_header *ehp;
   sframe_func_desc_entry *fdep;
@@ -1371,10 +1369,10 @@ bad:
 
 int
 sframe_encoder_add_funcdesc (sframe_encoder_ctx *encoder,
-			      int32_t start_addr,
-			      uint32_t func_size,
-			      unsigned char func_info,
-			      uint32_t num_fres __attribute__ ((unused)))
+			     int32_t start_addr,
+			     uint32_t func_size,
+			     unsigned char func_info,
+			     uint32_t num_fres __attribute__ ((unused)))
 {
   sframe_header *ehp;
   sf_funidx_tbl *fd_info;
-- 
2.37.2

Re: [PATCH 1/6] libsframe: minor formatting nits

[Nick Clifton]

Hi Indu,

> ChangeLog:
> 
> 	* libsframe/sframe.c: Fix formatting nits.

Approved  - please apply.

Cheers
   Nick

[PATCH 2/6] sframe.h: make some macros more precise

[Indu Bhagat]

include/ChangeLog:

	* sframe.h (SFRAME_V1_FUNC_INFO): Use specific bits only.
	(SFRAME_V1_FRE_INFO): Likewise.
---
 include/sframe.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/sframe.h b/include/sframe.h
index 7e31284e4d8..ba557b7bf7a 100644
--- a/include/sframe.h
+++ b/include/sframe.h
@@ -192,7 +192,7 @@ typedef struct sframe_func_desc_entry
 /* Macros to compose and decompose function info in FDE.  */
 
 #define SFRAME_V1_FUNC_INFO(fde_type, fre_enc_type) \
-  (((fde_type) & 0x1) << 4 | (fre_enc_type))
+  (((fde_type) & 0x1) << 4 | (fre_enc_type & 0xf))
 
 #define SFRAME_V1_FUNC_FRE_TYPE(data)	  ((data) & 0xf)
 #define SFRAME_V1_FUNC_FDE_TYPE(data)	  ((data >> 4) & 0x1)
@@ -240,7 +240,7 @@ typedef struct sframe_fre_info
 /* Macros to compose and decompose FRE info.  */
 
 #define SFRAME_V1_FRE_INFO(base_reg_id, offset_num, offset_size) \
-  ((offset_size << 5) | (offset_num << 1) | (base_reg_id))
+  (((offset_size & 0x3) << 5) | ((offset_num & 0xf) << 1) | (base_reg_id & 0x1))
 
 #define SFRAME_V1_FRE_CFA_BASE_REG_ID(data)	  ((data) & 0x1)
 #define SFRAME_V1_FRE_OFFSET_COUNT(data)	  (((data) >> 1) & 0xf)
-- 
2.37.2

Re: [PATCH 2/6] sframe.h: make some macros more precise

[Hans-Peter Nilsson]

On Wed, 7 Dec 2022, Indu Bhagat via Binutils wrote:
> include/ChangeLog:
> 
> 	* sframe.h (SFRAME_V1_FUNC_INFO): Use specific bits only.
> 	(SFRAME_V1_FRE_INFO): Likewise.
> ---
>  include/sframe.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sframe.h b/include/sframe.h
> index 7e31284e4d8..ba557b7bf7a 100644
> --- a/include/sframe.h
> +++ b/include/sframe.h
> @@ -192,7 +192,7 @@ typedef struct sframe_func_desc_entry
>  /* Macros to compose and decompose function info in FDE.  */
>  
>  #define SFRAME_V1_FUNC_INFO(fde_type, fre_enc_type) \
> -  (((fde_type) & 0x1) << 4 | (fre_enc_type))
> +  (((fde_type) & 0x1) << 4 | (fre_enc_type & 0xf))

Random comment:  you removed the full parenthesisation (sp?) of 
macro arguments here; compare to handling fde_type.  ITYM:
+ (((fde_type) & 0x1) << 4 | ((fre_enc_type) & 0xf))

Looks like the SFframe code "has improvement potential" in this 
regard, because right in the context of your patch, there's:

>  #define SFRAME_V1_FUNC_FRE_TYPE(data)	  ((data) & 0xf)
>  #define SFRAME_V1_FUNC_FDE_TYPE(data)	  ((data >> 4) & 0x1)

(Pre-existing badness for SFRAME_V1_FUNC_FDE_TYPE compared to 
the goodness for SFRAME_V1_FUNC_FRE_TYPE.)

> @@ -240,7 +240,7 @@ typedef struct sframe_fre_info
>  /* Macros to compose and decompose FRE info.  */
>  
>  #define SFRAME_V1_FRE_INFO(base_reg_id, offset_num, offset_size) \
> -  ((offset_size << 5) | (offset_num << 1) | (base_reg_id))
> +  (((offset_size & 0x3) << 5) | ((offset_num & 0xf) << 1) | (base_reg_id & 0x1))

Also, here too, you just added one more.  But better improve it 
than making it ever so slightly worse.

brgds, H-P

Re: [PATCH 2/6] sframe.h: make some macros more precise

[Indu Bhagat]

On 12/7/22 15:52, Hans-Peter Nilsson wrote:
> On Wed, 7 Dec 2022, Indu Bhagat via Binutils wrote:
>> include/ChangeLog:
>>
>> 	* sframe.h (SFRAME_V1_FUNC_INFO): Use specific bits only.
>> 	(SFRAME_V1_FRE_INFO): Likewise.
>> ---
>>   include/sframe.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/sframe.h b/include/sframe.h
>> index 7e31284e4d8..ba557b7bf7a 100644
>> --- a/include/sframe.h
>> +++ b/include/sframe.h
>> @@ -192,7 +192,7 @@ typedef struct sframe_func_desc_entry
>>   /* Macros to compose and decompose function info in FDE.  */
>>   
>>   #define SFRAME_V1_FUNC_INFO(fde_type, fre_enc_type) \
>> -  (((fde_type) & 0x1) << 4 | (fre_enc_type))
>> +  (((fde_type) & 0x1) << 4 | (fre_enc_type & 0xf))
> 
> Random comment:  you removed the full parenthesisation (sp?) of
> macro arguments here; compare to handling fde_type.  ITYM:
> + (((fde_type) & 0x1) << 4 | ((fre_enc_type) & 0xf))
> 
> Looks like the SFframe code "has improvement potential" in this
> regard, because right in the context of your patch, there's:
> 
>>   #define SFRAME_V1_FUNC_FRE_TYPE(data)	  ((data) & 0xf)
>>   #define SFRAME_V1_FUNC_FDE_TYPE(data)	  ((data >> 4) & 0x1)
> 
> (Pre-existing badness for SFRAME_V1_FUNC_FDE_TYPE compared to
> the goodness for SFRAME_V1_FUNC_FRE_TYPE.)
> 
>> @@ -240,7 +240,7 @@ typedef struct sframe_fre_info
>>   /* Macros to compose and decompose FRE info.  */
>>   
>>   #define SFRAME_V1_FRE_INFO(base_reg_id, offset_num, offset_size) \
>> -  ((offset_size << 5) | (offset_num << 1) | (base_reg_id))
>> +  (((offset_size & 0x3) << 5) | ((offset_num & 0xf) << 1) | (base_reg_id & 0x1))
> 
> Also, here too, you just added one more.  But better improve it
> than making it ever so slightly worse.
> 

I agree. I will fix these instances.

Thanks for reviewing,
Indu

[PATCH,V2 2/6] sframe.h: make some macros more precise

[Indu Bhagat]

[Changes in V2]
  - maintain parenthesisation of args
[End of changes in V2]

include/ChangeLog:

	* sframe.h (SFRAME_V1_FUNC_INFO): Use specific bits only.
	(SFRAME_V1_FRE_INFO): Likewise.
---
 include/sframe.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/sframe.h b/include/sframe.h
index 7e31284e4d8..7e167bf4dbe 100644
--- a/include/sframe.h
+++ b/include/sframe.h
@@ -192,10 +192,10 @@ typedef struct sframe_func_desc_entry
 /* Macros to compose and decompose function info in FDE.  */
 
 #define SFRAME_V1_FUNC_INFO(fde_type, fre_enc_type) \
-  (((fde_type) & 0x1) << 4 | (fre_enc_type))
+  ((((fde_type) & 0x1) << 4) | ((fre_enc_type) & 0xf))
 
 #define SFRAME_V1_FUNC_FRE_TYPE(data)	  ((data) & 0xf)
-#define SFRAME_V1_FUNC_FDE_TYPE(data)	  ((data >> 4) & 0x1)
+#define SFRAME_V1_FUNC_FDE_TYPE(data)	  (((data) >> 4) & 0x1)
 
 /* Size of stack frame offsets in an SFrame Frame Row Entry.  A single
    SFrame FRE has all offsets of the same size.  Offset size may vary
@@ -240,7 +240,8 @@ typedef struct sframe_fre_info
 /* Macros to compose and decompose FRE info.  */
 
 #define SFRAME_V1_FRE_INFO(base_reg_id, offset_num, offset_size) \
-  ((offset_size << 5) | (offset_num << 1) | (base_reg_id))
+  ((((offset_size) & 0x3) << 5) | (((offset_num) & 0xf) << 1) | \
+   ((base_reg_id) & 0x1))
 
 #define SFRAME_V1_FRE_CFA_BASE_REG_ID(data)	  ((data) & 0x1)
 #define SFRAME_V1_FRE_OFFSET_COUNT(data)	  (((data) >> 1) & 0xf)
-- 
2.37.2

Re: [PATCH,V2 2/6] sframe.h: make some macros more precise

[Nick Clifton]

Hi Indu,

> include/ChangeLog:
> 
> 	* sframe.h (SFRAME_V1_FUNC_INFO): Use specific bits only.
> 	(SFRAME_V1_FRE_INFO): Likewise.

Approved - please apply.

Cheers
   Nick

[PATCH 3/6] sframe: gas: libsframe: define constants and remove magic numbers

[Indu Bhagat]

Define constants in sframe.h for the various limits associated with the
range of offsets that can be encoded in the start address of an SFrame
FRE. E.g., sframe_frame_row_entry_addr1 is used when start address
offset can be encoded as 1-byte unsigned value.

Update the code in gas to use these defined constants as it checks for
these limits, and remove the usage of magic numbers.

ChangeLog:

	* gas/sframe-opt.c (sframe_estimate_size_before_relax):
	(sframe_convert_frag): Do not use magic numbers.
	* libsframe/sframe.c (sframe_calc_fre_type): Likewise.

include/ChangeLog:

	* sframe.h (SFRAME_FRE_TYPE_ADDR1_LIMIT): New constant.
	(SFRAME_FRE_TYPE_ADDR2_LIMIT): Likewise.
	(SFRAME_FRE_TYPE_ADDR4_LIMIT): Likewise.
---
 gas/sframe-opt.c   | 12 ++++++------
 include/sframe.h   | 15 +++++++++++++++
 libsframe/sframe.c |  6 +++---
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/gas/sframe-opt.c b/gas/sframe-opt.c
index c17fd6b8332..6901aa82a77 100644
--- a/gas/sframe-opt.c
+++ b/gas/sframe-opt.c
@@ -53,9 +53,9 @@ sframe_estimate_size_before_relax (fragS *frag)
       widthS = exp->X_op_symbol;
       width = resolve_symbol_value (widthS);
 
-      if (width < 0x100)
+      if (width < SFRAME_FRE_TYPE_ADDR1_LIMIT)
 	ret = 1;
-      else if (width < 0x10000)
+      else if (width < SFRAME_FRE_TYPE_ADDR2_LIMIT)
 	ret = 2;
       else
 	ret = 4;
@@ -109,9 +109,9 @@ sframe_convert_frag (fragS *frag)
     {
       fsizeS = frag->fr_symbol;
       fsize = resolve_symbol_value (fsizeS);
-      if (fsize < 0x100)
+      if (fsize < SFRAME_FRE_TYPE_ADDR1_LIMIT)
 	func_info = SFRAME_FRE_TYPE_ADDR1;
-      else if (fsize < 0x10000)
+      else if (fsize < SFRAME_FRE_TYPE_ADDR2_LIMIT)
 	func_info = SFRAME_FRE_TYPE_ADDR2;
       else
 	func_info = SFRAME_FRE_TYPE_ADDR4;
@@ -133,11 +133,11 @@ sframe_convert_frag (fragS *frag)
       switch (frag->fr_subtype & 7)
 	{
 	case 1:
-	  gas_assert (fsize < 0x100);
+	  gas_assert (fsize < SFRAME_FRE_TYPE_ADDR1_LIMIT);
 	  frag->fr_literal[frag->fr_fix] = diff;
 	  break;
 	case 2:
-	  gas_assert (fsize < 0x10000);
+	  gas_assert (fsize < SFRAME_FRE_TYPE_ADDR2_LIMIT);
 	  md_number_to_chars (frag->fr_literal + frag->fr_fix, diff, 2);
 	  break;
 	case 4:
diff --git a/include/sframe.h b/include/sframe.h
index ba557b7bf7a..85d1e54520d 100644
--- a/include/sframe.h
+++ b/include/sframe.h
@@ -272,6 +272,7 @@ typedef struct sframe_fre_info
     fi
 */
 
+/* Used when SFRAME_FRE_TYPE_ADDR1 is specified as FRE type.  */
 typedef struct sframe_frame_row_entry_addr1
 {
   /* Start address of the frame row entry.  Encoded as an 1-byte unsigned
@@ -280,6 +281,11 @@ typedef struct sframe_frame_row_entry_addr1
   sframe_fre_info sfre_info;
 } ATTRIBUTE_PACKED sframe_frame_row_entry_addr1;
 
+/* Upper limit of start address in sframe_frame_row_entry_addr1
+   is 0x100 (not inclusive).  */
+#define SFRAME_FRE_TYPE_ADDR1_LIMIT  ((SFRAME_FRE_TYPE_ADDR1+1)*8)
+
+/* Used when SFRAME_FRE_TYPE_ADDR2 is specified as FRE type.  */
 typedef struct sframe_frame_row_entry_addr2
 {
   /* Start address of the frame row entry.  Encoded as an 2-byte unsigned
@@ -288,6 +294,11 @@ typedef struct sframe_frame_row_entry_addr2
   sframe_fre_info sfre_info;
 } ATTRIBUTE_PACKED sframe_frame_row_entry_addr2;
 
+/* Upper limit of start address in sframe_frame_row_entry_addr2
+   is 0x10000 (not inclusive).  */
+#define SFRAME_FRE_TYPE_ADDR2_LIMIT  ((SFRAME_FRE_TYPE_ADDR2*2)*8)
+
+/* Used when SFRAME_FRE_TYPE_ADDR4 is specified as FRE type.  */
 typedef struct sframe_frame_row_entry_addr4
 {
   /* Start address of the frame row entry.  Encoded as a 4-byte unsigned
@@ -296,6 +307,10 @@ typedef struct sframe_frame_row_entry_addr4
   sframe_fre_info sfre_info;
 } ATTRIBUTE_PACKED sframe_frame_row_entry_addr4;
 
+/* Upper limit of start address in sframe_frame_row_entry_addr2
+   is 0x100000000 (not inclusive).  */
+#define SFRAME_FRE_TYPE_ADDR4_LIMIT  ((SFRAME_FRE_TYPE_ADDR4*2)*8)
+
 #ifdef	__cplusplus
 }
 #endif
diff --git a/libsframe/sframe.c b/libsframe/sframe.c
index 6e0eb7b6511..64fa9078d62 100644
--- a/libsframe/sframe.c
+++ b/libsframe/sframe.c
@@ -572,11 +572,11 @@ unsigned int
 sframe_calc_fre_type (unsigned int func_size)
 {
   unsigned int fre_type = 0;
-  if (func_size <= 0xff)
+  if (func_size < SFRAME_FRE_TYPE_ADDR1_LIMIT)
     fre_type = SFRAME_FRE_TYPE_ADDR1;
-  else if (func_size <= 0xffff)
+  else if (func_size < SFRAME_FRE_TYPE_ADDR2_LIMIT)
     fre_type = SFRAME_FRE_TYPE_ADDR2;
-  else if (func_size <= 0xffffffff)
+  else if (func_size < SFRAME_FRE_TYPE_ADDR4_LIMIT)
     fre_type = SFRAME_FRE_TYPE_ADDR4;
   return fre_type;
 }
-- 
2.37.2

Re: [PATCH 3/6] sframe: gas: libsframe: define constants and remove magic numbers

[Nick Clifton]

Hi Indu,

> +#define SFRAME_FRE_TYPE_ADDR1_LIMIT  ((SFRAME_FRE_TYPE_ADDR1+1)*8)

For readabilities sake, I would recommend adding whitespace around arithmetic
operations.  For example in the define above a quick glance would suggest that
the definition is for a symbol called ...ADDR11 rather than ...ADDR1 + 1.
So:

#define SFRAME_FRE_TYPE_ADDR1_LIMIT  ((SFRAME_FRE_TYPE_ADDR1 + 1) * 8)

Is better IMHO.

> +#define SFRAME_FRE_TYPE_ADDR2_LIMIT  ((SFRAME_FRE_TYPE_ADDR2*2)*8)
> +#define SFRAME_FRE_TYPE_ADDR4_LIMIT  ((SFRAME_FRE_TYPE_ADDR4*2)*8)

The same goes for these two definitions as well.

Patch approved with these changes.

Cheers
   Nick

PS.  Just checking, since I am not actually familiar with the sframe
format:  Is it correct that SFRAME_FRE_TYPE_ADDR1_LIMIT is defined
as "(...ADDR1 + 1) * 8" rather than "(...ADDR1 * 2) * 8)" ?  It is
just that the other two limits are defined using the second formula
and it seems slightly odd that it is not used for the first.

Re: [PATCH 3/6] sframe: gas: libsframe: define constants and remove magic numbers

[Indu Bhagat]

On 12/8/22 03:10, Nick Clifton wrote:
> Hi Indu,
> 
>> +#define SFRAME_FRE_TYPE_ADDR1_LIMIT  ((SFRAME_FRE_TYPE_ADDR1+1)*8)
> 
> For readabilities sake, I would recommend adding whitespace around 
> arithmetic
> operations.  For example in the define above a quick glance would 
> suggest that
> the definition is for a symbol called ...ADDR11 rather than ...ADDR1 + 1.
> So:
> 
> #define SFRAME_FRE_TYPE_ADDR1_LIMIT  ((SFRAME_FRE_TYPE_ADDR1 + 1) * 8)
> 
> Is better IMHO.
> 

Yes, I agree. I will fix it.

>> +#define SFRAME_FRE_TYPE_ADDR2_LIMIT  ((SFRAME_FRE_TYPE_ADDR2*2)*8)
>> +#define SFRAME_FRE_TYPE_ADDR4_LIMIT  ((SFRAME_FRE_TYPE_ADDR4*2)*8)
> 
> The same goes for these two definitions as well.
> 
> Patch approved with these changes.
> 
> Cheers
>    Nick
> 
> PS.  Just checking, since I am not actually familiar with the sframe
> format:  Is it correct that SFRAME_FRE_TYPE_ADDR1_LIMIT is defined
> as "(...ADDR1 + 1) * 8" rather than "(...ADDR1 * 2) * 8)" ?  It is
> just that the other two limits are defined using the second formula
> and it seems slightly odd that it is not used for the first.
> 

Yes, it does appear unpleasing to the eye. But it is the way it is 
because the constants are defined as following:

#define SFRAME_FRE_TYPE_ADDR1   0
#define SFRAME_FRE_TYPE_ADDR2   1
#define SFRAME_FRE_TYPE_ADDR4   2

All this scrambling because keeping 3-bits was deemed sufficient to 
encode 3 different values (size of 1 byte, 2 byte and 4 bytes 
respectively), and the rest of the bits were provisioned for other 
information.

Thanks for reviewing,
Indu

Re: [PATCH 3/6] sframe: gas: libsframe: define constants and remove magic numbers

[Indu Bhagat]

On 12/8/22 09:43, Indu Bhagat wrote:
> On 12/8/22 03:10, Nick Clifton wrote:
>> Hi Indu,
>>
>>> +#define SFRAME_FRE_TYPE_ADDR1_LIMIT  ((SFRAME_FRE_TYPE_ADDR1+1)*8)
>>
>> For readabilities sake, I would recommend adding whitespace around 
>> arithmetic
>> operations.  For example in the define above a quick glance would 
>> suggest that
>> the definition is for a symbol called ...ADDR11 rather than ...ADDR1 + 1.
>> So:
>>
>> #define SFRAME_FRE_TYPE_ADDR1_LIMIT  ((SFRAME_FRE_TYPE_ADDR1 + 1) * 8)
>>
>> Is better IMHO.
>>
> 
> Yes, I agree. I will fix it.
> 
>>> +#define SFRAME_FRE_TYPE_ADDR2_LIMIT  ((SFRAME_FRE_TYPE_ADDR2*2)*8)
>>> +#define SFRAME_FRE_TYPE_ADDR4_LIMIT  ((SFRAME_FRE_TYPE_ADDR4*2)*8)
>>
>> The same goes for these two definitions as well.
>>
>> Patch approved with these changes.
>>
>> Cheers
>>    Nick
>>
>> PS.  Just checking, since I am not actually familiar with the sframe
>> format:  Is it correct that SFRAME_FRE_TYPE_ADDR1_LIMIT is defined
>> as "(...ADDR1 + 1) * 8" rather than "(...ADDR1 * 2) * 8)" ?  It is
>> just that the other two limits are defined using the second formula
>> and it seems slightly odd that it is not used for the first.
>>
> 
> Yes, it does appear unpleasing to the eye. But it is the way it is 
> because the constants are defined as following:
> 
> #define SFRAME_FRE_TYPE_ADDR1   0
> #define SFRAME_FRE_TYPE_ADDR2   1
> #define SFRAME_FRE_TYPE_ADDR4   2
> 
> All this scrambling because keeping 3-bits was deemed sufficient to 
> encode 3 different values (size of 1 byte, 2 byte and 4 bytes 
> respectively), and the rest of the bits were provisioned for other 
> information.

[ And I Misspoke :) Sorry, I mixed up the fre_type with the stack 
offsets type. The latter has 2-bits reserved to encode 3 possible values 
(1 byte/2 byte/4 bytes); See "size of offsets" in fre_info. ]

Correction - There are 4-bits reserved for encoding the FRE types. At 
the moment there are 3 types of SFrame FREs being used (leaving space 
for the format to add other FRE types if needed). So the constants 
SFRAME_FRE_TYPE_ADDR1, SFRAME_FRE_TYPE_ADDR2, and SFRAME_FRE_TYPE_ADDR3 
are defined to use up the available space serially. Hence the weirdness 
in the *_LIMIT formulae.

[PATCH 4/6] gas: sframe: fine tune the fragment fixup for SFrame func info

[Indu Bhagat]

SFrame function info is an unsigned 8-bit field comprising of the following
(from LSB to MSB):
  - 4-bits: FRE type
  - 1-bit: FRE start address encoding
  - 3-bits: Unused

At the moment, the most-significat 4-bits are zero (The FRE start
address encoding of SFRAME_FDE_TYPE_PCINC has a value of zero, and the upper
3-bits are unused). So the current implementation works without this patch.

To be precise, however, the fragment fixup logic is meant to fixup only the
least-significant 4-bits (i.e., only the FRE type needs to be updated
according to the function size).

This patch makes the gas implementation a bit more resilient: In the
future, when the format does evolve to make use of the currently unused
3-bits in various ways, the values in those 3-bits can be propagated
unchanged while the fragment fixup continues to update the lowermost
4-bits to indicate the selected FRE type.

ChangeLog:

	* gas/gen-sframe.c (create_func_info_exp): New definition.
	(output_sframe_funcdesc): Call create_func_info_exp.
	* gas/sframe-opt.c (sframe_estimate_size_before_relax): The
	associated fragment uses O_modulus now.
	(sframe_convert_frag): Adjust the fragment fixup code according
	to the new composite exp.
---
 gas/gen-sframe.c | 55 ++++++++++++++++++++++++++++++++++++++----------
 gas/sframe-opt.c | 34 +++++++++++++++++++++---------
 2 files changed, 68 insertions(+), 21 deletions(-)

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index 896fbe44c6b..075720facd6 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -396,8 +396,8 @@ sframe_get_fre_offset_size (struct sframe_row_entry *sframe_fre)
     - <val> and <width> are themselves expressionS.
     - <val> stores the expression which when evaluated gives the value of the
       start address offset of the FRE.
-    - <width> stores the expression when when evaluated gives the number of
-      bytes needed to encode the start address offset of the FRE.
+    - <width> stores the expression when evaluated gives the number of bytes
+      needed to encode the start address offset of the FRE.
 
    The use of OP_absent as the X_op_symbol helps identify this expression
    later when fragments are fixed up.  */
@@ -431,6 +431,41 @@ create_fre_start_addr_exp (expressionS *cexp, symbolS *fre_pc_begin,
   cexp->X_add_number = 0;
 }
 
+/* Create a composite exression CEXP (for SFrame FDE function info) such that:
+
+      exp = <rest_of_func_info> OP_modulus <width>, where,
+
+    - <rest_of_func_info> and <width> are themselves expressionS.
+    - <rest_of_func_info> stores a constant expression where X_add_number is
+    used to stash away the func_info.  The upper 4-bits of the func_info are copied
+    back to the resulting byte by the fragment fixup logic.
+    - <width> stores the expression when evaluated gives the size of the
+    funtion in number of bytes.
+
+   The use of OP_modulus as the X_op_symbol helps identify this expression
+   later when fragments are fixed up.  */
+
+static void
+create_func_info_exp (expressionS *cexp, symbolS *dw_fde_end_addrS,
+		      symbolS *dw_fde_start_addrS, uint8_t func_info)
+{
+  expressionS width;
+  expressionS rest_of_func_info;
+
+  width.X_op = O_subtract;
+  width.X_add_symbol = dw_fde_end_addrS;
+  width.X_op_symbol = dw_fde_start_addrS;
+  width.X_add_number = 0;
+
+  rest_of_func_info.X_op = O_constant;
+  rest_of_func_info.X_add_number = func_info;
+
+  cexp->X_op = O_modulus;
+  cexp->X_add_symbol = make_expr_symbol (&rest_of_func_info);
+  cexp->X_op_symbol = make_expr_symbol (&width);
+  cexp->X_add_number = 0;
+}
+
 #endif
 
 static void
@@ -538,19 +573,17 @@ output_sframe_funcdesc (symbolS *start_of_fre_section,
   out_four (sframe_fde->num_fres);
 
   /* SFrame FDE function info.  */
+  unsigned char func_info;
+  func_info = sframe_set_func_info (SFRAME_FDE_TYPE_PCINC,
+				    SFRAME_FRE_TYPE_ADDR4);
 #if SFRAME_FRE_TYPE_SELECTION_OPT
-  expressionS width;
-  width.X_op = O_subtract;
-  width.X_add_symbol = dw_fde_end_addrS;
-  width.X_op_symbol = dw_fde_start_addrS;
-  width.X_add_number = 0;
+  expressionS cexp;
+  create_func_info_exp (&cexp, dw_fde_end_addrS, dw_fde_start_addrS,
+			func_info);
   frag_grow (1); /* Size of func info is unsigned char.  */
   frag_var (rs_sframe, 1, 0, (relax_substateT) 0,
-	    make_expr_symbol (&width), 0, (char *) frag_now);
+	    make_expr_symbol (&cexp), 0, (char *) frag_now);
 #else
-  unsigned char func_info;
-  func_info = sframe_set_func_info (SFRAME_FDE_TYPE_PCINC,
-				    SFRAME_FRE_TYPE_ADDR4);
   out_one (func_info);
 #endif
 }
diff --git a/gas/sframe-opt.c b/gas/sframe-opt.c
index 6901aa82a77..f08a424fd88 100644
--- a/gas/sframe-opt.c
+++ b/gas/sframe-opt.c
@@ -40,10 +40,10 @@ sframe_estimate_size_before_relax (fragS *frag)
      The two kind of fragments can be differentiated based on the opcode
      of the symbol.  */
   exp = symbol_get_value_expression (frag->fr_symbol);
-  gas_assert ((exp->X_op == O_subtract) || (exp->X_op == O_absent));
+  gas_assert ((exp->X_op == O_modulus) || (exp->X_op == O_absent));
   /* Fragment for function info in an SFrame FDE will always write
      only one byte.  */
-  if (exp->X_op == O_subtract)
+  if (exp->X_op == O_modulus)
     ret = 1;
   /* Fragment for the start address in an SFrame FRE may write out
      1/2/4 bytes depending on the value of the diff.  */
@@ -92,8 +92,12 @@ sframe_convert_frag (fragS *frag)
   offsetT fsize;
   offsetT diff;
   offsetT value;
-  unsigned char func_info = SFRAME_FRE_TYPE_ADDR4;
+
+  offsetT rest_of_data;
+  uint8_t fde_type, fre_type;
+
   expressionS *exp;
+  symbolS *dataS;
   symbolS *fsizeS, *diffS;
 
   /* We are dealing with two different kind of fragments here which need
@@ -103,19 +107,29 @@ sframe_convert_frag (fragS *frag)
      The two kind of fragments can be differentiated based on the opcode
      of the symbol.  */
   exp = symbol_get_value_expression (frag->fr_symbol);
-  gas_assert ((exp->X_op == O_subtract) || (exp->X_op == O_absent));
+  gas_assert ((exp->X_op == O_modulus) || (exp->X_op == O_absent));
   /* Fragment for function info in an SFrame FDE.  */
-  if (exp->X_op == O_subtract)
+  if (exp->X_op == O_modulus)
     {
-      fsizeS = frag->fr_symbol;
+      /* Gather the existing value of the rest of the data except
+	 the fre_type.  */
+      dataS = exp->X_add_symbol;
+      rest_of_data = (symbol_get_value_expression(dataS))->X_add_number;
+      fde_type = SFRAME_V1_FUNC_FDE_TYPE (rest_of_data);
+      gas_assert (fde_type == SFRAME_FDE_TYPE_PCINC);
+
+      /* Calculate the applicable fre_type.  */
+      fsizeS = exp->X_op_symbol;
       fsize = resolve_symbol_value (fsizeS);
       if (fsize < SFRAME_FRE_TYPE_ADDR1_LIMIT)
-	func_info = SFRAME_FRE_TYPE_ADDR1;
+	fre_type = SFRAME_FRE_TYPE_ADDR1;
       else if (fsize < SFRAME_FRE_TYPE_ADDR2_LIMIT)
-	func_info = SFRAME_FRE_TYPE_ADDR2;
+	fre_type = SFRAME_FRE_TYPE_ADDR2;
       else
-	func_info = SFRAME_FRE_TYPE_ADDR4;
-      value = func_info;
+	fre_type = SFRAME_FRE_TYPE_ADDR4;
+
+      /* Create the new function info.  */
+      value = SFRAME_V1_FUNC_INFO (fde_type, fre_type);
 
       frag->fr_literal[frag->fr_fix] = value;
     }
-- 
2.37.2

Re: [PATCH 4/6] gas: sframe: fine tune the fragment fixup for SFrame func info

[Nick Clifton]

Hi Indu,

> ChangeLog:
> 
> 	* gas/gen-sframe.c (create_func_info_exp): New definition.
> 	(output_sframe_funcdesc): Call create_func_info_exp.
> 	* gas/sframe-opt.c (sframe_estimate_size_before_relax): The
> 	associated fragment uses O_modulus now.
> 	(sframe_convert_frag): Adjust the fragment fixup code according
> 	to the new composite exp.

Approved - please apply.

Cheers
   Nick

[PATCH 5/6] libsframe: rename API sframe_fde_func_info to sframe_fde_create_func_info

[Indu Bhagat]

The new name better reflects the purpose of the function.

ChangeLog:

	* bfd/elfxx-x86.c (_bfd_x86_elf_create_sframe_plt): Use new
	name.
	* libsframe/sframe.c (sframe_fde_create_func_info): Rename
	sframe_fde_func_info to this.
	* libsframe/testsuite/libsframe.encode/encode-1.c: Use new name.

include/ChangeLog:

	* sframe-api.h (sframe_fde_create_func_info): Rename
	sframe_fde_func_info to this.
---
 bfd/elfxx-x86.c                                 | 6 +++---
 include/sframe-api.h                            | 4 ++--
 libsframe/sframe.c                              | 6 +++---
 libsframe/testsuite/libsframe.encode/encode-1.c | 8 ++++----
 4 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/bfd/elfxx-x86.c b/bfd/elfxx-x86.c
index bbf868826e5..88c34d397a8 100644
--- a/bfd/elfxx-x86.c
+++ b/bfd/elfxx-x86.c
@@ -1857,8 +1857,7 @@ _bfd_x86_elf_create_sframe_plt (bfd *output_bfd,
 
   /* FRE type is dependent on the size of the function.  */
   fre_type = sframe_calc_fre_type (dpltsec->size);
-  func_info = sframe_fde_func_info (fre_type,
-				    SFRAME_FDE_TYPE_PCINC);
+  func_info = sframe_fde_create_func_info (fre_type, SFRAME_FDE_TYPE_PCINC);
 
   /* Add SFrame FDE and the associated FREs for plt0 if plt0 has been
      generated.  */
@@ -1888,7 +1887,8 @@ _bfd_x86_elf_create_sframe_plt (bfd *output_bfd,
 	 pattern of the instructions in these entries.  Using this SFrame FDE
 	 type helps in keeping the unwind information for pltn entries
 	 compact.  */
-      func_info	= sframe_fde_func_info (fre_type, SFRAME_FDE_TYPE_PCMASK);
+      func_info	= sframe_fde_create_func_info (fre_type,
+					       SFRAME_FDE_TYPE_PCMASK);
       /* Add the SFrame FDE for all PCs starting at the first pltn entry (hence,
 	 function start address = plt0_entry_size.  As usual, this will be
 	 updated later at _bfd_elf_merge_section_sframe, by when the
diff --git a/include/sframe-api.h b/include/sframe-api.h
index c658474253f..0a86389857c 100644
--- a/include/sframe-api.h
+++ b/include/sframe-api.h
@@ -83,10 +83,10 @@ _SFRAME_ERRORS
 extern const char *
 sframe_errmsg (int error);
 
-/* Get FDE function info given a FRE_TYPE.  */
+/* Create an FDE function info bye given an FRE_TYPE and an FDE_TYPE.  */
 
 extern unsigned char
-sframe_fde_func_info (unsigned int fre_type, unsigned int fde_type);
+sframe_fde_create_func_info (unsigned int fre_type, unsigned int fde_type);
 
 /* Gather the FRE type given the function size.  */
 
diff --git a/libsframe/sframe.c b/libsframe/sframe.c
index 64fa9078d62..d4eaaee2297 100644
--- a/libsframe/sframe.c
+++ b/libsframe/sframe.c
@@ -548,12 +548,12 @@ sframe_decoder_free (sframe_decoder_ctx **decoder)
     }
 }
 
-/* Create a FDE function info byte given an FRE_TYPE and an FDE_TYPE.  */
+/* Create an FDE function info byte given an FRE_TYPE and an FDE_TYPE.  */
 /* FIXME API for linker.  Revisit if its better placed somewhere else?  */
 
 unsigned char
-sframe_fde_func_info (unsigned int fre_type,
-		      unsigned int fde_type)
+sframe_fde_create_func_info (unsigned int fre_type,
+			     unsigned int fde_type)
 {
   unsigned char func_info;
   sframe_assert (fre_type == SFRAME_FRE_TYPE_ADDR1
diff --git a/libsframe/testsuite/libsframe.encode/encode-1.c b/libsframe/testsuite/libsframe.encode/encode-1.c
index 4075591ffa1..01481106a62 100644
--- a/libsframe/testsuite/libsframe.encode/encode-1.c
+++ b/libsframe/testsuite/libsframe.encode/encode-1.c
@@ -39,8 +39,8 @@ add_fde1 (sframe_encoder_ctx *encode, int idx)
 	{0x1a, 0x5, {0x8, 0xf0, 0}}
       };
 
-  unsigned char finfo = sframe_fde_func_info (SFRAME_FRE_TYPE_ADDR1,
-					      SFRAME_FDE_TYPE_PCINC);
+  unsigned char finfo = sframe_fde_create_func_info (SFRAME_FRE_TYPE_ADDR1,
+						     SFRAME_FDE_TYPE_PCINC);
   err = sframe_encoder_add_funcdesc (encode, 0xfffff03e, 0x1b, finfo, 4);
   if (err == -1)
     return err;
@@ -64,8 +64,8 @@ add_fde2 (sframe_encoder_ctx *encode, int idx)
 	{0xf, 0x5, {0x8, 0xf0, 0}}
       };
 
-  unsigned char finfo = sframe_fde_func_info (SFRAME_FRE_TYPE_ADDR1,
-					      SFRAME_FDE_TYPE_PCINC);
+  unsigned char finfo = sframe_fde_create_func_info (SFRAME_FRE_TYPE_ADDR1,
+						     SFRAME_FDE_TYPE_PCINC);
   err = sframe_encoder_add_funcdesc (encode, 0xfffff059, 0x10, finfo, 4);
   if (err == -1)
     return err;
-- 
2.37.2

Re: [PATCH 5/6] libsframe: rename API sframe_fde_func_info to sframe_fde_create_func_info

[Nick Clifton]

Hi Indu,

> ChangeLog:
> 
> 	* bfd/elfxx-x86.c (_bfd_x86_elf_create_sframe_plt): Use new
> 	name.
> 	* libsframe/sframe.c (sframe_fde_create_func_info): Rename
> 	sframe_fde_func_info to this.
> 	* libsframe/testsuite/libsframe.encode/encode-1.c: Use new name.
> 
> include/ChangeLog:
> 
> 	* sframe-api.h (sframe_fde_create_func_info): Rename
> 	sframe_fde_func_info to this.

Approved - please apply.

Cheers
   Nick

[PATCH 6/6] objdump: sframe: fix memory leaks

[Indu Bhagat]

ChangeLog:

	* binutils/objdump.c (dump_section_sframe): free up contents and
	SFrame decoder context on exit.
---
 binutils/objdump.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/binutils/objdump.c b/binutils/objdump.c
index d95c8b68bf0..6695c5d343f 100644
--- a/binutils/objdump.c
+++ b/binutils/objdump.c
@@ -4871,12 +4871,18 @@ dump_section_sframe (bfd *abfd ATTRIBUTE_UNUSED,
   /* Decode the contents of the section.  */
   sfd_ctx = sframe_decode ((const char*)sframe_data, sf_size, &err);
   if (!sfd_ctx)
-    bfd_fatal (bfd_get_filename (abfd));
+    {
+      free (sframe_data);
+      bfd_fatal (bfd_get_filename (abfd));
+    }
 
   printf (_("Contents of the SFrame section %s:"),
 	  sanitize_string (sect_name));
   /* Dump the contents as text.  */
   dump_sframe (sfd_ctx, sf_vma);
+
+  free (sframe_data);
+  sframe_decoder_free (&sfd_ctx);
 }
 
 \f
-- 
2.37.2

Re: [PATCH 6/6] objdump: sframe: fix memory leaks

[Nick Clifton]

Hi Indu,

> ChangeLog:
> 
> 	* binutils/objdump.c (dump_section_sframe): free up contents and
> 	SFrame decoder context on exit.

Approved - please apply.

Cheers
   Nick