* [PATCH 1/6] libsframe: minor formatting nits
2022-12-07 19:52 [PATCH 0/6] Small improvements around SFrame support Indu Bhagat
@ 2022-12-07 19:52 ` Indu Bhagat
2022-12-08 11:03 ` Nick Clifton
2022-12-07 19:52 ` [PATCH 2/6] sframe.h: make some macros more precise Indu Bhagat
` (4 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Indu Bhagat @ 2022-12-07 19:52 UTC (permalink / raw)
To: binutils; +Cc: weimin.pan, 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/6] sframe.h: make some macros more precise
2022-12-07 19:52 [PATCH 0/6] Small improvements around SFrame support Indu Bhagat
2022-12-07 19:52 ` [PATCH 1/6] libsframe: minor formatting nits Indu Bhagat
@ 2022-12-07 19:52 ` Indu Bhagat
2022-12-07 23:52 ` Hans-Peter Nilsson
2022-12-07 19:52 ` [PATCH 3/6] sframe: gas: libsframe: define constants and remove magic numbers Indu Bhagat
` (3 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Indu Bhagat @ 2022-12-07 19:52 UTC (permalink / raw)
To: binutils; +Cc: weimin.pan, 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/6] sframe.h: make some macros more precise
2022-12-07 19:52 ` [PATCH 2/6] sframe.h: make some macros more precise Indu Bhagat
@ 2022-12-07 23:52 ` Hans-Peter Nilsson
2022-12-08 17:36 ` Indu Bhagat
2022-12-08 20:26 ` [PATCH,V2 " Indu Bhagat
0 siblings, 2 replies; 18+ messages in thread
From: Hans-Peter Nilsson @ 2022-12-07 23:52 UTC (permalink / raw)
To: Indu Bhagat; +Cc: binutils, weimin.pan
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/6] sframe.h: make some macros more precise
2022-12-07 23:52 ` Hans-Peter Nilsson
@ 2022-12-08 17:36 ` Indu Bhagat
2022-12-08 20:26 ` [PATCH,V2 " Indu Bhagat
1 sibling, 0 replies; 18+ messages in thread
From: Indu Bhagat @ 2022-12-08 17:36 UTC (permalink / raw)
To: Hans-Peter Nilsson; +Cc: binutils, weimin.pan
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH,V2 2/6] sframe.h: make some macros more precise
2022-12-07 23:52 ` Hans-Peter Nilsson
2022-12-08 17:36 ` Indu Bhagat
@ 2022-12-08 20:26 ` Indu Bhagat
2022-12-12 16:02 ` Nick Clifton
1 sibling, 1 reply; 18+ messages in thread
From: Indu Bhagat @ 2022-12-08 20:26 UTC (permalink / raw)
To: binutils; +Cc: weimin.pan, hp, 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/6] sframe: gas: libsframe: define constants and remove magic numbers
2022-12-07 19:52 [PATCH 0/6] Small improvements around SFrame support Indu Bhagat
2022-12-07 19:52 ` [PATCH 1/6] libsframe: minor formatting nits Indu Bhagat
2022-12-07 19:52 ` [PATCH 2/6] sframe.h: make some macros more precise Indu Bhagat
@ 2022-12-07 19:52 ` Indu Bhagat
2022-12-08 11:10 ` Nick Clifton
2022-12-07 19:52 ` [PATCH 4/6] gas: sframe: fine tune the fragment fixup for SFrame func info Indu Bhagat
` (2 subsequent siblings)
5 siblings, 1 reply; 18+ messages in thread
From: Indu Bhagat @ 2022-12-07 19:52 UTC (permalink / raw)
To: binutils; +Cc: weimin.pan, 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/6] sframe: gas: libsframe: define constants and remove magic numbers
2022-12-07 19:52 ` [PATCH 3/6] sframe: gas: libsframe: define constants and remove magic numbers Indu Bhagat
@ 2022-12-08 11:10 ` Nick Clifton
2022-12-08 17:43 ` Indu Bhagat
0 siblings, 1 reply; 18+ messages in thread
From: Nick Clifton @ 2022-12-08 11:10 UTC (permalink / raw)
To: Indu Bhagat, binutils; +Cc: weimin.pan
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/6] sframe: gas: libsframe: define constants and remove magic numbers
2022-12-08 11:10 ` Nick Clifton
@ 2022-12-08 17:43 ` Indu Bhagat
2022-12-08 18:38 ` Indu Bhagat
0 siblings, 1 reply; 18+ messages in thread
From: Indu Bhagat @ 2022-12-08 17:43 UTC (permalink / raw)
To: Nick Clifton, binutils; +Cc: weimin.pan
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/6] sframe: gas: libsframe: define constants and remove magic numbers
2022-12-08 17:43 ` Indu Bhagat
@ 2022-12-08 18:38 ` Indu Bhagat
0 siblings, 0 replies; 18+ messages in thread
From: Indu Bhagat @ 2022-12-08 18:38 UTC (permalink / raw)
To: Nick Clifton, binutils; +Cc: weimin.pan
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/6] gas: sframe: fine tune the fragment fixup for SFrame func info
2022-12-07 19:52 [PATCH 0/6] Small improvements around SFrame support Indu Bhagat
` (2 preceding siblings ...)
2022-12-07 19:52 ` [PATCH 3/6] sframe: gas: libsframe: define constants and remove magic numbers Indu Bhagat
@ 2022-12-07 19:52 ` Indu Bhagat
2022-12-08 11:12 ` Nick Clifton
2022-12-07 19:52 ` [PATCH 5/6] libsframe: rename API sframe_fde_func_info to sframe_fde_create_func_info Indu Bhagat
2022-12-07 19:52 ` [PATCH 6/6] objdump: sframe: fix memory leaks Indu Bhagat
5 siblings, 1 reply; 18+ messages in thread
From: Indu Bhagat @ 2022-12-07 19:52 UTC (permalink / raw)
To: binutils; +Cc: weimin.pan, 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 5/6] libsframe: rename API sframe_fde_func_info to sframe_fde_create_func_info
2022-12-07 19:52 [PATCH 0/6] Small improvements around SFrame support Indu Bhagat
` (3 preceding siblings ...)
2022-12-07 19:52 ` [PATCH 4/6] gas: sframe: fine tune the fragment fixup for SFrame func info Indu Bhagat
@ 2022-12-07 19:52 ` Indu Bhagat
2022-12-08 11:13 ` Nick Clifton
2022-12-07 19:52 ` [PATCH 6/6] objdump: sframe: fix memory leaks Indu Bhagat
5 siblings, 1 reply; 18+ messages in thread
From: Indu Bhagat @ 2022-12-07 19:52 UTC (permalink / raw)
To: binutils; +Cc: weimin.pan, 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 6/6] objdump: sframe: fix memory leaks
2022-12-07 19:52 [PATCH 0/6] Small improvements around SFrame support Indu Bhagat
` (4 preceding siblings ...)
2022-12-07 19:52 ` [PATCH 5/6] libsframe: rename API sframe_fde_func_info to sframe_fde_create_func_info Indu Bhagat
@ 2022-12-07 19:52 ` Indu Bhagat
2022-12-08 11:14 ` Nick Clifton
5 siblings, 1 reply; 18+ messages in thread
From: Indu Bhagat @ 2022-12-07 19:52 UTC (permalink / raw)
To: binutils; +Cc: weimin.pan, 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
^ permalink raw reply [flat|nested] 18+ messages in thread