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

[Jens Remus <jremus@linux.ibm.com>]

Am 18.04.2024 um 09:39 schrieb Indu Bhagat:
> 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.

I wrongly assumed them to be compound adjectives [1], which usually 
require a dash, when used in conjunction with e.g. "register". That is 
why I differentiated between e.g. "stack pointer" (noun) and 
"stack-pointer register" (falsely assumed "stack-pointer" to be a 
compound adjective). I just learned that they are compound nouns [2] and 
thus are always written without dash.

[1] https://www.grammarly.com/blog/compound-adjectives/
[2] https://www.grammarly.com/blog/compound-nouns/

> 
> 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.

Done.

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

Regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303) and z/VSE Support
+49-7031-16-1128 Office
jremus@de.ibm.com

IBM

IBM Deutschland Research & Development GmbH; Vorsitzender des 
Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der 
Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/