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

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

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