Re: [PATCH v3 02/15] gas: Enhance arch-specific SFrame configuration descriptions

[Indu Bhagat <indu.bhagat@oracle.com>]

On 4/12/24 07:47, Jens Remus wrote:
> Explicitly mention "SFrame" in the descriptions for the architecture-
> specific SFrame configuration macros, variables, and functions.
> 
> Use the term "frame pointer" (FP) instead of "base pointer". This aligns
> with the terminology used in the SFrame specification. Additionally it
> helps not to confuse "base-pointer register" with the term "BASE_REG"
> used in the specification to denote either the SP or FP register.
> 
> 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
> 
> Align the descriptions for definitions in the source files to the
> declarations in the header files.
> 
> gas/
> 	* config/tc-aarch64.h: Enhance architecture-specific SFrame
> 	configuration descriptions.
> 	* config/tc-aarch64.c: Likewise.
> 	* config/tc-i386.h: Likewise.
> 	* config/tc-i386.c: Likewise.
> 
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - Add "SFrame" to architecture-specific SFrame macro, variable, and
>        function descriptions as suggested by Indu. Do so for all and not
>        only those previously touched.
>      - Reword further SFrame macro, variable, and function descriptions
>        to align with those previously touched.
>      - Align description of definition in source with declaration in header.
>      - Corrected formatting of ChangeLog in commit message.
>      - Changed commit subject prefix from "sframe" to "gas".
> 
>   gas/config/tc-aarch64.c |  6 +++---
>   gas/config/tc-aarch64.h | 12 ++++++------
>   gas/config/tc-i386.c    |  5 +++++
>   gas/config/tc-i386.h    | 10 +++++-----
>   4 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/gas/config/tc-aarch64.c b/gas/config/tc-aarch64.c
> index 6ad4fae8b0ec..077cbd485979 100644
> --- a/gas/config/tc-aarch64.c
> +++ b/gas/config/tc-aarch64.c
> @@ -8870,7 +8870,7 @@ aarch64_support_sframe_p (void)
>     return (aarch64_abi == AARCH64_ABI_LP64);
>   }
>   
> -/* Specify if RA tracking is needed.  */
> +/* Whether SFrame return-address tracking is needed.  */
>   

Nit.  But then given the nature of this commit: I see that this commit 
uses both "return-address" and "return address" keywords.

Also, the code already was already using "stack-pointer" and 
"frame-pointer" (my bad), lets use the same style, and use
  - "frame pointer"
  - "stack pointer"
  - "return address"

OK with that change.

Thanks for your patch.

>   bool
>   aarch64_sframe_ra_tracking_p (void)
> @@ -8878,8 +8878,8 @@ aarch64_sframe_ra_tracking_p (void)
>     return true;
>   }
>   
> -/* Specify the fixed offset to recover RA from CFA.
> -   (useful only when RA tracking is not needed).  */
> +/* The fixed offset from CFA for SFrame to recover the return address.
> +   (useful only when SFrame RA tracking is not needed).  */
>   
>   offsetT
>   aarch64_sframe_cfa_ra_offset (void)
> diff --git a/gas/config/tc-aarch64.h b/gas/config/tc-aarch64.h
> index 1b8badad9fdc..63acbd46a379 100644
> --- a/gas/config/tc-aarch64.h
> +++ b/gas/config/tc-aarch64.h
> @@ -267,24 +267,24 @@ 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 DWARF register number for SFrame CFA tracking.  */
>   extern unsigned int aarch64_sframe_cfa_sp_reg;
>   #define SFRAME_CFA_SP_REG aarch64_sframe_cfa_sp_reg
>   
> -/* The base-pointer register number for CFA stack trace info.  */
> +/* The frame-pointer DWARF register number for SFrame 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 CFA stack trace info.  */
> +/* The return-address DWARF register number for SFrame RA tracking.  */
>   extern unsigned int aarch64_sframe_cfa_ra_reg;
>   #define SFRAME_CFA_RA_REG aarch64_sframe_cfa_ra_reg
>   
> -/* Specify if RA tracking is needed.  */
> +/* Whether SFrame return-address tracking is needed.  */
>   extern bool aarch64_sframe_ra_tracking_p (void);
>   #define sframe_ra_tracking_p aarch64_sframe_ra_tracking_p
>   
> -/* Specify the fixed offset to recover RA from CFA.
> -   (useful only when RA tracking is not needed).  */
> +/* The fixed offset from CFA for SFrame to recover the return address.
> +   (useful only when SFrame RA tracking is not needed).  */
>   extern offsetT aarch64_sframe_cfa_ra_offset (void);
>   #define sframe_cfa_ra_offset aarch64_sframe_cfa_ra_offset
>   
> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> index cd20e0c9415e..08a8eb2aeb69 100644
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -11181,6 +11181,7 @@ x86_cleanup (void)
>       subseg_set (seg, subseg);
>   }
>   
> +/* Whether SFrame stack trace info is supported.  */
>   bool
>   x86_support_sframe_p (void)
>   {
> @@ -11188,6 +11189,7 @@ x86_support_sframe_p (void)
>     return (x86_elf_abi == X86_64_ABI);
>   }
>   
> +/* Whether SFrame return-address tracking is needed.  */
>   bool
>   x86_sframe_ra_tracking_p (void)
>   {
> @@ -11197,6 +11199,8 @@ x86_sframe_ra_tracking_p (void)
>     return false;
>   }
>   
> +/* The fixed offset from CFA for SFrame to recover the return address.
> +   (useful only when SFrame RA tracking is not needed).  */
>   offsetT
>   x86_sframe_cfa_ra_offset (void)
>   {
> @@ -11204,6 +11208,7 @@ x86_sframe_cfa_ra_offset (void)
>     return (offsetT) -8;
>   }
>   
> +/* The abi/arch indentifier for SFrame.  */
>   unsigned char
>   x86_sframe_get_abi_arch (void)
>   {
> diff --git a/gas/config/tc-i386.h b/gas/config/tc-i386.h
> index 7aae7a33dc14..e98362111698 100644
> --- a/gas/config/tc-i386.h
> +++ b/gas/config/tc-i386.h
> @@ -441,20 +441,20 @@ 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 DWARF register number for SFrame 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 DWARF register number for SFrame CFA and FP tracking.  */
>   extern unsigned int x86_sframe_cfa_fp_reg;
>   #define SFRAME_CFA_FP_REG x86_sframe_cfa_fp_reg
>   
> -/* Specify if RA tracking is needed.  */
> +/* Whether SFrame return-address tracking is needed.  */
>   extern bool x86_sframe_ra_tracking_p (void);
>   #define sframe_ra_tracking_p x86_sframe_ra_tracking_p
>   
> -/* Specify the fixed offset to recover RA from CFA.
> -   (useful only when RA tracking is not needed).  */
> +/* The fixed offset from CFA for SFrame to recover the return address.
> +   (useful only when SFrame RA tracking is not needed).  */
>   extern offsetT x86_sframe_cfa_ra_offset (void);
>   #define sframe_cfa_ra_offset x86_sframe_cfa_ra_offset
>