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

[Jan Beulich]

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

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

Jan