[PATCH 1/3] libsframe: fix issue in finding FRE in SFRAME_FDE_TYPE_PCMASK type SFrame FDEs

[Jens Remus]

On 22.05.2025 08:02, Indu Bhagat wrote:
> On 5/21/25 1:38 AM, Jens Remus wrote:
>> On 20.05.2025 21:03, Indu Bhagat wrote:

>>> diff --git a/libsframe/sframe.c b/libsframe/sframe.c
>>
>>> @@ -398,10 +398,13 @@ sframe_fre_check_range_p (sframe_func_desc_entry *fdep,
>>>       }
>>>     else
>>>       {
>>> -      /* For FDEs for repetitive pattern of insns, we need to return the FRE
>>> -     where pc % rep_block_size is between start_ip_offset and
>>> -     end_ip_offset.  */
>>> -      masked_pc = pc % rep_block_size;
>>> +      /* For SFrame FDEs encoding information for repetitive pattern of insns,
>>> +     masking with the rep_block_size is necessary to find the matching FRE.
>>> +     start_ip_offset and end_ip_offset are _unsigned_values_ identifying
>>
>> Changing start_ip_offset and end_ip_offset to uint32_t is probably no
>> longer feasible, as this would break the libsframe API?
>>
> 
> Changing the argument types of the internal static APIs is possible, but
> not "uint32_t fre_start_addr" member of struct sframe_frame_row_entry in
> include/sframe-api.h.

To me it is just natural that struct sframe_frame_row_entry member
fre_start_addr is of type uint32_t, as the offset is from function start
address, and negative offsets do not make sense, as functions only grow
upwards (towards higher addresses).

Therefore I thought it would also make sense to use uint32_t in all
places where the value of fre_start_addr is passed around.  After all
you intended to add a comment in the case above to clarify this fact,
instead of using an unsigned integer type, which would make this
comment unnecessary.

I have created a patch and will send it based on your V2 together with
a minor related fixup for one of my patches you are now carrying.

> 
>>> +     the SFrame FRE.  So, perform a calculation to first get the distance
>>> +     between the pc and func_start_addr, followed by modulo to find the
>>> +     fitting range.  */
>>> +      masked_pc = (pc - func_start_addr) % rep_block_size;
>>>         ret = ((start_ip_offset <= masked_pc) && (end_ip_offset >= masked_pc));
>>>       }
>>
>> I think the whole function could be simplified.  See my patch
>> "[PATCH 1/3] libsframe: simplify SFrame FRE range check" [1], which is based
>> on top of yours and which you could squash into yours.
>>
>> [1]: [PATCH 1/3] libsframe: simplify SFrame FRE range check,
>>       https://inbox.sourceware.org/binutils/20250521083059.55048-2-jremus@linux.ibm.com/
>>
> 
> Thanks for the patch.  I will include the patches series containing your patch and send a V2.

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/