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

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

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