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

[Jens Remus]

Hello Indu and Jan,

thank you for the feedback!

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.

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

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


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.

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/