diff mbox series

[v3,15/15] gas: Validate SFrame RA tracking and fixed RA offset

Message ID 20240412144718.4191286-16-jremus@linux.ibm.com
State New
Headers
Series sframe: Enhancements to SFrame info generation |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Testing passed

Commit Message

Jens Remus April 12, 2024, 2:47 p.m. UTC 
  If an architecture uses SFrame return-address (RA) tracking it must
specify the fixed RA offset as invalid. Otherwise, if an architecture
does not use RA tracking, it must specify a valid fixed RA offset.

gas/
	* gen-sframe.c: Validate SFrame RA tracking and fixed
	RA offset.

Signed-off-by: Jens Remus <jremus@linux.ibm.com>
---

Notes (jremus):
    Changes v2 -> v3:
    - New patch.
    
    This could be made dependent on ENABLE_CHECKING (configure option
    --enable-checking).

 gas/gen-sframe.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Indu Bhagat April 18, 2024, 8:38 p.m. UTC | #1 
On 4/12/24 07:47, Jens Remus wrote:
> If an architecture uses SFrame return-address (RA) tracking it must
> specify the fixed RA offset as invalid. Otherwise, if an architecture
> does not use RA tracking, it must specify a valid fixed RA offset.
> 
> gas/
> 	* gen-sframe.c: Validate SFrame RA tracking and fixed
> 	RA offset.
> 
> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
> ---
> 
> Notes (jremus):
>      Changes v2 -> v3:
>      - New patch.
>      
>      This could be made dependent on ENABLE_CHECKING (configure option
>      --enable-checking).
> 
>   gas/gen-sframe.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
> index ca6565b0e45e..7e815f9603ef 100644
> --- a/gas/gen-sframe.c
> +++ b/gas/gen-sframe.c
> @@ -1532,6 +1532,18 @@ output_sframe (segT sframe_seg)
>     /* Setup the version specific access functions.  */
>     sframe_set_version (SFRAME_VERSION_2);
>   
> +#ifdef SFRAME_FRE_RA_TRACKING
> +  if (sframe_ra_tracking_p ())
> +    /* With RA tracking the fixed RA offset must be invalid.  */
> +    gas_assert (sframe_cfa_ra_offset () == SFRAME_CFA_FIXED_RA_INVALID);
> +  else
> +    /* Without RA tracking the fixed RA offset may not be invalid.  */
> +    gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
> +#else
> +  /* Without RA tracking the fixed RA offset may not be invalid.  */
> +  gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
> +#endif
> +

I am not sure if the detailed checks are worth it here (simply because 
of code patterns that follow).

We use the sframe_cfa_ra_offset () function later and only in 
output_sframe_internal () (shown below).  How about we simply put an 
assert there (and get rid of the proposed thunk above):

#ifdef sframe_ra_tracking_p
   if (!sframe_ra_tracking_p ())
     {
       fixed_ra_offset = sframe_cfa_ra_offset ();
       gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
     }
#endif
   out_one (fixed_ra_offset);

fixed_ra_offset is initialized to SFRAME_CFA_FIXED_RA_INVALID in 
output_sframe_internal ().

>     /* Process all fdes and create SFrame stack trace information.  */
>     create_sframe_all ();
>
Jens Remus May 3, 2024, 4:40 p.m. UTC | #2 
Am 18.04.2024 um 22:38 schrieb Indu Bhagat:
> On 4/12/24 07:47, Jens Remus wrote:
>> If an architecture uses SFrame return-address (RA) tracking it must
>> specify the fixed RA offset as invalid. Otherwise, if an architecture
>> does not use RA tracking, it must specify a valid fixed RA offset.
>>
>> gas/
>>     * gen-sframe.c: Validate SFrame RA tracking and fixed
>>     RA offset.
>>
>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>> ---
>>
>> Notes (jremus):
>>      Changes v2 -> v3:
>>      - New patch.
>>      This could be made dependent on ENABLE_CHECKING (configure option
>>      --enable-checking).
>>
>>   gas/gen-sframe.c | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>> index ca6565b0e45e..7e815f9603ef 100644
>> --- a/gas/gen-sframe.c
>> +++ b/gas/gen-sframe.c
>> @@ -1532,6 +1532,18 @@ output_sframe (segT sframe_seg)
>>     /* Setup the version specific access functions.  */
>>     sframe_set_version (SFRAME_VERSION_2);
>> +#ifdef SFRAME_FRE_RA_TRACKING
>> +  if (sframe_ra_tracking_p ())
>> +    /* With RA tracking the fixed RA offset must be invalid.  */
>> +    gas_assert (sframe_cfa_ra_offset () == SFRAME_CFA_FIXED_RA_INVALID);
>> +  else
>> +    /* Without RA tracking the fixed RA offset may not be invalid.  */
>> +    gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
>> +#else
>> +  /* Without RA tracking the fixed RA offset may not be invalid.  */
>> +  gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
>> +#endif
>> +
> 
> I am not sure if the detailed checks are worth it here (simply because 
> of code patterns that follow).

I agree, provided the checks are performed elsewhere as you suggest.

My intention was to have checks that assist with getting SFrame support 
for another architecture implemented correctly, without having to chase 
subtle issues.

> 
> We use the sframe_cfa_ra_offset () function later and only in 
> output_sframe_internal () (shown below).  How about we simply put an 
> assert there (and get rid of the proposed thunk above):
> 
> #ifdef sframe_ra_tracking_p
>    if (!sframe_ra_tracking_p ())

See below.

>      {
>        fixed_ra_offset = sframe_cfa_ra_offset ();
>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);

That is clever and accounts for one potential implementation issue!

>      }
> #endif
>    out_one (fixed_ra_offset);
> 
> fixed_ra_offset is initialized to SFRAME_CFA_FIXED_RA_INVALID in 
> output_sframe_internal ().

Above logic requires sframe_ra_tracking_p to be defined by an 
architecture that is not using RA tracking. Not defining 
sframe_ra_tracking_p would result in fixed_ra_offset being unexpectedly 
initialized to SFRAME_CFA_FIXED_RA_INVALID instead of being set to 
sframe_cfa_ra_offset().

All checks but this do test SFRAME_FRE_RA_TRACKING first, which ensures 
both sframe_ra_tracking_p and SFRAME_CFA_RA_REG are defined, and then 
the predicate sframe_ra_tracking_p to determine whether RA tracking is used.
If SFRAME_FRE_RA_TRACKING is defined and sframe_ra_tracking_p returns 
true, then RA tracking is used.
Likewise, if SFRAME_FRE_RA_TRACKING is not defined or if 
sframe_ra_tracking_p returns false (evaluating lazily) RA tracking is 
not used.

What about making the following change to make all RA tracking tests 
consistent depend on SFRAME_FRE_RA_TRACKING?

#ifdef SFRAME_FRE_RA_TRACKING
   if (!sframe_ra_tracking_p ())
#endif
     {
       fixed_ra_offset = sframe_cfa_ra_offset ();
       /* Without RA tracking the fixed RA offset may not be invalid.  */
       gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
     }
   out_one (fixed_ra_offset);

What would still not be checked is the implementation error to define 
sframe_ra_tracking_p and have it return true without also defining 
SFRAME_CFA_RA_REG. This would be treated as if RA tracking was not used.

Would it therefore make sense to add the following?

#if defined (sframe_ra_tracking_p) && !defined (SFRAME_CFA_RA_REG)
   gas_assert (!sframe_ra_tracking_p ())
#endif

Also when using RA tracking an architecture should implement 
sframe_cfa_ra_offset to return SFRAME_CFA_FIXED_RA_INVALID.

Would it therefore make sense to add the following?

#ifdef SFRAME_FRE_RA_TRACKING
   if (sframe_ra_tracking_p ())
     gas_assert (sframe_cfa_ra_offset () == SFRAME_CFA_FIXED_RA_INVALID);
#endif

> 
>>     /* Process all fdes and create SFrame stack trace information.  */
>>     create_sframe_all ();
> 

Thanks and regards,
Jens
Indu Bhagat May 4, 2024, 12:22 a.m. UTC | #3 
On 5/3/24 09:40, Jens Remus wrote:
> Am 18.04.2024 um 22:38 schrieb Indu Bhagat:
>> On 4/12/24 07:47, Jens Remus wrote:
>>> If an architecture uses SFrame return-address (RA) tracking it must
>>> specify the fixed RA offset as invalid. Otherwise, if an architecture
>>> does not use RA tracking, it must specify a valid fixed RA offset.
>>>
>>> gas/
>>>     * gen-sframe.c: Validate SFrame RA tracking and fixed
>>>     RA offset.
>>>
>>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>>> ---
>>>
>>> Notes (jremus):
>>>      Changes v2 -> v3:
>>>      - New patch.
>>>      This could be made dependent on ENABLE_CHECKING (configure option
>>>      --enable-checking).
>>>
>>>   gas/gen-sframe.c | 12 ++++++++++++
>>>   1 file changed, 12 insertions(+)
>>>
>>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>>> index ca6565b0e45e..7e815f9603ef 100644
>>> --- a/gas/gen-sframe.c
>>> +++ b/gas/gen-sframe.c
>>> @@ -1532,6 +1532,18 @@ output_sframe (segT sframe_seg)
>>>     /* Setup the version specific access functions.  */
>>>     sframe_set_version (SFRAME_VERSION_2);
>>> +#ifdef SFRAME_FRE_RA_TRACKING
>>> +  if (sframe_ra_tracking_p ())
>>> +    /* With RA tracking the fixed RA offset must be invalid.  */
>>> +    gas_assert (sframe_cfa_ra_offset () == 
>>> SFRAME_CFA_FIXED_RA_INVALID);
>>> +  else
>>> +    /* Without RA tracking the fixed RA offset may not be invalid.  */
>>> +    gas_assert (sframe_cfa_ra_offset () != 
>>> SFRAME_CFA_FIXED_RA_INVALID);
>>> +#else
>>> +  /* Without RA tracking the fixed RA offset may not be invalid.  */
>>> +  gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
>>> +#endif
>>> +
>>
>> I am not sure if the detailed checks are worth it here (simply because 
>> of code patterns that follow).
> 
> I agree, provided the checks are performed elsewhere as you suggest.
> 
> My intention was to have checks that assist with getting SFrame support 
> for another architecture implemented correctly, without having to chase 
> subtle issues.
> 
>>
>> We use the sframe_cfa_ra_offset () function later and only in 
>> output_sframe_internal () (shown below).  How about we simply put an 
>> assert there (and get rid of the proposed thunk above):
>>
>> #ifdef sframe_ra_tracking_p
>>    if (!sframe_ra_tracking_p ())
> 
> See below.
> 
>>      {
>>        fixed_ra_offset = sframe_cfa_ra_offset ();
>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
> 
> That is clever and accounts for one potential implementation issue!
> 
>>      }
>> #endif
>>    out_one (fixed_ra_offset);
>>
>> fixed_ra_offset is initialized to SFRAME_CFA_FIXED_RA_INVALID in 
>> output_sframe_internal ().
> 
> Above logic requires sframe_ra_tracking_p to be defined by an 
> architecture that is not using RA tracking. Not defining 
> sframe_ra_tracking_p would result in fixed_ra_offset being unexpectedly 
> initialized to SFRAME_CFA_FIXED_RA_INVALID instead of being set to 
> sframe_cfa_ra_offset().
> 
> All checks but this do test SFRAME_FRE_RA_TRACKING first, which ensures 
> both sframe_ra_tracking_p and SFRAME_CFA_RA_REG are defined, and then 
> the predicate sframe_ra_tracking_p to determine whether RA tracking is 
> used.
> If SFRAME_FRE_RA_TRACKING is defined and sframe_ra_tracking_p returns 
> true, then RA tracking is used.
> Likewise, if SFRAME_FRE_RA_TRACKING is not defined or if 
> sframe_ra_tracking_p returns false (evaluating lazily) RA tracking is 
> not used.
> 
> What about making the following change to make all RA tracking tests 
> consistent depend on SFRAME_FRE_RA_TRACKING?
> 
> #ifdef SFRAME_FRE_RA_TRACKING
>    if (!sframe_ra_tracking_p ())
> #endif
>      {
>        fixed_ra_offset = sframe_cfa_ra_offset ();
>        /* Without RA tracking the fixed RA offset may not be invalid.  */
>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>      }
>    out_one (fixed_ra_offset);
> 

Oops, that's my bad.  Guarding with SFRAME_FRE_RA_TRACKING is more 
appropriate.

But, I think calling the sframe_cfa_ra_offset () out of 
SFRAME_FRE_RA_TRACKING portrays an imprecise meaning.  Only backends 
which opt in for SFrame define these vars/functions. (The cross build 
will likely pass because of the way code is written, but I think you get 
the idea).

I would do:

#ifdef SFRAME_FRE_RA_TRACKING
    if (!sframe_ra_tracking_p ())
      {
        fixed_ra_offset = sframe_cfa_ra_offset ();
        /* Without RA tracking the fixed RA offset may not be invalid.  */
        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
      }
#endif
   out_one (fixed_ra_offset);


> What would still not be checked is the implementation error to define 
> sframe_ra_tracking_p and have it return true without also defining 
> SFRAME_CFA_RA_REG. This would be treated as if RA tracking was not used.
> 
> Would it therefore make sense to add the following?
> 
> #if defined (sframe_ra_tracking_p) && !defined (SFRAME_CFA_RA_REG)
>    gas_assert (!sframe_ra_tracking_p ())
> #endif
> 
> Also when using RA tracking an architecture should implement 
> sframe_cfa_ra_offset to return SFRAME_CFA_FIXED_RA_INVALID.
> 
> Would it therefore make sense to add the following?
> 
> #ifdef SFRAME_FRE_RA_TRACKING
>    if (sframe_ra_tracking_p ())
>      gas_assert (sframe_cfa_ra_offset () == SFRAME_CFA_FIXED_RA_INVALID);
> #endif
> 

All these checks are around guarding against implementation errors, 
opinions may vary. If you feel these add value, then it makes sense to 
add them.

(That said, I am thinking the name sframe_cfa_ra_offset is confusing; 
perhaps sframe_cfa_ra_fixed_offset () is better? I will think about it 
and may be include this in my list of sframe-next patches.)

>>
>>>     /* Process all fdes and create SFrame stack trace information.  */
>>>     create_sframe_all ();
>>
> 
> Thanks and regards,
> Jens
Jens Remus May 6, 2024, 11:41 a.m. UTC | #4 
Am 04.05.2024 um 02:22 schrieb Indu Bhagat:
> On 5/3/24 09:40, Jens Remus wrote:
>> Am 18.04.2024 um 22:38 schrieb Indu Bhagat:
>>> On 4/12/24 07:47, Jens Remus wrote:
>>>> If an architecture uses SFrame return-address (RA) tracking it must
>>>> specify the fixed RA offset as invalid. Otherwise, if an architecture
>>>> does not use RA tracking, it must specify a valid fixed RA offset.
>>>>
>>>> gas/
>>>>     * gen-sframe.c: Validate SFrame RA tracking and fixed
>>>>     RA offset.
>>>>
>>>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>>>> ---
>>>>
>>>> Notes (jremus):
>>>>      Changes v2 -> v3:
>>>>      - New patch.
>>>>      This could be made dependent on ENABLE_CHECKING (configure option
>>>>      --enable-checking).
>>>>
>>>>   gas/gen-sframe.c | 12 ++++++++++++
>>>>   1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>>>> index ca6565b0e45e..7e815f9603ef 100644
>>>> --- a/gas/gen-sframe.c
>>>> +++ b/gas/gen-sframe.c
>>>> @@ -1532,6 +1532,18 @@ output_sframe (segT sframe_seg)
>>>>     /* Setup the version specific access functions.  */
>>>>     sframe_set_version (SFRAME_VERSION_2);
>>>> +#ifdef SFRAME_FRE_RA_TRACKING
>>>> +  if (sframe_ra_tracking_p ())
>>>> +    /* With RA tracking the fixed RA offset must be invalid.  */
>>>> +    gas_assert (sframe_cfa_ra_offset () == 
>>>> SFRAME_CFA_FIXED_RA_INVALID);
>>>> +  else
>>>> +    /* Without RA tracking the fixed RA offset may not be invalid.  */
>>>> +    gas_assert (sframe_cfa_ra_offset () != 
>>>> SFRAME_CFA_FIXED_RA_INVALID);
>>>> +#else
>>>> +  /* Without RA tracking the fixed RA offset may not be invalid.  */
>>>> +  gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
>>>> +#endif
>>>> +
>>>
>>> I am not sure if the detailed checks are worth it here (simply 
>>> because of code patterns that follow).
>>
>> I agree, provided the checks are performed elsewhere as you suggest.
>>
>> My intention was to have checks that assist with getting SFrame 
>> support for another architecture implemented correctly, without having 
>> to chase subtle issues.
>>
>>>
>>> We use the sframe_cfa_ra_offset () function later and only in 
>>> output_sframe_internal () (shown below).  How about we simply put an 
>>> assert there (and get rid of the proposed thunk above):
>>>
>>> #ifdef sframe_ra_tracking_p
>>>    if (!sframe_ra_tracking_p ())
>>
>> See below.
>>
>>>      {
>>>        fixed_ra_offset = sframe_cfa_ra_offset ();
>>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>>
>> That is clever and accounts for one potential implementation issue!
>>
>>>      }
>>> #endif
>>>    out_one (fixed_ra_offset);
>>>
>>> fixed_ra_offset is initialized to SFRAME_CFA_FIXED_RA_INVALID in 
>>> output_sframe_internal ().
>>
>> Above logic requires sframe_ra_tracking_p to be defined by an 
>> architecture that is not using RA tracking. Not defining 
>> sframe_ra_tracking_p would result in fixed_ra_offset being 
>> unexpectedly initialized to SFRAME_CFA_FIXED_RA_INVALID instead of 
>> being set to sframe_cfa_ra_offset().
>>
>> All checks but this do test SFRAME_FRE_RA_TRACKING first, which 
>> ensures both sframe_ra_tracking_p and SFRAME_CFA_RA_REG are defined, 
>> and then the predicate sframe_ra_tracking_p to determine whether RA 
>> tracking is used.
>> If SFRAME_FRE_RA_TRACKING is defined and sframe_ra_tracking_p returns 
>> true, then RA tracking is used.
>> Likewise, if SFRAME_FRE_RA_TRACKING is not defined or if 
>> sframe_ra_tracking_p returns false (evaluating lazily) RA tracking is 
>> not used.
>>
>> What about making the following change to make all RA tracking tests 
>> consistent depend on SFRAME_FRE_RA_TRACKING?
>>
>> #ifdef SFRAME_FRE_RA_TRACKING
>>    if (!sframe_ra_tracking_p ())
>> #endif
>>      {
>>        fixed_ra_offset = sframe_cfa_ra_offset ();
>>        /* Without RA tracking the fixed RA offset may not be invalid.  */
>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>>      }
>>    out_one (fixed_ra_offset);
>>
> 
> Oops, that's my bad.  Guarding with SFRAME_FRE_RA_TRACKING is more 
> appropriate.

Included this in the previous patch in this series.

> 
> But, I think calling the sframe_cfa_ra_offset () out of 
> SFRAME_FRE_RA_TRACKING portrays an imprecise meaning.  Only backends 
> which opt in for SFrame define these vars/functions. (The cross build 
> will likely pass because of the way code is written, but I think you get 
> the idea).
> 
> I would do:
> 
> #ifdef SFRAME_FRE_RA_TRACKING
>     if (!sframe_ra_tracking_p ())
>       {
>         fixed_ra_offset = sframe_cfa_ra_offset ();
>         /* Without RA tracking the fixed RA offset may not be invalid.  */
>         gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>       }
> #endif
>    out_one (fixed_ra_offset);

Good catch! I did not consider that SFrame may not be implemented by an 
architecture at all.

> 
> 
>> What would still not be checked is the implementation error to define 
>> sframe_ra_tracking_p and have it return true without also defining 
>> SFRAME_CFA_RA_REG. This would be treated as if RA tracking was not used.
>>
>> Would it therefore make sense to add the following?
>>
>> #if defined (sframe_ra_tracking_p) && !defined (SFRAME_CFA_RA_REG)
>>    gas_assert (!sframe_ra_tracking_p ())
>> #endif
>>
>> Also when using RA tracking an architecture should implement 
>> sframe_cfa_ra_offset to return SFRAME_CFA_FIXED_RA_INVALID.
>>
>> Would it therefore make sense to add the following?
>>
>> #ifdef SFRAME_FRE_RA_TRACKING
>>    if (sframe_ra_tracking_p ())
>>      gas_assert (sframe_cfa_ra_offset () == SFRAME_CFA_FIXED_RA_INVALID);
>> #endif
>>
> 
> All these checks are around guarding against implementation errors, 
> opinions may vary. If you feel these add value, then it makes sense to 
> add them.
> 
> (That said, I am thinking the name sframe_cfa_ra_offset is confusing; 
> perhaps sframe_cfa_ra_fixed_offset () is better? I will think about it 
> and may be include this in my list of sframe-next patches.)

I'll leave it up to you then.

> 
>>>
>>>>     /* Process all fdes and create SFrame stack trace information.  */
>>>>     create_sframe_all ();
>>>
>>
>> Thanks and regards,
>> Jens
> 

Regards,
Jens
Jens Remus May 6, 2024, 2:39 p.m. UTC | #5 
Am 06.05.2024 um 13:41 schrieb Jens Remus:
> Am 04.05.2024 um 02:22 schrieb Indu Bhagat:
>> On 5/3/24 09:40, Jens Remus wrote:
>>> Am 18.04.2024 um 22:38 schrieb Indu Bhagat:
>>>> On 4/12/24 07:47, Jens Remus wrote:
>>>>> If an architecture uses SFrame return-address (RA) tracking it must
>>>>> specify the fixed RA offset as invalid. Otherwise, if an architecture
>>>>> does not use RA tracking, it must specify a valid fixed RA offset.
>>>>>
>>>>> gas/
>>>>>     * gen-sframe.c: Validate SFrame RA tracking and fixed
>>>>>     RA offset.
>>>>>
>>>>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>>>>> ---
>>>>>
>>>>> Notes (jremus):
>>>>>      Changes v2 -> v3:
>>>>>      - New patch.
>>>>>      This could be made dependent on ENABLE_CHECKING (configure option
>>>>>      --enable-checking).
>>>>>
>>>>>   gas/gen-sframe.c | 12 ++++++++++++
>>>>>   1 file changed, 12 insertions(+)
>>>>>
>>>>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>>>>> index ca6565b0e45e..7e815f9603ef 100644
>>>>> --- a/gas/gen-sframe.c
>>>>> +++ b/gas/gen-sframe.c
>>>>> @@ -1532,6 +1532,18 @@ output_sframe (segT sframe_seg)
>>>>>     /* Setup the version specific access functions.  */
>>>>>     sframe_set_version (SFRAME_VERSION_2);
>>>>> +#ifdef SFRAME_FRE_RA_TRACKING
>>>>> +  if (sframe_ra_tracking_p ())
>>>>> +    /* With RA tracking the fixed RA offset must be invalid.  */
>>>>> +    gas_assert (sframe_cfa_ra_offset () == 
>>>>> SFRAME_CFA_FIXED_RA_INVALID);
>>>>> +  else
>>>>> +    /* Without RA tracking the fixed RA offset may not be 
>>>>> invalid.  */
>>>>> +    gas_assert (sframe_cfa_ra_offset () != 
>>>>> SFRAME_CFA_FIXED_RA_INVALID);
>>>>> +#else
>>>>> +  /* Without RA tracking the fixed RA offset may not be invalid.  */
>>>>> +  gas_assert (sframe_cfa_ra_offset () != 
>>>>> SFRAME_CFA_FIXED_RA_INVALID);
>>>>> +#endif
>>>>> +
>>>>
>>>> I am not sure if the detailed checks are worth it here (simply 
>>>> because of code patterns that follow).
>>>
>>> I agree, provided the checks are performed elsewhere as you suggest.
>>>
>>> My intention was to have checks that assist with getting SFrame 
>>> support for another architecture implemented correctly, without 
>>> having to chase subtle issues.
>>>
>>>>
>>>> We use the sframe_cfa_ra_offset () function later and only in 
>>>> output_sframe_internal () (shown below).  How about we simply put an 
>>>> assert there (and get rid of the proposed thunk above):
>>>>
>>>> #ifdef sframe_ra_tracking_p
>>>>    if (!sframe_ra_tracking_p ())
>>>
>>> See below.
>>>
>>>>      {
>>>>        fixed_ra_offset = sframe_cfa_ra_offset ();
>>>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>>>
>>> That is clever and accounts for one potential implementation issue!
>>>
>>>>      }
>>>> #endif
>>>>    out_one (fixed_ra_offset);
>>>>
>>>> fixed_ra_offset is initialized to SFRAME_CFA_FIXED_RA_INVALID in 
>>>> output_sframe_internal ().
>>>
>>> Above logic requires sframe_ra_tracking_p to be defined by an 
>>> architecture that is not using RA tracking. Not defining 
>>> sframe_ra_tracking_p would result in fixed_ra_offset being 
>>> unexpectedly initialized to SFRAME_CFA_FIXED_RA_INVALID instead of 
>>> being set to sframe_cfa_ra_offset().
>>>
>>> All checks but this do test SFRAME_FRE_RA_TRACKING first, which 
>>> ensures both sframe_ra_tracking_p and SFRAME_CFA_RA_REG are defined, 
>>> and then the predicate sframe_ra_tracking_p to determine whether RA 
>>> tracking is used.
>>> If SFRAME_FRE_RA_TRACKING is defined and sframe_ra_tracking_p returns 
>>> true, then RA tracking is used.
>>> Likewise, if SFRAME_FRE_RA_TRACKING is not defined or if 
>>> sframe_ra_tracking_p returns false (evaluating lazily) RA tracking is 
>>> not used.
>>>
>>> What about making the following change to make all RA tracking tests 
>>> consistent depend on SFRAME_FRE_RA_TRACKING?
>>>
>>> #ifdef SFRAME_FRE_RA_TRACKING
>>>    if (!sframe_ra_tracking_p ())
>>> #endif
>>>      {
>>>        fixed_ra_offset = sframe_cfa_ra_offset ();
>>>        /* Without RA tracking the fixed RA offset may not be 
>>> invalid.  */
>>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>>>      }
>>>    out_one (fixed_ra_offset);
>>>
>>
>> Oops, that's my bad.  Guarding with SFRAME_FRE_RA_TRACKING is more 
>> appropriate.
> 
> Included this in the previous patch in this series.

This breaks x86-64, as gas/config/tc-i386.h does not define SFRAME_CFA_RA_REG.

Either this specific check must stay, possibly with an additional comment:

   /* Offset for the return address from CFA is fixed for some ABIs
      (e.g., AMD64), output a SFRAME_CFA_FIXED_RA_INVALID otherwise.
      NOTE: sframe_ra_tracking_p may be defined without SFRAME_CFA_RA_REG
            (e.g., AMD64), so that SFRAME_FRE_RA_TRACKING won't be defined.  */
#ifdef sframe_ra_tracking_p
   if (!sframe_ra_tracking_p ())
     {
       fixed_ra_offset = sframe_cfa_ra_offset ();
       /* Without RA tracking the fixed RA offset may not be invalid.  */
       gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
     }
#endif
   out_one (fixed_ra_offset);

or gas/config/tc-i386.h needs to define SFRAME_CFA_RA_REG, for instance as follows:

#define SFRAME_CFA_RA_REG DWARF2_DEFAULT_RETURN_COLUMN

Since an architecture may not have a RA register I wonder whether keeping the existing logic as is would be better. What is your opinion?

> 
>>
>> But, I think calling the sframe_cfa_ra_offset () out of 
>> SFRAME_FRE_RA_TRACKING portrays an imprecise meaning.  Only backends 
>> which opt in for SFrame define these vars/functions. (The cross build 
>> will likely pass because of the way code is written, but I think you 
>> get the idea).
>>
>> I would do:
>>
>> #ifdef SFRAME_FRE_RA_TRACKING
>>     if (!sframe_ra_tracking_p ())
>>       {
>>         fixed_ra_offset = sframe_cfa_ra_offset ();
>>         /* Without RA tracking the fixed RA offset may not be 
>> invalid.  */
>>         gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>>       }
>> #endif
>>    out_one (fixed_ra_offset);
> 
> Good catch! I did not consider that SFrame may not be implemented by an 
> architecture at all.
> 
>>
>>
>>> What would still not be checked is the implementation error to define 
>>> sframe_ra_tracking_p and have it return true without also defining 
>>> SFRAME_CFA_RA_REG. This would be treated as if RA tracking was not used.
>>>
>>> Would it therefore make sense to add the following?
>>>
>>> #if defined (sframe_ra_tracking_p) && !defined (SFRAME_CFA_RA_REG)
>>>    gas_assert (!sframe_ra_tracking_p ())
>>> #endif
>>>
>>> Also when using RA tracking an architecture should implement 
>>> sframe_cfa_ra_offset to return SFRAME_CFA_FIXED_RA_INVALID.
>>>
>>> Would it therefore make sense to add the following?
>>>
>>> #ifdef SFRAME_FRE_RA_TRACKING
>>>    if (sframe_ra_tracking_p ())
>>>      gas_assert (sframe_cfa_ra_offset () == 
>>> SFRAME_CFA_FIXED_RA_INVALID);
>>> #endif
>>>
>>
>> All these checks are around guarding against implementation errors, 
>> opinions may vary. If you feel these add value, then it makes sense to 
>> add them.
>>
>> (That said, I am thinking the name sframe_cfa_ra_offset is confusing; 
>> perhaps sframe_cfa_ra_fixed_offset () is better? I will think about it 
>> and may be include this in my list of sframe-next patches.)
> 
> I'll leave it up to you then.
> 
>>
>>>>
>>>>>     /* Process all fdes and create SFrame stack trace information.  */
>>>>>     create_sframe_all ();
>>>>
>>>
>>> Thanks and regards,
>>> Jens
>>
> 
> Regards,
> Jens

Thanks and regards,
Jens
Indu Bhagat May 16, 2024, 8:45 p.m. UTC | #6 
On 5/6/24 07:39, Jens Remus wrote:
> Am 06.05.2024 um 13:41 schrieb Jens Remus:
>> Am 04.05.2024 um 02:22 schrieb Indu Bhagat:
>>> On 5/3/24 09:40, Jens Remus wrote:
>>>> Am 18.04.2024 um 22:38 schrieb Indu Bhagat:
>>>>> On 4/12/24 07:47, Jens Remus wrote:
>>>>>> If an architecture uses SFrame return-address (RA) tracking it must
>>>>>> specify the fixed RA offset as invalid. Otherwise, if an architecture
>>>>>> does not use RA tracking, it must specify a valid fixed RA offset.
>>>>>>
>>>>>> gas/
>>>>>>     * gen-sframe.c: Validate SFrame RA tracking and fixed
>>>>>>     RA offset.
>>>>>>
>>>>>> Signed-off-by: Jens Remus <jremus@linux.ibm.com>
>>>>>> ---
>>>>>>
>>>>>> Notes (jremus):
>>>>>>      Changes v2 -> v3:
>>>>>>      - New patch.
>>>>>>      This could be made dependent on ENABLE_CHECKING (configure 
>>>>>> option
>>>>>>      --enable-checking).
>>>>>>
>>>>>>   gas/gen-sframe.c | 12 ++++++++++++
>>>>>>   1 file changed, 12 insertions(+)
>>>>>>
>>>>>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>>>>>> index ca6565b0e45e..7e815f9603ef 100644
>>>>>> --- a/gas/gen-sframe.c
>>>>>> +++ b/gas/gen-sframe.c
>>>>>> @@ -1532,6 +1532,18 @@ output_sframe (segT sframe_seg)
>>>>>>     /* Setup the version specific access functions.  */
>>>>>>     sframe_set_version (SFRAME_VERSION_2);
>>>>>> +#ifdef SFRAME_FRE_RA_TRACKING
>>>>>> +  if (sframe_ra_tracking_p ())
>>>>>> +    /* With RA tracking the fixed RA offset must be invalid.  */
>>>>>> +    gas_assert (sframe_cfa_ra_offset () == 
>>>>>> SFRAME_CFA_FIXED_RA_INVALID);
>>>>>> +  else
>>>>>> +    /* Without RA tracking the fixed RA offset may not be 
>>>>>> invalid.  */
>>>>>> +    gas_assert (sframe_cfa_ra_offset () != 
>>>>>> SFRAME_CFA_FIXED_RA_INVALID);
>>>>>> +#else
>>>>>> +  /* Without RA tracking the fixed RA offset may not be invalid.  */
>>>>>> +  gas_assert (sframe_cfa_ra_offset () != 
>>>>>> SFRAME_CFA_FIXED_RA_INVALID);
>>>>>> +#endif
>>>>>> +
>>>>>
>>>>> I am not sure if the detailed checks are worth it here (simply 
>>>>> because of code patterns that follow).
>>>>
>>>> I agree, provided the checks are performed elsewhere as you suggest.
>>>>
>>>> My intention was to have checks that assist with getting SFrame 
>>>> support for another architecture implemented correctly, without 
>>>> having to chase subtle issues.
>>>>
>>>>>
>>>>> We use the sframe_cfa_ra_offset () function later and only in 
>>>>> output_sframe_internal () (shown below).  How about we simply put 
>>>>> an assert there (and get rid of the proposed thunk above):
>>>>>
>>>>> #ifdef sframe_ra_tracking_p
>>>>>    if (!sframe_ra_tracking_p ())
>>>>
>>>> See below.
>>>>
>>>>>      {
>>>>>        fixed_ra_offset = sframe_cfa_ra_offset ();
>>>>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>>>>
>>>> That is clever and accounts for one potential implementation issue!
>>>>
>>>>>      }
>>>>> #endif
>>>>>    out_one (fixed_ra_offset);
>>>>>
>>>>> fixed_ra_offset is initialized to SFRAME_CFA_FIXED_RA_INVALID in 
>>>>> output_sframe_internal ().
>>>>
>>>> Above logic requires sframe_ra_tracking_p to be defined by an 
>>>> architecture that is not using RA tracking. Not defining 
>>>> sframe_ra_tracking_p would result in fixed_ra_offset being 
>>>> unexpectedly initialized to SFRAME_CFA_FIXED_RA_INVALID instead of 
>>>> being set to sframe_cfa_ra_offset().
>>>>

The current SFrame implementation does want the architecture/backend to 
define a sframe_ra_tracking_p; This is the only way to clearly know 
whether RA tracking is needed or not.

May be we enforce that sframe_ra_tracking_p be defined.

>>>> All checks but this do test SFRAME_FRE_RA_TRACKING first, which 
>>>> ensures both sframe_ra_tracking_p and SFRAME_CFA_RA_REG are defined, 
>>>> and then the predicate sframe_ra_tracking_p to determine whether RA 
>>>> tracking is used.
>>>> If SFRAME_FRE_RA_TRACKING is defined and sframe_ra_tracking_p 
>>>> returns true, then RA tracking is used.
>>>> Likewise, if SFRAME_FRE_RA_TRACKING is not defined or if 
>>>> sframe_ra_tracking_p returns false (evaluating lazily) RA tracking 
>>>> is not used.
>>>>
>>>> What about making the following change to make all RA tracking tests 
>>>> consistent depend on SFRAME_FRE_RA_TRACKING?
>>>>
>>>> #ifdef SFRAME_FRE_RA_TRACKING
>>>>    if (!sframe_ra_tracking_p ())
>>>> #endif
>>>>      {
>>>>        fixed_ra_offset = sframe_cfa_ra_offset ();
>>>>        /* Without RA tracking the fixed RA offset may not be 
>>>> invalid.  */
>>>>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>>>>      }
>>>>    out_one (fixed_ra_offset);
>>>>
>>>
>>> Oops, that's my bad.  Guarding with SFRAME_FRE_RA_TRACKING is more 
>>> appropriate.
>>
>> Included this in the previous patch in this series.
> 
> This breaks x86-64, as gas/config/tc-i386.h does not define 
> SFRAME_CFA_RA_REG.
> 
> Either this specific check must stay, possibly with an additional comment:
> 
>    /* Offset for the return address from CFA is fixed for some ABIs
>       (e.g., AMD64), output a SFRAME_CFA_FIXED_RA_INVALID otherwise.
>       NOTE: sframe_ra_tracking_p may be defined without SFRAME_CFA_RA_REG
>             (e.g., AMD64), so that SFRAME_FRE_RA_TRACKING won't be 
> defined.  */
> #ifdef sframe_ra_tracking_p
>    if (!sframe_ra_tracking_p ())
>      {
>        fixed_ra_offset = sframe_cfa_ra_offset ();
>        /* Without RA tracking the fixed RA offset may not be invalid.  */
>        gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
>      }
> #endif
>    out_one (fixed_ra_offset);
> 

I think the above is better than ...


> or gas/config/tc-i386.h needs to define SFRAME_CFA_RA_REG, for instance 
> as follows:
> 
> #define SFRAME_CFA_RA_REG DWARF2_DEFAULT_RETURN_COLUMN
> 

... doing this.  Forcing backends to #define like above is unnecessary: 
we should ideally not need arches to specify a (fake?) register for the 
Return Address when there is none.

> Since an architecture may not have a RA register I wonder whether 
> keeping the existing logic as is would be better. What is your opinion?


I wonder if we instead (in output_sframe_internal) do:

   /* All ABIs participating in SFrame generation must define
      sframe_ra_tracking_p.
      When RA tracking (in FREs) is not needed (e.g., AMD64),  SFrame 
assumes
      the RA is going to be at a fixed offset from CFA.  Check that the 
fixed RA
      offset is appropriately defined in all cases.  */
   if (!sframe_ra_tracking_p ())
    {
      fixed_ra_offset = sframe_cfa_ra_offset ();
      gas_assert (fixed_ra_offset != SFRAME_CFA_FIXED_RA_INVALID);
    }

   out_one (fixed_ra_offset);

Note we removed the #ifdef sframe_ra_tracking_p etc altogether, as we 
want to make sure the participating ABIs have this one defined, else 
there will be build failures.

(I am aware that this proposal contradicts a subset of my earlier stance 
in this thread, but please see if the above makes more sense, given that 
our intention is alert when new backends make silly mistakes...)
diff mbox series

Patch

diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
index ca6565b0e45e..7e815f9603ef 100644
--- a/gas/gen-sframe.c
+++ b/gas/gen-sframe.c
@@ -1532,6 +1532,18 @@  output_sframe (segT sframe_seg)
   /* Setup the version specific access functions.  */
   sframe_set_version (SFRAME_VERSION_2);
 
+#ifdef SFRAME_FRE_RA_TRACKING
+  if (sframe_ra_tracking_p ())
+    /* With RA tracking the fixed RA offset must be invalid.  */
+    gas_assert (sframe_cfa_ra_offset () == SFRAME_CFA_FIXED_RA_INVALID);
+  else
+    /* Without RA tracking the fixed RA offset may not be invalid.  */
+    gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
+#else
+  /* Without RA tracking the fixed RA offset may not be invalid.  */
+  gas_assert (sframe_cfa_ra_offset () != SFRAME_CFA_FIXED_RA_INVALID);
+#endif
+
   /* Process all fdes and create SFrame stack trace information.  */
   create_sframe_all ();