[PATCH v3 08/11] s390: Store SFrame CFA offset adjusted

[Jens Remus]

On 08.07.2025 14:36, Jan Beulich wrote:
> On 08.07.2025 14:14, Jens Remus wrote:
>> On 08.07.2025 10:06, Jan Beulich wrote:
>>> On 08.07.2025 09:31, Indu Bhagat wrote:
>>>> On 7/7/25 11:49 PM, Jan Beulich wrote:
>>>>> On 07.07.2025 19:48, Indu Bhagat wrote:
>>>>>> On 7/7/25 8:07 AM, Jens Remus wrote:
>>>>>>>>> @@ -134,10 +134,26 @@ sframe_fre_set_cfa_base_reg (struct sframe_row_entry *fre,
>>>>>>>>>       fre->merge_candidate = false;
>>>>>>>>>     }
>>>>>>>>>     +static offsetT
>>>>>>>>> +sframe_fre_get_cfa_offset (const struct sframe_row_entry * fre)
>>>>>>> The recent fix of relocatable SFrame links now include "sframe-api.h".
>>>>>>> This cases above new internal helper to clash with the external SFrame
>>>>>>> API function of same name.  For V4 I have prefixed the internal helper
>>>>>>> with "__", which I somewhat dislike.  Any better suggestion to resolve?
>>>>>>
>>>>>> Ugh, I did not like bringing in the "sframe-api.h" at all into
>>>>>> gas/gen-sframe.c just for the the SFRAME_F_LD_MUSTHAVE_FLAGS.
>>
>> Couldn't SFRAME_F_LD_MUSTHAVE_FLAGS be moved into sframe.h?  Not ideal,
>> but would resolve the function name clash issue.
> 
> I had specifically asked for it to live next to the individual flag
> definitions, so that potential updating of the value is less easy to forget.

As the individual flag definitions reside in sframe.h, can we agree to
move SFRAME_F_LD_MUSTHAVE_FLAGS from sframe-api.h to sframe.h and drop
the include of sframe-api.h in gen-sframe.c?  This would resolve the
function name clash I am running into.

>>> More generally though I think that it would be better to avoid any
>>> name collisions. Two entities of exactly the same name can easily be
>>> confusing.
>>
>> gen-sframe.c:
>> static void sframe_fre_set_cfa_base_reg (struct sframe_row_entry *fre, unsigned int cfa_base_reg)
>> static void sframe_fre_set_cfa_offset (struct sframe_row_entry *fre, offsetT cfa_offset)
>>
>> sframe-api.h:
>> extern int32_t sframe_fre_get_cfa_offset (sframe_decoder_ctx *dtcx, sframe_frame_row_entry *fre, int *errp);
>>
>> The following to be added helper in gen-sframe.c follows the existing
>> naming scheme:
>> static offsetT sframe_fre_get_cfa_offset (const struct sframe_row_entry * fre)
>>
>> What do you suggest?  Prefix every helper in gen-sframe.c with one or
>> two underscores to identify them as local helpers?  Remove the prefix
>> "sframe_" (or even "sframe_fre_") from every helper in gen-sframe.c?
>> Rename helpers to {set|get}_sframe_fre_cfa_offset (similar to
>> get_dw_fde_start_addrS) or sframe_fre_cfa_offset_{get|set}?  None
>> seems ideal.
> 
> Personally I'm in favor of removing (imo redundant) prefixes for static
> function or variable names. That would be sframe_ here. If for some reason
> you think the prefix is helpful, another option might be to simply prefix
> static function names with a single underscore.

A cleanup of gen-sframe.c static function names could then be done with
a separate patch (series).

>> As the newly introduced helper is only used in one place it could also
>> be open coded as follows to workaround the name clash.  Not ideal either.
>>
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>> @@ -1059,7 +1047,15 @@ sframe_xlate_do_def_cfa_register (struct sframe_xlate_ctx *xlate_ctx,
>>      }
>>    sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.r);
>>    if (last_fre)
>> -    sframe_fre_set_cfa_offset (cur_fre, __sframe_fre_get_cfa_offset (last_fre));
>> +    {
>> +      offsetT cfa_offset = last_fre->cfa_offset;
>> +
>> +      /* For s390x undo adjustment of CFA offset (to enable 8-bit offsets).  */
>> +      if (sframe_get_abi_arch () == SFRAME_ABI_S390X_ENDIAN_BIG)
>> +       cfa_offset = SFRAME_V2_FRE_S390X_CFA_OFFSET_DECODE (cfa_offset);
>> +
>> +      sframe_fre_set_cfa_offset (cur_fre, cfa_offset);
>> +    }
>>
>>    cur_fre->merge_candidate = false;
>>
>>
>> What if I change sframe_xlate_do_def_cfa_register as follows to not to
>> use any helpers at all?  I currently prefer that approach.
>>
>> diff --git a/gas/gen-sframe.c b/gas/gen-sframe.c
>> @@ -1059,8 +1047,8 @@ sframe_xlate_do_def_cfa_register (struct sframe_xlate_ctx *xlate_ctx,
>>      }
>>    sframe_fre_set_cfa_base_reg (cur_fre, cfi_insn->u.r);
>>    if (last_fre)
>> -    sframe_fre_set_cfa_offset (cur_fre, __sframe_fre_get_cfa_offset (last_fre));
>> -
>> +    /* Copy CFA offset value as-is, as it may be encoded (e.g., on s390x).  */
>> +    cur_fre->cfa_offset = last_fre->cfa_offset;
>>    cur_fre->merge_candidate = false;
>>
>>    return SFRAME_XLATE_OK;
>>
>>
>> Looking forward for your thoughts!  I would appreciate if we could come
>> up with and agree on a solution soon, so that the s390x SFrame support
>> could hopefully make it into 2.45.
> 
> I'd prefer to leave this part to Indu.

Thanks and regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
+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/