diff mbox series

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

Message ID 20221207195222.1182788-4-indu.bhagat@oracle.com
State Accepted
Headers
Series Small improvements around SFrame support |

Checks

Context Check Description
snail/binutils-gdb-check success Github commit url

Commit Message

Indu Bhagat Dec. 7, 2022, 7:52 p.m. UTC 
  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(-)

Comments

Nick Clifton Dec. 8, 2022, 11:10 a.m. UTC | #1 
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.
Indu Bhagat Dec. 8, 2022, 5:43 p.m. UTC | #2 
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
Indu Bhagat Dec. 8, 2022, 6:38 p.m. UTC | #3 
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.
diff mbox series

Patch

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;
 }