diff mbox series

[v2,3/9] sframe: Enhance comments for SFRAME_CFA_*_REG macros

Message ID 20240223170800.3993092-4-jremus@linux.ibm.com
State Accepted
Headers
Series s390: Initial support to generate SFrame info from CFI directives in assembler |

Checks

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

Commit Message

Jens Remus Feb. 23, 2024, 5:07 p.m. UTC 
  Specify what the SFRAME_CFA_*_REG register numbers are used for:
- SP (stack pointer): CFA tracking
- FP (frame pointer): CFA and FP tracking
- RA (return address): RA tracking

gas/
	* config/tc-aarch64.h: Enhance comments for SFRAME_CFA_*_REG
	  macros.
	* config/tc-i386.h: Likewise.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
 gas/config/tc-aarch64.h | 6 +++---
 gas/config/tc-i386.h    | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

Comments

Indu Bhagat Feb. 24, 2024, 7:46 a.m. UTC | #1 
On 2/23/24 09:07, Jens Remus wrote:
> Specify what the SFRAME_CFA_*_REG register numbers are used for:
> - SP (stack pointer): CFA tracking
> - FP (frame pointer): CFA and FP tracking
> - RA (return address): RA tracking
> 
> gas/
> 	* config/tc-aarch64.h: Enhance comments for SFRAME_CFA_*_REG
> 	  macros.
> 	* config/tc-i386.h: Likewise.
> 
> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
>   gas/config/tc-aarch64.h | 6 +++---
>   gas/config/tc-i386.h    | 4 ++--
>   2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
> index 599d78db7908..59c9b5a09ec0 100644
> --- a/gas/config/tc-aarch64.h
> +++ b/gas/config/tc-aarch64.h
> @@ -267,15 +267,15 @@ extern void aarch64_after_parse_args (void);
>   extern bool aarch64_support_sframe_p (void);
>   #define support_sframe_p aarch64_support_sframe_p
>   
> -/* The stack-pointer register number for SFrame stack trace info.  */
> +/* The stack-pointer register number for CFA tracking.  */

What do you think about including "SFrame" in all the touched comments 
in this patch.  So something like:

/* The stack-pointer register number for SFrame CFA tracking.  */

above ...

>   extern unsigned int aarch64_sframe_cfa_sp_reg;
>   #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
>   
> -/* The frame-pointer register number for SFrame stack trace info.  */
> +/* The frame-pointer register number for CFA and FP tracking.  */

... and here

>   extern unsigned int aarch64_sframe_cfa_fp_reg;
>   #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
>   
> -/* The return address register number for SFrame stack trace info.  */
> +/* The return address register number for RA tracking.  */

and here. And others below :)

Thanks

>   extern unsigned int aarch64_sframe_cfa_ra_reg;
>   #define SFRAME_CFA_RA_REG aarch64_sframe_cfa_ra_reg
>   
> diff --git a/gas/config/tc-i386.h b/gas/config/tc-i386.h
> index b93799a3b48c..0d6fb002166f 100644
> --- a/gas/config/tc-i386.h
> +++ b/gas/config/tc-i386.h
> @@ -440,11 +440,11 @@ extern bool x86_scfi_callee_saved_p (uint32_t dw2reg_num);
>   extern bool x86_support_sframe_p (void);
>   #define support_sframe_p x86_support_sframe_p
>   
> -/* The stack-pointer register number for SFrame stack trace info.  */
> +/* The stack-pointer register number for CFA tracking.  */
>   extern unsigned int x86_sframe_cfa_sp_reg;
>   #define SFRAME_CFA_SP_REG x86_sframe_cfa_sp_reg
>   
> -/* The frame-pointer register number for SFrame stack trace info.  */
> +/* The frame-pointer register number for CFA and FP tracking.  */
>   extern unsigned int x86_sframe_cfa_fp_reg;
>   #define SFRAME_CFA_FP_REG x86_sframe_cfa_fp_reg
>
Jan Beulich Feb. 26, 2024, 1:51 p.m. UTC | #2 
On 24.02.2024 08:46, Indu Bhagat wrote:
> On 2/23/24 09:07, Jens Remus wrote:
>> --- a/gas/config/tc-aarch64.h
>> +++ b/gas/config/tc-aarch64.h
>> @@ -267,15 +267,15 @@ extern void aarch64_after_parse_args (void);
>>   extern bool aarch64_support_sframe_p (void);
>>   #define support_sframe_p aarch64_support_sframe_p
>>   
>> -/* The stack-pointer register number for SFrame stack trace info.  */
>> +/* The stack-pointer register number for CFA tracking.  */
> 
> What do you think about including "SFrame" in all the touched comments 
> in this patch.  So something like:
> 
> /* The stack-pointer register number for SFrame CFA tracking.  */
> 
> above ...
> 
>>   extern unsigned int aarch64_sframe_cfa_sp_reg;
>>   #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
>>   
>> -/* The frame-pointer register number for SFrame stack trace info.  */
>> +/* The frame-pointer register number for CFA and FP tracking.  */
> 
> ... and here
> 
>>   extern unsigned int aarch64_sframe_cfa_fp_reg;
>>   #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
>>   
>> -/* The return address register number for SFrame stack trace info.  */
>> +/* The return address register number for RA tracking.  */
> 
> and here. And others below :)

Question is: In how far are these variables sframe-specific? On x86 at
least they exactly match the Dwarf2 register numbers, and hence are
in principle pretty generic as to potential future usage. In which
case rather than adding SFrame to the comments, I'd wonder in how far
"sframe" may want purging from their names instead.

In fact on x86 I think these could be #define-s instead of variables,
at least as long as only 64-bit code is supported for anything using
these values.

Jan
Indu Bhagat Feb. 27, 2024, 8:53 a.m. UTC | #3 
On 2/26/24 05:51, Jan Beulich wrote:
> On 24.02.2024 08:46, Indu Bhagat wrote:
>> On 2/23/24 09:07, Jens Remus wrote:
>>> --- a/gas/config/tc-aarch64.h
>>> +++ b/gas/config/tc-aarch64.h
>>> @@ -267,15 +267,15 @@ extern void aarch64_after_parse_args (void);
>>>    extern bool aarch64_support_sframe_p (void);
>>>    #define support_sframe_p aarch64_support_sframe_p
>>>    
>>> -/* The stack-pointer register number for SFrame stack trace info.  */
>>> +/* The stack-pointer register number for CFA tracking.  */
>>
>> What do you think about including "SFrame" in all the touched comments
>> in this patch.  So something like:
>>
>> /* The stack-pointer register number for SFrame CFA tracking.  */
>>
>> above ...
>>
>>>    extern unsigned int aarch64_sframe_cfa_sp_reg;
>>>    #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
>>>    
>>> -/* The frame-pointer register number for SFrame stack trace info.  */
>>> +/* The frame-pointer register number for CFA and FP tracking.  */
>>
>> ... and here
>>
>>>    extern unsigned int aarch64_sframe_cfa_fp_reg;
>>>    #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
>>>    
>>> -/* The return address register number for SFrame stack trace info.  */
>>> +/* The return address register number for RA tracking.  */
>>
>> and here. And others below :)
> 
> Question is: In how far are these variables sframe-specific? On x86 at
> least they exactly match the Dwarf2 register numbers, and hence are
> in principle pretty generic as to potential future usage. In which
> case rather than adding SFrame to the comments, I'd wonder in how far
> "sframe" may want purging from their names instead.
> 

So far, the names contained "sframe" solely because that was the only 
user of these definitions.

> In fact on x86 I think these could be #define-s instead of variables,
> at least as long as only 64-bit code is supported for anything using
> these values.
> 

Makes sense.  Now that we have some code in SCFI and elsewhere in x86 
using these DWARF numbers for SP/FP, we can revisit the names etc once 
Jan's patch that defines them for x86 goes in.

Indu
Jan Beulich Feb. 27, 2024, 8:57 a.m. UTC | #4 
On 27.02.2024 09:53, Indu Bhagat wrote:
> On 2/26/24 05:51, Jan Beulich wrote:
>> On 24.02.2024 08:46, Indu Bhagat wrote:
>>> On 2/23/24 09:07, Jens Remus wrote:
>>>> --- a/gas/config/tc-aarch64.h
>>>> +++ b/gas/config/tc-aarch64.h
>>>> @@ -267,15 +267,15 @@ extern void aarch64_after_parse_args (void);
>>>>    extern bool aarch64_support_sframe_p (void);
>>>>    #define support_sframe_p aarch64_support_sframe_p
>>>>    
>>>> -/* The stack-pointer register number for SFrame stack trace info.  */
>>>> +/* The stack-pointer register number for CFA tracking.  */
>>>
>>> What do you think about including "SFrame" in all the touched comments
>>> in this patch.  So something like:
>>>
>>> /* The stack-pointer register number for SFrame CFA tracking.  */
>>>
>>> above ...
>>>
>>>>    extern unsigned int aarch64_sframe_cfa_sp_reg;
>>>>    #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
>>>>    
>>>> -/* The frame-pointer register number for SFrame stack trace info.  */
>>>> +/* The frame-pointer register number for CFA and FP tracking.  */
>>>
>>> ... and here
>>>
>>>>    extern unsigned int aarch64_sframe_cfa_fp_reg;
>>>>    #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
>>>>    
>>>> -/* The return address register number for SFrame stack trace info.  */
>>>> +/* The return address register number for RA tracking.  */
>>>
>>> and here. And others below :)
>>
>> Question is: In how far are these variables sframe-specific? On x86 at
>> least they exactly match the Dwarf2 register numbers, and hence are
>> in principle pretty generic as to potential future usage. In which
>> case rather than adding SFrame to the comments, I'd wonder in how far
>> "sframe" may want purging from their names instead.
>>
> 
> So far, the names contained "sframe" solely because that was the only 
> user of these definitions.
> 
>> In fact on x86 I think these could be #define-s instead of variables,
>> at least as long as only 64-bit code is supported for anything using
>> these values.
>>
> 
> Makes sense.  Now that we have some code in SCFI and elsewhere in x86 
> using these DWARF numbers for SP/FP, we can revisit the names etc once 
> Jan's patch that defines them for x86 goes in.

I'm confused: Which my patch?

Jan
Indu Bhagat Feb. 27, 2024, 9:01 a.m. UTC | #5 
On 2/27/24 00:57, Jan Beulich wrote:
> On 27.02.2024 09:53, Indu Bhagat wrote:
>> On 2/26/24 05:51, Jan Beulich wrote:
>>> On 24.02.2024 08:46, Indu Bhagat wrote:
>>>> On 2/23/24 09:07, Jens Remus wrote:
>>>>> --- a/gas/config/tc-aarch64.h
>>>>> +++ b/gas/config/tc-aarch64.h
>>>>> @@ -267,15 +267,15 @@ extern void aarch64_after_parse_args (void);
>>>>>     extern bool aarch64_support_sframe_p (void);
>>>>>     #define support_sframe_p aarch64_support_sframe_p
>>>>>     
>>>>> -/* The stack-pointer register number for SFrame stack trace info.  */
>>>>> +/* The stack-pointer register number for CFA tracking.  */
>>>>
>>>> What do you think about including "SFrame" in all the touched comments
>>>> in this patch.  So something like:
>>>>
>>>> /* The stack-pointer register number for SFrame CFA tracking.  */
>>>>
>>>> above ...
>>>>
>>>>>     extern unsigned int aarch64_sframe_cfa_sp_reg;
>>>>>     #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
>>>>>     
>>>>> -/* The frame-pointer register number for SFrame stack trace info.  */
>>>>> +/* The frame-pointer register number for CFA and FP tracking.  */
>>>>
>>>> ... and here
>>>>
>>>>>     extern unsigned int aarch64_sframe_cfa_fp_reg;
>>>>>     #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
>>>>>     
>>>>> -/* The return address register number for SFrame stack trace info.  */
>>>>> +/* The return address register number for RA tracking.  */
>>>>
>>>> and here. And others below :)
>>>
>>> Question is: In how far are these variables sframe-specific? On x86 at
>>> least they exactly match the Dwarf2 register numbers, and hence are
>>> in principle pretty generic as to potential future usage. In which
>>> case rather than adding SFrame to the comments, I'd wonder in how far
>>> "sframe" may want purging from their names instead.
>>>
>>
>> So far, the names contained "sframe" solely because that was the only
>> user of these definitions.
>>
>>> In fact on x86 I think these could be #define-s instead of variables,
>>> at least as long as only 64-bit code is supported for anything using
>>> these values.
>>>
>>
>> Makes sense.  Now that we have some code in SCFI and elsewhere in x86
>> using these DWARF numbers for SP/FP, we can revisit the names etc once
>> Jan's patch that defines them for x86 goes in.
> 
> I'm confused: Which my patch?
> 

Sorry, I was thinking about "[PATCH v3] x86: adjust which Dwarf2 
register numbers to use" 
https://sourceware.org/pipermail/binutils/2024-February/132623.html, and 
mistakenly thought your patch defines the REG_FP/REG_SP afresh.
diff mbox series

Patch

diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
index 599d78db7908..59c9b5a09ec0 100644
--- a/gas/config/tc-aarch64.h
+++ b/gas/config/tc-aarch64.h
@@ -267,15 +267,15 @@  extern void aarch64_after_parse_args (void);
 extern bool aarch64_support_sframe_p (void);
 #define support_sframe_p aarch64_support_sframe_p
 
-/* The stack-pointer register number for SFrame stack trace info.  */
+/* The stack-pointer register number for CFA tracking.  */
 extern unsigned int aarch64_sframe_cfa_sp_reg;
 #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
 
-/* The frame-pointer register number for SFrame stack trace info.  */
+/* The frame-pointer register number for CFA and FP tracking.  */
 extern unsigned int aarch64_sframe_cfa_fp_reg;
 #define SFRAME_CFA_FP_REG aarch64_sframe_cfa_fp_reg
 
-/* The return address register number for SFrame stack trace info.  */
+/* The return address register number for RA tracking.  */
 extern unsigned int aarch64_sframe_cfa_ra_reg;
 #define SFRAME_CFA_RA_REG aarch64_sframe_cfa_ra_reg
 
diff --git a/gas/config/tc-i386.h b/gas/config/tc-i386.h
index b93799a3b48c..0d6fb002166f 100644
--- a/gas/config/tc-i386.h
+++ b/gas/config/tc-i386.h
@@ -440,11 +440,11 @@  extern bool x86_scfi_callee_saved_p (uint32_t dw2reg_num);
 extern bool x86_support_sframe_p (void);
 #define support_sframe_p x86_support_sframe_p
 
-/* The stack-pointer register number for SFrame stack trace info.  */
+/* The stack-pointer register number for CFA tracking.  */
 extern unsigned int x86_sframe_cfa_sp_reg;
 #define SFRAME_CFA_SP_REG x86_sframe_cfa_sp_reg
 
-/* The frame-pointer register number for SFrame stack trace info.  */
+/* The frame-pointer register number for CFA and FP tracking.  */
 extern unsigned int x86_sframe_cfa_fp_reg;
 #define SFRAME_CFA_FP_REG x86_sframe_cfa_fp_reg