diff mbox series

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

Message ID 20221207195222.1182788-3-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 
  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(-)

Comments

Hans-Peter Nilsson Dec. 7, 2022, 11:52 p.m. UTC | #1 
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
Indu Bhagat Dec. 8, 2022, 5:36 p.m. UTC | #2 
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
diff mbox series

Patch

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)