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

[Jan Beulich]

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.

>>>> To answer the question below I'd first like to understand why using this
>>>> header in gas/gen-sframe.c is (deemed) bad. It looks quite natural to me
>>>> that to be able to use any part of what's defined there (even if only
>>>> flag values), that header needs including. Or of that was a problem, the
>>>> header may need splitting.
>>>
>>> (sframe-api.h is the implementation header file for consumers using 
>>> libsframe APIs.)  gas has its own SFrame writing logic (via fragment 
>>> creation etc.) and has not needed the libsframe APIs so far.  To include 
>>> it only for SFRAME_F_LD_MUSTHAVE_FLAGS just seemed like tieing up an 
>>> interface which is not necessary.
>>>
>>> Inclusion of sframe-api.h causing collisions like Jens runs into, and 
>>> then needing to bypass them are both unpalatable, I thought.
>>
>> Including that header ought to be permissible anywhere, including gas
>> sources. If gas (and maybe other components) are interested in the
>> #define-s (and possibly types) only, can the header file maybe gain a
>> mode (off by default) by which the function declarations would be
>> hidden?
> 
> Hmm.  Optionally hide everything in sframe-api.h except for
> SFRAME_F_LD_MUSTHAVE_FLAGS?  Not ideal either.

The individual flag values also would need to remain available. And those
must be possible to obtain anyway, by any source file.

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

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

Jan