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

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

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