diff mbox series

[v2,1/9] x86: Remove unused SFrame CFI RA register variable

Message ID 20240223170800.3993092-2-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 
  gas/
	* config/tc-i386.c: Remove unused SFrame CFI RA register
	  variable.

Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---
 gas/config/tc-i386.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Indu Bhagat Feb. 24, 2024, 7:38 a.m. UTC | #1 
On 2/23/24 09:07, Jens Remus wrote:
> gas/
> 	* config/tc-i386.c: Remove unused SFrame CFI RA register
> 	  variable.
> 

The ChangeLog formatting is a bit off here and in the rest of the 
patches in this series.

The indentation of each line should be such that all content lines up 
below the *, like so:

<TAB>* xyz.c: first line content
<TAB>next line content

Other than that, LGTM.

Thanks
Indu

> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
>   gas/config/tc-i386.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
> index c56ca4a2b4b8..1a6fd52f229a 100644
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -627,7 +627,6 @@ static int shared = 0;
>   unsigned int x86_sframe_cfa_sp_reg;
>   /* The other CFA base register for SFrame stack trace info.  */
>   unsigned int x86_sframe_cfa_fp_reg;
> -unsigned int x86_sframe_cfa_ra_reg;
>   
>   #endif
>
Jan Beulich Feb. 26, 2024, 1:01 p.m. UTC | #2 
On 24.02.2024 08:38, Indu Bhagat wrote:
> On 2/23/24 09:07, Jens Remus wrote:
>> gas/
>> 	* config/tc-i386.c: Remove unused SFrame CFI RA register
>> 	  variable.
>>
> 
> The ChangeLog formatting is a bit off here and in the rest of the 
> patches in this series.
> 
> The indentation of each line should be such that all content lines up 
> below the *, like so:
> 
> <TAB>* xyz.c: first line content
> <TAB>next line content
> 
> Other than that, LGTM.

Just to clarify: It's not actually a missing SFRAME_CFA_RA_REG define
(and initialization of the variable)?

Jan
Jens Remus Feb. 26, 2024, 2:51 p.m. UTC | #3 
Am 26.02.2024 um 14:01 schrieb Jan Beulich:
> On 24.02.2024 08:38, Indu Bhagat wrote:
>> On 2/23/24 09:07, Jens Remus wrote:
>>> gas/
>>> 	* config/tc-i386.c: Remove unused SFrame CFI RA register
>>> 	  variable.
>>>
>>
>> The ChangeLog formatting is a bit off here and in the rest of the
>> patches in this series.
>>
>> The indentation of each line should be such that all content lines up
>> below the *, like so:
>>
>> <TAB>* xyz.c: first line content
>> <TAB>next line content
>>
>> Other than that, LGTM.
> 
> Just to clarify: It's not actually a missing SFRAME_CFA_RA_REG define
> (and initialization of the variable)?

My understanding is that on x86 AMD64 SFrame assumes the return address 
to be always passed on the stack at fixed offset -8 from CFA (see 
x86_sframe_cfa_ra_offset). That is why SFrame does not use 
SFRAME_CFA_RA_REG (= DWARF2_DEFAULT_RETURN_COLUMN) on that architecture 
to track the return address being saved on the stack (see 
x86_sframe_ra_tracking_p). SFrame only supports either FP/RA tracking or 
fixed offset to CFA.

Regards,
Jens
Jens Remus Feb. 26, 2024, 3:04 p.m. UTC | #4 
Am 24.02.2024 um 08:38 schrieb Indu Bhagat:
> On 2/23/24 09:07, Jens Remus wrote:
>> gas/
>>     * config/tc-i386.c: Remove unused SFrame CFI RA register
>>       variable.
>>
> 
> The ChangeLog formatting is a bit off here and in the rest of the 
> patches in this series.
> 
> The indentation of each line should be such that all content lines up 
> below the *, like so:
> 
> <TAB>* xyz.c: first line content
> <TAB>next line content

Thanks for letting me know! Have corrected that in the whole series.

> Other than that, LGTM.

Thank you for the review! Will wait for your response on Jan's question 
before pushing it to master.

> 
> Thanks
> Indu
> 
>> Reviewed-by: Andreas Krebbel <krebbel@linux.ibm.com>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>> ---
>>   gas/config/tc-i386.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
>> index c56ca4a2b4b8..1a6fd52f229a 100644
>> --- a/gas/config/tc-i386.c
>> +++ b/gas/config/tc-i386.c
>> @@ -627,7 +627,6 @@ static int shared = 0;
>>   unsigned int x86_sframe_cfa_sp_reg;
>>   /* The other CFA base register for SFrame stack trace info.  */
>>   unsigned int x86_sframe_cfa_fp_reg;
>> -unsigned int x86_sframe_cfa_ra_reg;
>>   #endif

Regards,
Jens
Indu Bhagat Feb. 27, 2024, 9:08 a.m. UTC | #5 
On 2/26/24 05:01, Jan Beulich wrote:
> On 24.02.2024 08:38, Indu Bhagat wrote:
>> On 2/23/24 09:07, Jens Remus wrote:
>>> gas/
>>> 	* config/tc-i386.c: Remove unused SFrame CFI RA register
>>> 	  variable.
>>>
>>
>> The ChangeLog formatting is a bit off here and in the rest of the
>> patches in this series.
>>
>> The indentation of each line should be such that all content lines up
>> below the *, like so:
>>
>> <TAB>* xyz.c: first line content
>> <TAB>next line content
>>
>> Other than that, LGTM.
> 
> Just to clarify: It's not actually a missing SFRAME_CFA_RA_REG define
> (and initialization of the variable)?
> 

For architectures where return address is at a fixed offset from CFA, 
like x86, it made sense to not define and initialize SFRAME_CFA_RA_REG.

So, the variable x86_sframe_cfa_ra_reg was unused and unnecessary.
Jan Beulich Feb. 27, 2024, 9:11 a.m. UTC | #6 
On 27.02.2024 10:08, Indu Bhagat wrote:
> On 2/26/24 05:01, Jan Beulich wrote:
>> On 24.02.2024 08:38, Indu Bhagat wrote:
>>> On 2/23/24 09:07, Jens Remus wrote:
>>>> gas/
>>>> 	* config/tc-i386.c: Remove unused SFrame CFI RA register
>>>> 	  variable.
>>>>
>>>
>>> The ChangeLog formatting is a bit off here and in the rest of the
>>> patches in this series.
>>>
>>> The indentation of each line should be such that all content lines up
>>> below the *, like so:
>>>
>>> <TAB>* xyz.c: first line content
>>> <TAB>next line content
>>>
>>> Other than that, LGTM.
>>
>> Just to clarify: It's not actually a missing SFRAME_CFA_RA_REG define
>> (and initialization of the variable)?
>>
> 
> For architectures where return address is at a fixed offset from CFA, 
> like x86, it made sense to not define and initialize SFRAME_CFA_RA_REG.
> 
> So, the variable x86_sframe_cfa_ra_reg was unused and unnecessary.

Okay. So Jens - feel free to put in.

Jan
diff mbox series

Patch

diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index c56ca4a2b4b8..1a6fd52f229a 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -627,7 +627,6 @@  static int shared = 0;
 unsigned int x86_sframe_cfa_sp_reg;
 /* The other CFA base register for SFrame stack trace info.  */
 unsigned int x86_sframe_cfa_fp_reg;
-unsigned int x86_sframe_cfa_ra_reg;
 
 #endif