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

[Indu Bhagat]

On 7/8/25 7:16 AM, Jan Beulich wrote:
> On 08.07.2025 16:12, Jens Remus wrote:
>> 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.
> 
> Hmm, definitely. I'm pretty sure I had asked for that one to be next to
> the individual flag #define-s. That doesn't invalidate my more general
> comments then, though, i.e. ...
> 

Yes, your comment was to add this to sframe.h.  But I hesitated to put 
such a implementation specific definitions in a publicly installed 
header.  So I rather chose sframe-api.h with less hestitation.

I have proposed this patch
[PATCH] gas: ld: sframe: add new internal header
https://sourceware.org/pipermail/binutils/2025-July/142333.html

which hopefully helps the issue here.

>>>>> 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).
> 
> ... yes, I think so.
> 

I agree. I would also first aim to get the s390x patches in at the earliest.

Thanks